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

StructuralEq trait docs are outdated #115881

Closed
RalfJung opened this issue Sep 15, 2023 · 4 comments · Fixed by #116167
Closed

StructuralEq trait docs are outdated #115881

RalfJung opened this issue Sep 15, 2023 · 4 comments · Fixed by #116167
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 15, 2023

The StructuralEq trait docs are saying

/// In a more ideal world, we could check that requirement by just checking that
/// the given type implements both the `StructuralPartialEq` trait *and*
/// the `Eq` trait. However, you can have ADTs that *do* `derive(PartialEq, Eq)`,
/// and be a case that we want the compiler to accept, and yet the constant's
/// type fails to implement `Eq`.
///
/// Namely, a case like this:
///
/// ```rust
/// #[derive(PartialEq, Eq)]
/// struct Wrap<X>(X);
///
/// fn higher_order(_: &()) { }
///
/// const CFN: Wrap<fn(&())> = Wrap(higher_order);
///
/// fn main() {
///     match CFN {
///         CFN => {}
///         _ => {}
///     }
/// }
/// ```
///
/// (The problem in the above code is that `Wrap<fn(&())>` does not implement
/// `PartialEq`, nor `Eq`, because `for<'a> fn(&'a _)` does not implement those
/// traits.)
///
/// Therefore, we cannot rely on naive check for `StructuralPartialEq` and
/// mere `Eq`.

However, with the FnPtr trait, fn(&()) does implement Eq and PartialEq.

Is the StructuralEq trait still necessary? Eq has no methods, so if PartialEq is structural then there's no way that Eq is not, right?

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 15, 2023
@RalfJung
Copy link
Member Author

Cc @rust-lang/types

@saethlin saethlin added T-types Relevant to the types team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 15, 2023
@RalfJung
Copy link
Member Author

Also I don't think the current consensus is that we should check matched constants to be Eq. #84045 got rejected, which is a fairly clear sign that PartialEq is sufficient for matching and everything else can be handled with lints.

So I don't think StructuralEq still serves any purpose.

@nikomatsakis
Copy link
Contributor

I would be very happy to settle this story once and for all. I still feel a bit confused about our plan here, though I recall thinking @RalfJung that your proposals made sense to me.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 16, 2023

I think my prior proposals don't entirely work any more with things like #67343 or code like this -- we have really honed in on StructuralPartialEq being a shallow property, not a statement about being able to create valtrees. We also have ConstParamTy as a separate trait now; I think that might be closer to what I originally imagined.

On the plus side, I seem to recall @lcnr was set on keeping StructuralPartialEq shallow; so maybe it's easier to come to an agreement now that I've come around to accepting that. :)

Also see this Zulip thread I just started.

bors added a commit to rust-lang-ci/rust that referenced this issue Jan 25, 2024
remove StructuralEq trait

The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.

One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust rust-lang#115893 to check for `Eq`, and rule out float matching for good.

Fixes rust-lang#115881
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 25, 2024
remove StructuralEq trait

The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.

One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust rust-lang#115893 to check for `Eq`, and rule out float matching for good.

Fixes rust-lang#115881
@bors bors closed this as completed in dd2559e Jan 26, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Jan 26, 2024
remove StructuralEq trait

The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.

One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust rust-lang/rust#115893 to check for `Eq`, and rule out float matching for good.

Fixes rust-lang/rust#115881
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this issue Jan 27, 2024
remove StructuralEq trait

The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.

One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust rust-lang/rust#115893 to check for `Eq`, and rule out float matching for good.

Fixes rust-lang/rust#115881
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Feb 8, 2024
remove StructuralEq trait

The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.

One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust rust-lang/rust#115893 to check for `Eq`, and rule out float matching for good.

Fixes rust-lang/rust#115881
GuillaumeGomez pushed a commit to GuillaumeGomez/rustc_codegen_gcc that referenced this issue Feb 15, 2024
remove StructuralEq trait

The documentation given for the trait is outdated: *all* function pointers implement `PartialEq` and `Eq` these days. So the `StructuralEq` trait doesn't really seem to have any reason to exist any more.

One side-effect of this PR is that we allow matching on some consts that do not implement `Eq`. However, we already allowed matching on floats and consts containing floats, so this is not new, it is just allowed in more cases now. IMO it makes no sense at all to allow float matching but also sometimes require an `Eq` instance. If we want to require `Eq` we should adjust rust-lang/rust#115893 to check for `Eq`, and rule out float matching for good.

Fixes rust-lang/rust#115881
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants