Skip to content

Commit 31497d6

Browse files
authored
fix: option_if_let_else FP when value partially moved (#14209)
fixes #13964 The lint `option_map_unwrap_or` used to have a similar issue in #10579, so I borrowed its solution to fix this one. changelog: [`option_if_let_else`]: fix FP when value partially moved
2 parents a422d9e + a3865b1 commit 31497d6

File tree

3 files changed

+124
-2
lines changed

3 files changed

+124
-2
lines changed

clippy_lints/src/option_if_let_else.rs

+84-2
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,23 @@
1+
use std::ops::ControlFlow;
2+
13
use clippy_utils::diagnostics::span_lint_and_sugg;
24
use clippy_utils::sugg::Sugg;
5+
use clippy_utils::ty::is_copy;
36
use clippy_utils::{
47
CaptureKind, can_move_expr_to_closure, eager_or_lazy, higher, is_else_clause, is_in_const_context,
58
is_res_lang_ctor, peel_blocks, peel_hir_expr_while,
69
};
10+
use rustc_data_structures::fx::FxHashSet;
711
use rustc_errors::Applicability;
812
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
913
use rustc_hir::def::Res;
14+
use rustc_hir::intravisit::{Visitor, walk_expr, walk_path};
1015
use rustc_hir::{
11-
Arm, BindingMode, Expr, ExprKind, MatchSource, Mutability, Pat, PatExpr, PatExprKind, PatKind, Path, QPath, UnOp,
16+
Arm, BindingMode, Expr, ExprKind, HirId, MatchSource, Mutability, Node, Pat, PatExpr, PatExprKind, PatKind, Path,
17+
QPath, UnOp,
1218
};
1319
use rustc_lint::{LateContext, LateLintPass};
20+
use rustc_middle::hir::nested_filter;
1421
use rustc_session::declare_lint_pass;
1522
use rustc_span::SyntaxContext;
1623

@@ -110,11 +117,12 @@ fn format_option_in_sugg(cond_sugg: Sugg<'_>, as_ref: bool, as_mut: bool) -> Str
110117
)
111118
}
112119

120+
#[expect(clippy::too_many_lines)]
113121
fn try_get_option_occurrence<'tcx>(
114122
cx: &LateContext<'tcx>,
115123
ctxt: SyntaxContext,
116124
pat: &Pat<'tcx>,
117-
expr: &Expr<'_>,
125+
expr: &'tcx Expr<'_>,
118126
if_then: &'tcx Expr<'_>,
119127
if_else: &'tcx Expr<'_>,
120128
) -> Option<OptionOccurrence> {
@@ -182,6 +190,26 @@ fn try_get_option_occurrence<'tcx>(
182190
Some(CaptureKind::Ref(Mutability::Not)) | None => (),
183191
}
184192
}
193+
} else if !is_copy(cx, cx.typeck_results().expr_ty(expr))
194+
// TODO: Cover more match cases
195+
&& matches!(
196+
expr.kind,
197+
ExprKind::Field(_, _) | ExprKind::Path(_) | ExprKind::Index(_, _, _)
198+
)
199+
{
200+
let mut condition_visitor = ConditionVisitor {
201+
cx,
202+
identifiers: FxHashSet::default(),
203+
};
204+
condition_visitor.visit_expr(cond_expr);
205+
206+
let mut reference_visitor = ReferenceVisitor {
207+
cx,
208+
identifiers: condition_visitor.identifiers,
209+
};
210+
if reference_visitor.visit_expr(none_body).is_break() {
211+
return None;
212+
}
185213
}
186214

187215
let mut app = Applicability::Unspecified;
@@ -219,6 +247,60 @@ fn try_get_option_occurrence<'tcx>(
219247
None
220248
}
221249

