Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decide what to do about impl ULE for () #6219

Open
sffc opened this issue Mar 4, 2025 · 3 comments
Open

Decide what to do about impl ULE for () #6219

sffc opened this issue Mar 4, 2025 · 3 comments
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake
Milestone

Comments

@sffc
Copy link
Member

sffc commented Mar 4, 2025

Even &() is mostly "valid" in the sense that it's a thing that makes sense in some generic contexts: it would be silly to exclude () from generic contexts just because its references are strange. But the ULEverse already excludes lots of types, its driven by explicit inclusion.

I don't really see a purpose for (): ULE beyond as a placeholder in some generic contexts, and there I see two ways for it to work, as a "never" type or as a stand in similar to how Vec<()> is basically just a usize. For that, I'd prefer having an explicit type that makes it clear what's going on. When I see (): ULE I don't have much clarity at all.

We ended up releasing this in zerovec anyway (I didn't want to further block landing this) so there's not much to do about undoing it without a breaking release.

But basically: I definitely do not think this impl is "something we can/should have regardless", and in the medium term I'd like to deprecate it.

Originally posted by @Manishearth in #6133 (comment)

@sffc sffc added the C-zerovec Component: Yoke, ZeroVec, DataBake label Mar 4, 2025
@sffc sffc added this to the Utilities 1.0 milestone Mar 4, 2025
@Manishearth
Copy link
Member

My opinion is that if we had not done this, I would have been happier, but I don't really wish to go through a deprecation cycle or do breaking changes because of this.

I think what I would want instead is an EmptyULE type that ULE's to itself with clearly documented ULE behavior. If we make that change at zerovec 1.0 that seems fine.

@sffc
Copy link
Member Author

sffc commented Mar 4, 2025

My opinion is that [()] is a clean type-system placeholder for an "empty but not uninhabited variable-size type" so we should make it work. I think the intent of [()] is clearer than the intent of [zerovec::ule::EmptyULE] which isn't really "empty" at all: it is a placeholder type.

Likewise, () is a clean placeholder for a sized ULE.

I went through multiple phases of adding Never types in datetime, and I ended up going back to "just implement the traits on ()" because (1) having the trait implemented on one type is easier since you can pass the type around to more places, like either::Either, and (2) the eye tends to gloss over () when reading code which is exactly the intent of a placeholder type. You're welcome to disagree with me on this, but this is my opinion from experience.

I don't have a strong opinion on whether impl ULE for () should have the behavior of "valid if bytes are empty" or "never valid". I lean toward "valid if bytes are empty" which is what I implemented.

@Manishearth
Copy link
Member

[()] is a clean type-system placeholder for an "empty but not uninhabited variable-size type" so we should make it work

If we want such a placeholder, we should make an explicit one. impl ULE for () immediately sets of alarm bells in my mind, my reaction to seeing it is not "this is the impl I need" it is "is this impl a mistake?". I'm not debating the usefulness of a placeholder type.

A cost I am applying here is that ZSTs are weird in unsafe code, because their pointer/array behavior is markedly different from everything else, and this is a crate that is 99% pointer/array things. Unsafe code in crates like this tends to often be written with the implicit assumption that ZSTs aren't being passed through. I've confirmed that's not the case here, but I'm not our users. All together, impl ULE for () is concerning because it could very easily be an oversight. An explicit type with the same semantics, on the other hand, is clearly intentional.

Doubly so because the use case here isn't even for the ULE universe, it's for the VarULE universe; VarULE for NeverVarULE would be cleaner and more discoverable. I am very much not convinced that other users who have similar needs will think of [()].

Likewise, () is a clean placeholder for a sized ULE.

I still don't really see a use case for the unit type in the pure-ULE (ZeroVec) universe. I suspect there are similar sentinel use cases though. My preference is adding such a placeholder when we actually need one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-zerovec Component: Yoke, ZeroVec, DataBake
Projects
None yet
Development

No branches or pull requests

2 participants