Merge single_call_fn post-crate visitor into lint pass
#12256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #12306) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Looks good, r=me after a rebase. |
y21
force-pushed
the
single_call_fn_rm_visitor
branch
from
8852748 to
9a56153
Compare
|
Rebased. @bors r=llogiq |
bors
added a commit
that referenced
this pull request
Feb 25, 2024
Merge `single_call_fn` post-crate visitor into lint pass The `single_call_fn` lint worked by first collecting a list of function definitions in the lint pass, then populating the list of uses for each function in a second visitor after the crate is checked. Doing another pass through the crate shouldn't be needed, and we should be able to do it in the same lint pass, by looking for path references to functions only and then processing them post-crate. Other changes: - `FxHashMap` -> `FxIndexMap` so that we emit warnings in a consistent order, as we see them (making the diff a bit confusing to look at, because warnings were moved around) - no longer storing a `Vec<Span>` per function: an enum representing "seen once" or "seen more than once" should be enough (only the first element is used later) - "used here" help is now a note I also noticed that it lints on trait methods with a default implementation, but not on regular trait methods without a body (because that's what `check_fn` does). I'm not sure if that's useful though, maybe we shouldn't lint trait methods at all? It's not like you can avoid it sometimes (but then again it's a restriction lint). Either way, I left the behavior where it was before so that there are no functional changes made in this PR and it's purely a refactor. I can change it though changelog: none
|
💔 Test failed - checks-action_test |
|
Spurious probably |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
single_call_fnlint worked by first collecting a list of function definitions in the lint pass, then populating the list of uses for each function in a second visitor after the crate is checked.Doing another pass through the crate shouldn't be needed, and we should be able to do it in the same lint pass, by looking for path references to functions only and then processing them post-crate.
Other changes:
FxHashMap->FxIndexMapso that we emit warnings in a consistent order, as we see them (making the diff a bit confusing to look at, because warnings were moved around)Vec<Span>per function: an enum representing "seen once" or "seen more than once" should be enough (only the first element is used later)I also noticed that it lints on trait methods with a default implementation, but not on regular trait methods without a body (because that's what
check_fndoes). I'm not sure if that's useful though, maybe we shouldn't lint trait methods at all? It's not like you can avoid it sometimes (but then again it's a restriction lint). Either way, I left the behavior where it was before so that there are no functional changes made in this PR and it's purely a refactor. I can change it thoughchangelog: none