diff options
| -rw-r--r-- | clippy_lints/src/unwrap.rs | 111 | ||||
| -rw-r--r-- | clippy_utils/src/usage.rs | 13 | ||||
| -rw-r--r-- | tests/ui/checked_unwrap/simple_conditionals.rs | 51 | ||||
| -rw-r--r-- | tests/ui/checked_unwrap/simple_conditionals.stderr | 70 |
4 files changed, 235 insertions, 10 deletions
diff --git a/clippy_lints/src/unwrap.rs b/clippy_lints/src/unwrap.rs index c99b0290c0c..9a0d83d83f1 100644 --- a/clippy_lints/src/unwrap.rs +++ b/clippy_lints/src/unwrap.rs @@ -1,15 +1,18 @@ use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::usage::is_potentially_mutated; +use clippy_utils::usage::is_potentially_local_place; use clippy_utils::{higher, path_to_local}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, Visitor}; -use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, PathSegment, UnOp}; +use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Node, PathSegment, UnOp}; +use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceWithHirId}; +use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::nested_filter; use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::Ty; +use rustc_middle::mir::FakeReadCause; +use rustc_middle::ty::{self, Ty, TyCtxt}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::def_id::LocalDefId; use rustc_span::source_map::Span; @@ -192,6 +195,55 @@ fn collect_unwrap_info<'tcx>( Vec::new() } +/// A HIR visitor delegate that checks if a local variable of type `Option<_>` is mutated, +/// *except* for if `Option::as_mut` is called. +/// The reason for why we allow that one specifically is that `.as_mut()` cannot change +/// the option to `None`, and that is important because this lint relies on the fact that +/// `is_some` + `unwrap` is equivalent to `if let Some(..) = ..`, which it would not be if +/// the option is changed to None between `is_some` and `unwrap`. +/// (And also `.as_mut()` is a somewhat common method that is still worth linting on.) +struct MutationVisitor<'tcx> { + is_mutated: bool, + local_id: HirId, + tcx: TyCtxt<'tcx>, +} + +/// Checks if the parent of the expression pointed at by the given `HirId` is a call to +/// `Option::as_mut`. +/// +/// Used by the mutation visitor to specifically allow `.as_mut()` calls. +/// In particular, the `HirId` that the visitor receives is the id of the local expression +/// (i.e. the `x` in `x.as_mut()`), and that is the reason for why we care about its parent +/// expression: that will be where the actual method call is. +fn is_option_as_mut_use(tcx: TyCtxt<'_>, expr_id: HirId) -> bool { + if let Node::Expr(mutating_expr) = tcx.hir().get_parent(expr_id) + && let ExprKind::MethodCall(path, ..) = mutating_expr.kind + { + path.ident.name.as_str() == "as_mut" + } else { + false + } +} + +impl<'tcx> Delegate<'tcx> for MutationVisitor<'tcx> { + fn borrow(&mut self, cat: &PlaceWithHirId<'tcx>, diag_expr_id: HirId, bk: ty::BorrowKind) { + if let ty::BorrowKind::MutBorrow = bk + && is_potentially_local_place(self.local_id, &cat.place) + && !is_option_as_mut_use(self.tcx, diag_expr_id) + { + self.is_mutated = true; + } + } + + fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) { + self.is_mutated = true; + } + + fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {} +} + impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> { fn visit_branch( &mut self, @@ -202,10 +254,26 @@ impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> { ) { let prev_len = self.unwrappables.len(); for unwrap_info in collect_unwrap_info(self.cx, if_expr, cond, branch, else_branch, true) { - if is_potentially_mutated(unwrap_info.local_id, cond, self.cx) - || is_potentially_mutated(unwrap_info.local_id, branch, self.cx) - { - // if the variable is mutated, we don't know whether it can be unwrapped: + let mut delegate = MutationVisitor { + tcx: self.cx.tcx, + is_mutated: false, + local_id: unwrap_info.local_id, + }; + + let infcx = self.cx.tcx.infer_ctxt().build(); + let mut vis = ExprUseVisitor::new( + &mut delegate, + &infcx, + cond.hir_id.owner.def_id, + self.cx.param_env, + self.cx.typeck_results(), + ); + vis.walk_expr(cond); + vis.walk_expr(branch); + + if delegate.is_mutated { + // if the variable is mutated, we don't know whether it can be unwrapped. + // it might have been changed to `None` in between `is_some` + `unwrap`. continue; } self.unwrappables.push(unwrap_info); @@ -215,6 +283,27 @@ impl<'a, 'tcx> UnwrappableVariablesVisitor<'a, 'tcx> { } } +enum AsRefKind { + AsRef, + AsMut, +} + +/// Checks if the expression is a method call to `as_{ref,mut}` and returns the receiver of it. +/// If it isn't, the expression itself is returned. +fn consume_option_as_ref<'tcx>(expr: &'tcx Expr<'tcx>) -> (&'tcx Expr<'tcx>, Option<AsRefKind>) { + if let ExprKind::MethodCall(path, recv, ..) = expr.kind { + if path.ident.name == sym::as_ref { + (recv, Some(AsRefKind::AsRef)) + } else if path.ident.name.as_str() == "as_mut" { + (recv, Some(AsRefKind::AsMut)) + } else { + (expr, None) + } + } else { + (expr, None) + } +} + impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> { type NestedFilter = nested_filter::OnlyBodies; @@ -233,6 +322,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> { // find `unwrap[_err]()` calls: if_chain! { if let ExprKind::MethodCall(method_name, self_arg, ..) = expr.kind; + let (self_arg, as_ref_kind) = consume_option_as_ref(self_arg); if let Some(id) = path_to_local(self_arg); if [sym::unwrap, sym::expect, sym!(unwrap_err)].contains(&method_name.ident.name); let call_to_unwrap = [sym::unwrap, sym::expect].contains(&method_name.ident.name); @@ -268,7 +358,12 @@ impl<'a, 'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'a, 'tcx> { unwrappable.check.span.with_lo(unwrappable.if_expr.span.lo()), "try", format!( - "if let {suggested_pattern} = {unwrappable_variable_name}", + "if let {suggested_pattern} = {borrow_prefix}{unwrappable_variable_name}", + borrow_prefix = match as_ref_kind { + Some(AsRefKind::AsRef) => "&", + Some(AsRefKind::AsMut) => "&mut ", + None => "", + }, ), // We don't track how the unwrapped value is used inside the // block or suggest deleting the unwrap, so we can't offer a diff --git a/clippy_utils/src/usage.rs b/clippy_utils/src/usage.rs index 39ef76348d7..ec131c7f6a3 100644 --- a/clippy_utils/src/usage.rs +++ b/clippy_utils/src/usage.rs @@ -4,7 +4,7 @@ use core::ops::ControlFlow; use hir::def::Res; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::{self as hir, Expr, ExprKind, HirId, HirIdSet}; -use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; +use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, Place, PlaceBase, PlaceWithHirId}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; @@ -37,6 +37,17 @@ pub fn is_potentially_mutated<'tcx>(variable: HirId, expr: &'tcx Expr<'_>, cx: & mutated_variables(expr, cx).map_or(true, |mutated| mutated.contains(&variable)) } +pub fn is_potentially_local_place(local_id: HirId, place: &Place<'_>) -> bool { + match place.base { + PlaceBase::Local(id) => id == local_id, + PlaceBase::Upvar(_) => { + // Conservatively assume yes. + true + }, + _ => false, + } +} + struct MutVarsDelegate { used_mutably: HirIdSet, skip: bool, diff --git a/tests/ui/checked_unwrap/simple_conditionals.rs b/tests/ui/checked_unwrap/simple_conditionals.rs index e82e7bcb06c..02f80cc52ac 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.rs +++ b/tests/ui/checked_unwrap/simple_conditionals.rs @@ -128,6 +128,57 @@ fn main() { assert!(x.is_ok(), "{:?}", x.unwrap_err()); } +fn issue11371() { + let option = Some(()); + + if option.is_some() { + option.as_ref().unwrap(); + //~^ ERROR: called `unwrap` on `option` after checking its variant with `is_some` + } else { + option.as_ref().unwrap(); + //~^ ERROR: this call to `unwrap()` will always panic + } + + let result = Ok::<(), ()>(()); + + if result.is_ok() { + result.as_ref().unwrap(); + //~^ ERROR: called `unwrap` on `result` after checking its variant with `is_ok` + } else { + result.as_ref().unwrap(); + //~^ ERROR: this call to `unwrap()` will always panic + } + + let mut option = Some(()); + if option.is_some() { + option.as_mut().unwrap(); + //~^ ERROR: called `unwrap` on `option` after checking its variant with `is_some` + } else { + option.as_mut().unwrap(); + //~^ ERROR: this call to `unwrap()` will always panic + } + + let mut result = Ok::<(), ()>(()); + if result.is_ok() { + result.as_mut().unwrap(); + //~^ ERROR: called `unwrap` on `result` after checking its variant with `is_ok` + } else { + result.as_mut().unwrap(); + //~^ ERROR: this call to `unwrap()` will always panic + } + + // This should not lint. Statics are, at the time of writing, not linted on anyway, + // but if at some point they are supported by this lint, it should correctly see that + // `X` is being mutated and not suggest `if let Some(..) = X {}` + static mut X: Option<i32> = Some(123); + unsafe { + if X.is_some() { + X = None; + X.unwrap(); + } + } +} + fn check_expect() { let x = Some(()); if x.is_some() { diff --git a/tests/ui/checked_unwrap/simple_conditionals.stderr b/tests/ui/checked_unwrap/simple_conditionals.stderr index ed603581ecd..a5afbba7317 100644 --- a/tests/ui/checked_unwrap/simple_conditionals.stderr +++ b/tests/ui/checked_unwrap/simple_conditionals.stderr @@ -168,5 +168,73 @@ LL | if x.is_err() { LL | x.unwrap_err(); | ^^^^^^^^^^^^^^ -error: aborting due to 17 previous errors +error: called `unwrap` on `option` after checking its variant with `is_some` + --> $DIR/simple_conditionals.rs:135:9 + | +LL | if option.is_some() { + | ------------------- help: try: `if let Some(..) = &option` +LL | option.as_ref().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> $DIR/simple_conditionals.rs:138:9 + | +LL | if option.is_some() { + | ---------------- because of this check +... +LL | option.as_ref().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `result` after checking its variant with `is_ok` + --> $DIR/simple_conditionals.rs:145:9 + | +LL | if result.is_ok() { + | ----------------- help: try: `if let Ok(..) = &result` +LL | result.as_ref().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> $DIR/simple_conditionals.rs:148:9 + | +LL | if result.is_ok() { + | -------------- because of this check +... +LL | result.as_ref().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `option` after checking its variant with `is_some` + --> $DIR/simple_conditionals.rs:154:9 + | +LL | if option.is_some() { + | ------------------- help: try: `if let Some(..) = &mut option` +LL | option.as_mut().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> $DIR/simple_conditionals.rs:157:9 + | +LL | if option.is_some() { + | ---------------- because of this check +... +LL | option.as_mut().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: called `unwrap` on `result` after checking its variant with `is_ok` + --> $DIR/simple_conditionals.rs:163:9 + | +LL | if result.is_ok() { + | ----------------- help: try: `if let Ok(..) = &mut result` +LL | result.as_mut().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this call to `unwrap()` will always panic + --> $DIR/simple_conditionals.rs:166:9 + | +LL | if result.is_ok() { + | -------------- because of this check +... +LL | result.as_mut().unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 25 previous errors |