Move constant write checks to temporary_assignment lint

They make more sense here
rust-lang · Dec 29, 2018 · 815e434 · 815e434
1 parent ece8b8e
commit 815e434
Show file tree

Hide file tree

Showing 6 changed files with 124 additions and 101 deletions.
diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs
@@ -98,20 +98,6 @@ fn has_no_effect(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
                     false
                 }
         },
-        ExprKind::Assign(ref left, ref right) => {
-            if has_no_effect(cx, left) {
-                let mut left = left;
-                while let ExprKind::Field(f, _) | ExprKind::Index(f, _) = &left.node {
-                    left = f;
-                }
-                if let ExprKind::Path(qpath) = &left.node {
-                    if let Def::Const(..) = cx.tables.qpath_def(qpath, left.hir_id) {
-                        return has_no_effect(cx, right);
-                    }
-                }
-            }
-            false
-        },
         _ => false,
     }
 }

diff --git a/clippy_lints/src/temporary_assignment.rs b/clippy_lints/src/temporary_assignment.rs
@@ -9,6 +9,7 @@
 
 use crate::utils::is_adjusted;
 use crate::utils::span_lint;
+use rustc::hir::def::Def;
 use rustc::hir::{Expr, ExprKind};
 use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::{declare_tool_lint, lint_array};
@@ -31,9 +32,16 @@ declare_clippy_lint! {
     "assignments to temporaries"
 }
 
