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.

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

Fix tt::Punct's spacing calculation #13548

Merged
merged 4 commits into from
Nov 11, 2022
Merged

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Nov 5, 2022

Fixes #13499

We currently set a tt::Punct's spacing to Spacing::Joint unless its next token is a trivia (i.e. whitespaces or comment). As I understand it, rustc only sets Spacing::Joint if the next token is an operator and we should follow it to guarantee the consistent behavior of proc macros.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2022
@@ -488,6 +484,7 @@ fn foo () {a .__ra_fixup ;}
}

#[test]
#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two tests fail in this assertion. This is because dummy ident insertion (to fix up the syntax error) changes the spacing of puncts before it with this patch. Although technically we can record and reverse it to preserve the spacing of the puncts in invalid syntax, it'd be somewhat complex and I'm wondering how essential this invariant to preserve the original tokentree is w.r.t. puncts' spacing.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, it seems odd to me that we check if spacing is preserved, macros already don't care about spacing so I don't think the tests should either, not sure how simple it'd would be but can we just check the tokens for equality ignoring any whitespace tokens in between?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's what I was thinking. I'll see how I can "fix" the tests without complicating much.

cc @flodiebold for you're the original author (#11444), if you have anything to add.

@lowr lowr marked this pull request as ready for review November 10, 2022 10:43
@Veykril
Copy link
Member

Veykril commented Nov 11, 2022

We really ought to look into fixing that damn syntax highlighting test that keeps failing randomly (though maybe we are slightly quadratic and just dancing on a thin line there)

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 11, 2022

📌 Commit 5b07061 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Nov 11, 2022

⌛ Testing commit 5b07061 with merge 6f313ce...

@bors
Copy link
Collaborator

bors commented Nov 11, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 6f313ce to master...

@bors bors merged commit 6f313ce into rust-lang:master Nov 11, 2022
bors added a commit that referenced this pull request Nov 16, 2022
internal: Update proc-macro-srv tests

Should have been included in #13548, but I didn't notice as those tests aren't run in our CI.

cc rust-lang/rust#104454
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.

Invalid macro expansion when async-trait and duplicate_item macros are used together
4 participants