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

Split Diagnostics for Uncommon Codepoints: Add Individual Identifier Types #120840

Merged
merged 1 commit into from
Feb 26, 2024
Merged

Split Diagnostics for Uncommon Codepoints: Add Individual Identifier Types #120840

merged 1 commit into from
Feb 26, 2024

Conversation

HTGAzureX1212
Copy link
Contributor

@HTGAzureX1212 HTGAzureX1212 commented Feb 9, 2024 •
edited by fmease
Loading

This pull request further modifies the uncommon_codepoints lint, adding the individual identifier types of Technical, Not_NFKC, Exclusion and Limited_Use to the diagnostic message.

Example

After:

warninig: identifier contains a non normalized (NFKC) character: 'ij'
  --> src/lib.rs:1:5
   |
LL | fn dijkstra() {}
   |    ^^^^^^^
   |
   = note: this character is included in the Not_NFKC Unicode general security profile

Before:

warning: identifier contains an uncommon Unicode codepoint: 'ij'
 --> src/lib.rs:1:4
  |
1 | fn dijkstra() {}
  |    ^^^^^^^

Second step of #120228.

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 9, 2024
@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 13, 2024
@HTGAzureX1212
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 15, 2024
@bors
Copy link
Contributor

bors commented Feb 16, 2024

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

fmease
fmease reviewed Feb 18, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Sorry for taking a bit. I have a couple of suggestions.

compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Show resolved Hide resolved
compiler/rustc_lint/src/lints.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/non_ascii_idents.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/non_ascii_idents.rs Outdated Show resolved Hide resolved
compiler/rustc_lint/src/non_ascii_idents.rs Outdated Show resolved Hide resolved
@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2024
@Manishearth
Copy link
Member

I'll probably have wording suggestions when I look at this, mostly we shouldn't be mentioning the Not_NFKC or whatever as much as the underlying meaning. I'll help with that when I get a chance hopefully in a few days.

If it's taking too long it's ultimately fine to land this if properly reviewed by someone and we can patch those up later.

@HTGAzureX1212 HTGAzureX1212 force-pushed the HTGAzureX1212/unicode-identifier-types branch from 2f6e822 to 3bf4164 Compare February 18, 2024 09:16
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@HTGAzureX1212
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 18, 2024
fmease
fmease approved these changes Feb 19, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks! I have two suggestions but apart from that I really like it! However, I'll let Manishearth have the final say :)

compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/src/non_ascii_idents.rs Outdated Show resolved Hide resolved
@fmease fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024
@HTGAzureX1212 HTGAzureX1212 force-pushed the HTGAzureX1212/unicode-identifier-types branch from edd9cf9 to 9afe7e6 Compare February 19, 2024 10:49
@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 19, 2024
@HTGAzureX1212
Copy link
Contributor Author

HTGAzureX1212 commented Feb 19, 2024 •
edited
Loading

Comments resolved and commits squashed!

Manishearth
Manishearth reviewed Feb 20, 2024
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Good start with the wording, I don't actually think we should mention unicode codepoint, "character" is fine in this context (even though generally in unicode contexts it can be ambiguous).

I've suggested fixes (make sure to fix both the singular and the plural!)

compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
@HTGAzureX1212
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 24, 2024
fmease
fmease approved these changes Feb 24, 2024
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks
@bors r=fmease,Manishearth

@HTGAzureX1212
Copy link
Contributor Author

Did bors give up? 🤔🤔🤔

@fmease
Copy link
Member

fmease commented Feb 25, 2024 •
edited
Loading

Ah, I put it in a review comment which it doesn't recognize.

@bors r=fmease,Manishearth

@bors
Copy link
Contributor

bors commented Feb 25, 2024

📌 Commit fcc577a has been approved by fmease,Manishearth

It is now in the queue for this repository.

@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 Feb 25, 2024
fmease
fmease reviewed Feb 25, 2024
View reviewed changes
compiler/rustc_lint/messages.ftl Outdated Show resolved Hide resolved
tests/ui/lexer/lex-emoji-identifiers.stderr Outdated Show resolved Hide resolved
@fmease
Copy link
Member

fmease commented Feb 25, 2024

Sorry about that
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 25, 2024
@HTGAzureX1212 HTGAzureX1212 force-pushed the HTGAzureX1212/unicode-identifier-types branch from dcd5118 to 8bccceb Compare February 26, 2024 02:09
@HTGAzureX1212
Copy link
Contributor Author

Done!

@HTGAzureX1212
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 26, 2024
@fmease
Copy link
Member

fmease commented Feb 26, 2024

@bors r=fmease,Manishearth

@bors
Copy link
Contributor

bors commented Feb 26, 2024

📌 Commit 8bccceb has been approved by fmease,Manishearth

It is now in the queue for this repository.

@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 Feb 26, 2024
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 26, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
@bors
Auto merge of rust-lang#121627 - GuillaumeGomez:rollup-udp3m0k, r=Gui…
ee933f6 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#120656 (Allow tests to specify a `//@ filecheck-flags:` header)
 - rust-lang#120840 (Split Diagnostics for Uncommon Codepoints: Add Individual Identifier Types)
 - rust-lang#121554 (Don't unnecessarily change `SIGPIPE` disposition in `unix_sigpipe` tests)
 - rust-lang#121590 (Correctly handle if rustdoc JS script hash changed)
 - rust-lang#121620 (Fix more rust-lang#121208 fallout (round 3))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 91d337d into rust-lang:master Feb 26, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 26, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 26, 2024
@rust-timer
Unrolled build for rust-lang#120840
7cb79ef 
Rollup merge of rust-lang#120840 - HTGAzureX1212:HTGAzureX1212/unicode-identifier-types, r=fmease,Manishearth

Split Diagnostics for Uncommon Codepoints: Add Individual Identifier Types

This pull request further modifies the `uncommon_codepoints` lint, adding the individual identifier types of `Technical`, `Not_NFKC`, `Exclusion` and `Limited_Use` to the diagnostic message.

Example rendered diagnostic:
```
error: identifier contains a Unicode codepoint that is not used in normalized strings: 'ij'
  --> $DIR/lint-uncommon-codepoints.rs:6:4
   |
LL | fn dijkstra() {}
   |    ^^^^^^^
   = note: this character is included in the Not_NFKC Unicode general security profile
```

Second step of rust-lang#120228.
@HTGAzureX1212 HTGAzureX1212 deleted the HTGAzureX1212/unicode-identifier-types branch February 26, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@fmease fmease fmease approved these changes

@Manishearth Manishearth Manishearth approved these changes

Assignees

@Manishearth Manishearth

@fmease fmease

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.78.0
Development

Successfully merging this pull request may close these issues.
7 participants
@HTGAzureX1212 @rustbot @bors @Manishearth @rust-log-analyzer @fmease @compiler-errors