-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Use next_point
to avoid ICE
#68735
Use next_point
to avoid ICE
#68735
Conversation
@@ -671,12 +671,12 @@ impl<'a> Parser<'a> { | |||
true | |||
} | |||
token::BinOp(token::Shl) => { | |||
let span = self.token.span.with_lo(self.token.span.lo() + BytePos(1)); |
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.
I see a bunch of other instances of self.token.span.lo() + BytePos(1)
in this file. Should they be updated too?
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.
I'm fine to do so if @estebank also agrees with that.
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.
Uhm, I found out it shouldn't, it'll break many spans.
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.
What kind of breakage occurs?
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.
Indeed we can use next_ponit
for eat_plus
but it seems to cause breakage in other places (expect_and
, expect_or
, and expect_gt
). It will expand or reduce current span size.
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.
I guess we should also tweak later bump_with
s?
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.
Can you file a ticket to follow up on this?
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.
Filed as #68783.
I'm +1 r=me on the change as is, but if we can avoid the other |
@bors r+ |
📌 Commit 6f5a61b has been approved by |
☀️ Test successful - checks-azure |
stop using BytePos for computing spans in librustc_parse/parser/mod.rs Computing spans using logic such as `self.token.span.lo() + BytePos(1)` can cause internal compiler errors like rust-lang#68730 when non-ascii characters are given as input. rust-lang#68735 partially addressed this problem, but only for one case. Moreover, its usage of `next_point()` does not actually align with what `bump_with()` expects. For example, given the token `>>=`, we should pass the span consisting of the final two characters `>=`, but `next_point()` advances the span beyond the end of the `=`. This pull request instead computes the start of the new span by doing `start_point(self.token.span).hi()`. This matches `self.token.span.lo() + BytePos(1)` in the common case where the characters are ascii, and it gracefully handles multibyte characters. Fixes rust-lang#68783.
Fixes #68730
r? @estebank (I think you're familiar with that)