Change derive macro for Eq to not impl Eq trait fn

[KiChjang]

View all comments

Fixes #152504.

r? @cyrgani

[rustbot]

Changes to the code generated for builtin derived traits.

cc @nnethercote

[theemathas]

A test case that seems like might break with this:

trait Trait<T> {}

#[derive(PartialEq, Eq)]
struct Thing<T: Trait<Self>>(T);

[theemathas]

A less exotic test case:

#[derive(PartialEq, Eq)]
struct Thing(Option<Box<Self>>);

[Kivooeo]

Nick, I would reassign to you (feel free to reroll) since @cyrgani is a triage member and can't approve compiler changes

r? nnethercote

[KiChjang]

A less exotic test case:

#[derive(PartialEq, Eq)]
struct Thing(Option<Box<Self>>);

Gah, because we're now generating a const item instead of a trait impl block, we'd need to rewrite the Self reference to its concrete type, but this is a bit tough as it would require us to supply the correct generic params to the Self concrete type if it has any.

A better solution now would be to emit the following:

impl<...> Type<...> {
    const fn assert_fields_are_eq() {
        let _: ::core::cmp::AssertParamIsEq<Option<Box<Self>>>;
    }
}

This would side-step the issue of rewriting Self into its concrete type.

[KiChjang]

Looks like CI is passing now, r? @nnethercote

[rustbot]

Requested reviewer is already assigned to this pull request.

Please choose another assignee.

[cyrgani]

You should be able to delete the fn assert_fields_are_eq(&self) {} method from the Eq trait again now.

[cyrgani]

Could you also add a test that something like

fn main() {
    X::assert_fields_are_eq();
}
#[derive(PartialEq, Eq)]
struct X(u8);

will not compile?

[nnethercote]

This is now completely different to the original idea of using a const. I'm no lang expert, but injecting X::assert_fields_are_eq doesn't seem acceptable to me.

Beyond that, I've only skimmed the PR so far. I think the 8 commits should be squashed down to 1 or 2. I'm also surprised at how much extra code is required in eq.rs to generate a slight variation on what is currently generated.

View changes since this review

[KiChjang]

Could you also add a test that something like

fn main() {
    X::assert_fields_are_eq();
}
#[derive(PartialEq, Eq)]
struct X(u8);

will not compile?

This probably would still compile, since what I essentially did was move the trait method to be under an inherent impl instead, which is probably why it isn't desirable.

Given so, I'd probably need to revert to the original idea of using a const item block, however the const fn would still have to stay the same because I'd still need to somehow bring all lifetime/type parameters into scope, and if we don't use an impl, then a fn is the minimal vehicle to do so.

[nnethercote]

This probably would still compile, since what I essentially did was move the trait method to be under an inherent impl instead, which is probably why it isn't desirable.

That's right. Currently we have a hidden method in Eq, which users can (intentionally or unintentionally) interact with. With an inherent method that exposure is increased. With an anonymous const it becomes impossible for users to interact with it.

[madsmtm]

Did you explore what I wrote briefly in #149978 (comment), to do something like:

#[derive(PartialEq, Eq)]
pub struct A<'a, T> {
    field1: &'a [T],
    field2: Option<Box<Self>>,
}

// Generates

const _: () = {
    struct __AssertIsEq<'a, T: core::eq::Eq> {
        field1: core::eq::AssertParamIsEq<&'a [T]>,
        field2: core::eq::AssertParamIsEq<Option<Box<Self>>>,
    }
    impl<'a, T: core::eq::PartialEq> PartialEq for __AssertIsEq<'a, T> {
        #[inline]
        fn eq(&self, other: &Self) -> bool { unimplemented!() }
    }
    impl<'a, T: core::eq::Eq> Eq for __AssertIsEq<'a, T> {}
};

Also, this is apparently perf-sensitive, please remember to do a perf-run before merging.

View changes since this review

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[KiChjang]

@bors try @rust-timer queue

[rust-bors]

@KiChjang: 🔑 Insufficient privileges: not in try users

[rust-timer]

Insufficient permissions to issue commands to rust-timer.

[KiChjang]

Did you explore what I wrote briefly in #149978 (comment), to do something like:

#[derive(PartialEq, Eq)]
pub struct A<'a, T> {
    field1: &'a [T],
    field2: Option<Box<Self>>,
}

// Generates

const _: () = {
    struct __AssertIsEq<'a, T: core::eq::Eq> {
        field1: core::eq::AssertParamIsEq<&'a [T]>,
        field2: core::eq::AssertParamIsEq<Option<Box<Self>>>,
    }
    impl<'a, T: core::eq::PartialEq> PartialEq for __AssertIsEq<'a, T> {
        #[inline]
        fn eq(&self, other: &Self) -> bool { unimplemented!() }
    }
    impl<'a, T: core::eq::Eq> Eq for __AssertIsEq<'a, T> {}
};

Also, this is apparently perf-sensitive, please remember to do a perf-run before merging.

View changes since this review

This seems to emit quite a lot of boilerplate for the purpose of making sure that AssertParamIsEq is being properly typechecked, so I opted for a const fn approach. I may be wrong though -- let's use benchmarking to test out whether I'm putting in more overhead or not.

Would appreciate some help in putting this on the timer queue though as I don't have sufficient permissions.

[cyrgani]

This isn't even valid syntax, it should be const _: () = {.

[KiChjang]

This is the code that gets generated when I tell the compiler to emit a ast::ItemKind::ConstBlock:

rust/compiler/rustc_ast/src/ast.rs

Lines 3955 to 3959 in 8d50bcc

	/// A module-level const block.
	/// Equivalent to `const _: () = const { ... };`.
	///
	/// E.g., `const { assert!(true) }`.
	ConstBlock(ConstBlockItem),

[nnethercote]

Weird. Maybe a pretty printing issue? Would be good to understand what's happening here.

[nnethercote]

Please put blank lines between the methods, here and elsewhere.

[nnethercote]

Blank line after the struct, and likewise below.

[nnethercote]

Also, the three visitors need a brief comment explaining what they are doing and why.

[nnethercote]

A brief example of the kind of code being generated would be helpful here.

[nnethercote]

I assume the parameter to assert_fields_are_eq is necessary in the generic case to make the trait bounds work? But could the parameter be avoided for non-generic functions like this one?

[nnethercote]

Also, why is the parameter (when present) *const T? Could it just be T?

[madsmtm]

Also, why is the parameter (when present) *const T? Could it just be T?

Probably to work with ?Sized types?

[madsmtm]

Another option to make it work properly with Self, while not doing an inherent method:

Add an extra trait with the method on it instead.

// `#[doc(hidden)]` trait (and unstable if we can?)
impl ::core::cmp::EqHelperTrait for $ty {
    #[inline]
    fn assert_fields_are_eq() {
        let _: ::core::cmp::AssertParamIsEq<$param1>;
        let _: ::core::cmp::AssertParamIsEq<$param2>;
        // ...
    }
}

View changes since this review

[madsmtm]

Please add a test case like this:

macro_rules! get_self {
    () => {
        Self
    };
}

#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Default)]
struct SelfFromMacro {
    this: Option<Box<get_self!()>>
}

(I'm commenting this specifically because I think that any solution that matches on the token Self isn't gonna work).