unnecessary_operation: don't suggest autofixes on composite types with fields of uncertain types
#15460
Conversation
5477205 to
dff9f73
Compare
| if let StmtKind::Semi(expr) = stmt.kind | ||
| && !stmt.span.in_external_macro(cx.sess().source_map()) | ||
| && let ctxt = stmt.span.ctxt() | ||
| && 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.
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
| applicability: &mut Applicability, | ||
| ) -> Option<Vec<&'a Expr<'a>>> { |
There was a problem hiding this comment.
It might be better to return the Applicability/include it in the Option instead of mutating it. There is now an implicit invariant here that if None is returned, the applicability must not have been mutated (otherwise e.g. lines where we do .or_else(|| Some(vec![...])) may be using an irrelevant/outdated applicability from a nested expression that we ended up not using)
WDYT?
There was a problem hiding this comment.
That does sound better, yes. One problem is that it would make it more difficult to reduce recursively: we'll need to fold the returned applicabilities to choose the weakest one. I remember seeing an enum similar to Applicability somewhere which encoded that logic using the overloaded BitOr/BitOrAssign impls -- it would be cool to have that here as well
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.
| Some([start, end].into_iter().collect()) | |
| Some(vec![start, end]) |
There was a problem hiding this comment.
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.
- 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.
I've implemented 2. for now
dff9f73 to
5198c64
Compare
this makes the test case `unfixable`, so move it to a separate file
5198c64 to
e783aea
Compare
fixes #15381
changelog: [
unnecessary_operation]: reduce applicability on exprs with uncertain types