Skip to content

rustc: Fix joint-ness of stringified token-streams #50838

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

Merged
merged 1 commit into from
May 22, 2018

Conversation

alexcrichton
Copy link
Member

This commit fixes StringReader's parsing of tokens which have been stringified
through procedural macros. Whether or not a token tree is joint is defined by
span information, but when working with procedural macros these spans are often
dummy and/or overridden which means that they end up considering all operators
joint if they can!

The fix here is to track the raw source span as opposed to the overridden span.
With this information we can more accurately classify Punct structs as either
joint or not.

Closes #50700

@rust-highfive
Copy link
Contributor

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2018
@eddyb
Copy link
Member

eddyb commented May 17, 2018

@bors r+

@bors
Copy link
Collaborator

bors commented May 17, 2018

📌 Commit dda8797 has been approved by eddyb

@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 May 17, 2018
@bors
Copy link
Collaborator

bors commented May 18, 2018

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 18, 2018
This commit fixes `StringReader`'s parsing of tokens which have been stringified
through procedural macros. Whether or not a token tree is joint is defined by
span information, but when working with procedural macros these spans are often
dummy and/or overridden which means that they end up considering all operators
joint if they can!

The fix here is to track the raw source span as opposed to the overridden span.
With this information we can more accurately classify `Punct` structs as either
joint or not.

Closes rust-lang#50700
@alexcrichton
Copy link
Member Author

@bors: r=eddyb

@bors
Copy link
Collaborator

bors commented May 21, 2018

📌 Commit 0ee031a has been approved by eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 21, 2018
@bors
Copy link
Collaborator

bors commented May 22, 2018

⌛ Testing commit 0ee031a with merge 6835748...

bors added a commit that referenced this pull request May 22, 2018
rustc: Fix joint-ness of stringified token-streams

This commit fixes `StringReader`'s parsing of tokens which have been stringified
through procedural macros. Whether or not a token tree is joint is defined by
span information, but when working with procedural macros these spans are often
dummy and/or overridden which means that they end up considering all operators
joint if they can!

The fix here is to track the raw source span as opposed to the overridden span.
With this information we can more accurately classify `Punct` structs as either
joint or not.

Closes #50700
@bors
Copy link
Collaborator

bors commented May 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 6835748 to master...

@bors bors merged commit 0ee031a into rust-lang:master May 22, 2018
@alexcrichton alexcrichton deleted the token-impls branch June 29, 2018 19:46
@@ -121,6 +132,7 @@ impl<'a> StringReader<'a> {
sp: self.peek_span,
};
self.advance_token()?;
self.span_src_raw = self.peek_span_src_raw;
Copy link
Member

@matklad matklad May 13, 2019

Choose a reason for hiding this comment

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

Heh, I've just spend like an hour perusing this line, and I came to the conclusion that:

  • this code is buggy
  • this code is observationaly correct

The code is buggy because, while span and token point to this token, span_src points to the next token. In other words, span_src_raw and peek_span_src_raw are just equal all the time, because, first advance_token sets peek_span_src_raw, and then we assign peek_span_src_raw to span_src_raw.

However, this bug does not show up, because we only use peek span to check for jointness. And if spans A and B are adjacent, than spans following A and B are adjacent as well.

In other words, for ++ we don't check that first + is adjacent to the next, we check that the token following the first + (the second +) is adjacent to the token, following the second + (whitespace).

Similarly, for + + we actually check that whitespace following + is not adjacent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tokens in an attribute macro receive wrong spacing
5 participants