unwrap.rs cleanup (#14761)

This cleans up `unwrap.rs`:

- use interned symbols instead of strings
- update names to reflect the current implementation
- replaced a cascaded `if` by a shorter `match`

changelog: none

Author: dswij

clippy_lints/src/unwrap.rs

@@ -4,15 +4,15 @@ use clippy_utils::usage::is_potentially_local_place;
 use clippy_utils::{higher, path_to_local, sym};
 use rustc_errors::Applicability;
 use rustc_hir::intravisit::{FnKind, Visitor, walk_expr, walk_fn};
-use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Node, PathSegment, UnOp};
+use rustc_hir::{BinOpKind, Body, Expr, ExprKind, FnDecl, HirId, Node, UnOp};
 use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceWithHirId};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::nested_filter;
 use rustc_middle::mir::FakeReadCause;
 use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_session::declare_lint_pass;
-use rustc_span::Span;
 use rustc_span::def_id::LocalDefId;
+use rustc_span::{Span, Symbol};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -111,7 +111,7 @@ struct UnwrapInfo<'tcx> {
     /// The check, like `x.is_ok()`
     check: &'tcx Expr<'tcx>,
     /// The check's name, like `is_ok`
-    check_name: &'tcx PathSegment<'tcx>,
+    check_name: Symbol,
     /// The branch where the check takes place, like `if x.is_ok() { .. }`
     branch: &'tcx Expr<'tcx>,
     /// Whether `is_some()` or `is_ok()` was called (as opposed to `is_err()` or `is_none()`).
@@ -133,12 +133,12 @@ fn collect_unwrap_info<'tcx>(
     invert: bool,
     is_entire_condition: bool,
 ) -> Vec<UnwrapInfo<'tcx>> {
-    fn is_relevant_option_call(cx: &LateContext<'_>, ty: Ty<'_>, method_name: &str) -> bool {
-        is_type_diagnostic_item(cx, ty, sym::Option) && ["is_some", "is_none"].contains(&method_name)
+    fn is_relevant_option_call(cx: &LateContext<'_>, ty: Ty<'_>, method_name: Symbol) -> bool {
+        is_type_diagnostic_item(cx, ty, sym::Option) && matches!(method_name, sym::is_none | sym::is_some)
     }
 
-    fn is_relevant_result_call(cx: &LateContext<'_>, ty: Ty<'_>, method_name: &str) -> bool {
-        is_type_diagnostic_item(cx, ty, sym::Result) && ["is_ok", "is_err"].contains(&method_name)
+    fn is_relevant_result_call(cx: &LateContext<'_>, ty: Ty<'_>, method_name: Symbol) -> bool {
+        is_type_diagnostic_item(cx, ty, sym::Result) && matches!(method_name, sym::is_err | sym::is_ok)
     }
 
     if let ExprKind::Binary(op, left, right) = &expr.kind {
@@ -155,14 +155,10 @@ fn collect_unwrap_info<'tcx>(
     } else if let ExprKind::MethodCall(method_name, receiver, [], _) = &expr.kind
         && let Some(local_id) = path_to_local(receiver)
         && let ty = cx.typeck_results().expr_ty(receiver)
-        && let name = method_name.ident.as_str()
+        && let name = method_name.ident.name
         && (is_relevant_option_call(cx, ty, name) || is_relevant_result_call(cx, ty, name))
     {
-        let unwrappable = match name {
-            "is_some" | "is_ok" => true,
-            "is_err" | "is_none" => false,
-            _ => unreachable!(),
-        };
+        let unwrappable = matches!(name, sym::is_some | sym::is_ok);
         let safe_to_unwrap = unwrappable != invert;
         let kind = if is_type_diagnostic_item(cx, ty, sym::Option) {
             UnwrappableKind::Option
@@ -174,7 +170,7 @@ fn collect_unwrap_info<'tcx>(
             local_id,
             if_expr,
             check: expr,
-            check_name: method_name,
+            check_name: name,
             branch,
             safe_to_unwrap,
             kind,
@@ -184,12 +180,12 @@ 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.
+/// A HIR visitor delegate that checks if a local variable of type `Option` or `Result` is mutated,
+/// *except* for if `.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
+/// the variant, 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`.
+/// the option is changed to None between `is_some` and `unwrap`, ditto for `Result`.
 /// (And also `.as_mut()` is a somewhat common method that is still worth linting on.)
 struct MutationVisitor<'tcx> {
     is_mutated: bool,
@@ -198,13 +194,13 @@ struct MutationVisitor<'tcx> {
 }
 
 /// Checks if the parent of the expression pointed at by the given `HirId` is a call to
-/// `Option::as_mut`.
+/// `.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 {
+fn is_as_mut_use(tcx: TyCtxt<'_>, expr_id: HirId) -> bool {
     if let Node::Expr(mutating_expr) = tcx.parent_hir_node(expr_id)
         && let ExprKind::MethodCall(path, _, [], _) = mutating_expr.kind
     {
@@ -218,7 +214,7 @@ 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::Mutable = bk
             && is_potentially_local_place(self.local_id, &cat.place)
-            && !is_option_as_mut_use(self.tcx, diag_expr_id)
+            && !is_as_mut_use(self.tcx, diag_expr_id)
         {
             self.is_mutated = true;
         }
@@ -278,12 +274,10 @@ enum AsRefKind {
 /// 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 == sym::as_mut {
-            (recv, Some(AsRefKind::AsMut))
-        } else {
-            (expr, None)
+        match path.ident.name {
+            sym::as_ref => (recv, Some(AsRefKind::AsRef)),
+            sym::as_mut => (recv, Some(AsRefKind::AsMut)),
+            _ => (expr, None),
         }
     } else {
         (expr, None)
@@ -334,8 +328,7 @@ impl<'tcx> Visitor<'tcx> for UnwrappableVariablesVisitor<'_, 'tcx> {
                         expr.span,
                         format!(
                             "called `{}` on `{unwrappable_variable_name}` after checking its variant with `{}`",
-                            method_name.ident.name,
-                            unwrappable.check_name.ident.as_str(),
+                            method_name.ident.name, unwrappable.check_name,
                         ),
                         |diag| {
                             if is_entire_condition {