From 815e434a1fcb3817fa81c5aebd469c30493a0e51 Mon Sep 17 00:00:00 2001 From: Konrad Borowski Date: Sun, 30 Dec 2018 00:18:55 +0100 Subject: [PATCH] Move constant write checks to temporary_assignment lint They make more sense here --- clippy_lints/src/no_effect.rs | 14 ----- clippy_lints/src/temporary_assignment.rs | 24 +++++--- tests/ui/no_effect.rs | 27 --------- tests/ui/no_effect.stderr | 76 ++++++++---------------- tests/ui/temporary_assignment.rs | 38 ++++++++++++ tests/ui/temporary_assignment.stderr | 46 +++++++++++++- 6 files changed, 124 insertions(+), 101 deletions(-) diff --git a/clippy_lints/src/no_effect.rs b/clippy_lints/src/no_effect.rs index 02ca5ebedfa5b..c2cffadf6c120 100644 --- 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 index a454b6fd997ac..381efd5713569 100644 --- 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 index 8431f00e445c5..6b51c50dcde94 100644 --- 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 index b6aab53e50f81..cc3b069f0b52c 100644 --- a/tests/ui/no_effect.stderr +++ b/tests/ui/no_effect.stderr @@ -1,5 +1,5 @@ error: statement with no effect - --> $DIR/no_effect.rs:89:5 + --> $DIR/no_effect.rs:74:5 | LL | 0; | ^^ @@ -7,172 +7,148 @@ 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 index 79c090f05722a..5581f5be766de 100644 --- 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; } diff --git a/tests/ui/temporary_assignment.stderr b/tests/ui/temporary_assignment.stderr index a973638504837..13ece2858b994 100644 --- a/tests/ui/temporary_assignment.stderr +++ b/tests/ui/temporary_assignment.stderr @@ -1,5 +1,5 @@ error: assignment to temporary - --> $DIR/temporary_assignment.rs:39:5 + --> $DIR/temporary_assignment.rs:56:5 | LL | Struct { field: 0 }.field = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -7,10 +7,50 @@ LL | Struct { field: 0 }.field = 1; = note: `-D clippy::temporary-assignment` implied by `-D warnings` error: assignment to temporary - --> $DIR/temporary_assignment.rs:40:5 + --> $DIR/temporary_assignment.rs:57:5 + | +LL | / MultiStruct { +LL | | structure: Struct { field: 0 }, +LL | | } +LL | | .structure +LL | | .field = 1; + | |______________^ + +error: assignment to temporary + --> $DIR/temporary_assignment.rs:62:5 + | +LL | ArrayStruct { array: [0] }.array[0] = 1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: assignment to temporary + --> $DIR/temporary_assignment.rs:63:5 | LL | (0, 0).0 = 1; | ^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: assignment to temporary + --> $DIR/temporary_assignment.rs:65:5 + | +LL | A.0 = 2; + | ^^^^^^^ + +error: assignment to temporary + --> $DIR/temporary_assignment.rs:66:5 + | +LL | B.field = 2; + | ^^^^^^^^^^^ + +error: assignment to temporary + --> $DIR/temporary_assignment.rs:67:5 + | +LL | C.structure.field = 2; + | ^^^^^^^^^^^^^^^^^^^^^ + +error: assignment to temporary + --> $DIR/temporary_assignment.rs:68:5 + | +LL | D.array[0] = 2; + | ^^^^^^^^^^^^^^ + +error: aborting due to 8 previous errors