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

Don't escape unicode escape braces in print_literal #11265

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

Alexendoo
Copy link
Member

Fixes #11264

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Jul 31, 2023

r? @llogiq

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 31, 2023
@Alexendoo Alexendoo force-pushed the print-literal-unicode-escapes branch from 258c6d5 to b1a4236 Compare July 31, 2023 14:42
Copy link
Contributor

@llogiq llogiq left a comment

Choose a reason for hiding this comment

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

This looks ok to me, but I'd like to see more tests.

@@ -42,4 +42,7 @@ fn main() {
// The string literal from `file!()` has a callsite span that isn't marked as coming from an
// expansion
println!("file: {}", file!());

// Braces in unicode escapes should not be escaped
println!("{}", "{} \u{ab123} {}");
Copy link
Contributor

Choose a reason for hiding this comment

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

How about a test case with a format string of "\\u{ab123}" with binding of ab123 to something?

Or a test case with additional format arg things like debug output or width specification?

Just to make sure we cover all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

\\u{..} was a good call, it didn't handle that correctly so I've replaced the regex with a state machine

@Alexendoo Alexendoo force-pushed the print-literal-unicode-escapes branch 3 times, most recently from 97587e8 to ec76c7c Compare July 31, 2023 20:49
@bors
Copy link
Collaborator

bors commented Aug 11, 2023

☔ The latest upstream changes (presumably #11239) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo Alexendoo force-pushed the print-literal-unicode-escapes branch from ec76c7c to 8e3b1a6 Compare August 14, 2023 13:05
@samueltardieu
Copy link
Contributor

Isn't this PR ready to be merged?

@bors
Copy link
Collaborator

bors commented Sep 28, 2023

☔ The latest upstream changes (presumably #11576) made this pull request unmergeable. Please resolve the merge conflicts.

@Alexendoo Alexendoo force-pushed the print-literal-unicode-escapes branch from 8e3b1a6 to 9ba8342 Compare October 1, 2023 16:47
@llogiq
Copy link
Contributor

llogiq commented Oct 1, 2023

I think another test case with \\\u{1234} would be useful, otherwise this looks good. Thanks for the heads-up and sorry for the delay.

@Alexendoo Alexendoo force-pushed the print-literal-unicode-escapes branch from 9ba8342 to 258b9a8 Compare October 1, 2023 21:43
@llogiq
Copy link
Contributor

llogiq commented Oct 1, 2023

Thank you!

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 1, 2023

📌 Commit 258b9a8 has been approved by llogiq

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 1, 2023

⌛ Testing commit 258b9a8 with merge 331d01e...

@bors
Copy link
Collaborator

bors commented Oct 1, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 331d01e to master...

@bors bors merged commit 331d01e into rust-lang:master Oct 1, 2023
5 checks passed
@Alexendoo Alexendoo deleted the print-literal-unicode-escapes branch October 1, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clippy::write_literal uses format-string escaping incorrectly in suggested fix
5 participants