Fixed FP in unused_io_amount for Ok(lit), unrachable! and unwrap de…
#12217
Conversation
rustbot
added
the
S-waiting-on-review
|
Sorry for opening this can of worms, and for taking as long to fix this ._. |
clippy_lints/src/unused_io_amount.rs
Outdated
| @@ -130,23 +170,46 @@ fn should_lint<'a>(cx: &LateContext<'a>, mut inner: &'a hir::Expr<'a>) -> Option | |||
| check_io_mode(cx, inner) | |||
| } | |||
|
|
|||
| fn pattern_is_ignored_ok(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> bool { | |||
| enum OkPatternKind { | |||
There was a problem hiding this comment.
This could be removed and all instances removed by booleans, and all checking would be easier.
clippy_lints/src/unused_io_amount.rs
Outdated
| if cx.tcx.hir().is_inside_const_context(expr.hir_id) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
| if cx.tcx.hir().is_inside_const_context(expr.hir_id) { | |
| return false; | |
| } | |
| return true; | |
| return !cx.tcx.hir().is_inside_const_context(expr.hir_id); |
clippy_lints/src/unused_io_amount.rs
Outdated
| if let Some(OkPatternKind::WildOrDotDot) = extract_ok_pattern(cx, arm.pat) { | ||
| return ControlFlow::Break(arm.pat); | ||
| } |
There was a problem hiding this comment.
With that previous comment, this could be:
| if let Some(OkPatternKind::WildOrDotDot) = extract_ok_pattern(cx, arm.pat) { | |
| return ControlFlow::Break(arm.pat); | |
| } | |
| return extract_ok_pattern(cx, arm.pat); |
This would also need to return a bool. But as this is the only the branch that returns meaningful information, making the switch won't be hard I'd suppose.
There was a problem hiding this comment.
If we return a bool here, then we lose information on which arm is doing what. These are used to pull the right patterns out of a slice of 2 pats.
clippy_lints/src/unused_io_amount.rs
Outdated
| if let (ControlFlow::Break(ok_pat), ControlFlow::Break(_)) = (ok_arm, err_arm) { | ||
| emit_lint(cx, expr.span, op, &[ok_pat.span]); |
There was a problem hiding this comment.
This would then be:
| if let (ControlFlow::Break(ok_pat), ControlFlow::Break(_)) = (ok_arm, err_arm) { | |
| emit_lint(cx, expr.span, op, &[ok_pat.span]); | |
| if ok_arm && err_arm { | |
| emit_lint(cx, expr.span, op, &[ok_arm.pat.span]); |
|
Do you agree with the changes to the structural matching of the match? |
|
Watch match specifically? Like, the general pattern matching? It's good, even if a little bit confusing (I assume because of the time urgency) |
|
Sorry, basically based on your comments on the issue, I changed the matching logic to only consider two arm cases and nothing else. Once this is stabilized I'd like to clean it up further. |
|
Ok, this looks better. |
|
I hold no objections, but I have the hunch that this can be achieved in a simpler manner. If you think it's okay to merge — given the urgency — then okay. Worst case we revert both commits completely. Would you like me to squash? |
|
Please :) |
We introduce the following rules for match exprs. - `panic!` and `unreachable!` are treated as consumption. - guard expressions in any arm imply consumption. For match exprs: - Lint only if exacrtly 2 non-consuming arms exist - Lint only if one arm is an `Ok(_)` and the other is `Err(_)` Added additional requirement that for a block return expression that is a match, the source must be `Normal`. changelog: FP [`unused_io_amount`] when matching Ok(literal)
|
Done, thanks again and sorry :( |
|
No need to be sorry, you didn't see an edge case, in the same way that I didn't. Instead of having shame of our human error, we should be thankful of the person that filled the issue that led to this moment. @bors r+ |
Fixed FP in `unused_io_amount` for Ok(lit), unrachable! and unwrap de… …sugar Fixes fp caused by linting on Ok(_) for all cases outside binding. We introduce the following rules for match exprs. - `panic!` and `unreachable!` are treated as consumed. - `Ok( )` patterns outside `DotDot` and `Wild` are treated as consuming. changelog: FP [`unused_io_amount`] when matching Ok(literal) or unreachable fixes #12208 r? `@blyxyas`
|
💔 Test failed - checks-action_test |
|
@bors retry |
Fixed FP in `unused_io_amount` for Ok(lit), unrachable! and unwrap de… …sugar Fixes fp caused by linting on Ok(_) for all cases outside binding. We introduce the following rules for match exprs. - `panic!` and `unreachable!` are treated as consumed. - `Ok( )` patterns outside `DotDot` and `Wild` are treated as consuming. changelog: FP [`unused_io_amount`] when matching Ok(literal) or unreachable fixes #12208 r? `@blyxyas`
|
💔 Test failed - checks-action_test |
|
@bors retry |
Fixed FP in `unused_io_amount` for Ok(lit), unrachable! and unwrap de… …sugar Fixes fp caused by linting on Ok(_) for all cases outside binding. We introduce the following rules for match exprs. - `panic!` and `unreachable!` are treated as consumed. - `Ok( )` patterns outside `DotDot` and `Wild` are treated as consuming. changelog: FP [`unused_io_amount`] when matching Ok(literal) or unreachable fixes #12208 r? `@blyxyas`
|
💔 Test failed - checks-action_test |
|
@bors retry |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
…sugar
Fixes fp caused by linting on Ok(_) for all cases outside binding.
We introduce the following rules for match exprs.
panic!andunreachable!are treated as consumed.Ok( )patterns outsideDotDotandWildare treated as consuming.changelog: FP [
unused_io_amount] when matching Ok(literal) or unreachablefixes #12208
r? @blyxyas