Auto merge of #11387 - y21:issue11371, r=blyxyas

[`unnecessary_unwrap`]: lint on `.as_ref().unwrap()`

Closes #11371

This turned out to be a little more code than I originally thought, because the lint also makes sure to not lint if the user tries to mutate the option:
```rs
if option.is_some() {
  option = None;
  option.unwrap(); // don't lint here
}
```
... which means that even if we taught this lint to recognize `.as_mut()`, it would *still* not lint because that would count as a mutation. So we need to allow `.as_mut()` calls but reject other kinds of mutations.
Unfortunately it doesn't look like this is possible with `is_potentially_mutated` (seeing what kind of mutation happened).
This replaces it with a custom little visitor that does basically what it did before, but also allows `.as_mut()`.

changelog: [`unnecessary_unwrap`]: lint on `.as_ref().unwrap()`

Author: bors

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

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,

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() {

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