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

Compiling a file with a non-ASCII name with --error-format=short enabled makes error messages contain excessive spaces after "filename:line:column" #65119

Closed
AnthonyMikh opened this issue Oct 5, 2019 · 2 comments · Fixed by #65120
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-unicode Area: Unicode C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@AnthonyMikh
Copy link
Contributor

AnthonyMikh commented Oct 5, 2019

Consider the following rust source (in test.rs):

fn main() {
    println!("hello world
}

It is obvious that it has syntax error. It also seems obvious that the generated error shouldn't depend on name of file except noting the name of file. However, it doesn't happen in practice:

$ rustc test.rs --error-format=short
test.rs:2:14: error: unterminated double quote string
error: aborting due to previous error
$ mv test.rs 'тест.rs'
$ rustc 'тест.rs' --error-format=short
тест.rs:2:14:     error: unterminated double quote string
error: aborting due to previous error

Note extra spaces after тест.rs:2:14:.

Rust version (although it is not strictly relevant, see below):

$ rustc -vV
rustc 1.38.0 (625451e37 2019-09-23)
binary: rustc
commit-hash: 625451e376bb2e5283fc4741caa0a3e8a2ca4d54
commit-date: 2019-09-23
host: x86_64-unknown-linux-gnu
release: 1.38.0
LLVM version: 9.0

The core issue lies in the implementation of StyledBuffer::prepend method. The idea of the code is to make enough space in the beginning of line (filled with spaces) and then overwrite it character by character. However, in order to estimate the required space, it uses .len() which is wrong since str in Rust can contain non-ASCII which require more than one byte for storage and StyledBuffer keeps lines in Vec<char>s. It means that this isuue reveals itself only when a non-ASCII string is passed as an argument — and that's exactly what's happening in EmitterWriter::emit_message_default under very specific circumstances: when processing a primary file, when self.short_message is set (i. e. --error-message=short) and when name of the primary file happens to have a non-ASCII name.

The errorneous code for StyledBuffer::prepend was introduced in 2e8e73c. This bug landed in stable in 1.28.0.

The obvious fix is to change the line in prepend into

        let string_len = string.chars().count();

cc @estebank

@AnthonyMikh
Copy link
Contributor Author

I want to raise a question: is it really an issue worth fixing? It fires under very specific circumstances and the fix makes prepend slower for every case, not only these specific ones.

@estebank
Copy link
Contributor

estebank commented Oct 5, 2019

I want to raise a question: is it really an issue worth fixing?

I believe so.

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints A-unicode Area: Unicode C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 5, 2019
Centril added a commit to Centril/rust that referenced this issue Oct 7, 2019
Correctly estimate the required space for string in `StyledBuffer::prepend`

Fix rust-lang#65119

r? @estebank
Centril added a commit to Centril/rust that referenced this issue Oct 8, 2019
Correctly estimate the required space for string in `StyledBuffer::prepend`

Fix rust-lang#65119

r? @estebank
@bors bors closed this as completed in 153d3c3 Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-unicode Area: Unicode C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants