-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 a byte/char confusion issue in the error emitter #44081
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
0854250
to
b4b0d3e
Compare
src/librustc_errors/emitter.rs
Outdated
if source_string.chars() | ||
.enumerate() | ||
.filter(|&(i, _)| i < ann.start_col) | ||
.map(|(_, v)|v) |
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.
.chars().take(ann.start_col)
?
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.
Oh right, that's better. Not very familiar with iterator APIs.
r? @rkruppe |
I don't think @rkruppe can r+? Let me know when this is ready. |
src/librustc_errors/emitter.rs
Outdated
if source_string[0..ann.start_col].trim() == "" { | ||
if source_string.chars() | ||
.take(ann.start_col) | ||
.collect::<String>() |
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.
Wait, why is this collecting to a string?
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.
You can do the same btw with .all(|c| c.is_whitespace())
.
@@ -311,7 +311,11 @@ impl EmitterWriter { | |||
if line.annotations.len() == 1 { | |||
if let Some(ref ann) = line.annotations.get(0) { | |||
if let AnnotationType::MultilineStart(depth) = ann.annotation_type { | |||
if source_string[0..ann.start_col].trim() == "" { |
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.
Why is the byte offset unavailable here? Converting to column should be delayed until the last moment IMO (and should take into account unicode grapheme width).
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.
@eddyb I'm not sure what you mean but the start_col is given as columns, as per this comment:
rust/src/librustc_errors/snippet.rs
Lines 123 to 124 in 32b50e2
/// Start column, 0-based indexing -- counting *characters*, not | |
/// utf-8 bytes. Note that it is important that this field goes |
Fixes rust-lang#44078. Fixes rust-lang#44023. The start_col member is given in chars, while the code previously assumed it was given in bytes. The more basic issue rust-lang#44080 doesn't get fixed.
r? @eddyb |
@bors r+ |
📌 Commit 5a71e12 has been approved by |
Fix a byte/char confusion issue in the error emitter Fixes rust-lang#44078. Fixes rust-lang#44023. The start_col member is given in chars, while the code previously assumed it was given in bytes. The more basic issue rust-lang#44080 doesn't get fixed.
☀️ Test successful - status-appveyor, status-travis |
Fixes #44078. Fixes #44023.
The start_col member is given in chars, while the code previously assumed it was given in bytes.
The more basic issue #44080 doesn't get fixed.