-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix tt::Punct
's spacing calculation
#13548
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
crates/hir-expand/src/fixup.rs
Outdated
@@ -488,6 +484,7 @@ fn foo () {a .__ra_fixup ;} | |||
} | |||
|
|||
#[test] | |||
#[ignore] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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+ |
☀️ Test successful - checks-actions |
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
Fixes #13499
We currently set a
tt::Punct
's spacing toSpacing::Joint
unless its next token is a trivia (i.e. whitespaces or comment). As I understand it, rustc only setsSpacing::Joint
if the next token is an operator and we should follow it to guarantee the consistent behavior of proc macros.