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.

[cyrgani]

I've built the compiler with this PR's changes applied and the code I posted above still (correctly) does not compile.(probably due to some hygiene rules that make the function name unaccessible outside the #[derive(Eq)] invocation). So this does not seem to be a problem?

[KiChjang]

Ah, it's likely due to visibility issues -- it's not accessible publicly, but if you call it within an impl block for the type, then it would compile. Example:

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

I have local changes that revert the emitted code to be using a const impl block with a const fn inside of it, however some UI test expectations need to be updated as a result of that change. Will push as soon as I get around to fix them.

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