Skip to content

Make question_mark to remove redundant block when outer scope will not be polluted #16936

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.

Sign up for GitHub

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

Open
profetia wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make question_mark to remove redundant block when outer scope will not be polluted #16936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 8 additions & 126 deletions clippy_lints/src/matches/match_single_binding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
use std::ops::ControlFlow;

use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::macros::HirNode as _;
use clippy_utils::source::{indent_of, reindent_multiline, snippet, snippet_block_with_context, snippet_with_context};
use clippy_utils::usage::{variable_names_of_pat, variable_names_used_after_expr};
use clippy_utils::{is_expr_identity_of_pat, is_refutable, peel_blocks};
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir::def::Res;
use rustc_hir::intravisit::{Visitor, walk_block, walk_expr, walk_path, walk_stmt};
use rustc_hir::{Arm, Block, Expr, ExprKind, HirId, Item, ItemKind, Node, PatKind, Path, Stmt, StmtKind};
use rustc_hir::{Arm, Expr, ExprKind, Item, ItemKind, Node, PatKind, StmtKind};
use rustc_lint::LateContext;
use rustc_span::{Span, Symbol};
use rustc_span::Span;

use super::MATCH_SINGLE_BINDING;

Expand Down Expand Up @@ -57,7 +53,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
&mut app,
Some(span),
true,
is_var_binding_used_later(cx, expr, &arms[0]),
variable_names_from_match_used_after_expr(cx, expr, &arms[0]),
);

span_lint_and_sugg(
Expand Down Expand Up @@ -103,7 +99,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
&mut app,
None,
true,
is_var_binding_used_later(cx, expr, &arms[0]),
variable_names_from_match_used_after_expr(cx, expr, &arms[0]),
);
(expr.span, sugg)
},
Expand Down Expand Up @@ -157,123 +153,9 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e
}
}

struct VarBindingVisitor<'a, 'tcx> {
cx: &'a LateContext<'tcx>,
identifiers: FxHashSet<Symbol>,
}

impl<'tcx> Visitor<'tcx> for VarBindingVisitor<'_, 'tcx> {
type Result = ControlFlow<()>;

fn visit_path(&mut self, path: &Path<'tcx>, _: HirId) -> Self::Result {
if let Res::Local(_) = path.res
&& let [segment] = path.segments
&& self.identifiers.contains(&segment.ident.name)
{
return ControlFlow::Break(());
}

walk_path(self, path)
}

fn visit_block(&mut self, block: &'tcx Block<'tcx>) -> Self::Result {
let before = self.identifiers.clone();
walk_block(self, block)?;
self.identifiers = before;
ControlFlow::Continue(())
}

fn visit_stmt(&mut self, stmt: &'tcx Stmt<'tcx>) -> Self::Result {
if let StmtKind::Let(let_stmt) = stmt.kind {
if let Some(init) = let_stmt.init {
self.visit_expr(init)?;
}

let_stmt.pat.each_binding(|_, _, _, ident| {
self.identifiers.remove(&ident.name);
});
}
walk_stmt(self, stmt)
}

fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> Self::Result {
match expr.kind {
ExprKind::If(
Expr {
kind: ExprKind::Let(let_expr),
..
},
then,
else_,
) => {
self.visit_expr(let_expr.init)?;
let before = self.identifiers.clone();
let_expr.pat.each_binding(|_, _, _, ident| {
self.identifiers.remove(&ident.name);
});

self.visit_expr(then)?;
self.identifiers = before;
if let Some(else_) = else_ {
self.visit_expr(else_)?;
}
ControlFlow::Continue(())
},
ExprKind::Closure(closure) => {
let body = self.cx.tcx.hir_body(closure.body);
let before = self.identifiers.clone();
for param in body.params {
param.pat.each_binding(|_, _, _, ident| {
self.identifiers.remove(&ident.name);
});
}
self.visit_expr(body.value)?;
self.identifiers = before;
ControlFlow::Continue(())
},
ExprKind::Match(expr, arms, _) => {
self.visit_expr(expr)?;
for arm in arms {
let before = self.identifiers.clone();
arm.pat.each_binding(|_, _, _, ident| {
self.identifiers.remove(&ident.name);
});
if let Some(guard) = arm.guard {
self.visit_expr(guard)?;
}
self.visit_expr(arm.body)?;
self.identifiers = before;
}
ControlFlow::Continue(())
},
_ => walk_expr(self, expr),
}
}
}

