-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Fix syntax in -Zunpretty-expanded output for derived PartialEq.
#107488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Why not just parenthesize the comparison like ({ a } == { b })?
I was about to ask, what's the invalid syntax? Sounds like some disambiguation issue?
The syntax is invalid because the parser will parse { a } == { b } in statement-position as:
// One statement
{ a }
// Second statement, invalid beginning of expression
== { b }
Why not just parenthesize the comparison like
({ a } == { b })?
That would also work, but I don't think it's clearly better, and using &{ a } == &{ b } fits more naturally within the existing code.
fits more naturally within the existing code.
Makes sense I guess. Seems like it just needs a review to make sure the correctness with packed structs is preserved with these & (.. it should be, right? these are new temporaries cause of the { .. }?).
Yes, the { .. } causes copying into temporaries, so the fields need to impl Copy, which was just made official by #104429.
And note that
&{ .. }is already used for other built-in derives, e.g. Hash and Ord.
Yeah but those don't auto-ref, right? == always has an implicit ==, regular function arguments do not. There is a comment near the code you changed saying error messages are better when we can avoid the extra &.
Does ({ ... }) == ({ ... }) work?
With this change, the optimizer has to additionally inline this function
If you do `derive(PartialEq)` on a packed struct, the output shown by
`-Zunpretty=expanded` includes expressions like this:
```
{ self.x } == { other.x }
```
This is invalid syntax. This doesn't break compilation, because the AST
nodes are constructed within the compiler. But it does mean anyone using
`-Zunpretty=expanded` output as a guide for hand-written impls could get
a nasty surprise.
This commit fixes things by instead using this form:
```
({ self.x }) == ({ other.x })
```
249370e to
75e87d1
Compare
Does
({ ... }) == ({ ... })work?
It does. I have updated to use that instead.
I'm not very familiar with the AST, but this looks plausible and reasonably simple. Thanks!
@bors r+
⌛ Testing commit 75e87d1 with merge 781b7441c40cea3edb07749cd65a788daf865afb...
💔 Test failed - checks-actions
@bors retry CI was broken
@bors rollup=always
...r=RalfJung
Fix syntax in `-Zunpretty-expanded` output for derived `PartialEq`.
If you do `derive(PartialEq)` on a packed struct, the output shown by `-Zunpretty=expanded` includes expressions like this:
```
{ self.x } == { other.x }
```
This is invalid syntax. This doesn't break compilation, because the AST nodes are constructed within the compiler. But it does mean anyone using `-Zunpretty=expanded` output as a guide for hand-written impls could get a nasty surprise.
This commit fixes things by instead using this form:
```
({ self.x }) == ({ other.x })
```
r? `@RalfJung`
...iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#107201 (Remove confusing 'while checking' note from opaque future type mismatches) - rust-lang#107312 (Add Style Guide rules for let-else statements) - rust-lang#107488 (Fix syntax in `-Zunpretty-expanded` output for derived `PartialEq`.) - rust-lang#107531 (Inline CSS background images directly into the CSS) - rust-lang#107576 (Add proc-macro boilerplate to crt-static test) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Uh oh!
There was an error while loading. Please reload this page.
If you do
derive(PartialEq)on a packed struct, the output shown by-Zunpretty=expandedincludes expressions like this:This is invalid syntax. This doesn't break compilation, because the AST nodes are constructed within the compiler. But it does mean anyone using
-Zunpretty=expandedoutput as a guide for hand-written impls could get a nasty surprise.This commit fixes things by instead using this form:
r? @RalfJung