250+
/// This visitor looks for bindings in the <then> block that mention a local variable. Then gets the
251+
/// identifiers. The list of identifiers will then be used to check if the <none> block mentions the
252+
/// same local. See [`ReferenceVisitor`] for more.
253+
struct ConditionVisitor<'a, 'tcx> {
254+
cx: &'a LateContext<'tcx>,
255+
identifiers: FxHashSet<HirId>,
256+
}
257+
258+
impl<'tcx> Visitor<'tcx> for ConditionVisitor<'_, 'tcx> {
259+
type NestedFilter = nested_filter::All;
260+
261+
fn visit_path(&mut self, path: &Path<'tcx>, _: HirId) {
262+
if let Res::Local(local_id) = path.res
263+
&& let Node::Pat(pat) = self.cx.tcx.hir_node(local_id)
264+
&& let PatKind::Binding(_, local_id, ..) = pat.kind
265+
{
266+
self.identifiers.insert(local_id);
267+
}
268+
walk_path(self, path);
269+
}
270+
271+
fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt {
272+
self.cx.tcx
273+
}
274+
}
275+
276+
/// This visitor checks if the <none> block contains references to the local variables that are
277+
/// used in the <then> block. See [`ConditionVisitor`] for more.
278+
struct ReferenceVisitor<'a, 'tcx> {
279+
cx: &'a LateContext<'tcx>,
280+
identifiers: FxHashSet<HirId>,
281+
}
282+
283+
impl<'tcx> Visitor<'tcx> for ReferenceVisitor<'_, 'tcx> {
284+
type NestedFilter = nested_filter::All;
285+
type Result = ControlFlow<()>;
286+
fn visit_expr(&mut self, expr: &'tcx Expr<'_>) -> ControlFlow<()> {
287+
if let ExprKind::Path(ref path) = expr.kind
288+
&& let QPath::Resolved(_, path) = path
289+
&& let Res::Local(local_id) = path.res
290+
&& let Node::Pat(pat) = self.cx.tcx.hir_node(local_id)
291+
&& let PatKind::Binding(_, local_id, ..) = pat.kind
292+
&& self.identifiers.contains(&local_id)
293+
{
294+
return ControlFlow::Break(());
295+
}
296+
walk_expr(self, expr)
297+
}
298+
299+
fn maybe_tcx(&mut self) -> Self::MaybeTyCtxt {
300+
self.cx.tcx
301+
}
302+
}
303+
222304
fn try_get_inner_pat_and_is_result<'tcx>(cx: &LateContext<'tcx>, pat: &Pat<'tcx>) -> Option<(&'tcx Pat<'tcx>, bool)> {
223305
if let PatKind::TupleStruct(ref qpath, [inner_pat], ..) = pat.kind {
224306
let res = cx.qpath_res(qpath, pat.hir_id);

tests/ui/option_if_let_else.fixed

+20
Original file line numberDiff line numberDiff line change
@@ -268,3 +268,23 @@ fn issue11893() {
268268
panic!("Haven't thought about this condition.");
269269
}
270270
}
271+
272+
mod issue13964 {
273+
#[derive(Debug)]
274+
struct A(Option<String>);
275+
276+
fn foo(a: A) {
277+
let _ = match a.0 {
278+
Some(x) => x,
279+
None => panic!("{a:?} is invalid."),
280+
};
281+
}
282+
283+
fn bar(a: A) {
284+
let _ = if let Some(x) = a.0 {
285+
x
286+
} else {
287+
panic!("{a:?} is invalid.")
288+
};
289+
}
290+
}

tests/ui/option_if_let_else.rs

+20
Original file line numberDiff line numberDiff line change
@@ -331,3 +331,23 @@ fn issue11893() {
331331
panic!("Haven't thought about this condition.");
332332
}
333333
}
334+
335+
mod issue13964 {
336+
#[derive(Debug)]
337+
struct A(Option<String>);
338+
339+
fn foo(a: A) {
340+
let _ = match a.0 {
341+
Some(x) => x,
342+
None => panic!("{a:?} is invalid."),
343+
};
344+
}
345+
346+
fn bar(a: A) {
347+
let _ = if let Some(x) = a.0 {
348+
x
349+
} else {
350+
panic!("{a:?} is invalid.")
351+
};
352+
}
353+
}

0 commit comments

Comments
 (0)