fn is_var_binding_used_later(cx: &LateContext<'_>, expr: &Expr<'_>, arm: &Arm<'_>) -> bool {
let Node::Stmt(stmt) = cx.tcx.parent_hir_node(expr.hir_id) else {
return false;
};
let Node::Block(block) = cx.tcx.parent_hir_node(stmt.hir_id) else {
return false;
};

let mut identifiers = FxHashSet::default();
arm.pat.each_binding(|_, _, _, ident| {
identifiers.insert(ident.name);
});

let mut visitor = VarBindingVisitor { cx, identifiers };
block
.stmts
.iter()
.skip_while(|s| s.hir_id != stmt.hir_id)
.skip(1)
.any(|stmt| matches!(visitor.visit_stmt(stmt), ControlFlow::Break(())))
|| block
.expr
.is_some_and(|expr| matches!(visitor.visit_expr(expr), ControlFlow::Break(())))
fn variable_names_from_match_used_after_expr(cx: &LateContext<'_>, expr: &Expr<'_>, arm: &Arm<'_>) -> bool {
let names = variable_names_of_pat(arm.pat);
variable_names_used_after_expr(cx, names, expr)
}

/// Returns true if the `ex` match expression is in a local (`let`) or assign expression
Expand Down
109 changes: 80 additions & 29 deletions clippy_lints/src/question_mark.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@ use clippy_utils::res::{MaybeDef as _, MaybeQPath as _, MaybeResPath as _};
use clippy_utils::source::{indent_of, reindent_multiline, snippet_with_applicability, snippet_with_context};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{implements_trait, is_copy};
use clippy_utils::usage::local_used_after_expr;
use clippy_utils::usage::{
local_used_after_expr, variable_names_of_block, variable_names_of_pat, variable_names_used_after_expr,
};
use clippy_utils::{
eq_expr_value, fn_def_id_with_node_args, higher, is_else_clause, is_in_const_context, is_lint_allowed,
is_none_expr, is_none_pattern, pat_and_expr_can_be_question_mark, peel_blocks, peel_blocks_with_stmt,
Expand All @@ -23,7 +25,6 @@ use rustc_hir::{
};
use rustc_lint::{LateContext, LateLintPass, impl_lint_pass};
use rustc_middle::ty::{self, Ty};
use rustc_span::Span;
use rustc_span::symbol::Symbol;

declare_clippy_lint! {
Expand Down Expand Up @@ -387,7 +388,7 @@ fn check_arm_is_some_or_ok<'tcx>(
return Some(if peel_blocks(arm.body).res_local_id() == Some(binding) {
IfLetOrMatchThen::DirectReturn
} else {
IfLetOrMatchThen::ManualUnwrap(val_binding.span, arm.body)
IfLetOrMatchThen::ManualUnwrap(val_binding, arm.body)
});
}

Expand Down Expand Up @@ -462,7 +463,7 @@ enum IfLetOrMatchThen<'tcx> {
/// Return the binding from an if let or match arm as is.
DirectReturn,
/// Working on the binding from an if let or match arm as if it comes from a `?`.
ManualUnwrap(Span, &'tcx Expr<'tcx>),
ManualUnwrap(&'tcx Pat<'tcx>, &'tcx Expr<'tcx>),
}

fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
Expand Down Expand Up @@ -490,23 +491,16 @@ fn check_if_try_match<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
applicability,
);
},
IfLetOrMatchThen::ManualUnwrap(binding_span, arm_body) => {
let indent = indent_of(cx, expr.span).unwrap_or_default();
let arm_body_snippet = snippet_with_applicability(cx, arm_body.span, "..", &mut applicability);
let mut sugg = reindent_multiline(&arm_body_snippet, true, Some(indent));
let binding_snippet = snippet_with_applicability(cx, binding_span, "..", &mut applicability);
let inner_indent = " ".repeat(indent + 4);
if matches!(arm_body.kind, ExprKind::Block(..)) && sugg.starts_with('{') {
sugg.insert_str(
1,
&format!("\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}?;"),
);
} else {
let outer_indent = " ".repeat(indent);
sugg = format!(
"{{\n{inner_indent}let {binding_snippet} = {scrutinee_snippet}?;\n{inner_indent}{sugg}\n{outer_indent}}}"
);
}
IfLetOrMatchThen::ManualUnwrap(binding, arm_body) => {
let sugg = build_suggestion_for_if_let_or_match(
cx,
expr,
binding,
arm_body,
&scrutinee_snippet,
&mut applicability,
false,
);
diag.span_suggestion(expr.span, "try instead", sugg, applicability);
},
}
Expand Down Expand Up @@ -563,9 +557,12 @@ fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr:
|diag| {
let mut applicability = Applicability::MachineApplicable;
let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
let parent = cx.tcx.parent_hir_node(expr.hir_id);
let requires_semi = match parent {
Node::Stmt(stmt) => matches!(stmt.kind, StmtKind::Expr(_)),
_ => cx.typeck_results().expr_ty(expr).is_unit(),
};
if !is_option_early_return || peel_blocks(if_then).res_local_id() == Some(bind_id) {
let parent = cx.tcx.parent_hir_node(expr.hir_id);
let requires_semi = matches!(parent, Node::Stmt(_)) || cx.typeck_results().expr_ty(expr).is_unit();
let method_call_str = match by_ref {
ByRef::Yes(_, Mutability::Mut) => ".as_mut()",
ByRef::Yes(_, Mutability::Not) => ".as_ref()",
Expand All @@ -586,19 +583,73 @@ fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr:
return;
}

let mut sugg = snippet_with_applicability(cx, if_then.span, "..", &mut applicability).into_owned();
let binding_snippet = snippet_with_applicability(cx, field.span, "..", &mut applicability);
let indent = indent_of(cx, expr.span).unwrap_or_default();
sugg.insert_str(
1,
&format!("\n{}let {binding_snippet} = {receiver_str}?;", " ".repeat(indent + 4)),
let sugg = build_suggestion_for_if_let_or_match(
cx,
expr,
field,
if_then,
&receiver_str,
&mut applicability,
requires_semi,
);
diag.span_suggestion(expr.span, "replace it with", sugg, applicability);
},
);
}
}

