ConstParamTy: require Eq as supertrait #116125
Conversation
|
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
| @@ -986,11 +986,16 @@ pub trait Tuple {} | |||
| pub trait PointerLike {} | |||
|
|
|||
| /// A marker for types which can be used as types of `const` generic parameters. | |||
| /// | |||
| /// These types must have a proper equivalence relation (`Eq`) and it must be automatically | |||
There was a problem hiding this comment.
Is this "just in case"? Like, there are no types that implement StructuralEq that don't also implement Eq, right, at least without a manual StructuralEq impl?
There was a problem hiding this comment.
Would be nice if you could add a test showing where possibly dubious code goes from pass -> err.
There was a problem hiding this comment.
Like, there are no types that implement StructuralEq that don't also implement Eq, right, at least without a manual StructuralEq impl?
Not quite.
A generic type with derive(Eq) will implement StructuralEq without any bounds on the generic parameters.
Is this "just in case"?
This is mostly to make the 2nd half of the comment coherent: we want to say that valtree equality agrees with ==. Without at least a PartialEq bound, that statement is ill-formed, since the type might not even have ==.
There was a problem hiding this comment.
Would be nice if you could add a test showing where possibly dubious code goes from pass -> err.
I don't know any such test (without a manual StructualEq impl), but currently the invariants that achieve that ConstParamTy implies Eq are rather subtle. This makes them a lot more obvious.
There was a problem hiding this comment.
currently the invariants that achieve that ConstParamTy implies Eq are rather subtle. This makes them a lot more obvious.
Yeah, ok, that's all I wanted to really know. This isn't necessarily for "correctness" (but perhaps only by accident do we imply the right thing due to recursively checking fields for ConstParamTy in impls).
There was a problem hiding this comment.
I also view this as preparation for removing the StructuralEq trait entirely (at which point only the StructuralPartialEq bound would be left otherwise).
I can do that all in the same PR if you prefer.
There was a problem hiding this comment.
Nah, I'll probably have more questions about behavioral changes on that PR anyways 😅
|
@bors r+ |
|
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (27b4eb9): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 630.463s -> 632.394s (0.31%) |
As discussed with @BoxyUwU on Zulip.
We want to say that valtree equality on const generic params agrees with
==, but that only makes sense if==actually exists, hence we should have an appropriate bound. Valtree equality is an equivalence relation, so such a type can always beEqand not justPartialEq.