From f97e06db45077eba33e42e17d1dbd3ffc42aaf06 Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Sat, 17 Jun 2023 11:42:48 -0500 Subject: [PATCH 1/2] new lint `local_assigned_single_value` --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 3 + .../src/local_assigned_single_value.rs | 355 ++++++++++++++++++ tests/ui/diverging_sub_expression.rs | 2 +- tests/ui/explicit_counter_loop.rs | 6 +- tests/ui/explicit_counter_loop.stderr | 18 +- tests/ui/let_unit.fixed | 8 +- tests/ui/let_unit.rs | 8 +- tests/ui/let_unit.stderr | 20 +- tests/ui/local_assigned_single_value.rs | 99 +++++ tests/ui/local_assigned_single_value.stderr | 58 +++ tests/ui/match_expr_like_matches_macro.fixed | 1 + tests/ui/match_expr_like_matches_macro.rs | 1 + tests/ui/match_expr_like_matches_macro.stderr | 28 +- .../ui/mixed_read_write_in_expression.stderr | 13 +- tests/ui/temporary_assignment.stderr | 13 +- tests/ui/unnested_or_patterns.stderr | 10 +- 18 files changed, 605 insertions(+), 40 deletions(-) create mode 100644 clippy_lints/src/local_assigned_single_value.rs create mode 100644 tests/ui/local_assigned_single_value.rs create mode 100644 tests/ui/local_assigned_single_value.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index a7284a70a1ff..8203bde36d1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4926,6 +4926,7 @@ Released 2018-09-13 [`lines_filter_map_ok`]: https://rust-lang.github.io/rust-clippy/master/index.html#lines_filter_map_ok [`linkedlist`]: https://rust-lang.github.io/rust-clippy/master/index.html#linkedlist [`little_endian_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#little_endian_bytes +[`local_assigned_single_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#local_assigned_single_value [`logic_bug`]: https://rust-lang.github.io/rust-clippy/master/index.html#logic_bug [`lossy_float_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#lossy_float_literal [`macro_use_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#macro_use_imports diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index dbd4a53d8367..656c7418a049 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -249,6 +249,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::literal_representation::MISTYPED_LITERAL_SUFFIXES_INFO, crate::literal_representation::UNREADABLE_LITERAL_INFO, crate::literal_representation::UNUSUAL_BYTE_GROUPINGS_INFO, + crate::local_assigned_single_value::LOCAL_ASSIGNED_SINGLE_VALUE_INFO, crate::loops::EMPTY_LOOP_INFO, crate::loops::EXPLICIT_COUNTER_LOOP_INFO, crate::loops::EXPLICIT_INTO_ITER_LOOP_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d926b7b84ea7..368e7260b420 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -24,6 +24,7 @@ extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_ast_pretty; +extern crate rustc_const_eval; extern crate rustc_data_structures; extern crate rustc_driver; extern crate rustc_errors; @@ -177,6 +178,7 @@ mod let_with_type_underscore; mod lifetimes; mod lines_filter_map_ok; mod literal_representation; +mod local_assigned_single_value; mod loops; mod macro_use; mod main_recursion; @@ -1068,6 +1070,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: needless_raw_string_hashes_allow_one, }) }); + store.register_late_pass(|_| Box::new(local_assigned_single_value::LocalAssignedSingleValue)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/local_assigned_single_value.rs b/clippy_lints/src/local_assigned_single_value.rs new file mode 100644 index 000000000000..c7f8cfb2d0e2 --- /dev/null +++ b/clippy_lints/src/local_assigned_single_value.rs @@ -0,0 +1,355 @@ +use clippy_utils::diagnostics::span_lint; +use clippy_utils::fn_has_unsatisfiable_preds; +use clippy_utils::source::snippet_opt; +use itertools::Itertools; +use rustc_const_eval::interpret::Scalar; +use rustc_hir::def_id::LocalDefId; +use rustc_hir::{intravisit::FnKind, Body, FnDecl}; +use rustc_index::IndexVec; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::mir::{ + self, interpret::ConstValue, visit::Visitor, Constant, Location, Mutability, Operand, Place, Rvalue, +}; +use rustc_middle::mir::{AggregateKind, PlaceElem, Terminator, TerminatorKind}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Spanned; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Checks for locals that are always assigned the same value. + /// + /// ### Why is this bad? + /// It's almost always a typo. If not, it can be made immutable, or turned into a constant. + /// + /// ### Example + /// ```rust + /// let mut x = 1; + /// x = 1; + /// ``` + /// Use instead: + /// ```rust + /// let x = 1; + /// ``` + #[clippy::version = "1.72.0"] + pub LOCAL_ASSIGNED_SINGLE_VALUE, + correctness, + "disallows assigning locals many times with the same value" +} +declare_lint_pass!(LocalAssignedSingleValue => [LOCAL_ASSIGNED_SINGLE_VALUE]); + +impl LateLintPass<'_> for LocalAssignedSingleValue { + fn check_fn( + &mut self, + cx: &LateContext<'_>, + _: FnKind<'_>, + _: &FnDecl<'_>, + _: &Body<'_>, + _: Span, + def_id: LocalDefId, + ) { + // Building MIR for `fn`s with unsatisfiable preds results in ICE. + if fn_has_unsatisfiable_preds(cx, def_id.to_def_id()) { + return; + } + + let mir = cx.tcx.optimized_mir(def_id.to_def_id()); + let mut v = V { + body: mir, + cx, + map: mir.local_decls.iter().map(|_| LocalUsageValues::default()).collect(), + }; + v.visit_body(mir); + + // for (local, i) in v.map.iter_enumerated() { + // dbg!(local, i); + // } + + for (local, i) in v.map.iter_enumerated() { + if !i.assigned_non_const_rvalue + && !i.mut_ref_acquired + && nested_locals_are_not_from_bad_instr(&v.map, i) + && assignments_all_same_value(&v.map, i) + { + let LocalUsageValues { + usage, + mut_ref_acquired: _, + assigned_non_const_rvalue: _, + is_from_bad_instr: _, + } = i; + + if let Some(local_decl) = mir.local_decls.get(local) + && let [dbg_info] = &*mir + .var_debug_info + .iter() + .filter(|info| info.source_info.span == local_decl.source_info.span) + .collect_vec() + // Don't handle function arguments. + && dbg_info.argument_index.is_none() + // Ignore anything from a procedural macro, or locals we cannot prove aren't + // temporaries + && let Some(snippet) = snippet_opt(cx, dbg_info.source_info.span) + && snippet.ends_with(dbg_info.name.as_str()) + { + span_lint( + cx, + LOCAL_ASSIGNED_SINGLE_VALUE, + usage.iter().map(|i| i.span).collect_vec(), + "local only ever assigned single value", + ); + } + } + } + + /* + for (local, usage) in &v.local_usage { + if should_lint(&v.local_usage, *local, usage) { + let LocalUsageValues { + usage, + mut_ref_acquired: _, + assigned_non_const: _, + } = usage; + + if let Some(local_decl) = mir.local_decls.get(*local) + && let [dbg_info] = &*mir + .var_debug_info + .iter() + .filter(|info| info.source_info.span == local_decl.source_info.span) + .collect_vec() + // Don't handle function arguments. + && dbg_info.argument_index.is_none() + // Ignore anything from a procedural macro, or locals we cannot prove aren't + // temporaries + && let Some(snippet) = snippet_opt(cx, dbg_info.source_info.span) + && snippet.ends_with(dbg_info.name.as_str()) + { + span_lint( + cx, + LOCAL_ASSIGNED_SINGLE_VALUE, + usage.iter().map(|(span, _)| *span).collect_vec(), + "local only ever assigned single value", + ); + } + } + } + */ + } +} + +type LocalUsageMap<'tcx> = IndexVec>; + +/// Holds the data we have for the usage of a local. +#[derive(Debug, Default)] +struct LocalUsageValues<'tcx> { + /// Where and what this local is assigned. + usage: Vec>>, + /// Whether it's mutably borrowed, ever. We should not lint this. + mut_ref_acquired: bool, + /// Whether it's assigned a value we cannot prove is constant, ever. We should not lint this. + assigned_non_const_rvalue: bool, + /// Whether it's assigned a value that we know cannot be constant. This is differentiated from + /// `assigned_non_const` since we check this for nested locals. + /// + /// This is set to `true` for function calls or binary operations. + is_from_bad_instr: bool, +} + +#[derive(Debug)] +enum LocalUsage<'tcx> { + /// A single `Scalar`, for stuff like `i32` or `bool`. + Scalar(Scalar), + /// Any number of `Scalar`s. This is used for handling arrays and tuples + Scalars(Vec), + /// When a `Local` depends on the value of another local + DependsOn(mir::Local), + /// When a `Local` depends on the value of another local by accessing any of its fields or + /// indexing + DependsOnWithProj(mir::Local, &'tcx PlaceElem<'tcx>), +} + +struct V<'a, 'tcx> { + body: &'a mir::Body<'tcx>, + cx: &'a LateContext<'tcx>, + map: LocalUsageMap<'tcx>, +} + +impl<'a, 'tcx> Visitor<'tcx> for V<'a, 'tcx> { + fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, loc: Location) { + let Self { body, cx, map } = self; + let Some(stmt) = body.stmt_at(loc).left() else { + return; + }; + + if stmt.source_info.span.from_expansion() { + return; + } + + // Do not lint if there are any mutable borrows to a local + if let Rvalue::Ref(_, mir::BorrowKind::Unique | mir::BorrowKind::Mut { .. }, place) + | Rvalue::AddressOf(Mutability::Mut, place) = rvalue + { + map[place.local].mut_ref_acquired = true; + return; + } + + let usage = &mut map[place.local]; + + if let Rvalue::Use(Operand::Constant(constant)) = rvalue + && let Constant { literal, .. } = **constant + && let Some(ConstValue::Scalar(scalar)) = literal.try_to_value(cx.tcx) + { + usage.usage.push(Spanned { node: LocalUsage::Scalar(scalar), span: stmt.source_info.span }); + } else if let Rvalue::Use(operand) = rvalue + && let Some(place) = operand.place() + { + if let [base_proj, ..] = place.projection.as_slice() + // Handle `let [x, y] = [1, 1]` and `let (x, y) = (1, 1)` + && matches!(base_proj, PlaceElem::Field(..) | PlaceElem::Index(..)) + { + usage.usage.push(Spanned { + node: LocalUsage::DependsOnWithProj(place.local, base_proj), + span: stmt.source_info.span, + }); + } else { + // It seems sometimes a local's just moved, no projections, so let's make sure we + // don't set `assigned_non_const` to true for these cases + usage.usage.push(Spanned { + node: LocalUsage::DependsOn(place.local), + span: stmt.source_info.span + }); + } + } + // Handle creation of tuples/arrays, otherwise the above would be useless + else if let Rvalue::Aggregate(kind, fields) = rvalue + // TODO: Handle `Adt`s, if possible. + && let AggregateKind::Array(..) | AggregateKind::Tuple = **kind + // TODO: Let's remove this `cloned` call, if possible. + && let Some(scalars) = extract_scalars(cx, fields.into_iter().cloned()) + { + usage.usage.push(Spanned { + node: LocalUsage::Scalars(scalars), + span: stmt.source_info.span, + }) + } else if let Rvalue::BinaryOp(..) | Rvalue::CheckedBinaryOp(..) = rvalue { + usage.is_from_bad_instr = true; + } else { + // We can also probably handle stuff like `x += 1` here, maybe. But this would be + // very very complex. Let's keep it simple enough. + usage.assigned_non_const_rvalue = true; + } + } + + fn visit_terminator(&mut self, terminator: &Terminator<'_>, _: Location) { + let Self { body: _, cx: _, map } = self; + + // TODO: Maybe we can allow const fns, if we can evaluate them of course + if let TerminatorKind::Call { destination, .. } = terminator.kind { + map[destination.local].is_from_bad_instr = true; + } + } +} + +/// `None` means any one of the `Operand`s is not an `Operand::Constant`. +fn extract_scalars<'tcx, O>(cx: &LateContext<'tcx>, operands: O) -> Option> +where + O: IntoIterator>, +{ + operands + .into_iter() + .map(|operand| -> Option<_> { + if let Operand::Constant(constant) = operand + && let Constant { literal, .. } = *constant + && let ConstValue::Scalar(scalar) = literal.try_to_value(cx.tcx)? + { + return Some(scalar); + } + + None + }) + .collect::>>() +} + +fn assignments_all_same_value(map: &LocalUsageMap<'_>, usage: &LocalUsageValues<'_>) -> bool { + let LocalUsageValues { + usage, + mut_ref_acquired: _, + assigned_non_const_rvalue: _, + is_from_bad_instr: _, + } = usage; + + if usage.len() <= 1 { + return false; + } + + // TODO: This code is clone-hell. + + let mut all_assignments = vec![]; + for assignment in usage { + match &assignment.node { + LocalUsage::Scalar(scalar) => { + all_assignments.push(scalar.clone()); + }, + // FIXME: This doesn't handle assignment of tuples, arrays or anything else currently. + LocalUsage::Scalars(_) => {}, + // FIXME: This doesn't allow nested dependencies, currently. + // FIXME: This only allows one assignment for dependencies. + LocalUsage::DependsOn(local) => { + let [assignment] = &*map[*local].usage else { + continue; + }; + match assignment.node { + LocalUsage::Scalar(scalar) => all_assignments.push(scalar.clone()), + LocalUsage::Scalars(_) => {}, + _ => return false, + } + }, + LocalUsage::DependsOnWithProj(local, base_proj) => { + let [assignment] = &*map[*local].usage else { + continue; + }; + match base_proj { + PlaceElem::Field(idx, _) if let LocalUsage::Scalars(scalars) = &assignment.node => { + all_assignments.push(scalars[idx.as_usize()].clone()); + }, + PlaceElem::Index(idx) if let LocalUsage::Scalars(scalars) = &assignment.node => { + all_assignments.push(scalars[idx.as_usize()].clone()); + }, + _ => return false, + } + }, + } + } + + if let [head, tail @ ..] = &*all_assignments && tail.iter().all(|i| i == head) { + return true; + } + + false +} + +fn nested_locals_are_not_from_bad_instr(map: &LocalUsageMap<'_>, usage: &LocalUsageValues<'_>) -> bool { + // FIXME: This is a hacky workaround to not have a stack overflow. Instead, we should fix the root + // cause. + nested_locals_are_not_from_bad_instr_inner(map, usage, 0) +} + +fn nested_locals_are_not_from_bad_instr_inner( + map: &LocalUsageMap<'_>, + usage: &LocalUsageValues<'_>, + depth: usize, +) -> bool { + if depth < 10 && !usage.is_from_bad_instr { + let mut all_good_instrs = true; + for assignment in &usage.usage { + match assignment.node { + LocalUsage::Scalar(_) | LocalUsage::Scalars(_) => continue, + LocalUsage::DependsOn(local) | LocalUsage::DependsOnWithProj(local, _) => { + all_good_instrs &= nested_locals_are_not_from_bad_instr_inner(map, &map[local], depth + 1); + }, + } + } + return all_good_instrs; + } + + false +} diff --git a/tests/ui/diverging_sub_expression.rs b/tests/ui/diverging_sub_expression.rs index 9b1619baf0e6..fc0ee8937f06 100644 --- a/tests/ui/diverging_sub_expression.rs +++ b/tests/ui/diverging_sub_expression.rs @@ -1,6 +1,6 @@ #![warn(clippy::diverging_sub_expression)] #![allow(clippy::match_same_arms, clippy::overly_complex_bool_expr)] -#![allow(clippy::nonminimal_bool)] +#![allow(clippy::local_assigned_single_value, clippy::nonminimal_bool)] #[allow(clippy::empty_loop)] fn diverge() -> ! { loop {} diff --git a/tests/ui/explicit_counter_loop.rs b/tests/ui/explicit_counter_loop.rs index e02b8f62b3dd..c69bb8bf5df2 100644 --- a/tests/ui/explicit_counter_loop.rs +++ b/tests/ui/explicit_counter_loop.rs @@ -1,5 +1,9 @@ #![warn(clippy::explicit_counter_loop)] -#![allow(clippy::uninlined_format_args, clippy::useless_vec)] +#![allow( + clippy::local_assigned_single_value, + clippy::uninlined_format_args, + clippy::useless_vec +)] fn main() { let mut vec = vec![1, 2, 3, 4]; diff --git a/tests/ui/explicit_counter_loop.stderr b/tests/ui/explicit_counter_loop.stderr index 0677e4d78c8b..1eeac65e6418 100644 --- a/tests/ui/explicit_counter_loop.stderr +++ b/tests/ui/explicit_counter_loop.stderr @@ -1,5 +1,5 @@ error: the variable `_index` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:7:5 + --> $DIR/explicit_counter_loop.rs:11:5 | LL | for _v in &vec { | ^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter().enumerate()` @@ -7,49 +7,49 @@ LL | for _v in &vec { = note: `-D clippy::explicit-counter-loop` implied by `-D warnings` error: the variable `_index` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:13:5 + --> $DIR/explicit_counter_loop.rs:17:5 | LL | for _v in &vec { | ^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter().enumerate()` error: the variable `_index` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:18:5 + --> $DIR/explicit_counter_loop.rs:22:5 | LL | for _v in &mut vec { | ^^^^^^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.iter_mut().enumerate()` error: the variable `_index` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:23:5 + --> $DIR/explicit_counter_loop.rs:27:5 | LL | for _v in vec { | ^^^^^^^^^^^^^ help: consider using: `for (_index, _v) in vec.into_iter().enumerate()` error: the variable `count` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:110:9 + --> $DIR/explicit_counter_loop.rs:114:9 | LL | for ch in text.chars() { | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()` error: the variable `count` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:121:9 + --> $DIR/explicit_counter_loop.rs:125:9 | LL | for ch in text.chars() { | ^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `for (count, ch) in text.chars().enumerate()` error: the variable `count` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:179:9 + --> $DIR/explicit_counter_loop.rs:183:9 | LL | for _i in 3..10 { | ^^^^^^^^^^^^^^^ help: consider using: `for (count, _i) in (3..10).enumerate()` error: the variable `idx_usize` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:219:9 + --> $DIR/explicit_counter_loop.rs:223:9 | LL | for _item in slice { | ^^^^^^^^^^^^^^^^^^ help: consider using: `for (idx_usize, _item) in slice.iter().enumerate()` error: the variable `idx_u32` is used as a loop counter - --> $DIR/explicit_counter_loop.rs:231:9 + --> $DIR/explicit_counter_loop.rs:235:9 | LL | for _item in slice { | ^^^^^^^^^^^^^^^^^^ help: consider using: `for (idx_u32, _item) in (0_u32..).zip(slice.iter())` diff --git a/tests/ui/let_unit.fixed b/tests/ui/let_unit.fixed index 8ba89ec78bbd..8f317b6e1980 100644 --- a/tests/ui/let_unit.fixed +++ b/tests/ui/let_unit.fixed @@ -2,7 +2,13 @@ #![feature(lint_reasons)] #![warn(clippy::let_unit_value)] -#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)] +#![allow( + unused, + clippy::local_assigned_single_value, + clippy::no_effect, + clippy::needless_late_init, + path_statements +)] macro_rules! let_and_return { ($n:expr) => {{ diff --git a/tests/ui/let_unit.rs b/tests/ui/let_unit.rs index 7e8764a482a2..5dff4bee5389 100644 --- a/tests/ui/let_unit.rs +++ b/tests/ui/let_unit.rs @@ -2,7 +2,13 @@ #![feature(lint_reasons)] #![warn(clippy::let_unit_value)] -#![allow(unused, clippy::no_effect, clippy::needless_late_init, path_statements)] +#![allow( + unused, + clippy::local_assigned_single_value, + clippy::no_effect, + clippy::needless_late_init, + path_statements +)] macro_rules! let_and_return { ($n:expr) => {{ diff --git a/tests/ui/let_unit.stderr b/tests/ui/let_unit.stderr index 49da74ca7e1c..4d897df6fdf1 100644 --- a/tests/ui/let_unit.stderr +++ b/tests/ui/let_unit.stderr @@ -1,5 +1,5 @@ error: this let-binding has unit value - --> $DIR/let_unit.rs:14:5 + --> $DIR/let_unit.rs:20:5 | LL | let _x = println!("x"); | ^^^^^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `println!("x");` @@ -7,13 +7,13 @@ LL | let _x = println!("x"); = note: `-D clippy::let-unit-value` implied by `-D warnings` error: this let-binding has unit value - --> $DIR/let_unit.rs:18:9 + --> $DIR/let_unit.rs:24:9 | LL | let _a = (); | ^^^^^^^^^^^^ help: omit the `let` binding: `();` error: this let-binding has unit value - --> $DIR/let_unit.rs:53:5 + --> $DIR/let_unit.rs:59:5 | LL | / let _ = v LL | | .into_iter() @@ -36,7 +36,7 @@ LL + .unwrap(); | error: this let-binding has unit value - --> $DIR/let_unit.rs:80:5 + --> $DIR/let_unit.rs:86:5 | LL | let x: () = f(); // Lint. | ^^^^-^^^^^^^^^^^ @@ -44,7 +44,7 @@ LL | let x: () = f(); // Lint. | help: use a wild (`_`) binding: `_` error: this let-binding has unit value - --> $DIR/let_unit.rs:83:5 + --> $DIR/let_unit.rs:89:5 | LL | let x: () = f2(0i32); // Lint. | ^^^^-^^^^^^^^^^^^^^^^ @@ -52,19 +52,19 @@ LL | let x: () = f2(0i32); // Lint. | help: use a wild (`_`) binding: `_` error: this let-binding has unit value - --> $DIR/let_unit.rs:85:5 + --> $DIR/let_unit.rs:91:5 | LL | let _: () = f3(()); // Lint | ^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f3(());` error: this let-binding has unit value - --> $DIR/let_unit.rs:86:5 + --> $DIR/let_unit.rs:92:5 | LL | let x: () = f3(()); // Lint | ^^^^^^^^^^^^^^^^^^^ help: omit the `let` binding: `f3(());` error: this let-binding has unit value - --> $DIR/let_unit.rs:102:5 + --> $DIR/let_unit.rs:108:5 | LL | let x: () = if true { f() } else { f2(0) }; // Lint | ^^^^-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -72,7 +72,7 @@ LL | let x: () = if true { f() } else { f2(0) }; // Lint | help: use a wild (`_`) binding: `_` error: this let-binding has unit value - --> $DIR/let_unit.rs:113:5 + --> $DIR/let_unit.rs:119:5 | LL | / let _: () = match Some(0) { LL | | None => f2(1), @@ -93,7 +93,7 @@ LL + }; | error: this let-binding has unit value - --> $DIR/let_unit.rs:160:13 + --> $DIR/let_unit.rs:166:13 | LL | let _: () = z; | ^^^^^^^^^^^^^^ help: omit the `let` binding: `z;` diff --git a/tests/ui/local_assigned_single_value.rs b/tests/ui/local_assigned_single_value.rs new file mode 100644 index 000000000000..0e943d2c7cbf --- /dev/null +++ b/tests/ui/local_assigned_single_value.rs @@ -0,0 +1,99 @@ +//@aux-build:proc_macros.rs +#![allow(unused)] +#![warn(clippy::local_assigned_single_value)] + +#[macro_use] +extern crate proc_macros; + +fn a(a: &mut i32) {} + +struct A(u32, u32); + +#[repr(i32)] +enum B { + A, + B, + C, +} + +fn g(x: i32) -> i32 { + x + 1 +} + +fn h() -> i32 { + let mut x = 42; + x = g(x); + + x +} + +fn main() { + let mut _a = A(1, 2); + // Do not lint, unfortunately. + let mut x = _a.0; + x = 1; + x = 1; + // This lints, though! + let mut x = (1,).0; + x = 1; + x = 1; + let mut x = 1; + x = 1; + x = 1; + // Do not lint + x += 1; + let mut x = 1; + x = 1; + x = 1; + x = true as i32; + x = B::B as i32; + { + x = 1; + } + let mut x = 1.0f32; + x = 1.0; + x = 1.0; + { + x = 1.0; + } + // Do not lint, unfortunately. + let (mut x, y) = (1, 2); + let [mut x, y] = [1, 2]; + let mut x = 1; + x = 1; + x = 1; + // Don't lint + a(&mut x); + let mut x = 1; + x = 1; + x = 1; + x = 1; + x = 2; + // Don't lint + a(&mut x); + let mut y = 1; + // FIXME: Linting this requires nested dependencies, thus we don't currently, but please fix + (x, y) = (1, 1); + y = 1; + // Don't lint + let [mut x, y] = [1, 2]; + x = 1; + x = 1; + { + x = 1; + } + let mut x = 1; + x = 1; + external! { + let mut x = 1; + x = 1; + x = 1; + } + with_span! { + span + let mut x = 1; + x = 1; + x = 1; + } + let mut x = _a.1; +} diff --git a/tests/ui/local_assigned_single_value.stderr b/tests/ui/local_assigned_single_value.stderr new file mode 100644 index 000000000000..678fdf4fd7b7 --- /dev/null +++ b/tests/ui/local_assigned_single_value.stderr @@ -0,0 +1,58 @@ +error: local only ever assigned single value + --> $DIR/local_assigned_single_value.rs:33:17 + | +LL | let mut x = _a.0; + | ^^^^ +LL | x = 1; + | ^^^^^ +LL | x = 1; + | ^^^^^ + | + = note: `-D clippy::local-assigned-single-value` implied by `-D warnings` + +error: local only ever assigned single value + --> $DIR/local_assigned_single_value.rs:37:17 + | +LL | let mut x = (1,).0; + | ^^^^^^ +LL | x = 1; + | ^^^^^ +LL | x = 1; + | ^^^^^ + +error: local only ever assigned single value + --> $DIR/local_assigned_single_value.rs:53:17 + | +LL | let mut x = 1.0f32; + | ^^^^^^ +LL | x = 1.0; + | ^^^^^^^ +LL | x = 1.0; + | ^^^^^^^ +LL | { +LL | x = 1.0; + | ^^^^^^^ + +error: local only ever assigned single value + --> $DIR/local_assigned_single_value.rs:79:10 + | +LL | let [mut x, y] = [1, 2]; + | ^^^^^ +LL | x = 1; + | ^^^^^ +LL | x = 1; + | ^^^^^ +LL | { +LL | x = 1; + | ^^^^^ + +error: local only ever assigned single value + --> $DIR/local_assigned_single_value.rs:85:17 + | +LL | let mut x = 1; + | ^ +LL | x = 1; + | ^^^^^ + +error: aborting due to 5 previous errors + diff --git a/tests/ui/match_expr_like_matches_macro.fixed b/tests/ui/match_expr_like_matches_macro.fixed index 60f590661735..3996a01a1b4b 100644 --- a/tests/ui/match_expr_like_matches_macro.fixed +++ b/tests/ui/match_expr_like_matches_macro.fixed @@ -4,6 +4,7 @@ #![allow( unreachable_patterns, dead_code, + clippy::local_assigned_single_value, clippy::equatable_if_let, clippy::needless_borrowed_reference )] diff --git a/tests/ui/match_expr_like_matches_macro.rs b/tests/ui/match_expr_like_matches_macro.rs index afdf1069f5e4..3f680f240019 100644 --- a/tests/ui/match_expr_like_matches_macro.rs +++ b/tests/ui/match_expr_like_matches_macro.rs @@ -4,6 +4,7 @@ #![allow( unreachable_patterns, dead_code, + clippy::local_assigned_single_value, clippy::equatable_if_let, clippy::needless_borrowed_reference )] diff --git a/tests/ui/match_expr_like_matches_macro.stderr b/tests/ui/match_expr_like_matches_macro.stderr index b72fe10b7480..52189db48dbf 100644 --- a/tests/ui/match_expr_like_matches_macro.stderr +++ b/tests/ui/match_expr_like_matches_macro.stderr @@ -1,5 +1,5 @@ error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:15:14 + --> $DIR/match_expr_like_matches_macro.rs:16:14 | LL | let _y = match x { | ______________^ @@ -11,7 +11,7 @@ LL | | }; = note: `-D clippy::match-like-matches-macro` implied by `-D warnings` error: redundant pattern matching, consider using `is_some()` - --> $DIR/match_expr_like_matches_macro.rs:21:14 + --> $DIR/match_expr_like_matches_macro.rs:22:14 | LL | let _w = match x { | ______________^ @@ -23,7 +23,7 @@ LL | | }; = note: `-D clippy::redundant-pattern-matching` implied by `-D warnings` error: redundant pattern matching, consider using `is_none()` - --> $DIR/match_expr_like_matches_macro.rs:27:14 + --> $DIR/match_expr_like_matches_macro.rs:28:14 | LL | let _z = match x { | ______________^ @@ -33,7 +33,7 @@ LL | | }; | |_____^ help: try this: `x.is_none()` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:33:15 + --> $DIR/match_expr_like_matches_macro.rs:34:15 | LL | let _zz = match x { | _______________^ @@ -43,13 +43,13 @@ LL | | }; | |_____^ help: try this: `!matches!(x, Some(r) if r == 0)` error: if let .. else expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:39:16 + --> $DIR/match_expr_like_matches_macro.rs:40:16 | LL | let _zzz = if let Some(5) = x { true } else { false }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `matches!(x, Some(5))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:63:20 + --> $DIR/match_expr_like_matches_macro.rs:64:20 | LL | let _ans = match x { | ____________________^ @@ -60,7 +60,7 @@ LL | | }; | |_________^ help: try this: `matches!(x, E::A(_) | E::B(_))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:73:20 + --> $DIR/match_expr_like_matches_macro.rs:74:20 | LL | let _ans = match x { | ____________________^ @@ -73,7 +73,7 @@ LL | | }; | |_________^ help: try this: `matches!(x, E::A(_) | E::B(_))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:83:20 + --> $DIR/match_expr_like_matches_macro.rs:84:20 | LL | let _ans = match x { | ____________________^ @@ -84,7 +84,7 @@ LL | | }; | |_________^ help: try this: `!matches!(x, E::B(_) | E::C)` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:143:18 + --> $DIR/match_expr_like_matches_macro.rs:144:18 | LL | let _z = match &z { | __________________^ @@ -94,7 +94,7 @@ LL | | }; | |_________^ help: try this: `matches!(z, Some(3))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:152:18 + --> $DIR/match_expr_like_matches_macro.rs:153:18 | LL | let _z = match &z { | __________________^ @@ -104,7 +104,7 @@ LL | | }; | |_________^ help: try this: `matches!(&z, Some(3))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:169:21 + --> $DIR/match_expr_like_matches_macro.rs:170:21 | LL | let _ = match &z { | _____________________^ @@ -114,7 +114,7 @@ LL | | }; | |_____________^ help: try this: `matches!(&z, AnEnum::X)` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:183:20 + --> $DIR/match_expr_like_matches_macro.rs:184:20 | LL | let _res = match &val { | ____________________^ @@ -124,7 +124,7 @@ LL | | }; | |_________^ help: try this: `matches!(&val, &Some(ref _a))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:195:20 + --> $DIR/match_expr_like_matches_macro.rs:196:20 | LL | let _res = match &val { | ____________________^ @@ -134,7 +134,7 @@ LL | | }; | |_________^ help: try this: `matches!(&val, &Some(ref _a))` error: match expression looks like `matches!` macro - --> $DIR/match_expr_like_matches_macro.rs:253:14 + --> $DIR/match_expr_like_matches_macro.rs:254:14 | LL | let _y = match Some(5) { | ______________^ diff --git a/tests/ui/mixed_read_write_in_expression.stderr b/tests/ui/mixed_read_write_in_expression.stderr index 8cc68b0ac7b4..0832e9998bf7 100644 --- a/tests/ui/mixed_read_write_in_expression.stderr +++ b/tests/ui/mixed_read_write_in_expression.stderr @@ -1,3 +1,14 @@ +error: local only ever assigned single value + --> $DIR/mixed_read_write_in_expression.rs:91:19 + | +LL | let mut tup = (0, 0); + | ^^^^^^ +LL | let c = { +LL | tup.0 = 1; + | ^^^^^^^^^ + | + = note: `#[deny(clippy::local_assigned_single_value)]` on by default + error: unsequenced read of `x` --> $DIR/mixed_read_write_in_expression.rs:14:9 | @@ -47,5 +58,5 @@ note: whether read occurs before this write depends on evaluation order LL | x = 20; | ^^^^^^ -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors diff --git a/tests/ui/temporary_assignment.stderr b/tests/ui/temporary_assignment.stderr index 7d79901a28d1..0ecf93d77d3e 100644 --- a/tests/ui/temporary_assignment.stderr +++ b/tests/ui/temporary_assignment.stderr @@ -1,3 +1,14 @@ +error: local only ever assigned single value + --> $DIR/temporary_assignment.rs:45:17 + | +LL | let mut t = (0, 0); + | ^^^^^^ +... +LL | t.0 = 1; + | ^^^^^^^ + | + = note: `#[deny(clippy::local_assigned_single_value)]` on by default + error: assignment to temporary --> $DIR/temporary_assignment.rs:47:5 | @@ -28,5 +39,5 @@ error: assignment to temporary LL | (0, 0).0 = 1; | ^^^^^^^^^^^^ -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors diff --git a/tests/ui/unnested_or_patterns.stderr b/tests/ui/unnested_or_patterns.stderr index d7b582fc8bdd..7b536fa23820 100644 --- a/tests/ui/unnested_or_patterns.stderr +++ b/tests/ui/unnested_or_patterns.stderr @@ -186,5 +186,13 @@ help: nest the patterns LL | if let [1 | 53] = [0] {} | ~~~~~~~~ -error: aborting due to 17 previous errors +error: local only ever assigned single value + --> $DIR/unnested_or_patterns.rs:22:12 + | +LL | if let x @ 0 | x @ 2 = 0 {} + | ^ ^ + | + = note: `#[deny(clippy::local_assigned_single_value)]` on by default + +error: aborting due to 18 previous errors From 564394532a2980b1b18dac15549cc810fa748bde Mon Sep 17 00:00:00 2001 From: Centri3 <114838443+Centri3@users.noreply.github.com> Date: Sun, 18 Jun 2023 16:28:16 -0500 Subject: [PATCH 2/2] refactor, and make it work for more cases --- .../src/local_assigned_single_value.rs | 279 ++++++++---------- tests/ui/local_assigned_single_value.rs | 24 +- tests/ui/local_assigned_single_value.stderr | 39 ++- .../ui/mixed_read_write_in_expression.stderr | 13 +- tests/ui/temporary_assignment.stderr | 13 +- tests/ui/unnested_or_patterns.stderr | 10 +- 6 files changed, 165 insertions(+), 213 deletions(-) diff --git a/clippy_lints/src/local_assigned_single_value.rs b/clippy_lints/src/local_assigned_single_value.rs index c7f8cfb2d0e2..a521b150c5ed 100644 --- a/clippy_lints/src/local_assigned_single_value.rs +++ b/clippy_lints/src/local_assigned_single_value.rs @@ -3,6 +3,7 @@ use clippy_utils::fn_has_unsatisfiable_preds; use clippy_utils::source::snippet_opt; use itertools::Itertools; use rustc_const_eval::interpret::Scalar; +use rustc_data_structures::fx::FxHashSet; use rustc_hir::def_id::LocalDefId; use rustc_hir::{intravisit::FnKind, Body, FnDecl}; use rustc_index::IndexVec; @@ -10,7 +11,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::mir::{ self, interpret::ConstValue, visit::Visitor, Constant, Location, Mutability, Operand, Place, Rvalue, }; -use rustc_middle::mir::{AggregateKind, PlaceElem, Terminator, TerminatorKind}; +use rustc_middle::mir::{AggregateKind, CastKind, PlaceElem, Terminator, TerminatorKind}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::source_map::Spanned; use rustc_span::Span; @@ -61,78 +62,35 @@ impl LateLintPass<'_> for LocalAssignedSingleValue { }; v.visit_body(mir); - // for (local, i) in v.map.iter_enumerated() { - // dbg!(local, i); - // } - for (local, i) in v.map.iter_enumerated() { - if !i.assigned_non_const_rvalue - && !i.mut_ref_acquired - && nested_locals_are_not_from_bad_instr(&v.map, i) - && assignments_all_same_value(&v.map, i) - { - let LocalUsageValues { - usage, - mut_ref_acquired: _, - assigned_non_const_rvalue: _, - is_from_bad_instr: _, - } = i; - - if let Some(local_decl) = mir.local_decls.get(local) - && let [dbg_info] = &*mir - .var_debug_info - .iter() - .filter(|info| info.source_info.span == local_decl.source_info.span) - .collect_vec() - // Don't handle function arguments. - && dbg_info.argument_index.is_none() - // Ignore anything from a procedural macro, or locals we cannot prove aren't - // temporaries - && let Some(snippet) = snippet_opt(cx, dbg_info.source_info.span) - && snippet.ends_with(dbg_info.name.as_str()) - { - span_lint( - cx, - LOCAL_ASSIGNED_SINGLE_VALUE, - usage.iter().map(|i| i.span).collect_vec(), - "local only ever assigned single value", - ); - } - } - } - - /* - for (local, usage) in &v.local_usage { - if should_lint(&v.local_usage, *local, usage) { - let LocalUsageValues { - usage, - mut_ref_acquired: _, - assigned_non_const: _, - } = usage; + let LocalUsageValues { + usage, + mut_ref_acquired, + } = i; - if let Some(local_decl) = mir.local_decls.get(*local) + if !mut_ref_acquired && eval_nested_locals_are_constant(&v.map, i) + && eval_local(&v.map, i) + && let Some(local_decl) = mir.local_decls.get(local) && let [dbg_info] = &*mir .var_debug_info .iter() .filter(|info| info.source_info.span == local_decl.source_info.span) .collect_vec() - // Don't handle function arguments. - && dbg_info.argument_index.is_none() - // Ignore anything from a procedural macro, or locals we cannot prove aren't - // temporaries - && let Some(snippet) = snippet_opt(cx, dbg_info.source_info.span) - && snippet.ends_with(dbg_info.name.as_str()) - { - span_lint( - cx, - LOCAL_ASSIGNED_SINGLE_VALUE, - usage.iter().map(|(span, _)| *span).collect_vec(), - "local only ever assigned single value", - ); - } + // Don't handle function arguments. + && dbg_info.argument_index.is_none() + // Ignore anything from a procedural macro, or locals we cannot prove aren't + // temporaries + && let Some(snippet) = snippet_opt(cx, dbg_info.source_info.span) + && snippet.ends_with(dbg_info.name.as_str()) + { + span_lint( + cx, + LOCAL_ASSIGNED_SINGLE_VALUE, + usage.iter().map(|i| i.span).collect_vec(), + "local only ever assigned single value", + ); } } - */ } } @@ -145,13 +103,6 @@ struct LocalUsageValues<'tcx> { usage: Vec>>, /// Whether it's mutably borrowed, ever. We should not lint this. mut_ref_acquired: bool, - /// Whether it's assigned a value we cannot prove is constant, ever. We should not lint this. - assigned_non_const_rvalue: bool, - /// Whether it's assigned a value that we know cannot be constant. This is differentiated from - /// `assigned_non_const` since we check this for nested locals. - /// - /// This is set to `true` for function calls or binary operations. - is_from_bad_instr: bool, } #[derive(Debug)] @@ -165,6 +116,10 @@ enum LocalUsage<'tcx> { /// When a `Local` depends on the value of another local by accessing any of its fields or /// indexing DependsOnWithProj(mir::Local, &'tcx PlaceElem<'tcx>), + /// Used when a local's assigned a value we cannot prove is constant. + NonConst, + /// This is always overwritten. + Pending, } struct V<'a, 'tcx> { @@ -193,30 +148,28 @@ impl<'a, 'tcx> Visitor<'tcx> for V<'a, 'tcx> { } let usage = &mut map[place.local]; + let mut spanned = Spanned { + node: LocalUsage::Pending, + span: stmt.source_info.span, + }; - if let Rvalue::Use(Operand::Constant(constant)) = rvalue - && let Constant { literal, .. } = **constant - && let Some(ConstValue::Scalar(scalar)) = literal.try_to_value(cx.tcx) - { - usage.usage.push(Spanned { node: LocalUsage::Scalar(scalar), span: stmt.source_info.span }); - } else if let Rvalue::Use(operand) = rvalue - && let Some(place) = operand.place() - { - if let [base_proj, ..] = place.projection.as_slice() - // Handle `let [x, y] = [1, 1]` and `let (x, y) = (1, 1)` - && matches!(base_proj, PlaceElem::Field(..) | PlaceElem::Index(..)) + if let Rvalue::Use(operand) = rvalue { + if let Operand::Constant(constant) = operand + && let Constant { literal, .. } = **constant + && let Some(ConstValue::Scalar(scalar)) = literal.try_to_value(cx.tcx) { - usage.usage.push(Spanned { - node: LocalUsage::DependsOnWithProj(place.local, base_proj), - span: stmt.source_info.span, - }); - } else { - // It seems sometimes a local's just moved, no projections, so let's make sure we - // don't set `assigned_non_const` to true for these cases - usage.usage.push(Spanned { - node: LocalUsage::DependsOn(place.local), - span: stmt.source_info.span - }); + spanned.node = LocalUsage::Scalar(scalar); + } else if let Some(place) = operand.place() { + if let [base_proj, ..] = place.projection.as_slice() + // Handle `let [x, y] = [1, 1]` and `let (x, y) = (1, 1)` + && matches!(base_proj, PlaceElem::Field(..) | PlaceElem::Index(..)) + { + spanned.node = LocalUsage::DependsOnWithProj(place.local, base_proj); + } else { + // It seems sometimes a local's just moved, no projections, so let's make sure we + // don't set `assigned_non_const` to true for these cases + spanned.node = LocalUsage::DependsOn(place.local); + } } } // Handle creation of tuples/arrays, otherwise the above would be useless @@ -226,25 +179,45 @@ impl<'a, 'tcx> Visitor<'tcx> for V<'a, 'tcx> { // TODO: Let's remove this `cloned` call, if possible. && let Some(scalars) = extract_scalars(cx, fields.into_iter().cloned()) { - usage.usage.push(Spanned { - node: LocalUsage::Scalars(scalars), - span: stmt.source_info.span, - }) - } else if let Rvalue::BinaryOp(..) | Rvalue::CheckedBinaryOp(..) = rvalue { - usage.is_from_bad_instr = true; + spanned.node = LocalUsage::Scalars(scalars); + } else if let Rvalue::Cast(kind, operand, _) = rvalue { + if let Operand::Constant(constant) = operand + && matches!( + kind, + CastKind::IntToInt | CastKind::FloatToInt | CastKind::FloatToFloat | CastKind::IntToFloat, + ) + && let Constant { literal, .. } = **constant + && let Some(ConstValue::Scalar(scalar)) = literal.try_to_value(cx.tcx) + { + spanned.node = LocalUsage::Scalar(scalar); + } else if let Operand::Move(place) = operand { + if let [base_proj, ..] = place.projection.as_slice() + && matches!(base_proj, PlaceElem::Field(..) | PlaceElem::Index(..)) + { + // Probably unnecessary + spanned.node = LocalUsage::DependsOnWithProj(place.local, base_proj); + } else { + // Handle casts from enum discriminants + spanned.node = LocalUsage::DependsOn(place.local); + } + } } else { - // We can also probably handle stuff like `x += 1` here, maybe. But this would be - // very very complex. Let's keep it simple enough. - usage.assigned_non_const_rvalue = true; + spanned.node = LocalUsage::NonConst; + } + + if !matches!(spanned.node, LocalUsage::Pending) { + usage.usage.push(spanned); } } fn visit_terminator(&mut self, terminator: &Terminator<'_>, _: Location) { let Self { body: _, cx: _, map } = self; - // TODO: Maybe we can allow const fns, if we can evaluate them of course if let TerminatorKind::Call { destination, .. } = terminator.kind { - map[destination.local].is_from_bad_instr = true; + map[destination.local].usage.push(Spanned { + node: LocalUsage::NonConst, + span: terminator.source_info.span, + }); } } } @@ -269,87 +242,85 @@ where .collect::>>() } -fn assignments_all_same_value(map: &LocalUsageMap<'_>, usage: &LocalUsageValues<'_>) -> bool { - let LocalUsageValues { - usage, - mut_ref_acquired: _, - assigned_non_const_rvalue: _, - is_from_bad_instr: _, - } = usage; +fn eval_local(map: &LocalUsageMap<'_>, local: &LocalUsageValues<'_>) -> bool { + let mut assignments = vec![]; - if usage.len() <= 1 { + if local.usage.len() == 1 { return false; } - // TODO: This code is clone-hell. - - let mut all_assignments = vec![]; - for assignment in usage { - match &assignment.node { - LocalUsage::Scalar(scalar) => { - all_assignments.push(scalar.clone()); - }, - // FIXME: This doesn't handle assignment of tuples, arrays or anything else currently. - LocalUsage::Scalars(_) => {}, - // FIXME: This doesn't allow nested dependencies, currently. - // FIXME: This only allows one assignment for dependencies. + for assignment in &local.usage { + match assignment.node { + LocalUsage::Scalar(scalar) => assignments.push(scalar), LocalUsage::DependsOn(local) => { - let [assignment] = &*map[*local].usage else { - continue; + let [assignment] = &*map[local].usage else { + return false; }; - match assignment.node { - LocalUsage::Scalar(scalar) => all_assignments.push(scalar.clone()), - LocalUsage::Scalars(_) => {}, - _ => return false, + if let LocalUsage::Scalar(scalar) = assignment.node { + assignments.push(scalar); + } else { + return false; } }, LocalUsage::DependsOnWithProj(local, base_proj) => { - let [assignment] = &*map[*local].usage else { - continue; + let [assignment] = &*map[local].usage else { + return false; }; match base_proj { PlaceElem::Field(idx, _) if let LocalUsage::Scalars(scalars) = &assignment.node => { - all_assignments.push(scalars[idx.as_usize()].clone()); + assignments.push(scalars[idx.as_usize()]); }, PlaceElem::Index(idx) if let LocalUsage::Scalars(scalars) = &assignment.node => { - all_assignments.push(scalars[idx.as_usize()].clone()); + assignments.push(scalars[idx.as_usize()]); }, _ => return false, } }, + _ => return false, } } - if let [head, tail @ ..] = &*all_assignments && tail.iter().all(|i| i == head) { + if let Some(assignments) = assignments.iter().map(|i| { + if let Scalar::Int(int) = i { + return Some(int.to_bits(int.size()).ok()).flatten(); + }; + + None + }) + .collect::>>() + && let [head, tail @ ..] = &*assignments + && tail.iter().all(|&i| i == *head) + { return true; } false } -fn nested_locals_are_not_from_bad_instr(map: &LocalUsageMap<'_>, usage: &LocalUsageValues<'_>) -> bool { - // FIXME: This is a hacky workaround to not have a stack overflow. Instead, we should fix the root - // cause. - nested_locals_are_not_from_bad_instr_inner(map, usage, 0) +fn eval_nested_locals_are_constant(map: &LocalUsageMap<'_>, local: &LocalUsageValues<'_>) -> bool { + eval_nested_locals_are_constant_with_visited_locals(map, local, &mut FxHashSet::default()) } -fn nested_locals_are_not_from_bad_instr_inner( +/// Do not call this manually - use `eval_nested_locals_are_constant` instead. +fn eval_nested_locals_are_constant_with_visited_locals( map: &LocalUsageMap<'_>, - usage: &LocalUsageValues<'_>, - depth: usize, + local: &LocalUsageValues<'_>, + visited_locals: &mut FxHashSet, ) -> bool { - if depth < 10 && !usage.is_from_bad_instr { - let mut all_good_instrs = true; - for assignment in &usage.usage { - match assignment.node { - LocalUsage::Scalar(_) | LocalUsage::Scalars(_) => continue, - LocalUsage::DependsOn(local) | LocalUsage::DependsOnWithProj(local, _) => { - all_good_instrs &= nested_locals_are_not_from_bad_instr_inner(map, &map[local], depth + 1); - }, - } + let mut constness = true; + for assignment in &local.usage { + match assignment.node { + LocalUsage::DependsOn(local) | LocalUsage::DependsOnWithProj(local, _) => { + if !visited_locals.insert(local) { + // Short-circuit to ensure we don't get stuck in a loop + return false; + } + + constness &= eval_nested_locals_are_constant_with_visited_locals(map, &map[local], visited_locals); + }, + LocalUsage::NonConst => return false, + _ => {}, } - return all_good_instrs; } - - false + constness } diff --git a/tests/ui/local_assigned_single_value.rs b/tests/ui/local_assigned_single_value.rs index 0e943d2c7cbf..ad5b37bc4fe1 100644 --- a/tests/ui/local_assigned_single_value.rs +++ b/tests/ui/local_assigned_single_value.rs @@ -1,7 +1,15 @@ -//@aux-build:proc_macros.rs -#![allow(unused)] +//@aux-build:proc_macros.rs:proc-macro +#![allow(clippy::almost_swapped, unused)] #![warn(clippy::local_assigned_single_value)] +// Ensure this does not get stuck in a loop +fn foo() { + let mut a = 0; + let mut b = 9; + a = b; + b = a; +} + #[macro_use] extern crate proc_macros; @@ -11,6 +19,13 @@ struct A(u32, u32); #[repr(i32)] enum B { + A = 0, + B = 1, + C = 2, +} + +#[repr(i32)] +enum NonCLike { A, B, C, @@ -46,7 +61,9 @@ fn main() { x = 1; x = 1; x = true as i32; - x = B::B as i32; + // FIXME: We should lint this. For non C-like enums, this works, but not for C-like enums. + // x = B::B as i32; + x = NonCLike::B as i32; { x = 1; } @@ -72,7 +89,6 @@ fn main() { // Don't lint a(&mut x); let mut y = 1; - // FIXME: Linting this requires nested dependencies, thus we don't currently, but please fix (x, y) = (1, 1); y = 1; // Don't lint diff --git a/tests/ui/local_assigned_single_value.stderr b/tests/ui/local_assigned_single_value.stderr index 678fdf4fd7b7..fac18bbf2811 100644 --- a/tests/ui/local_assigned_single_value.stderr +++ b/tests/ui/local_assigned_single_value.stderr @@ -1,8 +1,8 @@ error: local only ever assigned single value - --> $DIR/local_assigned_single_value.rs:33:17 + --> $DIR/local_assigned_single_value.rs:52:17 | -LL | let mut x = _a.0; - | ^^^^ +LL | let mut x = (1,).0; + | ^^^^^^ LL | x = 1; | ^^^^^ LL | x = 1; @@ -11,17 +11,25 @@ LL | x = 1; = note: `-D clippy::local-assigned-single-value` implied by `-D warnings` error: local only ever assigned single value - --> $DIR/local_assigned_single_value.rs:37:17 + --> $DIR/local_assigned_single_value.rs:60:17 | -LL | let mut x = (1,).0; - | ^^^^^^ +LL | let mut x = 1; + | ^ LL | x = 1; | ^^^^^ LL | x = 1; | ^^^^^ +LL | x = true as i32; + | ^^^^^^^^^^^^^^^ +... +LL | x = NonCLike::B as i32; + | ^^^^^^^^^^^^^^^^^^^^^^ +LL | { +LL | x = 1; + | ^^^^^ error: local only ever assigned single value - --> $DIR/local_assigned_single_value.rs:53:17 + --> $DIR/local_assigned_single_value.rs:70:17 | LL | let mut x = 1.0f32; | ^^^^^^ @@ -34,25 +42,12 @@ LL | x = 1.0; | ^^^^^^^ error: local only ever assigned single value - --> $DIR/local_assigned_single_value.rs:79:10 - | -LL | let [mut x, y] = [1, 2]; - | ^^^^^ -LL | x = 1; - | ^^^^^ -LL | x = 1; - | ^^^^^ -LL | { -LL | x = 1; - | ^^^^^ - -error: local only ever assigned single value - --> $DIR/local_assigned_single_value.rs:85:17 + --> $DIR/local_assigned_single_value.rs:101:17 | LL | let mut x = 1; | ^ LL | x = 1; | ^^^^^ -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/mixed_read_write_in_expression.stderr b/tests/ui/mixed_read_write_in_expression.stderr index 0832e9998bf7..8cc68b0ac7b4 100644 --- a/tests/ui/mixed_read_write_in_expression.stderr +++ b/tests/ui/mixed_read_write_in_expression.stderr @@ -1,14 +1,3 @@ -error: local only ever assigned single value - --> $DIR/mixed_read_write_in_expression.rs:91:19 - | -LL | let mut tup = (0, 0); - | ^^^^^^ -LL | let c = { -LL | tup.0 = 1; - | ^^^^^^^^^ - | - = note: `#[deny(clippy::local_assigned_single_value)]` on by default - error: unsequenced read of `x` --> $DIR/mixed_read_write_in_expression.rs:14:9 | @@ -58,5 +47,5 @@ note: whether read occurs before this write depends on evaluation order LL | x = 20; | ^^^^^^ -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/temporary_assignment.stderr b/tests/ui/temporary_assignment.stderr index 0ecf93d77d3e..7d79901a28d1 100644 --- a/tests/ui/temporary_assignment.stderr +++ b/tests/ui/temporary_assignment.stderr @@ -1,14 +1,3 @@ -error: local only ever assigned single value - --> $DIR/temporary_assignment.rs:45:17 - | -LL | let mut t = (0, 0); - | ^^^^^^ -... -LL | t.0 = 1; - | ^^^^^^^ - | - = note: `#[deny(clippy::local_assigned_single_value)]` on by default - error: assignment to temporary --> $DIR/temporary_assignment.rs:47:5 | @@ -39,5 +28,5 @@ error: assignment to temporary LL | (0, 0).0 = 1; | ^^^^^^^^^^^^ -error: aborting due to 5 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/unnested_or_patterns.stderr b/tests/ui/unnested_or_patterns.stderr index 7b536fa23820..d7b582fc8bdd 100644 --- a/tests/ui/unnested_or_patterns.stderr +++ b/tests/ui/unnested_or_patterns.stderr @@ -186,13 +186,5 @@ help: nest the patterns LL | if let [1 | 53] = [0] {} | ~~~~~~~~ -error: local only ever assigned single value - --> $DIR/unnested_or_patterns.rs:22:12 - | -LL | if let x @ 0 | x @ 2 = 0 {} - | ^ ^ - | - = note: `#[deny(clippy::local_assigned_single_value)]` on by default - -error: aborting due to 18 previous errors +error: aborting due to 17 previous errors