-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
lexer: Disallow bare CR in raw byte strings #60793
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
src/libsyntax/lib.rs
Outdated
@@ -18,6 +18,7 @@ | |||
#![feature(step_trait)] | |||
#![feature(try_trait)] | |||
#![feature(unicode_internals)] | |||
#![feature(rustc_private)] |
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.
The changes from the first commit (so cargo check
works) were of drive-by nature, can remove them if needed
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.
Yes, please remove.
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.
Done. Out of curiosity, why should we skip this? Is it because it adds noise and is somewhat obvious? (after all we're in the compiler code)
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.
Less noise and impossible to forget something if it's added automatically.
rustc crates are not supposed to be built in isolation, outside of the rustc build system, so boilerplate common for all rustc crates is regularly moved to the workspace or rustbuild.
The rustc_private
features in particular were moved in #41847.
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.
IIRC, x.py check
should provide the same functionality as cargo check
.
I think changing However, I think here it's counter-intiutively more important to preserve implementation simplicity (that's where all interesting bits are) rather than interface simplicity (interface is a glue code, which is isolated), so if internal iteration is easier to implement, I'd keep it. |
The direction seems right (same as #60261 basically). |
☔ The latest upstream changes (presumably #60965) made this pull request unmergeable. Please resolve the merge conflicts. |
9085079
to
03e4521
Compare
It was commented out as part of rust-lang@8a8e497. Done probably by accident, since the code in question was moved to a match arm, along with newly introduced logic to detect bare CRs in raw strings.
03e4521
to
6d7bc3b
Compare
@matklad maybe we could use Rebased the PR and would like for this to land and then land a separate one which translates \r\n -> \n if possible. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
7e5ef6d
to
e5d5a08
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Applied feedback and actually implemented CRLF -> LF rewrite in raw byte strings (forgot about the byte variant 🤦♂️ ) in 735ac05#diff-a8037ce27e05cd3a29b041f9ec00f94eR176 |
LGTM now, let's wait for @petrochenkov review! |
r=me with #60793 (comment) fixed |
@bors r+ |
📌 Commit 630d5f3 has been approved by |
lexer: Disallow bare CR in raw byte strings Handles bare CR ~but doesn't translate `\r\n` to `\n` yet in raw strings yet~ and translates CRLF to LF in raw strings. As a side-note I think it'd be good to change the `unescape_` to return plain iterators to reduce some boilerplate (e.g. `has_error` could benefit from collecting `Result<T>` and aborting early on errors) but will do that separately, unless I missed something here that prevents it. @matklad @petrochenkov thoughts?
☀️ Test successful - checks-travis, status-appveyor |
Tested on commit rust-lang/rust@5e2c110. Direct link to PR: <rust-lang/rust#60793> 💔 rls on linux: test-pass → test-fail (cc @Xanewok, @rust-lang/infra).
…klad Add unit tests for unescaping raw (byte) strings Adds unit tests for functionality introduced in rust-lang#60793. r? @matklad @petrochenkov
Handles bare CR
but doesn't translateand translates CRLF to LF in raw strings.\r\n
to\n
yet in raw strings yetAs a side-note I think it'd be good to change the
unescape_
to return plain iterators to reduce some boilerplate (e.g.has_error
could benefit from collectingResult<T>
and aborting early on errors) but will do that separately, unless I missed something here that prevents it.@matklad @petrochenkov thoughts?