Skip to content

Implement confusable_idents lint. #71542

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
Merged
merged 2 commits into from
May 3, 2020
Merged

Implement confusable_idents lint. #71542

merged 2 commits into from
May 3, 2020

Conversation

@crlf0710
Copy link
Member

@crlf0710 crlf0710 commented Apr 25, 2020 •
edited
Loading

This collects all identifier symbols into ParseSession and examines them within the non-ascii-idents lint.

The skeleton generation part needs to be added to unicode-security crate. Will update this PR when the crate is updated.

r? @petrochenkov

EDIT: also included the concat_idents part.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2020
@crlf0710 crlf0710 requested a review from petrochenkov April 25, 2020 01:57
@crlf0710 crlf0710 force-pushed the confusable_idents branch from 1b49fc2 to d19b5d4 Compare April 25, 2020 02:16
@crlf0710 crlf0710 mentioned this pull request Apr 25, 2020
Merged
@phansch phansch mentioned this pull request Apr 25, 2020
Closed
petrochenkov
petrochenkov reviewed Apr 25, 2020
View reviewed changes
@petrochenkov petrochenkov 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 Apr 25, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 25, 2020

This will also need a perf run.
If there's too much locking, then we'll perhaps need to buffer identifiers in the lexer before inserting them into the "gallery".
@crlf0710 crlf0710 force-pushed the confusable_idents branch from d19b5d4 to 190c1f3 Compare April 28, 2020 01:56
@crlf0710 crlf0710 mentioned this pull request Apr 28, 2020
Merged
@crlf0710 crlf0710 force-pushed the confusable_idents branch from 190c1f3 to 39bd7d7 Compare April 28, 2020 02:08
@crlf0710
Copy link
Member Author

crlf0710 commented Apr 28, 2020

Rebased and addressed review comments. The actual skeleton calculation is pending on unicode-rs/unicode-security#12 Should we do the perf run before or after the actual calculation is added? @petrochenkov
@crlf0710 crlf0710 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 Apr 28, 2020
@crlf0710 crlf0710 force-pushed the confusable_idents branch from 39bd7d7 to 7d19c01 Compare April 28, 2020 11:53
@rust-highfive

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the confusable_idents branch from 7d19c01 to 6384348 Compare April 28, 2020 14:00
@crlf0710
Copy link
Member Author

crlf0710 commented Apr 28, 2020

@bors try @rust-timer queue
@rust-timer
Copy link
Collaborator

rust-timer commented Apr 28, 2020

Insufficient permissions to issue commands to rust-timer.
@bors
Copy link
Collaborator

bors commented Apr 28, 2020

@crlf0710: 🔑 Insufficient privileges: not in try users
@crlf0710 crlf0710 requested a review from petrochenkov April 28, 2020 14:04
@rust-highfive

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the confusable_idents branch from 6384348 to 10cd0a4 Compare April 28, 2020 17:21
@rust-highfive

This comment has been minimized.

Sign in to view
@crlf0710 crlf0710 force-pushed the confusable_idents branch 3 times, most recently from 5eede9d to df8f2b0 Compare April 29, 2020 03:07
@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented May 1, 2020

☀️ Try build successful - checks-azure
Build commit: faa97a0482a1a2dd0650603410128fe53ec06fbc (faa97a0482a1a2dd0650603410128fe53ec06fbc)
@rust-timer
Copy link
Collaborator

rust-timer commented May 1, 2020

Queued faa97a0482a1a2dd0650603410128fe53ec06fbc with parent bd0bacc, future comparison URL.
@rust-timer
Copy link
Collaborator

rust-timer commented May 2, 2020

Finished benchmarking try commit faa97a0482a1a2dd0650603410128fe53ec06fbc, comparison URL.
@crlf0710
Copy link
Member Author

crlf0710 commented May 2, 2020

So it seems from the results after the optimizations does improve a little compared to the initial commit.
@petrochenkov
Copy link
Contributor

petrochenkov commented May 2, 2020 •
edited
Loading

So, the slowdown is from processing the symbol gallery rather than from collecting it?
That's great news.

It's an isolated piece of code that can be benchmarked easily.
petrochenkov
petrochenkov reviewed May 2, 2020
View reviewed changes
petrochenkov
petrochenkov reviewed May 2, 2020
View reviewed changes
@petrochenkov
Copy link
Contributor

petrochenkov commented May 2, 2020

Since the lint is allowed by default right now (and has too many false positives to be un-allowed), I wouldn't personally care too much about the slowdown and would merge this as is.
@petrochenkov petrochenkov 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 May 2, 2020
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented May 2, 2020

If we land this with it as allow by default then I agree the performance of warning doesn't matter much.
@crlf0710
Copy link
Member Author

crlf0710 commented May 2, 2020

Thanks, let me address the comments, and re-allow the lint for now then.
@crlf0710 crlf0710 force-pushed the confusable_idents branch from 7f75533 to f3ec00a Compare May 2, 2020 18:34
@crlf0710 crlf0710 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 May 2, 2020
@petrochenkov
Copy link
Contributor

petrochenkov commented May 2, 2020

@bors r+
@bors
Copy link
Collaborator

bors commented May 2, 2020

📌 Commit f3ec00a has been approved by petrochenkov
@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 May 2, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 3, 2020
@Dylan-DPC
Rollup merge of rust-lang#71542 - crlf0710:confusable_idents, r=petro…
e5be1f8 
…chenkov Implement `confusable_idents` lint. This collects all identifier symbols into `ParseSession` and examines them within the non-ascii-idents lint. The skeleton generation part needs to be added to `unicode-security` crate. Will update this PR when the crate is updated. r? @petrochenkov EDIT: also included the `concat_idents` part.
This was referenced May 3, 2020
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2020
@bors
Auto merge of rust-lang#71828 - Dylan-DPC:rollup-qf5h2w5, r=Dylan-DPC
d626e4d 
Rollup of 5 pull requests Successful merges: - rust-lang#71165 (`slice::fill`: use `T` instead of generic arg) - rust-lang#71314 (Implement RFC 2523, `#[cfg(version(..))]`) - rust-lang#71542 (Implement `confusable_idents` lint.) - rust-lang#71806 (typo) - rust-lang#71813 (Decode qualifs for associated const defaults) Failed merges: r? @ghost
@bors bors merged commit 24e101c into rust-lang:master May 3, 2020
This was referenced May 3, 2020
Closed
Closed
@crlf0710 crlf0710 deleted the confusable_idents branch May 3, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

F-non_ascii_idents `#![feature(non_ascii_idents)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

8 participants

@crlf0710 @petrochenkov @rust-highfive @rust-timer @bors @wesleywiser @Mark-Simulacrum @Manishearth