fix(#141141): When expanding `PartialEq`, check equality of scalar types first. #141724

Sol-Ell · 2025-05-29T07:53:04Z

Now, cs_eq function of partial_eq.rs compares scalar types first.

Add is_scalar field to FieldInfo.
Add is_scalar method to TyKind.
Pass FieldInfo via CsFold::Combine and refactor code relying on it.
Implement TryFrom<&str> and TryFrom<Symbol> for FloatTy.
Implement TryFrom<&str> and TryFrom<Symbol> for IntTy.
Implement TryFrom<&str> and TryFrom<Symbol> for UintTy.

rustbot · 2025-05-29T07:53:09Z

rustbot has assigned @oli-obk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-05-29T14:25:49Z

Changes to the code generated for builtin derived traits.

cc @nnethercote

tests/ui/deriving/deriving-all-codegen.rs

tests/ui/deriving/deriving-all-codegen.stdout

nnethercote · 2025-05-30T09:38:36Z

tests/ui/deriving/deriving-all-codegen.stdout

+        self.b2 == other.b2 && self.b4 == other.b4 && self.b5 == other.b5 &&
+                            self.b6 == other.b6 && self.b7 == other.b7 &&
+                    self.b8 == other.b8 && self.b10 == other.b10 &&
+            (self.b1 == other.b1 && self.b3 == other.b3 && self._b == other._b


Can these parentheses be avoided? They're not necessary.

Parentheses are generated because the comparison expressions for scalar and compound fields are grouped and then combined in a single pass. As far as I understand, removing these parentheses would require iterating over the fields twice: once to collect all scalar-typed fields, and a second time to add the comparisons for compound-typed fields. Would making two passes over the fields be acceptable for this purpose?

Two passes sounds fine, the number of fields is always small so the performance effect should be negligible.

nnethercote · 2025-05-30T09:39:04Z

tests/ui/deriving/deriving-all-codegen.stdout

+                (ReorderEnum::G(__self_0, __self_1, __self_2),
+                    ReorderEnum::G(__arg1_0, __arg1_1, __arg1_2)) =>
+                    __self_1 == __arg1_1 &&
+                        (__self_0 == __arg1_0 && __self_2 == __arg1_2),


Can these parens be avoided?

compiler/rustc_ast/src/ast.rs

nnethercote

Looking good, just a few nits to fix. Thanks.

@rustbot author

nnethercote · 2025-06-02T00:38:01Z

compiler/rustc_ast/src/ast.rs

@@ -2452,6 +2452,32 @@ impl TyKind {
            None
        }
    }
+
+    pub fn maybe_scalar(&self) -> bool {
+        let Some(ty_kind) = self.is_simple_path() else {


ty_sym would be a better name than ty_kind. I was momentarily confused because I thought this would have TyKind.

nnethercote · 2025-06-02T00:40:15Z

compiler/rustc_ast/src/ast.rs

@@ -2452,6 +2452,32 @@ impl TyKind {
            None
        }
    }
+
+    pub fn maybe_scalar(&self) -> bool {


A brief comment explaining the maybe_ prefix would be good, i.e. that it's not perfectly accurate because of typedefs.

nnethercote · 2025-06-02T01:01:08Z

tests/ui/deriving/deriving-all-codegen.stdout

+    #[inline]
+    fn partial_cmp(&self, other: &Reorder)
+        -> ::core::option::Option<::core::cmp::Ordering> {
+        match ::core::cmp::PartialOrd::partial_cmp(&self.b1, &other.b1) {


Scalars are being checked first for eq. But partial_cmp is still using the old field ordering. Is this intentional?

And likewise for the enum case below.

Optimizing partial_cmp while keeping its guarantees would require a separate issue. This PR focuses on the PartialEq::eq.

nnethercote · 2025-06-02T10:40:05Z

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

+
+/// Generates the equality expression for a struct or enum variant when deriving `PartialEq`.
+///
+/// This function generates an expression that checks if all fields of a struct or enum variant are equal.


A few lines exceed 100 chars, please reflow the comments.

rustbot · 2025-06-02T10:47:36Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

…he order of declaration.

Sol-Ell · 2025-06-02T14:24:36Z

@rustbot ready

nnethercote · 2025-06-03T09:18:50Z

Thank you!

@bors r+

bors · 2025-06-03T09:18:53Z

📌 Commit a6a1c1b has been approved by nnethercote

It is now in the queue for this repository.

Rollup of 8 pull requests Successful merges: - #141724 (fix(#141141): When expanding `PartialEq`, check equality of scalar types first.) - #141833 (`tests/ui`: A New Order [2/N]) - #141861 (Switch `x86_64-msvc-{1,2}` back to Windows Server 2025 images) - #141914 (redesign stage 0 std follow-ups) - #141918 (Deconstruct values in the THIR visitor) - #141923 (Update books) - #141931 (Deconstruct values in the THIR visitor) - #141956 (Remove two trait methods from cg_ssa) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of #141724 - Sol-Ell:issue-141141-fix, r=nnethercote fix(#141141): When expanding `PartialEq`, check equality of scalar types first. Fixes #141141. Now, `cs_eq` function of `partial_eq.rs` compares [scalar types](https://doc.rust-lang.org/rust-by-example/primitives.html#scalar-types) first. - Add `is_scalar` field to `FieldInfo`. - Add `is_scalar` method to `TyKind`. - Pass `FieldInfo` via `CsFold::Combine` and refactor code relying on it. - Implement `TryFrom<&str>` and `TryFrom<Symbol>` for FloatTy. - Implement `TryFrom<&str>` and `TryFrom<Symbol>` for IntTy. - Implement `TryFrom<&str>` and `TryFrom<Symbol>` for UintTy.

rustbot assigned oli-obk May 29, 2025

rustbot added S-waiting-on-review T-compiler labels May 29, 2025

This comment has been minimized.

Sign in to view

Sol-Ell force-pushed the issue-141141-fix branch from 6e72625 to 3f53218 Compare May 29, 2025 14:25

Sol-Ell force-pushed the issue-141141-fix branch from 3f53218 to e640d3d Compare May 29, 2025 14:31

nnethercote reviewed May 29, 2025

View reviewed changes

tests/ui/deriving/deriving-all-codegen.rs Outdated Show resolved Hide resolved

tests/ui/deriving/deriving-all-codegen.stdout Outdated Show resolved Hide resolved

Sol-Ell force-pushed the issue-141141-fix branch from e640d3d to 843083a Compare May 30, 2025 09:09

nnethercote reviewed May 30, 2025

View reviewed changes

oli-obk assigned nnethercote and unassigned oli-obk May 30, 2025

Sol-Ell force-pushed the issue-141141-fix branch from 843083a to 12c73a9 Compare May 30, 2025 15:34

nnethercote added S-waiting-on-author and removed S-waiting-on-review labels May 30, 2025

workingjubilee reviewed May 30, 2025

View reviewed changes

compiler/rustc_ast/src/ast.rs Outdated Show resolved Hide resolved

Sol-Ell force-pushed the issue-141141-fix branch from 12c73a9 to 235b472 Compare May 31, 2025 09:25

nnethercote reviewed Jun 2, 2025

View reviewed changes

Separately check equality of the scalar types and compound types in t…

a6a1c1b

…he order of declaration.

Sol-Ell force-pushed the issue-141141-fix branch from 235b472 to a6a1c1b Compare June 2, 2025 12:33

rustbot added S-waiting-on-review and removed S-waiting-on-author labels Jun 2, 2025

bors added S-waiting-on-bors and removed S-waiting-on-review labels Jun 3, 2025

matthiaskrgr mentioned this pull request Jun 3, 2025

Rollup of 8 pull requests #141961

Merged

bors merged commit 1943766 into rust-lang:master Jun 3, 2025
10 checks passed

rustbot added this to the 1.89.0 milestone Jun 3, 2025

Sol-Ell deleted the issue-141141-fix branch June 5, 2025 09:44

fix(#141141): When expanding PartialEq, check equality of scalar types first. #141724

fix(#141141): When expanding PartialEq, check equality of scalar types first. #141724

Uh oh!

Conversation

Sol-Ell commented May 29, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented May 29, 2025

Uh oh!

This comment has been minimized.

rustbot commented May 29, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nnethercote left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

Sol-Ell commented Jun 2, 2025

Uh oh!

nnethercote commented Jun 3, 2025

Uh oh!

bors commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

fix(#141141): When expanding `PartialEq`, check equality of scalar types first. #141724

fix(#141141): When expanding `PartialEq`, check equality of scalar types first. #141724

Sol-Ell commented May 29, 2025 •

edited by rustbot

Loading