Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix syntax in -Zunpretty-expanded output for derived PartialEq. #107488

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Fix syntax in -Zunpretty-expanded output for derived PartialEq. #107488

merged 1 commit into from
Feb 2, 2023

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jan 30, 2023 •
edited
Loading

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 30, 2023
@compiler-errors
Copy link
Member

Why not just parenthesize the comparison like ({ a } == { b })?

@RalfJung
Copy link
Member

I was about to ask, what's the invalid syntax? Sounds like some disambiguation issue?

@compiler-errors
Copy link
Member

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 }

@nnethercote
Copy link
Contributor Author

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.

@compiler-errors
Copy link
Member

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 { .. }?).

@nnethercote
Copy link
Contributor Author

Yes, the { .. } causes copying into temporaries, so the fields need to impl Copy, which was just made official by #104429.

@nnethercote
Copy link
Contributor Author

And note that &{ .. } is already used for other built-in derives, e.g. Hash and Ord.

@RalfJung
Copy link
Member

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?

@kadiwa4
Copy link
Contributor

kadiwa4 commented Jan 31, 2023

With this change, the optimizer has to additionally inline this function

@nnethercote
Fix syntax in -Zunpretty-expanded output for derived PartialEq.
75e87d1 
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 })
```
@nnethercote
Copy link
Contributor Author

Does ({ ... }) == ({ ... }) work?

It does. I have updated to use that instead.

@RalfJung
Copy link
Member

RalfJung commented Feb 1, 2023

I'm not very familiar with the AST, but this looks plausible and reasonably simple. Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 1, 2023

📌 Commit 75e87d1 has been approved by RalfJung

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2023
@bors
Copy link
Contributor

bors commented Feb 1, 2023

⌛ Testing commit 75e87d1 with merge 781b7441c40cea3edb07749cd65a788daf865afb...

@bors
Copy link
Contributor

bors commented Feb 1, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 1, 2023
@compiler-errors
Copy link
Member

@bors retry CI was broken

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2023
@nnethercote
Copy link
Contributor Author

@bors rollup=always

compiler-errors added a commit to compiler-errors/rust that referenced this pull request Feb 2, 2023
@compiler-errors
Rollup merge of rust-lang#107488 - nnethercote:fix-PartialEq-syntax, …
191ee56 
…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`
@compiler-errors compiler-errors mentioned this pull request Feb 2, 2023
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 2, 2023
Merged
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 2, 2023
@bors
Auto merge of rust-lang#107584 - matthiaskrgr:rollup-vav4ljz, r=matth…
a9985cf 
…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
@bors bors merged commit 150b9d7 into rust-lang:master Feb 2, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 2, 2023
@nnethercote nnethercote deleted the fix-PartialEq-syntax branch February 2, 2023 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @compiler-errors @RalfJung @kadiwa4 @bors @rust-log-analyzer @rustbot