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 ICE when pointing at multi bytes character #80185

Merged
merged 1 commit into from
Dec 30, 2020

Conversation

JohnTitor
Copy link
Member

Fixes #80134

Seems this ICE was introduced by #73953, checking width of span to avoid ICE.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Dec 19, 2020
@JohnTitor
Copy link
Member Author

Oh, find_width_of_character_at_span doesn't work well on the original case, one sec...

@JohnTitor
Copy link
Member Author

So, in next_point, width will be 1 since it's shrinked and next_point doesn't return the span on multibyte character correctly here.

/// Returns a new span representing the next character after the end-point of this span.
pub fn next_point(&self, sp: Span) -> Span {
let start_of_next_point = sp.hi().0;
let width = self.find_width_of_character_at_span(sp.shrink_to_hi(), true);
// If the width is 1, then the next span should point to the same `lo` and `hi`. However,
// in the case of a multibyte character, where the width != 1, the next span should
// span multiple bytes to include the whole character.
let end_of_next_point =
start_of_next_point.checked_add(width - 1).unwrap_or(start_of_next_point);
let end_of_next_point = BytePos(cmp::max(sp.lo().0 + 1, end_of_next_point));
Span::new(BytePos(start_of_next_point), end_of_next_point, sp.ctxt())
}

I think using sp for now is the easist way, though it regresses some suggestion (but I think it's fine as MaybeIncorrect).

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

I'd prefer that we work out how to fix next_point (or find_width_of_character_at_span) so that it returns a correct span here. Assuming that sp is valid (and if it isn't, we should fix that), I'd expect that calling next_point should always do the correct thing.

src/test/ui/parser/issue-80134.stderr Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
fn main() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename this test file to be more descriptive?

Copy link
Member Author

Choose a reason for hiding this comment

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

What the name do you prefer?

Copy link
Member

Choose a reason for hiding this comment

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

multibyte-char-use-seperator-issue-80134.rs? It's long but I'm trying to avoid landing more tests that are named issue-XXXXX.rs.

@JohnTitor
Copy link
Member Author

I'd prefer that we work out how to fix next_point (or find_width_of_character_at_span) so that it returns a correct span here.

The next_point behavior was changed by 3250057 (cc @estebank), I'm not sure how we could deal with that while preventing known ICEs...

@davidtwco
Copy link
Member

I'd prefer that we work out how to fix next_point (or find_width_of_character_at_span) so that it returns a correct span here.

The next_point behavior was changed by 3250057 (cc @estebank), I'm not sure how we could deal with that while preventing known ICEs...

If you don't think there's a more general fix here then I'm happy to land this - I'll approve and r=me after the last comment is resolved :)

@davidtwco davidtwco 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2020
@JohnTitor
Copy link
Member Author

Rebased and applied filename suggestion on review.

If you don't think there's a more general fix here then I'm happy to land this - I'll approve and r=me after the last comment is resolved :)

I don't have any idea how we could fix it right now, I'm going to r=you for now as the regression would affect rustfix only. Created a Zulip stream for further discussion: https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/.60SourceMap.3A.3Anext_point.60.20cannot.20point.20multibyte.20character

@bors r=davidtwco

@bors
Copy link
Contributor

bors commented Dec 30, 2020

📌 Commit 4ae99cc has been approved by davidtwco

@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 Dec 30, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 30, 2020
Rollup of 7 pull requests

Successful merges:

 - rust-lang#80185 (Fix ICE when pointing at multi bytes character)
 - rust-lang#80260 (slightly more typed interface to panic implementation)
 - rust-lang#80311 (Improvements to NatVis support)
 - rust-lang#80337 (Use `desc` as a doc-comment for queries if there are no doc comments)
 - rust-lang#80381 (Revert "Cleanup markdown span handling")
 - rust-lang#80492 (remove empty wraps, don't return Results from from infallible functions)
 - rust-lang#80509 (where possible, pass slices instead of &Vec or &String (clippy::ptr_arg))

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1caa5b0 into rust-lang:master Dec 30, 2020
@rustbot rustbot added this to the 1.51.0 milestone Dec 30, 2020
@JohnTitor JohnTitor deleted the issue-80134 branch December 30, 2020 18:44
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.

"é" in the code caused "bpos.to_u32() >= mbc.pos.to_u32() + mbc.bytes as u32"
5 participants