fn build_suggestion_for_if_let_or_match<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'tcx>,
pat: &Pat<'_>,
then: &Expr<'_>,
scrutinee_snippet: &str,
applicability: &mut Applicability,
mut requires_semi: bool,
) -> String {
let then_snippet = snippet_with_applicability(cx, then.span, "..", applicability);
let pat_snippet = snippet_with_applicability(cx, pat.span, "..", applicability);

let then_is_block = matches!(then.kind, ExprKind::Block(..));
let sugg = if then_is_block && then_snippet.starts_with('{') {
then_snippet.trim_start_matches('{').trim_end_matches('}').trim()
} else {
// Add a semicolon if `then` is a block without braces, which indicates it is an assignment
// desugaring, e.g. `(a, b) = (c, d)`.
if then_is_block {
requires_semi = true;
}
&then_snippet
};

let indent = indent_of(cx, expr.span).unwrap_or_default();
let parent = cx.tcx.parent_hir_node(expr.hir_id);
let outer_indent = " ".repeat(indent);
if !matches!(parent, Node::Stmt(_) | Node::Block(_)) || {
let mut names = variable_names_of_pat(pat);
if let ExprKind::Block(block, _) = then.kind {
#[expect(
rustc::potential_query_instability,
reason = "checking if variable names are used is not sensitive to order"
)]
names.extend(variable_names_of_block(block));
}
variable_names_used_after_expr(cx, names, expr)
} {
let inner_indent = " ".repeat(indent + 4);
format!(
"{{\n{inner_indent}let {pat_snippet} = {scrutinee_snippet}?;\n{inner_indent}{}\n{outer_indent}}}",
reindent_multiline(sugg, true, Some(indent + 4))
)
} else {
format!(
"let {pat_snippet} = {scrutinee_snippet}?;\n{outer_indent}{}{}",
reindent_multiline(sugg, true, Some(indent)),
if requires_semi { ";" } else { "" }
)
}
}

impl QuestionMark {
fn inside_try_block(&self) -> bool {
self.try_block_depth_stack.last() > Some(&0)
Expand Down
Loading