Auto merge of rust-lang#6660 - camsteffen:path-to-local, r=llogiq

Cleanup path-to-local checks

changelog: none

It seemed like too much ceremony going on to check if an expression matches a variable. So I created two util functions `path_to_local(Expr) -> Option<HirId>` and `path_to_local_id(Expr, HirId) -> bool` to make this easier, and used them wherever applicable.

I changed logic in a few places to use `HirId` instead of `Symbol` where it was easy to do so. I believe this is more correct and may even fix some bugs.

I also removed some calls to `qpath_res`. This is not needed if you are only looking for a `Res::Local`.

As a note, I wanted to name the util functions in a way that encourages understanding of the HIR.

Author: bors

clippy_lints/src/collapsible_match.rs

@@ -1,10 +1,10 @@
 use crate::utils::visitors::LocalUsedVisitor;
-use crate::utils::{span_lint_and_then, SpanlessEq};
+use crate::utils::{path_to_local, span_lint_and_then, SpanlessEq};
 use if_chain::if_chain;
 use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res};
 use rustc_hir::{Arm, Expr, ExprKind, Guard, HirId, Pat, PatKind, QPath, StmtKind, UnOp};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::{DefIdTree, TyCtxt};
+use rustc_middle::ty::{DefIdTree, TyCtxt, TypeckResults};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 use rustc_span::{MultiSpan, Span};
 
