Require VarULE concrete fns to behave the same as trait fns

[sffc]

I was wanting to call some of these fns in a const context, which I can do if they are concrete fns instead of trait fns, at least until rust-lang/rust#143874 lands. WDYT about adding the safety requirement in this PR?

I updated all of the concrete impls, but not sure what to do about the derive impls.

[Manishearth]

I don't think this is an appropriate use of a safety invariant, and we explicitly violate it in a few cases so the invariant as written is simply wrong.

Users of this trait in contexts where the invariants are relevant should be careful when calling functions in non-generic contexts that the dispatch works correctly; this is part and parcel of writing correct unsafe code and not something we need to cover explicitly.

I also don't see what this achieves. Correct me if I'm wrong: Is your goal to be able to call the "concrete const" versions of these functions and assume they have the same behavior? If so I think it makes more sense to document a "safety usable invariant" on the concrete functions where you say "can be guaranteed to have identical behavior to <Self as VarULE>::foo().

[sffc]

we explicitly violate it in a few cases

Citation?

...
I also don't see what this achieves
...

Did you see the latest change in #7394?

I have a macro_rules that calls concrete functions (since that's all you can do in const), requires a trait bound to be satisfied, and uses that trait bound to justify a generated unsafe block. It makes the macro 100% safe to call.

You expressed openness (but distaste) to using trait safety requirements to require certain behavior on concrete functions in a similar context previously: #6576 (comment)

[Manishearth]

Citation?

Sorry, I misunderstood what the invariant was, I mistakenly read "There are no concrete methods with the same name as
VarULE trait methods" as the invariant rather than as a safety comment.

Did you see the latest change in #7394?

Ah. I hadn't realized that was that how that worked.

---

Now that I'm up to speed, I think there are three reasons why I don't want to do this:

1. This is a strange safety requirement on an already complicated trait. This is not typically how safety requirements work, and as such this makes it much trickier to review. Good safety requirements can be verified locally.
2. This is a safety requirement relevant to all implementors, not just implementors who wish to use the macro.
3. There's a problem with generic impls, and with traits

With generic impls, as a safety requirement it is simply impossible to uphold in general. You brush against this problem here:

//  8. There are no methods generically on [T] that have the same name as the VarULE functions.
//     However, there are some functions on specific [T] that have the same name:
//     - [AsciiChar]::as_bytes() has the correct behavior.
//     - [MaybeUninit<T>]::as_bytes() has the correct behavior, though the return value is different.

These are not impls under our control. Fortunately, it is often okay to make stronger safety claims about stdlib impls even if they themselves do not explicitly have such guarantees¹. Unfortunately, trait impls can work around this.

Fortunately, trait impls can't be const. Unfortunately, that's a temporary restriction and can be opted out of in nightly.

Note that the the real tricky thing is that methods can be brought in scope at the macro call site, this isn't something local to the crate with the impl.

While reasoning about dispatch is important for reviewing unsafe macros, reasoning this carefully about dispatch is something unsafe reviewers will not wish to do. It took me a while to work out the full implications here and I'm pretty sure I've missed some things. This is stuff that people have to think about for every impl they see.

I would recommend against trying to rebuild the framework of unsafe trait via carefully spread out safety comments. This, to me, is a strong motivator for real const support in traits.

---

You noted correctly that I was begrudgingly okay with this in #6576. I think there are two things that make this different from that case.

Firstly: that requires an inherent associated unsafe method. Dispatch isn't ambiguous: that function will always be called by the generated code, since inherent methods are preferred over trait methods. As far as unsafe review is concerned, it's a strange, nonstandard, invariant that requires some looking around to verify, but you either have the method or you don't, you don't have to think about dispatch at all.

Whereas here, because this is about an optional function, it's impossible in general to be sure that such functions aren't being introduced indirectly.

Secondly, that scales with its use case: regular users of Bake are infected by the strange safety requirement, only the users of CustomBakeUnsafe.

Whereas here, this invariant affects each and every implementor of VarULE. This is objection no. 2 above.

As such, a way to make this more aligned with #6576 would be to add some sort of unsafe trait ConstUsableVarULE trait that requires certain inherent functions with identical behavior to VarULE. This solves the dispatch problem, and also solves objection no 2.

This is still a strange invariant and I'm not sure it pulls its weight for the desired functionality here, but that would address the much more strongly blocking concerns.

Footnotes

1. That said, this is often a matter of an unsafe reviewer's taste. There's a large amount of variance in what an unsafe reviewer may or may not accept here. ↩

[sffc]

Putting this on a separate trait is okay with me. I considered both designs and made a guess that you would prefer the simplicity of adding a mostly-neutral 8th requirement to the list of 7 over adding Yet Another Trait to the zerovec crate.

I'm not aware of a call site in Rust where an as_bytes() function would be invoked that isn't either the concrete one or the trait one, even after const trait functions are finally available.

I'll wait for a conclusion on #7394 before investing more time in this PR

[Manishearth]

Yeah, so to be clear I'm also not in favor of adding Yet Another Trait, but the current design with the new invariant has very strongly motivated safety-related concerns, whereas that is more of a crate vision situation and subjective in nature.