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

Implement confusable_idents lint. #71542

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

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 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
src/librustc_session/parse.rs Show resolved Hide resolved
src/librustc_session/parse.rs Outdated Show resolved Hide resolved
src/librustc_lint/non_ascii_idents.rs Outdated Show resolved Hide resolved
src/librustc_lint/non_ascii_idents.rs Show resolved Hide resolved
@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

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 mentioned this pull request Apr 28, 2020
Merged
@crlf0710
Copy link
Member Author

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
@rust-highfive

This comment has been minimized.

Sign in to view
@crlf0710
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@bors
Copy link
Contributor

bors commented Apr 28, 2020

@crlf0710: 🔑 Insufficient privileges: not in try users

@rust-highfive

This comment has been minimized.

Sign in to view
@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
Contributor

bors commented May 1, 2020

☀️ Try build successful - checks-azure
Build commit: faa97a0482a1a2dd0650603410128fe53ec06fbc (faa97a0482a1a2dd0650603410128fe53ec06fbc)

@rust-timer
Copy link
Collaborator

Queued faa97a0482a1a2dd0650603410128fe53ec06fbc with parent bd0bacc, future comparison URL.

@rust-timer
Copy link
Collaborator

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

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
src/librustc_lint/non_ascii_idents.rs Outdated Show resolved Hide resolved
src/librustc_lint/non_ascii_idents.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

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

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 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

@bors r+

@bors
Copy link
Contributor

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
Reviewers

@Manishearth Manishearth Manishearth left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@crlf0710 @petrochenkov @rust-highfive @rust-timer @bors @wesleywiser @Mark-Simulacrum @Manishearth