@@ -72,7 +72,7 @@ fn check_arm(arm: &Arm<'_>, wild_outer_arm: &Arm<'_>, cx: &LateContext<'_>) {
         if arms_inner.iter().all(|arm| arm.guard.is_none());
         // match expression must be a local binding
         // match <local> { .. }
-        if let Some(binding_id) = addr_adjusted_binding(expr_in, cx);
+        if let Some(binding_id) = path_to_local(strip_ref_operators(expr_in, cx.typeck_results()));
         // one of the branches must be "wild-like"
         if let Some(wild_inner_arm_idx) = arms_inner.iter().rposition(|arm_inner| arm_is_wild_like(arm_inner, cx.tcx));
         let (wild_inner_arm, non_wild_inner_arm) =
@@ -175,19 +175,15 @@ fn is_none_ctor(res: Res, tcx: TyCtxt<'_>) -> bool {
     false
 }
 
-/// Retrieves a binding ID with optional `&` and/or `*` operators removed. (e.g. `&**foo`)
-/// Returns `None` if a non-reference type is de-referenced.
-/// For example, if `Vec` is de-referenced to a slice, `None` is returned.
-fn addr_adjusted_binding(mut expr: &Expr<'_>, cx: &LateContext<'_>) -> Option<HirId> {
+/// Removes `AddrOf` operators (`&`) or deref operators (`*`), but only if a reference type is
+/// dereferenced. An overloaded deref such as `Vec` to slice would not be removed.
+fn strip_ref_operators<'hir>(mut expr: &'hir Expr<'hir>, typeck_results: &TypeckResults<'_>) -> &'hir Expr<'hir> {
     loop {
         match expr.kind {
             ExprKind::AddrOf(_, _, e) => expr = e,
-            ExprKind::Path(QPath::Resolved(None, path)) => match path.res {
-                Res::Local(binding_id) => break Some(binding_id),
-                _ => break None,
-            },
-            ExprKind::Unary(UnOp::UnDeref, e) if cx.typeck_results().expr_ty(e).is_ref() => expr = e,
-            _ => break None,
+            ExprKind::Unary(UnOp::UnDeref, e) if typeck_results.expr_ty(e).is_ref() => expr = e,
+            _ => break,
         }
     }
+    expr
 }

clippy_lints/src/eval_order_dependence.rs

@@ -1,7 +1,6 @@
-use crate::utils::{get_parent_expr, span_lint, span_lint_and_note};
-use if_chain::if_chain;
+use crate::utils::{get_parent_expr, path_to_local, path_to_local_id, span_lint, span_lint_and_note};
 use rustc_hir::intravisit::{walk_expr, NestedVisitorMap, Visitor};
-use rustc_hir::{def, BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, QPath, Stmt, StmtKind};
+use rustc_hir::{BinOpKind, Block, Expr, ExprKind, Guard, HirId, Local, Node, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::hir::map::Map;
 use rustc_middle::ty;
@@ -72,20 +71,14 @@ impl<'tcx> LateLintPass<'tcx> for EvalOrderDependence {
         // Find a write to a local variable.
         match expr.kind {
             ExprKind::Assign(ref lhs, ..) | ExprKind::AssignOp(_, ref lhs, _) => {
-                if let ExprKind::Path(ref qpath) = lhs.kind {
-                    if let QPath::Resolved(_, ref path) = *qpath {
-                        if path.segments.len() == 1 {
-                            if let def::Res::Local(var) = cx.qpath_res(qpath, lhs.hir_id) {
-                                let mut visitor = ReadVisitor {
-                                    cx,
-                                    var,
-                                    write_expr: expr,
-                                    last_expr: expr,
-                                };
-                                check_for_unsequenced_reads(&mut visitor);
-                            }
-                        }
-                    }
+                if let Some(var) = path_to_local(lhs) {
+                    let mut visitor = ReadVisitor {
+                        cx,
+                        var,
+                        write_expr: expr,
+                        last_expr: expr,
+                    };
+                    check_for_unsequenced_reads(&mut visitor);
                 }
             },
             _ => {},
@@ -304,27 +297,20 @@ impl<'a, 'tcx> Visitor<'tcx> for ReadVisitor<'a, 'tcx> {
             return;
         }
 
-        match expr.kind {
-            ExprKind::Path(ref qpath) => {
-                if_chain! {
-                    if let QPath::Resolved(None, ref path) = *qpath;
-                    if path.segments.len() == 1;
-                    if let def::Res::Local(local_id) = self.cx.qpath_res(qpath, expr.hir_id);
-                    if local_id == self.var;
-                    // Check that this is a read, not a write.
-                    if !is_in_assignment_position(self.cx, expr);
-                    then {
-                        span_lint_and_note(
-                            self.cx,
-                            EVAL_ORDER_DEPENDENCE,
-                            expr.span,
-                            "unsequenced read of a variable",
-                            Some(self.write_expr.span),
-                            "whether read occurs before this write depends on evaluation order"
-                        );
-                    }
-                }
+        if path_to_local_id(expr, self.var) {
+            // Check that this is a read, not a write.
+            if !is_in_assignment_position(self.cx, expr) {
+                span_lint_and_note(
+                    self.cx,
+                    EVAL_ORDER_DEPENDENCE,
+                    expr.span,
+                    "unsequenced read of a variable",
+                    Some(self.write_expr.span),
+                    "whether read occurs before this write depends on evaluation order",
+                );
             }
+        }
+        match expr.kind {
             // We're about to descend a closure. Since we don't know when (or
             // if) the closure will be evaluated, any reads in it might not
             // occur here (or ever). Like above, bail to avoid false positives.

clippy_lints/src/functions.rs

@@ -1,15 +1,15 @@
 use crate::utils::{
     attr_by_name, attrs::is_proc_macro, is_must_use_ty, is_trait_impl_item, is_type_diagnostic_item, iter_input_pats,
-    last_path_segment, match_def_path, must_use_attr, return_ty, snippet, snippet_opt, span_lint, span_lint_and_help,
-    span_lint_and_then, trait_ref_of_method, type_is_unsafe_function,
+    last_path_segment, match_def_path, must_use_attr, path_to_local, return_ty, snippet, snippet_opt, span_lint,
+    span_lint_and_help, span_lint_and_then, trait_ref_of_method, type_is_unsafe_function,
 };
 use if_chain::if_chain;
 use rustc_ast::ast::Attribute;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_hir::intravisit;
-use rustc_hir::{def::Res, def_id::DefId};
+use rustc_hir::{def::Res, def_id::DefId, QPath};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::lint::in_external_macro;
@@ -658,16 +658,14 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for DerefVisitor<'a, 'tcx> {
 
 impl<'a, 'tcx> DerefVisitor<'a, 'tcx> {
     fn check_arg(&self, ptr: &hir::Expr<'_>) {
-        if let hir::ExprKind::Path(ref qpath) = ptr.kind {
-            if let Res::Local(id) = self.cx.qpath_res(qpath, ptr.hir_id) {
-                if self.ptrs.contains(&id) {
-                    span_lint(
-                        self.cx,
-                        NOT_UNSAFE_PTR_ARG_DEREF,
-                        ptr.span,
-                        "this public function dereferences a raw pointer but is not marked `unsafe`",
-                    );
-                }
+        if let Some(id) = path_to_local(ptr) {
+            if self.ptrs.contains(&id) {
+                span_lint(
+                    self.cx,
+                    NOT_UNSAFE_PTR_ARG_DEREF,
+                    ptr.span,
+                    "this public function dereferences a raw pointer but is not marked `unsafe`",
+                );
             }
         }
     }
@@ -698,7 +696,7 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for StaticMutVisitor<'a, 'tcx> {
                             arg.span,
                             &mut tys,
                         )
-                        && is_mutated_static(self.cx, arg)
+                        && is_mutated_static(arg)
                     {
                         self.mutates_static = true;
                         return;
@@ -707,7 +705,7 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for StaticMutVisitor<'a, 'tcx> {
                 }
             },
             Assign(ref target, ..) | AssignOp(_, ref target, _) | AddrOf(_, hir::Mutability::Mut, ref target) => {
-                self.mutates_static |= is_mutated_static(self.cx, target)
+                self.mutates_static |= is_mutated_static(target)
             },
             _ => {},
         }
@@ -718,12 +716,13 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for StaticMutVisitor<'a, 'tcx> {
     }
 }
 
-fn is_mutated_static(cx: &LateContext<'_>, e: &hir::Expr<'_>) -> bool {
+fn is_mutated_static(e: &hir::Expr<'_>) -> bool {
     use hir::ExprKind::{Field, Index, Path};
 
     match e.kind {
-        Path(ref qpath) => !matches!(cx.qpath_res(qpath, e.hir_id), Res::Local(_)),
-        Field(ref inner, _) | Index(ref inner, _) => is_mutated_static(cx, inner),
+        Path(QPath::Resolved(_, path)) => !matches!(path.res, Res::Local(_)),
+        Path(_) => true,
+        Field(ref inner, _) | Index(ref inner, _) => is_mutated_static(inner),
         _ => false,
     }
 }

clippy_lints/src/let_if_seq.rs

@@ -1,8 +1,7 @@
-use crate::utils::{snippet, span_lint_and_then, visitors::LocalUsedVisitor};
+use crate::utils::{path_to_local_id, snippet, span_lint_and_then, visitors::LocalUsedVisitor};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
-use rustc_hir::def::Res;
 use rustc_hir::BindingAnnotation;
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
@@ -66,7 +65,7 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
                 if let hir::ExprKind::If(ref cond, ref then, ref else_) = if_.kind;
                 if !LocalUsedVisitor::new(canonical_id).check_expr(cond);
                 if let hir::ExprKind::Block(ref then, _) = then.kind;
-                if let Some(value) = check_assign(cx, canonical_id, &*then);
+                if let Some(value) = check_assign(canonical_id, &*then);
                 if !LocalUsedVisitor::new(canonical_id).check_expr(value);
                 then {
                     let span = stmt.span.to(if_.span);
@@ -79,7 +78,7 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
 
                     let (default_multi_stmts, default) = if let Some(ref else_) = *else_ {
                         if let hir::ExprKind::Block(ref else_, _) = else_.kind {
-                            if let Some(default) = check_assign(cx, canonical_id, else_) {
+                            if let Some(default) = check_assign(canonical_id, else_) {
                                 (else_.stmts.len() > 1, default)
                             } else if let Some(ref default) = local.init {
                                 (true, &**default)
@@ -134,19 +133,13 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
     }
 }
 
-fn check_assign<'tcx>(
-    cx: &LateContext<'tcx>,
-    decl: hir::HirId,
-    block: &'tcx hir::Block<'_>,
-) -> Option<&'tcx hir::Expr<'tcx>> {
+fn check_assign<'tcx>(decl: hir::HirId, block: &'tcx hir::Block<'_>) -> Option<&'tcx hir::Expr<'tcx>> {
     if_chain! {
         if block.expr.is_none();
         if let Some(expr) = block.stmts.iter().last();
         if let hir::StmtKind::Semi(ref expr) = expr.kind;
         if let hir::ExprKind::Assign(ref var, ref value, _) = expr.kind;
-        if let hir::ExprKind::Path(ref qpath) = var.kind;
-        if let Res::Local(local_id) = cx.qpath_res(qpath, var.hir_id);
-        if decl == local_id;
+        if path_to_local_id(var, decl);
         then {
             let mut v = LocalUsedVisitor::new(decl);