Skip to content

Reimplement and complete spanless_eq for the AST - #15599

Open
Jarcho wants to merge 2 commits into
rust-lang:masterfrom
Jarcho:ast_spanless_eq
Open

Reimplement and complete spanless_eq for the AST#15599
Jarcho wants to merge 2 commits into
rust-lang:masterfrom
Jarcho:ast_spanless_eq

Conversation

@Jarcho

@Jarcho Jarcho commented Aug 31, 2025

Copy link
Copy Markdown
Contributor

This replaces the many exported functions that used to exist with a single spanless_eq function. The macros implementing this would be derives, but that's not possible so nearly copying the struct/enum definitions is required.

The new implementation has full exhaustiveness checking on both fields and variants. This way any changes made to the AST in rustc will require changes here.

changelog: none

@rustbot

rustbot commented Aug 31, 2025

Copy link
Copy Markdown
Collaborator

r? @Alexendoo

rustbot has assigned @Alexendoo.
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 rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 31, 2025
Comment thread clippy_utils/src/ast/mod.rs Outdated
Comment thread clippy_utils/src/ast/spanless.rs Outdated
Comment on lines +547 to +577
$(
(
$name::$vname { $($fname: $lname),*},
$name::$vname { $($fname: $rname),*}
) => {
$(on_unequal!(@$unequal return false);)?
$(eq_enum_field!($(@$unordered)? cx $lname $rname) &&)* true
},
)*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use ${concat} here to allow a similar syntax to the struct definitions?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into hygiene issues when I tried it the first time, but it might be a thing I can work around.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried some other options. The feature is just not implemented enough currently to work.

@Jarcho Jarcho Sep 3, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concat doesn't work in nested repetitions and pushing the concat into an inner macro means the generated identifier can only the inner macro can access the name due to hygiene rules. Basically neither of these work:

// with repetitions: ice
$(
  (
    $var { $($name: ${concat(l, $name)}),*  },
    $var { $($name: ${concat(r, $name)}),*  },
  ) => $(eq!(${concat(l, $name)} ${concat(r, $name)}))&&*,
)*

// with inner: binding names can't be used outside the `name` macro
$(
  (
    $var { $($name: name!(l, $name)),*  },
    $var { $($name: name!(r, $name)),*  },
  ) => $(eq!($name))&&*,
)*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could accept it as is then, but I'm worried the syntax will be confusing for rustc contributions

A temporary paste dependency could also work, it's a pretty small dependency at least

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little strange, but anytime you have to change things there are a huge pile of examples to look at right next to where you need to make changes. If those plus the doc comments on the macros aren't enough the small syntax change isn't going to make the difference.

A comment at the top of the file about where to add new things might be helpful though.

@rustbot

rustbot commented Oct 4, 2025

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (possibly #15773) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants