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.

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

Refactor useless_vec lint #14503

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

y21
Copy link
Member

@y21 y21 commented Mar 29, 2025

Currently there's an "implicit" invariant in this lint's code, which has been bugging me for a while (but especially so because I recently wanted to extend this lint): when we see a vec![] expression that we can't suggest changing to an array because of surrounding expressions (e.g. it's passed to a function that requires a Vec), we must insert that into this self.span_to_lint_map map with None.

See FP issue #11861 for the motivating example of why this lint is doing that (and the doc comment in the code I added).

This invariant is really easy to miss and error prone: in the old code, every other } else { branch needed a self.span_to_lint_map.insert(.., None) call.

This PR moves out the logic that checks if an expression needs to be a vec based on its surrounding environment. This way we can cover all cases with one else at its callsite and only need to insert None in one isolated place.

I was also working on extending this lint with another pattern, where refactoring the "does this expression need to be a vec or does a slice work too" into its own function would be very convenient.

(Tbh, I don't think this invariant/FP actually matters much in practice, because it's a bit of an edge case IMO. I don't think it's really worth all the complexity (even though I was the one to suggest how we could fix this in the linked issue). This is also already an issue with every other lint that can change an expression's type. I also don't know if this is compatible with "incremental lints", which IIUC work per-module (I know that MSRV handling recently had to change due to that). In any case, I just decided to keep it for now so that this is purely an internal refactor without user facing changes.)

Fixes #14531 (indirectly, added a test for it)

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2025

r? @dswij

rustbot has assigned @dswij.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 29, 2025
@y21 y21 force-pushed the useless_vec_rework branch 2 times, most recently from e6ccc88 to 834da8f Compare March 29, 2025 23:58
@y21 y21 force-pushed the useless_vec_rework branch from 834da8f to 61ae64b Compare April 3, 2025 15:26
@y21
Copy link
Member Author

y21 commented Apr 3, 2025

Looks like this also fixes the bug in #14531 indirectly (previously it hardcoded SuggestedType::Array for the suggested type but that's obviously wrong for &vec![], with this change we determine whether to suggest an array or a slice for all detected patterns in the same way). Nice.

I added a test for this in the second commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

clippy --fix generates unbuildable code for a useless_vec fix
3 participants