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.

[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.

[KiChjang]

Hmm, so it's apparently a feature gated behind const_block_items in #149226. I'm also curious as to why it's necessary to generate a new ItemKind specifically for it when it's equivalent to const _: () = const { ... } in all cases and purposes. Will change to use the const item instead.

[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).

[nnethercote]

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.

The whole point of this PR is to eliminate a hidden name. Let's not introduce a new hidden name, even if it is unstable.

[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.

The whole point of this PR is to eliminate a hidden name. Let's not introduce a new hidden name, even if it is unstable.

Idk., I think there's a difference between a hidden name on Eq, and a hidden type somewhere else. But I agree that if we can find some other solution, then that's preferable.

Details

Ideally we'd be able to do this:

struct Foo;
impl Foo {
    const _: () = {
        // reference `Self` here.
    };
}

But that's probably a bit far off, see rust-lang/rfcs#3527.

[KiChjang]

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

This was what I had in an earlier revision, but it was not desirable as mentioned by @nnethercote since it still introduces a new name.

I am thinking however that maybe we could instead make all assert bindings be associated const bindings instead? E.g.

struct Foo(u32);
impl Foo {
    const _: ::core::cmp::AssertParamIsEq<u32> = ::core::cmp::AssertParamIsEq { _field: PhantomData };
}

[nnethercote]

I am thinking however that maybe we could instead make all assert bindings be associated const bindings instead? E.g.

struct Foo(u32);
impl Foo {
    const _: ::core::cmp::AssertParamIsEq<u32> = ::core::cmp::AssertParamIsEq { _field: PhantomData };
}

Sounds promising! It's anonymous, and uses impl Foo so you can reference Self which makes things simpler. It's a shame the ::core::cmp::AssertParamIsEq is repeated. I guess that's because consts must be assigned, unlike lets?

(Thanks for your patience on this, BTW. It has turned out more complicated than it first appeared.)

[theemathas]

@KiChjang @nnethercote That won't work. It seems like associated consts must have a proper name:

struct Foo;
impl Foo {
    const _: i32 = 1;
}

error: `const` items in this context need a name
 --> src/lib.rs:3:11
  |
3 |     const _: i32 = 1;
  |           ^ `_` is not a valid name for this `const` item

[KiChjang]

Ah crud, I experimented around and consulted the Rust lang reference a couple of times, and it looks like at this point in time, Rust does not allow ANY associated items in an impl block to be anonymous (i.e. underscore).

This means we can't avoid introducing a new name if we go the impl block route -- we MUST use a const item block instead, but that then makes the Self type unparseable if it is generated from a macro.

I now wonder if there's a way for us to tell the compiler to relax some restrictions around naming for associated items in impl blocks if the code in question is generated from an compiler builtin macro...

But if we are going down this route, I'm also wondering whether it is ok to special case this for the aristocratic and special builtin derive macros? Maybe the better way is to instead open an RFC allowing for anonymous associated items?

[madsmtm]

But if we are going down this route, I'm also wondering whether it is ok to special case this for the aristocratic and special builtin derive macros? Maybe the better way is to instead open an RFC allowing for anonymous associated items?

One exists: rust-lang/rfcs#3527.