feat({unnecessary,panicking}_unwrap): lint field accesses #15949
Conversation
rustbot
added
the
S-waiting-on-review
| is_potentially_local_place(*local_id, place) | ||
| // If there were projections other than field projections, err on the side of caution and say that they | ||
| // _might_ be mutating something. | ||
| // | ||
| // The reason we use `<=` and not `==` is that a mutation of `struct` or `struct.field1` should count as | ||
| // mutation of the child fields such as `struct.field1.field2` | ||
| && place.projections.len() <= field_indices.len() | ||
| && iter::zip(&place.projections, field_indices.iter().copied().rev()).all(|(proj, field_idx)| { | ||
| match proj.kind { | ||
| ProjectionKind::Field(f_idx, _) => f_idx == field_idx, | ||
| // If this is a projection we don't expect, it _might_ be mutating something | ||
| _ => false, | ||
| } | ||
| }) |
There was a problem hiding this comment.
This effectively extends is_potentially_local_place with support for locals with field projections -- maybe this change should be incorporated into is_potentially_local_place directly?
There was a problem hiding this comment.
I agree, it looks like it. However, I'm currently not sure whether this is (or will be) used elsewhere.
ada4a
force-pushed
the
unnecessary_unwrap
branch
from
0550663 to
23ee56d
Compare
|
Lintcheck changes for 215cfed
This comment will be updated if you push new changes |
|
Oh hey look, the multi-step-field-access pattern does come up |
ada4a
force-pushed
the
unnecessary_unwrap
branch
from
23ee56d to
176748c
Compare
This comment has been minimized.
This comment has been minimized.
|
I think the most interesting question is whether those newly limited cases are worth the additional complexity. |
|
I asked for people's opinions on Zulip: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/Kind-of-FCP.20.60unnecessary_unwrap.60.20for.20field.20accesses/with/555067994 |
|
@Alexendoo noted that we'd need to account for partial moves. His example was: if foo.bar.is_some() {
f(&foo);
foo.bar.unwrap();
}We should have something like this in our tests. |
ada4a
force-pushed
the
unnecessary_unwrap
branch
from
176748c to
215cfed
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
rustbot
removed
the
S-waiting-on-review
3 participants
Resolves #15321
Unresolved questions:
I did implement support for nested field accesses, but I'm not sure if that's desired
EDIT: Decided to keep this after discussion on Zulip
changelog: [
unnecessary_unwrap]: lint field accesseschangelog: [
panicking_unwrap]: lint field accesses