-
Notifications
You must be signed in to change notification settings - Fork 1.8k
unnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
#15460
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
base: master
Are you sure you want to change the base?
unnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
#15460
Conversation
5477205 to
dff9f73
Compare
| && expr.span.ctxt() == ctxt | ||
| && let Some(reduced) = reduce_expression(cx, expr) | ||
| && let Some(reduced) = reduce_expression(cx, expr, &mut applicability) | ||
| && reduced.iter().all(|e| e.span.ctxt() == ctxt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of off-topic: I was forced to use the more verbose Vec::new() syntax in all the tests because, when I used vec![], this check would prevent the lint from firing. That doesn't sound quite right to me?
unnecessary_operation: don't lint on composite types with fields of uncertain typesunnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
clippy_lints/src/no_effect.rs
Outdated
| && let Some(end) = range.end => | ||
| { | ||
| if [start, end].into_iter().any(|e| expr_type_is_certain(cx, e)) { | ||
| Some([start, end].into_iter().collect()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Some([start, end].into_iter().collect()) | |
| Some(vec![start, end]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also, two more high level questions here:
-
Should this also recurse into
reduce_expression()with the start and end expressions of the range, to reduce it even further? I see this also isn't consistently done for some other kinds of expressions, like for array literals and struct expressions, so I'd also be fine with leaving it, but I'm not sure why it's done this way. -
Why do we return
None/not reduce ranges at all if their type isn't certain, instead of only reducing the applicability like we do for structs in general? It still seems worth linting range expression statements that exist solely to make the inner expressions type-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think reducing too much in one go can end up creating some pretty confusing suggestions.. Although maybe they could make sense for the target audience of this lint (who I'm not really sure who is, except for people that have made a typo in a quick reproducer -- in which case the complete reduction might be actively counter-productive for understanding what the lint wants from them)
- I could totally make that change, yes. I guess it was just me too closely copying the structure of the regular
Structarm -- there, the suggestion is deleted when the type has an explicitDropimpl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented 2. for now
5198c64 to
e783aea
Compare
e783aea to
aaa0940
Compare
This comment has been minimized.
This comment has been minimized.
91c4c17 to
d7dbefd
Compare
|
@y21 friendly ping |
This comment has been minimized.
This comment has been minimized.
d7dbefd to
9e1cd08
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9e1cd08 to
6555bfb
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. |
fixes #15381
changelog: [
unnecessary_operation]: reduce applicability on exprs with uncertain types