Fix logical error with what text is considered whitespace.

[harrisonkaiser]

There appears to be a logical issue around what counts as leading white-space. There is code which does a subtraction assuming that no errors will be reported inside the leading whitespace. However we compute the length of that whitespace with std::char::is_whitespace and not rustc_lexer::is_whitespace. The former will include a no-break space while later will excluded it. We can only safely make the assumption that no errors will be reported in whitespace if it is all "Rust Standard" whitespace. Indeed an error does occur in unicode whitespace if it contains a no-break space. In that case the subtraction will cause a ICE (for a compiler in debug mode) as described in #132918.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jieyouxu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[harrisonkaiser]

Addresses: #132918 (comment)

[jieyouxu]

Thanks for the PR. We'll need to add a regression test for #132918 to prevent it from happening again in the future. Note that the test probably will need to be marked with

//@ needs-rustc-debug-assertions

to catch the overflow assertion that only happens if rustc was built with debug assertions.

Might also need to artificially constrain diagnostics width with something like

//@ compile-flags: --diagnostics-width=80

to prevent terminal size from interfering with test outcome. I'm not super familiar with what the logic here is intended to do, so I'm going to reroll a WG-diagnostics reviewer.

[jieyouxu]

r? diagnostics

[harrisonkaiser]

Thank you for the prompt response and guidance on adding a compilation tests.

I've fixed the typo and added a test case and confirmed that it doesn't pass on master and does pass on this branch.

[harrisonkaiser]

I noticed also that is_whitespace is also called here: (

rust/compiler/rustc_errors/src/emitter.rs

Line 789 in 1e33dd1

if source_string.chars().take(ann.start_col.display).all(|c| c.is_whitespace()) {

), and I'm don't know if that should also be rustc_lexer::is_whitespace. I figured I'd stick to the failure case I could re-produce.

[harrisonkaiser]

@rustbot review

[davidtwco]

I noticed also that is_whitespace is also called here: (

rust/compiler/rustc_errors/src/emitter.rs

Line 789 in 1e33dd1

if source_string.chars().take(ann.start_col.display).all(|c| c.is_whitespace()) {

), and I'm don't know if that should also be rustc_lexer::is_whitespace. I figured I'd stick to the failure case I could re-produce.

I'd be happy to see this changed to rustc_lexer::is_whitespace, it's probably better to be consistent. But that can be a follow-up so this can land quicker.

@bors r+ rollup

[bors]

📌 Commit 1e33dd1 has been approved by davidtwco

It is now in the queue for this repository.