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

Add warning when whitespace is not skipped after an escaped newline #87596

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

jesyspa
Copy link
Contributor

@jesyspa jesyspa commented Jul 29, 2021

Fixes issue #87318, also simplifies issue #87319.

  • Add support to the lexer to emit warnings as well as errors.
  • Emit a warning when a string literal contains an escaped newline, but when (some of) the whitespace on the next line is not skipped due to it being non-ASCII.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2021
@jesyspa
Copy link
Contributor Author

jesyspa commented Jul 29, 2021

One thing I was wondering about: prior to this PR, the lexer only has support for emitting errors. I've added warnings by changing the callback, but I'm not sure it's the best approach. Is it too messy?

One other option would be to pass an emit_char and an emit_err callback, since those require mostly orthogonal code, but that would require a bit more reworking of the unescape.rs internals.

@rust-log-analyzer

This comment has been minimized.

@jesyspa
Copy link
Contributor Author

jesyspa commented Jul 29, 2021

Sorry, I missed the clippy code when testing locally. I'll update the PR and re-run the checks.

@jesyspa jesyspa force-pushed the issue-87318-hidden-whitespace branch 2 times, most recently from 53a297d to d7f524f Compare July 29, 2021 18:51
compiler/rustc_lexer/src/unescape.rs Outdated Show resolved Hide resolved
compiler/rustc_lexer/src/unescape.rs Outdated Show resolved Hide resolved
compiler/rustc_lexer/src/unescape.rs Outdated Show resolved Hide resolved
Comment on lines 63 to 74
pub enum EscapeWarning {
/// After a line ending with '\', the next line contains whitespace
/// characters that are not skipped.
UnskippedWhitespaceAfterEscapedNewline,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

In principle I'm ok with the approach of nested Results, but in this case it feels like a better way would be to make UnskippedWhitespaceAfterEscapedNewline a variant of EscapeError. I know it muddles the waters a little bit by mixing warnings and errors, but I feel it is worth it due to the flattening of the code in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I've changed it to be a single type. Should I also change the name to EscapeDiagnostic, or is that too vague?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like EscapeError is slightly misleading, but it'll make more sense than EscapeDiagnostic.

@jesyspa jesyspa force-pushed the issue-87318-hidden-whitespace branch from fcd287c to bc8bbeb Compare July 30, 2021 14:11
@rust-log-analyzer

This comment has been minimized.

@jesyspa jesyspa force-pushed the issue-87318-hidden-whitespace branch from bc8bbeb to 5d59b44 Compare July 30, 2021 14:27
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 30, 2021

📌 Commit 5d59b44 has been approved by estebank

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 30, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 30, 2021
…ce, r=estebank

Add warning when whitespace is not skipped after an escaped newline

Fixes issue rust-lang#87318, also simplifies issue rust-lang#87319.

* Add support to the lexer to emit warnings as well as errors.
* Emit a warning when a string literal contains an escaped newline, but when (some of) the whitespace on the next line is not skipped due to it being non-ASCII.
This was referenced Jul 30, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#86072 (Cross compiling rustc_llvm on Darwin requires zlib.)
 - rust-lang#87385 (Make `SEMICOLON_IN_EXPRESSIONS_FROM_MACROS` warn by default)
 - rust-lang#87547 (Add missing examples for NonNull)
 - rust-lang#87557 (Fix issue with autofix for ambiguous associated function from Rust 2021 prelude when struct is generic)
 - rust-lang#87559 (Tweak borrowing suggestion in `for` loop)
 - rust-lang#87596 (Add warning when whitespace is not skipped after an escaped newline)
 - rust-lang#87606 (Add some TAIT-related regression tests)
 - rust-lang#87609 (Add docs about performance and `Iterator::map` to `[T; N]::map`)
 - rust-lang#87616 (Fix missing word in rustdoc book)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aa9e6aa into rust-lang:master Jul 30, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 30, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 3, 2021
…whitespace, r=estebank"

This reverts commit aa9e6aa, reversing
changes made to 5e2655d.
@jesyspa jesyspa deleted the issue-87318-hidden-whitespace branch August 20, 2021 09:39
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.

6 participants