-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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(parse): avoid panic when cfg wrapper by bracket under capture-cfg
mode
#113056
Conversation
r? @davidtwco (rustbot has picked a reviewer for you, use r? to override) |
Hi @Aaron1011, I see that you assigned this task to yourself, but since it has been two years since the original and I try to solve it. By the way, I will appreciate it if you can help to review this change from the author view? |
I'm not familiar enough with this part of the compiler, I'll let Aaron take a look. r? @Aaron1011 |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry for the delay in getting to this. I don't think this is right - that assertion is to ensure that we correctly capture tokens, since capturing an unglued token didn't work at the time I wrote this. Can you add a test using an attribute that prints/modifies the tokens, and make sure that it sees the correct tokens with this change? |
However, it appears that there is no appropriate position for updating
Since attributes cannot print the result of the macro expansion, I've used hir printing to ensure correctness. |
Hi @Aaron1011, it looks like this is ready for another round of review when you get a chance. Cheers! |
r? @estebank |
if !(self.token.kind == TokenKind::Gt && self.unmatched_angle_bracket_count > 0) { | ||
assert!(!self.break_last_token, "Should not have unglued last token with cfg attr"); | ||
} |
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.
@petrochenkov this change is so that we no longer ICE on the face of
struct S<#[cfg(feature = "alloc")] N: A<T>>;
^^ these currently get split and cause the assertion to fail unconditionally
I believe that this change is reasonable. Is there any reason we shouldn't land this PR as is? Put another way, are there any non-local knock down effects from this change that come to mind?
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.
gentle ping @petrochenkov for this comment. thanks.
This needs to be tested with proc macros working on tokens, not with The derive should see the structure with the |
@bvanjoi triage: can you post your status on this? |
The job Click to see the possible cause of the failure (guessed by this bot)
|
I still haven't found the right solution, so I'm closing this. |
Fixes #87577
The parser expected to encounter the
Token::Gt
but instead encounteredBinOp(shr)
when parsingA<T>
during macro expand. As a result, theself.token_cursor.break_last_token = true
inbreak_and_eat
had been executed. This caused the parser to encounterassert!(!self.token_cursor.break_last_token, xxx)
incollect_tokens_trailing_token
undercpature-cfg
mode.To prevent this issue, an extra restriction was added to the
assert
statement. By the way, I think the better solution may be to simply remove theassert
altogether.