Add matches_if_let lint for if matches! conditions#16739
Add matches_if_let lint for if matches! conditions#16739Taym95 wants to merge 2 commits intorust-lang:masterfrom
Conversation
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
|
r? @Jarcho rustbot has assigned @Jarcho. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
Lintcheck changes for 94a3813
This comment will be updated if you push new changes |
| if !is_matches_expansion(first_arm, second_arm) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This isn't needed. We already know this is from the matches macro.
| cx: &LateContext<'tcx>, | ||
| expr: &'tcx Expr<'tcx>, | ||
| scrutinee: &'tcx Expr<'tcx>, | ||
| arms: &'tcx [Arm<'tcx>], |
There was a problem hiding this comment.
This should be taking the single arm of interest.
| let Some(if_expr) = get_parent_expr(cx, expr) else { | ||
| return; | ||
| }; | ||
|
|
||
| let ExprKind::If(cond, then, _) = if_expr.kind else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
This will catch if matches!(), but not if x && matches!().
This won't block the PR, but this should be handled.
There was a problem hiding this comment.
Thanks, I tried to fix it part of this RR, please have another look.
| let mut uses_binding_name_in_body = false; | ||
| first_arm.pat.each_binding_or_first(&mut |_, _, _, ident| { | ||
| uses_binding_name_in_body |= contains_name(ident.name, then, cx); | ||
| }); |
There was a problem hiding this comment.
This should be collecting the binding IDs then walking the body once. SsoHashSet would be appropriate here.
| let pat = snippet_with_context(cx, first_arm.pat.span, ctxt, "..", &mut app).0; | ||
| let scrutinee = Sugg::hir_with_context(cx, scrutinee, ctxt, "..", &mut app) | ||
| .maybe_paren() | ||
| .into_string(); |
There was a problem hiding this comment.
Sugg implements Display. No need to call into_string.
| let suggestion = if let Some(guard) = first_arm.guard { | ||
| let guard = Sugg::hir_with_context(cx, guard, ctxt, "..", &mut app) | ||
| .maybe_paren() | ||
| .into_string(); |
There was a problem hiding this comment.
Sugg implements Display. No need to call into_string.
| MATCHES_IF_LET, | ||
| matches_span, | ||
| "this `matches!` can be written as an `if let`", | ||
| "consider using `if let`", |
There was a problem hiding this comment.
We use the action to take here so just "use `if let`".
| cx, | ||
| MATCHES_IF_LET, | ||
| matches_span, | ||
| "this `matches!` can be written as an `if let`", |
There was a problem hiding this comment.
This would be better as something like "`matches!` used as an 'if' condition". All matches can technically be replaced.
| return; | ||
| }; | ||
|
|
||
| if cond.hir_id != expr.hir_id || if_expr.span.from_expansion() { |
There was a problem hiding this comment.
This should be checking that the if is from the same context as the matches call.
|
Reminder, once the PR becomes ready for a review, use |
|
@rustbot label lint-nominated |
|
This lint has been nominated for inclusion. |
Signed-off-by: Taym Haddadi <haddadi.taym@gmail.com>
| if op.node == BinOpKind::And && (left.hir_id == child_id || right.hir_id == child_id) => | ||
| { | ||
| child_id = parent_id; | ||
| needs_if_let_chain = true; | ||
| }, | ||
| ExprKind::DropTemps(inner) if inner.hir_id == child_id => child_id = parent_id, |
There was a problem hiding this comment.
The checks for the child ID on both arms are always true. No need to check.
| } | ||
|
|
||
| let mut visitor = BodyUsesBindingName { cx, binding_names }; | ||
| visitor.visit_expr(then).is_break() |
There was a problem hiding this comment.
This also needs to visit every expression in the condition in the if after the matches.
| ExprKind::Binary(op, left, right) | ||
| if op.node == BinOpKind::And && (left.hir_id == child_id || right.hir_id == child_id) => | ||
| { | ||
| child_id = parent_id; | ||
| needs_if_let_chain = true; | ||
| }, |
There was a problem hiding this comment.
You'll need to record the rhs expression every time you come from the lhs for the binding use check.
fixes #16660
changelog: [
matches_if_let]: add a new restriction lint that suggests replacingif matches!(...)withif letwhen the rewrite is safe, including guarded cases whenif letchains are available..stderrfile)cargo testpasses locallycargo dev update_lintscargo dev fmt