Skip to content

Tweak confusable idents checking #75349

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
Aug 10, 2020
Merged

Tweak confusable idents checking #75349

merged 2 commits into from
Aug 10, 2020

Conversation

nnethercote
Copy link
Contributor

The confusable idents checking does some sub-optimal things with symbols.

r? @petrochenkov
cc @crlf0710

@nnethercote
Fix symbol ordering for confusable idents detection.
75b67c2 
Confusable idents detection uses a type `BTreeMap<Symbol, Span>`. This is
highly dubious given that `Symbol` doesn't guarantee a meaningful order. (In
practice, it currently gives an order that mostly matches source code order.)

As a result, changes in `Symbol` representation make the
`lint-confusable-idents.rs` test fail, because this error message:

> identifier pair considered confusable between `ｓ` and `s`

is changed to this:

> identifier pair considered confusable between `s` and `ｓ`

and the corresponding span pointers get swapped erroneously, leading to
an incorrect "previous identifier" label.

This commit sorts the relevant symbols by span before doing the checking,
which ensures that the ident that appears first in the code will be mentioned
first in the message. The commit also extends the test slightly to be more
thorough.
@nnethercote
Remove CowBoxSymStr.
0a597bd 
`CowBoxSymStr` is a type that either holds a `SymbolStr` (which is much
the same as a `Symbol`), or an owned string. When computing skeletons,
a `SymbolStr` is stored if the skeleton is the same as the original
string, otherwise an owned string is stored.

So, basically, `CowBoxSymStr` is a type for string interning. But we
already have one of those: `Symbol` itself. This PR removes
`CowBoxSymStr`, using `Symbol` instead. A good thing about this is that
it avoids storing `SymbolStr` values in `skeleton_map`, something that
is discouraged.

The PR also inlines and removes the `calc_skeleton()` function because
that simplifies the code.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2020
@nnethercote
Copy link
Contributor Author

These two commits are the first two commits from #74554.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 10, 2020

📌 Commit 0a597bd 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 Aug 10, 2020
@petrochenkov
Copy link
Contributor

Actually, @bors r-.
The non-ASCII ident lints (which are now enabled by default) caused perf regressions, and a lot of this CowBoxSymStr stuff was done with intention to make them faster.
I'm not sure how much those intentions corresponded to reality, but it still would be prudent to run this through perf.

@petrochenkov
Copy link
Contributor

petrochenkov commented Aug 10, 2020 •
edited
Loading

@bors try @rust-timer queue

zzz

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Aug 10, 2020

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

@petrochenkov
Copy link
Contributor

@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 Aug 10, 2020
@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Aug 10, 2020

⌛ Trying commit 0a597bd with merge 47c28a19a7d1cfc337986990a4a31c207c1c46eb...

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2020
@bors
Copy link
Collaborator

bors commented Aug 10, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 47c28a19a7d1cfc337986990a4a31c207c1c46eb (47c28a19a7d1cfc337986990a4a31c207c1c46eb)

@rust-timer
Copy link
Collaborator

Queued 47c28a19a7d1cfc337986990a4a31c207c1c46eb with parent 08324fe, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (47c28a19a7d1cfc337986990a4a31c207c1c46eb): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Mark-Simulacrum
Copy link
Member

NonAsciiIdents is still allow by default I think, but some of the other lints like confusable idents are not. Regardless this seems perf-neutral or a slight improvement, even, so @bors r=petrochenkov

@bors
Copy link
Collaborator

bors commented Aug 10, 2020

📌 Commit 0a597bd has been approved by petrochenkov

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 10, 2020
@bors
Copy link
Collaborator

bors commented Aug 10, 2020

⌛ Testing commit 0a597bd with merge 770bd3d...

@bors
Copy link
Collaborator

bors commented Aug 10, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: petrochenkov
Pushing 770bd3d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 10, 2020
@bors bors merged commit 770bd3d into rust-lang:master Aug 10, 2020
@nnethercote nnethercote deleted the tweak-confusable-idents-checking branch August 11, 2020 00:05
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-perf Status: Waiting on a perf run to be completed.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @petrochenkov @bors @rust-timer @Mark-Simulacrum @cuviper @rust-highfive