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.

Sign up for GitHub

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

Jump to bottom

Print a note if a character literal contains a variation selector #88795

Merged
merged 1 commit into from
Sep 22, 2021
Merged

Print a note if a character literal contains a variation selector #88795

merged 1 commit into from
Sep 22, 2021
+137 −14

Conversation

FabianWolff
Copy link
Contributor

Fixes #88684.

@rust-highfive
Copy link
Contributor

r? @wesleywiser

(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 Sep 9, 2021
eggyal
eggyal reviewed Sep 10, 2021
View reviewed changes
compiler/rustc_parse/src/lexer/unescape_error_reporting.rs Outdated
"character literal may only contain one codepoint",
);
if lit.chars().count() == 2
&& ('\u{FE00}'..='\u{FE0F}').contains(&lit.chars().last().unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget Variation Selectors Supplement and the Mongolian Variation Selectors... indeed, perhaps unicode_normalization::char::is_combining_mark could offer a more general approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have expanded my implementation now to print a note for all combining marks, as you suggested. I've also added a suggestion to use the normalized form if the character can be represented as one codepoint (as in this example).

Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice!

wesleywiser
wesleywiser approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

This is fantastic, thank you!

@wesleywiser
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Sep 21, 2021

📌 Commit 0d8245b has been approved by wesleywiser

@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 Sep 21, 2021
@the8472 the8472 mentioned this pull request Sep 21, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2021
@bors
Auto merge of rust-lang#89158 - the8472:rollup-3e4ijth, r=the8472
d8d1d10 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#88795 (Print a note if a character literal contains a variation selector)
 - rust-lang#89015 (core::ascii::escape_default: reduce struct size)
 - rust-lang#89078 (Cleanup: Remove needless reference in ParentHirIterator)
 - rust-lang#89086 (Stabilize `Iterator::map_while`)
 - rust-lang#89096 ([bootstrap] Improve the error message when `ninja` is not found to link to installation instructions)
 - rust-lang#89113 (dont `.ensure()` the `thir_abstract_const` query call in `mir_build`)
 - rust-lang#89114 (Fixes a technicality regarding the size of C's `char` type)
 - rust-lang#89115 (:arrow_up: rust-analyzer)
 - rust-lang#89126 (Fix ICE when `indirect_structural_match` is allowed)
 - rust-lang#89141 (Impl `Error` for `FromSecsError` without foreign type)
 - rust-lang#89142 (Fix match for placeholder region)
 - rust-lang#89147 (add case for checking const refs in check_const_value_eq)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c2cdba4 into rust-lang:master Sep 22, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 22, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 28, 2021
@bors
Auto merge of rust-lang#89158 - the8472:rollup-3e4ijth, r=the8472
b51b172 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#88795 (Print a note if a character literal contains a variation selector)
 - rust-lang#89015 (core::ascii::escape_default: reduce struct size)
 - rust-lang#89078 (Cleanup: Remove needless reference in ParentHirIterator)
 - rust-lang#89086 (Stabilize `Iterator::map_while`)
 - rust-lang#89096 ([bootstrap] Improve the error message when `ninja` is not found to link to installation instructions)
 - rust-lang#89113 (dont `.ensure()` the `thir_abstract_const` query call in `mir_build`)
 - rust-lang#89114 (Fixes a technicality regarding the size of C's `char` type)
 - rust-lang#89115 (:arrow_up: rust-analyzer)
 - rust-lang#89126 (Fix ICE when `indirect_structural_match` is allowed)
 - rust-lang#89141 (Impl `Error` for `FromSecsError` without foreign type)
 - rust-lang#89142 (Fix match for placeholder region)
 - rust-lang#89147 (add case for checking const refs in check_const_value_eq)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eggyal eggyal eggyal left review comments

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

Improve diagnostics for character literals containing multiple codepoints
6 participants
@FabianWolff @rust-highfive @wesleywiser @bors @eggyal @rustbot