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 new lint octal_escapes #8007

Merged
merged 4 commits into from
Nov 22, 2021
Merged

Add new lint octal_escapes #8007

merged 4 commits into from
Nov 22, 2021

Conversation

birkenfeld
Copy link
Contributor

This checks for sequences in strings that would be octal character
escapes in C, but are not supported in Rust. It suggests either
to use the \x00 escape, or an equivalent hex escape if the octal
was intended.

Fixes #7981


Please write a short comment explaining your change (or "none" for internal only changes)

changelog: Add new lint [octal_escapes], which checks for literals like "\033[0m".

@rust-highfive
Copy link

r? @xFrednet

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 20, 2021
This checks for sequences in strings that would be octal character
escapes in C, but are not supported in Rust.  It suggests either
to use the `\x00` escape, or an equivalent hex escape if the octal
was intended.
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, the general structure is already good IMO :)

tests/ui/octal_escapes.rs Show resolved Hide resolved
clippy_lints/src/octal_escapes.rs Outdated Show resolved Hide resolved
tests/ui/octal_escapes.stderr Outdated Show resolved Hide resolved
clippy_lints/src/octal_escapes.rs Outdated Show resolved Hide resolved
clippy_lints/src/octal_escapes.rs Outdated Show resolved Hide resolved
@xFrednet xFrednet added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 21, 2021
@birkenfeld
Copy link
Contributor Author

Ok, I think I got all the requested changes.

However, I just found a pretty unfortunate problem: the format strings of println! and friends are not recognized, because at the point of the check, "\033" is already normalized to "\u{0}33" - I don't know where this happens. Is there any way to catch this?

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The logic looks good to me! I've marked to more test cases which would be good to have, then it's ready to be merged 🙃

if let Some((_, '0'..='7')) = iter.next() {
to += 1;
}
emit(cx, &contents, from, to + 1, span, is_string);
Copy link
Member

Choose a reason for hiding this comment

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

In the past, Clippy had some errors with string indexing and Unicode characters. This should be safe, could you still add these test cases to ensure that is stays stable:

let _test_unicode = "锈\011锈"
let _test_unicode = "锈\01锈"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, that's a good point.

tests/ui/octal_escapes.rs Show resolved Hide resolved
@xFrednet
Copy link
Member

However, I just found a pretty unfortunate problem: the format strings of println! and friends are not recognized, because at the point of the check, "\033" is already normalized to "\u{0}33" - I don't know where this happens. Is there any way to catch this?

Interesting. @camsteffen I think you worked with the format! macro quite a bit recently, do you maybe have an idea for this? We could use snip the get the original string literal, but that would be a bit hacky 🤔

@camsteffen
Copy link
Contributor

This is a problem that has come up a few times. Format strings are just not available in their original form in post-expanded code. So I don't have a neat solution unfortunately. Getting a snippet and parsing it is indeed hacky and I think we should avoid re-parsing the string from scratch.

@xFrednet
Copy link
Member

Getting a snippet and parsing it is indeed hacky and I think we should avoid reparsing the string from scratch.

Agreed, then that's just something we accept. It might be good to add this to the Known problems section @birkenfeld, could you maybe do this as well? 😅

@xFrednet
Copy link
Member

Looks good to me, thank you for the implementation and for the adjustments. I like this final version!

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 22, 2021

📌 Commit 1210bb4 has been approved by xFrednet

@bors
Copy link
Collaborator

bors commented Nov 22, 2021

⌛ Testing commit 1210bb4 with merge 57a8804...

@birkenfeld
Copy link
Contributor Author

Thanks! It was fun getting back to clippy after quite a few years :)

@xFrednet
Copy link
Member

Happy to hear that it's nice to have you back! I just looked back into the history, and you were a really early contributor. That's really awesome to see. The project and codebase probably changed quite a bit since back then ^^

@bors
Copy link
Collaborator

bors commented Nov 22, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 57a8804 to master...

@bors bors merged commit 57a8804 into rust-lang:master Nov 22, 2021
@birkenfeld birkenfeld deleted the octal_escapes branch November 23, 2021 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint possible octal escape sequence in string
7 participants