-fn is_temporary(expr: &Expr) -> bool {
-    match expr.node {
+fn is_temporary(cx: &LateContext<'_, '_>, expr: &Expr) -> bool {
+    match &expr.node {
         ExprKind::Struct(..) | ExprKind::Tup(..) => true,
+        ExprKind::Path(qpath) => {
+            if let Def::Const(..) = cx.tables.qpath_def(qpath, expr.hir_id) {
+                true
+            } else {
+                false
+            }
+        },
         _ => false,
     }
 }
@@ -49,11 +57,13 @@ impl LintPass for Pass {
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
-        if let ExprKind::Assign(ref target, _) = expr.node {
-            if let ExprKind::Field(ref base, _) = target.node {
-                if is_temporary(base) && !is_adjusted(cx, base) {
-                    span_lint(cx, TEMPORARY_ASSIGNMENT, expr.span, "assignment to temporary");
-                }
+        if let ExprKind::Assign(target, _) = &expr.node {
+            let mut base = target;
+            while let ExprKind::Field(f, _) | ExprKind::Index(f, _) = &base.node {
+                base = f;
+            }
+            if is_temporary(cx, base) && !is_adjusted(cx, base) {
+                span_lint(cx, TEMPORARY_ASSIGNMENT, expr.span, "assignment to temporary");
             }
         }
     }

diff --git a/tests/ui/no_effect.rs b/tests/ui/no_effect.rs
@@ -67,21 +67,6 @@ unsafe fn unsafe_fn() -> i32 {
     0
 }
 
-struct A(i32);
-struct B {
-    field: i32,
-}
-struct C {
-    b: B,
-}
-struct D {
-    arr: [i32; 1],
-}
-const A_CONST: A = A(1);
-const B: B = B { field: 1 };
-const C: C = C { b: B { field: 1 } };
-const D: D = D { arr: [1] };
-
 fn main() {
     let s = get_struct();
     let s2 = get_struct();
@@ -114,10 +99,6 @@ fn main() {
     || x += 5;
     let s: String = "foo".into();
     FooString { s: s };
-    A_CONST.0 = 2;
-    B.field = 2;
-    C.b.field = 2;
-    D.arr[0] = 2;
 
     // Do not warn
     get_number();
@@ -127,12 +108,4 @@ fn main() {
     DropTuple(0);
     DropEnum::Tuple(0);
     DropEnum::Struct { field: 0 };
-    let mut a_mut = A(1);
-    a_mut.0 = 2;
-    let mut b_mut = B { field: 1 };
-    b_mut.field = 2;
-    let mut c_mut = C { b: B { field: 1 } };
-    c_mut.b.field = 2;
-    let mut d_mut = D { arr: [1] };
-    d_mut.arr[0] = 2;
 }
diff --git a/tests/ui/no_effect.stderr b/tests/ui/no_effect.stderr
@@ -1,178 +1,154 @@
 error: statement with no effect
-  --> $DIR/no_effect.rs:89:5
+  --> $DIR/no_effect.rs:74:5
    |
 LL |     0;
    |     ^^
    |
    = note: `-D clippy::no-effect` implied by `-D warnings`
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:90:5
+  --> $DIR/no_effect.rs:75:5
    |
 LL |     s2;
    |     ^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:91:5
+  --> $DIR/no_effect.rs:76:5
    |
 LL |     Unit;
    |     ^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:92:5
+  --> $DIR/no_effect.rs:77:5
    |
 LL |     Tuple(0);
    |     ^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:93:5
+  --> $DIR/no_effect.rs:78:5
    |
 LL |     Struct { field: 0 };
    |     ^^^^^^^^^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:94:5
+  --> $DIR/no_effect.rs:79:5
    |
 LL |     Struct { ..s };
    |     ^^^^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:95:5
+  --> $DIR/no_effect.rs:80:5
    |
 LL |     Union { a: 0 };
    |     ^^^^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:96:5
+  --> $DIR/no_effect.rs:81:5
    |
 LL |     Enum::Tuple(0);
    |     ^^^^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:97:5
+  --> $DIR/no_effect.rs:82:5
    |
 LL |     Enum::Struct { field: 0 };
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:98:5
+  --> $DIR/no_effect.rs:83:5
    |
 LL |     5 + 6;
    |     ^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:99:5
+  --> $DIR/no_effect.rs:84:5
    |
 LL |     *&42;
    |     ^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:100:5
+  --> $DIR/no_effect.rs:85:5
    |
 LL |     &6;
    |     ^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:101:5
+  --> $DIR/no_effect.rs:86:5
    |
 LL |     (5, 6, 7);
    |     ^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:102:5
+  --> $DIR/no_effect.rs:87:5
    |
 LL |     box 42;
    |     ^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:103:5
+  --> $DIR/no_effect.rs:88:5
    |
 LL |     ..;
    |     ^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:104:5
+  --> $DIR/no_effect.rs:89:5
    |
 LL |     5..;
    |     ^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:105:5
+  --> $DIR/no_effect.rs:90:5
    |
 LL |     ..5;
    |     ^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:106:5
+  --> $DIR/no_effect.rs:91:5
    |
 LL |     5..6;
    |     ^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:108:5
+  --> $DIR/no_effect.rs:93:5
    |
 LL |     [42, 55];
    |     ^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:109:5
+  --> $DIR/no_effect.rs:94:5
    |
 LL |     [42, 55][1];
    |     ^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:110:5
+  --> $DIR/no_effect.rs:95:5
    |
 LL |     (42, 55).1;
    |     ^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:111:5
+  --> $DIR/no_effect.rs:96:5
    |
 LL |     [42; 55];
    |     ^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:112:5
+  --> $DIR/no_effect.rs:97:5
    |
 LL |     [42; 55][13];
    |     ^^^^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:114:5
+  --> $DIR/no_effect.rs:99:5
    |
 LL |     || x += 5;
    |     ^^^^^^^^^^
 
 error: statement with no effect
-  --> $DIR/no_effect.rs:116:5
+  --> $DIR/no_effect.rs:101:5
    |
 LL |     FooString { s: s };
    |     ^^^^^^^^^^^^^^^^^^^
 
-error: statement with no effect
-  --> $DIR/no_effect.rs:117:5
-   |
-LL |     A_CONST.0 = 2;
-   |     ^^^^^^^^^^^^^^
-
-error: statement with no effect
-  --> $DIR/no_effect.rs:118:5
-   |
-LL |     B.field = 2;
-   |     ^^^^^^^^^^^^
-
-error: statement with no effect
-  --> $DIR/no_effect.rs:119:5
-   |
-LL |     C.b.field = 2;
-   |     ^^^^^^^^^^^^^^
-
-error: statement with no effect
-  --> $DIR/no_effect.rs:120:5
-   |
-LL |     D.arr[0] = 2;
-   |     ^^^^^^^^^^^^^
-
-error: aborting due to 29 previous errors
+error: aborting due to 25 previous errors
 
diff --git a/tests/ui/temporary_assignment.rs b/tests/ui/temporary_assignment.rs
@@ -11,10 +11,16 @@
 
 use std::ops::{Deref, DerefMut};
 
+struct TupleStruct(i32);
+
 struct Struct {
     field: i32,
 }
 
+struct MultiStruct {
+    structure: Struct,
+}
+
 struct Wrapper<'a> {
     inner: &'a mut Struct,
 }
@@ -32,15 +38,47 @@ impl<'a> DerefMut for Wrapper<'a> {
     }
 }
 
+struct ArrayStruct {
+    array: [i32; 1],
+}
+
+const A: TupleStruct = TupleStruct(1);
+const B: Struct = Struct { field: 1 };
+const C: MultiStruct = MultiStruct {
+    structure: Struct { field: 1 },
+};
+const D: ArrayStruct = ArrayStruct { array: [1] };
+
 fn main() {
     let mut s = Struct { field: 0 };
     let mut t = (0, 0);
 
     Struct { field: 0 }.field = 1;
+    MultiStruct {
+        structure: Struct { field: 0 },
+    }
+    .structure
+    .field = 1;
+    ArrayStruct { array: [0] }.array[0] = 1;
     (0, 0).0 = 1;
 
+    A.0 = 2;
+    B.field = 2;
+    C.structure.field = 2;
+    D.array[0] = 2;
+
     // no error
     s.field = 1;
     t.0 = 1;
     Wrapper { inner: &mut s }.field = 1;
+    let mut a_mut = TupleStruct(1);
+    a_mut.0 = 2;
+    let mut b_mut = Struct { field: 1 };
+    b_mut.field = 2;
+    let mut c_mut = MultiStruct {
+        structure: Struct { field: 1 },
+    };
+    c_mut.structure.field = 2;
+    let mut d_mut = ArrayStruct { array: [1] };
+    d_mut.array[0] = 2;
 }