From f422ef141a1185f721093205fabc6f5a57e3dcc0 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Mon, 10 Aug 2020 07:16:30 -0400 Subject: [PATCH 1/2] Add CONST_ITEM_MUTATION lint Fixes #74053 Fixes #55721 This PR adds a new lint `CONST_ITEM_MUTATION`. Given an item `const FOO: SomeType = ..`, this lint fires on: * Attempting to write directly to a field (`FOO.field = some_val`) or array entry (`FOO.array_field[0] = val`) * Taking a mutable reference to the `const` item (`&mut FOO`), including through an autoderef `FOO.some_mut_self_method()` The lint message explains that since each use of a constant creates a new temporary, the original `const` item will not be modified. --- compiler/rustc_middle/src/mir/mod.rs | 2 + .../src/borrow_check/diagnostics/mod.rs | 97 ++++++--------- .../transform/check_const_item_mutation.rs | 114 ++++++++++++++++++ compiler/rustc_mir/src/transform/mod.rs | 2 + compiler/rustc_mir/src/util/find_self_call.rs | 35 ++++++ compiler/rustc_mir/src/util/mod.rs | 2 + .../src/build/expr/as_constant.rs | 2 +- .../rustc_mir_build/src/build/expr/as_temp.rs | 3 + compiler/rustc_mir_build/src/thir/cx/expr.rs | 15 ++- compiler/rustc_mir_build/src/thir/mod.rs | 4 + compiler/rustc_session/src/lint/builtin.rs | 7 ++ src/test/ui/error-codes/E0017.rs | 2 + src/test/ui/error-codes/E0017.stderr | 41 ++++++- src/test/ui/error-codes/E0388.rs | 2 + src/test/ui/error-codes/E0388.stderr | 39 +++++- src/test/ui/lint/lint-const-item-mutation.rs | 21 ++++ .../ui/lint/lint-const-item-mutation.stderr | 89 ++++++++++++++ 17 files changed, 406 insertions(+), 71 deletions(-) create mode 100644 compiler/rustc_mir/src/transform/check_const_item_mutation.rs create mode 100644 compiler/rustc_mir/src/util/find_self_call.rs create mode 100644 src/test/ui/lint/lint-const-item-mutation.rs create mode 100644 src/test/ui/lint/lint-const-item-mutation.stderr diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs index 1181ba6bbf946..d6d76595f3b9b 100644 --- a/compiler/rustc_middle/src/mir/mod.rs +++ b/compiler/rustc_middle/src/mir/mod.rs @@ -899,6 +899,8 @@ pub enum LocalInfo<'tcx> { User(ClearCrossCrate>), /// A temporary created that references the static with the given `DefId`. StaticRef { def_id: DefId, is_thread_local: bool }, + /// A temporary created that references the const with the given `DefId` + ConstRef { def_id: DefId }, } impl<'tcx> LocalDecl<'tcx> { diff --git a/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs b/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs index 88ff0271228e0..f51bf7730ea09 100644 --- a/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs +++ b/compiler/rustc_mir/src/borrow_check/diagnostics/mod.rs @@ -804,68 +804,51 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { debug!("move_spans: target_temp = {:?}", target_temp); if let Some(Terminator { - kind: TerminatorKind::Call { func, args, fn_span, from_hir_call, .. }, - .. + kind: TerminatorKind::Call { fn_span, from_hir_call, .. }, .. }) = &self.body[location.block].terminator { - let mut method_did = None; - if let Operand::Constant(box Constant { literal: ty::Const { ty, .. }, .. }) = func { - if let ty::FnDef(def_id, _) = *ty.kind() { - debug!("move_spans: fn = {:?}", def_id); - if let Some(ty::AssocItem { fn_has_self_parameter, .. }) = - self.infcx.tcx.opt_associated_item(def_id) - { - if *fn_has_self_parameter { - method_did = Some(def_id); - } - } - } - } + let method_did = if let Some(method_did) = + crate::util::find_self_call(self.infcx.tcx, &self.body, target_temp, location.block) + { + method_did + } else { + return normal_ret; + }; let tcx = self.infcx.tcx; - let method_did = if let Some(did) = method_did { did } else { return normal_ret }; - - if let [Operand::Move(self_place), ..] = **args { - if self_place.as_local() == Some(target_temp) { - let parent = tcx.parent(method_did); - let is_fn_once = parent == tcx.lang_items().fn_once_trait(); - let is_operator = !from_hir_call - && parent.map_or(false, |p| { - tcx.lang_items().group(LangItemGroup::Op).contains(&p) - }); - let fn_call_span = *fn_span; - - let self_arg = tcx.fn_arg_names(method_did)[0]; - - let kind = if is_fn_once { - FnSelfUseKind::FnOnceCall - } else if is_operator { - FnSelfUseKind::Operator { self_arg } - } else { - debug!( - "move_spans: method_did={:?}, fn_call_span={:?}", - method_did, fn_call_span - ); - let implicit_into_iter = matches!( - fn_call_span.desugaring_kind(), - Some(DesugaringKind::ForLoop(ForLoopLoc::IntoIter)) - ); - FnSelfUseKind::Normal { self_arg, implicit_into_iter } - }; - return FnSelfUse { - var_span: stmt.source_info.span, - fn_call_span, - fn_span: self - .infcx - .tcx - .sess - .source_map() - .guess_head_span(self.infcx.tcx.def_span(method_did)), - kind, - }; - } - } + let parent = tcx.parent(method_did); + let is_fn_once = parent == tcx.lang_items().fn_once_trait(); + let is_operator = !from_hir_call + && parent.map_or(false, |p| tcx.lang_items().group(LangItemGroup::Op).contains(&p)); + let fn_call_span = *fn_span; + + let self_arg = tcx.fn_arg_names(method_did)[0]; + + let kind = if is_fn_once { + FnSelfUseKind::FnOnceCall + } else if is_operator { + FnSelfUseKind::Operator { self_arg } + } else { + debug!("move_spans: method_did={:?}, fn_call_span={:?}", method_did, fn_call_span); + let implicit_into_iter = matches!( + fn_call_span.desugaring_kind(), + Some(DesugaringKind::ForLoop(ForLoopLoc::IntoIter)) + ); + FnSelfUseKind::Normal { self_arg, implicit_into_iter } + }; + + return FnSelfUse { + var_span: stmt.source_info.span, + fn_call_span, + fn_span: self + .infcx + .tcx + .sess + .source_map() + .guess_head_span(self.infcx.tcx.def_span(method_did)), + kind, + }; } normal_ret } diff --git a/compiler/rustc_mir/src/transform/check_const_item_mutation.rs b/compiler/rustc_mir/src/transform/check_const_item_mutation.rs new file mode 100644 index 0000000000000..589268e39bda9 --- /dev/null +++ b/compiler/rustc_mir/src/transform/check_const_item_mutation.rs @@ -0,0 +1,114 @@ +use rustc_errors::DiagnosticBuilder; +use rustc_middle::lint::LintDiagnosticBuilder; +use rustc_middle::mir::visit::Visitor; +use rustc_middle::mir::*; +use rustc_middle::ty::TyCtxt; +use rustc_session::lint::builtin::CONST_ITEM_MUTATION; +use rustc_span::def_id::DefId; + +use crate::transform::{MirPass, MirSource}; + +pub struct CheckConstItemMutation; + +impl<'tcx> MirPass<'tcx> for CheckConstItemMutation { + fn run_pass(&self, tcx: TyCtxt<'tcx>, _src: MirSource<'tcx>, body: &mut Body<'tcx>) { + let mut checker = ConstMutationChecker { body, tcx, target_local: None }; + checker.visit_body(&body); + } +} + +struct ConstMutationChecker<'a, 'tcx> { + body: &'a Body<'tcx>, + tcx: TyCtxt<'tcx>, + target_local: Option, +} + +impl<'a, 'tcx> ConstMutationChecker<'a, 'tcx> { + fn is_const_item(&self, local: Local) -> Option { + if let Some(box LocalInfo::ConstRef { def_id }) = self.body.local_decls[local].local_info { + Some(def_id) + } else { + None + } + } + fn lint_const_item_usage( + &self, + const_item: DefId, + location: Location, + decorate: impl for<'b> FnOnce(LintDiagnosticBuilder<'b>) -> DiagnosticBuilder<'b>, + ) { + let source_info = self.body.source_info(location); + let lint_root = self.body.source_scopes[source_info.scope] + .local_data + .as_ref() + .assert_crate_local() + .lint_root; + + self.tcx.struct_span_lint_hir(CONST_ITEM_MUTATION, lint_root, source_info.span, |lint| { + decorate(lint) + .span_note(self.tcx.def_span(const_item), "`const` item defined here") + .emit() + }); + } +} + +impl<'a, 'tcx> Visitor<'tcx> for ConstMutationChecker<'a, 'tcx> { + fn visit_statement(&mut self, stmt: &Statement<'tcx>, loc: Location) { + if let StatementKind::Assign(box (lhs, _)) = &stmt.kind { + // Check for assignment to fields of a constant + // Assigning directly to a constant (e.g. `FOO = true;`) is a hard error, + // so emitting a lint would be redundant. + if !lhs.projection.is_empty() { + if let Some(def_id) = self.is_const_item(lhs.local) { + self.lint_const_item_usage(def_id, loc, |lint| { + let mut lint = lint.build("attempting to modify a `const` item"); + lint.note("each usage of a `const` item creates a new temporary - the original `const` item will not be modified"); + lint + }) + } + } + // We are looking for MIR of the form: + // + // ``` + // _1 = const FOO; + // _2 = &mut _1; + // method_call(_2, ..) + // ``` + // + // Record our current LHS, so that we can detect this + // pattern in `visit_rvalue` + self.target_local = lhs.as_local(); + } + self.super_statement(stmt, loc); + self.target_local = None; + } + fn visit_rvalue(&mut self, rvalue: &Rvalue<'tcx>, loc: Location) { + if let Rvalue::Ref(_, BorrowKind::Mut { .. }, place) = rvalue { + let local = place.local; + if let Some(def_id) = self.is_const_item(local) { + // If this Rvalue is being used as the right-hand side of a + // `StatementKind::Assign`, see if it ends up getting used as + // the `self` parameter of a method call (as the terminator of our current + // BasicBlock). If so, we emit a more specific lint. + let method_did = self.target_local.and_then(|target_local| { + crate::util::find_self_call(self.tcx, &self.body, target_local, loc.block) + }); + let lint_loc = + if method_did.is_some() { self.body.terminator_loc(loc.block) } else { loc }; + self.lint_const_item_usage(def_id, lint_loc, |lint| { + let mut lint = lint.build("taking a mutable reference to a `const` item"); + lint + .note("each usage of a `const` item creates a new temporary") + .note("the mutable reference will refer to this temporary, not the original `const` item"); + + if let Some(method_did) = method_did { + lint.span_note(self.tcx.def_span(method_did), "mutable reference created due to call to this method"); + } + + lint + }); + } + } + self.super_rvalue(rvalue, loc); + } +} diff --git a/compiler/rustc_mir/src/transform/mod.rs b/compiler/rustc_mir/src/transform/mod.rs index c3a34756122a1..98f93ae6da0fe 100644 --- a/compiler/rustc_mir/src/transform/mod.rs +++ b/compiler/rustc_mir/src/transform/mod.rs @@ -16,6 +16,7 @@ use std::borrow::Cow; pub mod add_call_guards; pub mod add_moves_for_packed_drops; pub mod add_retag; +pub mod check_const_item_mutation; pub mod check_consts; pub mod check_packed_ref; pub mod check_unsafety; @@ -307,6 +308,7 @@ fn mir_const<'tcx>( &[&[ // MIR-level lints. &check_packed_ref::CheckPackedRef, + &check_const_item_mutation::CheckConstItemMutation, // What we need to do constant evaluation. &simplify::SimplifyCfg::new("initial"), &rustc_peek::SanityCheck, diff --git a/compiler/rustc_mir/src/util/find_self_call.rs b/compiler/rustc_mir/src/util/find_self_call.rs new file mode 100644 index 0000000000000..049b5f01214cf --- /dev/null +++ b/compiler/rustc_mir/src/util/find_self_call.rs @@ -0,0 +1,35 @@ +use rustc_middle::mir::*; +use rustc_middle::ty::{self, TyCtxt}; +use rustc_span::def_id::DefId; + +/// Checks if the specified `local` is used as the `self` prameter of a method call +/// in the provided `BasicBlock`. If it is, then the `DefId` of the called method is +/// returned. +pub fn find_self_call( + tcx: TyCtxt<'_>, + body: &Body<'_>, + local: Local, + block: BasicBlock, +) -> Option { + debug!("find_self_call(local={:?}): terminator={:?}", local, &body[block].terminator); + if let Some(Terminator { kind: TerminatorKind::Call { func, args, .. }, .. }) = + &body[block].terminator + { + debug!("find_self_call: func={:?}", func); + if let Operand::Constant(box Constant { literal: ty::Const { ty, .. }, .. }) = func { + if let ty::FnDef(def_id, _) = *ty.kind() { + if let Some(ty::AssocItem { fn_has_self_parameter: true, .. }) = + tcx.opt_associated_item(def_id) + { + debug!("find_self_call: args={:?}", args); + if let [Operand::Move(self_place) | Operand::Copy(self_place), ..] = **args { + if self_place.as_local() == Some(local) { + return Some(def_id); + } + } + } + } + } + } + None +} diff --git a/compiler/rustc_mir/src/util/mod.rs b/compiler/rustc_mir/src/util/mod.rs index ed0fafb1aac16..699f3bcf0146f 100644 --- a/compiler/rustc_mir/src/util/mod.rs +++ b/compiler/rustc_mir/src/util/mod.rs @@ -7,12 +7,14 @@ pub mod storage; mod alignment; pub mod collect_writes; +mod find_self_call; mod graphviz; pub(crate) mod pretty; pub(crate) mod spanview; pub use self::aggregate::expand_aggregate; pub use self::alignment::is_disaligned; +pub use self::find_self_call::find_self_call; pub use self::graphviz::write_node_label as write_graphviz_node_label; pub use self::graphviz::{graphviz_safe_def_name, write_mir_graphviz}; pub use self::pretty::{dump_enabled, dump_mir, write_mir_pretty, PassWhere}; diff --git a/compiler/rustc_mir_build/src/build/expr/as_constant.rs b/compiler/rustc_mir_build/src/build/expr/as_constant.rs index 982aefcf6045c..244a70f83b03e 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_constant.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_constant.rs @@ -21,7 +21,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let Expr { ty, temp_lifetime: _, span, kind } = expr; match kind { ExprKind::Scope { region_scope: _, lint_level: _, value } => this.as_constant(value), - ExprKind::Literal { literal, user_ty } => { + ExprKind::Literal { literal, user_ty, const_id: _ } => { let user_ty = user_ty.map(|user_ty| { this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation { span, diff --git a/compiler/rustc_mir_build/src/build/expr/as_temp.rs b/compiler/rustc_mir_build/src/build/expr/as_temp.rs index a9cc0cc2f2475..9984b527ffdb4 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_temp.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_temp.rs @@ -76,6 +76,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { local_decl.local_info = Some(box LocalInfo::StaticRef { def_id, is_thread_local: true }); } + ExprKind::Literal { const_id: Some(def_id), .. } => { + local_decl.local_info = Some(box LocalInfo::ConstRef { def_id }); + } _ => {} } this.local_decls.push(local_decl) diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index e9e49d054b88d..70c7abc265492 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -247,6 +247,7 @@ fn make_mirror_unadjusted<'a, 'tcx>( hir::ExprKind::Lit(ref lit) => ExprKind::Literal { literal: cx.const_eval_literal(&lit.node, expr_ty, lit.span, false), user_ty: None, + const_id: None, }, hir::ExprKind::Binary(op, ref lhs, ref rhs) => { @@ -306,6 +307,7 @@ fn make_mirror_unadjusted<'a, 'tcx>( ExprKind::Literal { literal: cx.const_eval_literal(&lit.node, expr_ty, lit.span, true), user_ty: None, + const_id: None, } } else { ExprKind::Unary { op: UnOp::Neg, arg: arg.to_ref() } @@ -447,6 +449,7 @@ fn make_mirror_unadjusted<'a, 'tcx>( kind: ExprKind::Literal { literal: ty::Const::zero_sized(cx.tcx, ty), user_ty, + const_id: None, }, } .to_ref(), @@ -473,6 +476,7 @@ fn make_mirror_unadjusted<'a, 'tcx>( kind: ExprKind::Literal { literal: ty::Const::zero_sized(cx.tcx, ty), user_ty: None, + const_id: None, }, } .to_ref(), @@ -585,7 +589,7 @@ fn make_mirror_unadjusted<'a, 'tcx>( temp_lifetime, ty: var_ty, span: expr.span, - kind: ExprKind::Literal { literal, user_ty: None }, + kind: ExprKind::Literal { literal, user_ty: None, const_id: None }, } .to_ref() }; @@ -714,7 +718,11 @@ fn method_callee<'a, 'tcx>( temp_lifetime, ty, span, - kind: ExprKind::Literal { literal: ty::Const::zero_sized(cx.tcx(), ty), user_ty }, + kind: ExprKind::Literal { + literal: ty::Const::zero_sized(cx.tcx(), ty), + user_ty, + const_id: None, + }, } } @@ -777,6 +785,7 @@ fn convert_path_expr<'a, 'tcx>( ExprKind::Literal { literal: ty::Const::zero_sized(cx.tcx, cx.typeck_results().node_type(expr.hir_id)), user_ty, + const_id: None, } } @@ -794,6 +803,7 @@ fn convert_path_expr<'a, 'tcx>( .tcx .mk_const(ty::Const { val, ty: cx.typeck_results().node_type(expr.hir_id) }), user_ty: None, + const_id: Some(def_id), } } @@ -810,6 +820,7 @@ fn convert_path_expr<'a, 'tcx>( ty: cx.typeck_results().node_type(expr.hir_id), }), user_ty, + const_id: Some(def_id), } } diff --git a/compiler/rustc_mir_build/src/thir/mod.rs b/compiler/rustc_mir_build/src/thir/mod.rs index 2837bfa040ff7..4d57fd5c64f8d 100644 --- a/compiler/rustc_mir_build/src/thir/mod.rs +++ b/compiler/rustc_mir_build/src/thir/mod.rs @@ -273,6 +273,10 @@ crate enum ExprKind<'tcx> { Literal { literal: &'tcx Const<'tcx>, user_ty: Option>>, + /// The `DefId` of the `const` item this literal + /// was produced from, if this is not a user-written + /// literal value. + const_id: Option, }, /// A literal containing the address of a `static`. /// diff --git a/compiler/rustc_session/src/lint/builtin.rs b/compiler/rustc_session/src/lint/builtin.rs index 2db4d2a7f51d9..74b68bfe2a5c3 100644 --- a/compiler/rustc_session/src/lint/builtin.rs +++ b/compiler/rustc_session/src/lint/builtin.rs @@ -232,6 +232,12 @@ declare_lint! { "detects unaligned references to fields of packed structs", } +declare_lint! { + pub CONST_ITEM_MUTATION, + Warn, + "detects attempts to mutate a `const` item", +} + declare_lint! { pub SAFE_PACKED_BORROWS, Warn, @@ -572,6 +578,7 @@ declare_lint_pass! { CONST_ERR, RENAMED_AND_REMOVED_LINTS, UNALIGNED_REFERENCES, + CONST_ITEM_MUTATION, SAFE_PACKED_BORROWS, PATTERNS_IN_FNS_WITHOUT_BODY, LATE_BOUND_LIFETIME_ARGUMENTS, diff --git a/src/test/ui/error-codes/E0017.rs b/src/test/ui/error-codes/E0017.rs index 818dec1207b96..54d3cc54a84d9 100644 --- a/src/test/ui/error-codes/E0017.rs +++ b/src/test/ui/error-codes/E0017.rs @@ -3,9 +3,11 @@ const C: i32 = 2; static mut M: i32 = 3; const CR: &'static mut i32 = &mut C; //~ ERROR E0764 + //~| WARN taking a mutable static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR E0764 //~| ERROR E0019 //~| ERROR cannot borrow static CONST_REF: &'static mut i32 = &mut C; //~ ERROR E0764 + //~| WARN taking a mutable static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; //~ ERROR E0764 fn main() {} diff --git a/src/test/ui/error-codes/E0017.stderr b/src/test/ui/error-codes/E0017.stderr index c1d96de1dca7c..40ef6bd97b3b0 100644 --- a/src/test/ui/error-codes/E0017.stderr +++ b/src/test/ui/error-codes/E0017.stderr @@ -1,3 +1,18 @@ +warning: taking a mutable reference to a `const` item + --> $DIR/E0017.rs:5:30 + | +LL | const CR: &'static mut i32 = &mut C; + | ^^^^^^ + | + = note: `#[warn(const_item_mutation)]` on by default + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: `const` item defined here + --> $DIR/E0017.rs:2:1 + | +LL | const C: i32 = 2; + | ^^^^^^^^^^^^^^^^^ + error[E0764]: mutable references are not allowed in constants --> $DIR/E0017.rs:5:30 | @@ -5,7 +20,7 @@ LL | const CR: &'static mut i32 = &mut C; | ^^^^^^ `&mut` is only allowed in `const fn` error[E0019]: static contains unimplemented expression type - --> $DIR/E0017.rs:6:39 + --> $DIR/E0017.rs:7:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ @@ -13,30 +28,44 @@ LL | static STATIC_REF: &'static mut i32 = &mut X; = help: add `#![feature(const_mut_refs)]` to the crate attributes to enable error[E0764]: mutable references are not allowed in statics - --> $DIR/E0017.rs:6:39 + --> $DIR/E0017.rs:7:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ `&mut` is only allowed in `const fn` error[E0596]: cannot borrow immutable static item `X` as mutable - --> $DIR/E0017.rs:6:39 + --> $DIR/E0017.rs:7:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ cannot borrow as mutable +warning: taking a mutable reference to a `const` item + --> $DIR/E0017.rs:10:38 + | +LL | static CONST_REF: &'static mut i32 = &mut C; + | ^^^^^^ + | + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: `const` item defined here + --> $DIR/E0017.rs:2:1 + | +LL | const C: i32 = 2; + | ^^^^^^^^^^^^^^^^^ + error[E0764]: mutable references are not allowed in statics - --> $DIR/E0017.rs:9:38 + --> $DIR/E0017.rs:10:38 | LL | static CONST_REF: &'static mut i32 = &mut C; | ^^^^^^ `&mut` is only allowed in `const fn` error[E0764]: mutable references are not allowed in statics - --> $DIR/E0017.rs:10:52 + --> $DIR/E0017.rs:12:52 | LL | static STATIC_MUT_REF: &'static mut i32 = unsafe { &mut M }; | ^^^^^^ `&mut` is only allowed in `const fn` -error: aborting due to 6 previous errors +error: aborting due to 6 previous errors; 2 warnings emitted Some errors have detailed explanations: E0019, E0596, E0764. For more information about an error, try `rustc --explain E0019`. diff --git a/src/test/ui/error-codes/E0388.rs b/src/test/ui/error-codes/E0388.rs index 13131017c2e07..8ad586bb30f1e 100644 --- a/src/test/ui/error-codes/E0388.rs +++ b/src/test/ui/error-codes/E0388.rs @@ -2,9 +2,11 @@ static X: i32 = 1; const C: i32 = 2; const CR: &'static mut i32 = &mut C; //~ ERROR E0764 + //~| WARN taking a mutable static STATIC_REF: &'static mut i32 = &mut X; //~ ERROR E0019 //~| ERROR cannot borrow //~| ERROR E0764 static CONST_REF: &'static mut i32 = &mut C; //~ ERROR E0764 + //~| WARN taking a mutable fn main() {} diff --git a/src/test/ui/error-codes/E0388.stderr b/src/test/ui/error-codes/E0388.stderr index f09100bac43ce..39bc717ceec3e 100644 --- a/src/test/ui/error-codes/E0388.stderr +++ b/src/test/ui/error-codes/E0388.stderr @@ -1,3 +1,18 @@ +warning: taking a mutable reference to a `const` item + --> $DIR/E0388.rs:4:30 + | +LL | const CR: &'static mut i32 = &mut C; + | ^^^^^^ + | + = note: `#[warn(const_item_mutation)]` on by default + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: `const` item defined here + --> $DIR/E0388.rs:2:1 + | +LL | const C: i32 = 2; + | ^^^^^^^^^^^^^^^^^ + error[E0764]: mutable references are not allowed in constants --> $DIR/E0388.rs:4:30 | @@ -5,7 +20,7 @@ LL | const CR: &'static mut i32 = &mut C; | ^^^^^^ `&mut` is only allowed in `const fn` error[E0019]: static contains unimplemented expression type - --> $DIR/E0388.rs:5:39 + --> $DIR/E0388.rs:6:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ @@ -13,24 +28,38 @@ LL | static STATIC_REF: &'static mut i32 = &mut X; = help: add `#![feature(const_mut_refs)]` to the crate attributes to enable error[E0764]: mutable references are not allowed in statics - --> $DIR/E0388.rs:5:39 + --> $DIR/E0388.rs:6:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ `&mut` is only allowed in `const fn` error[E0596]: cannot borrow immutable static item `X` as mutable - --> $DIR/E0388.rs:5:39 + --> $DIR/E0388.rs:6:39 | LL | static STATIC_REF: &'static mut i32 = &mut X; | ^^^^^^ cannot borrow as mutable +warning: taking a mutable reference to a `const` item + --> $DIR/E0388.rs:9:38 + | +LL | static CONST_REF: &'static mut i32 = &mut C; + | ^^^^^^ + | + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: `const` item defined here + --> $DIR/E0388.rs:2:1 + | +LL | const C: i32 = 2; + | ^^^^^^^^^^^^^^^^^ + error[E0764]: mutable references are not allowed in statics - --> $DIR/E0388.rs:8:38 + --> $DIR/E0388.rs:9:38 | LL | static CONST_REF: &'static mut i32 = &mut C; | ^^^^^^ `&mut` is only allowed in `const fn` -error: aborting due to 5 previous errors +error: aborting due to 5 previous errors; 2 warnings emitted Some errors have detailed explanations: E0019, E0596, E0764. For more information about an error, try `rustc --explain E0019`. diff --git a/src/test/ui/lint/lint-const-item-mutation.rs b/src/test/ui/lint/lint-const-item-mutation.rs new file mode 100644 index 0000000000000..92d29a7dae475 --- /dev/null +++ b/src/test/ui/lint/lint-const-item-mutation.rs @@ -0,0 +1,21 @@ +// check-pass + +struct MyStruct { + field: bool, + inner_array: [char; 1], +} +impl MyStruct { + fn use_mut(&mut self) {} +} + +const ARRAY: [u8; 1] = [25]; +const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] }; + +fn main() { + ARRAY[0] = 5; //~ WARN attempting to modify + MY_STRUCT.field = false; //~ WARN attempting to modify + MY_STRUCT.inner_array[0] = 'b'; //~ WARN attempting to modify + MY_STRUCT.use_mut(); //~ WARN taking + &mut MY_STRUCT; //~ WARN taking + (&mut MY_STRUCT).use_mut(); //~ WARN taking +} diff --git a/src/test/ui/lint/lint-const-item-mutation.stderr b/src/test/ui/lint/lint-const-item-mutation.stderr new file mode 100644 index 0000000000000..2d8f2c49744ba --- /dev/null +++ b/src/test/ui/lint/lint-const-item-mutation.stderr @@ -0,0 +1,89 @@ +warning: attempting to modify a `const` item + --> $DIR/lint-const-item-mutation.rs:15:5 + | +LL | ARRAY[0] = 5; + | ^^^^^^^^^^^^ + | + = note: `#[warn(const_item_mutation)]` on by default + = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified +note: `const` item defined here + --> $DIR/lint-const-item-mutation.rs:11:1 + | +LL | const ARRAY: [u8; 1] = [25]; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: attempting to modify a `const` item + --> $DIR/lint-const-item-mutation.rs:16:5 + | +LL | MY_STRUCT.field = false; + | ^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified +note: `const` item defined here + --> $DIR/lint-const-item-mutation.rs:12:1 + | +LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: attempting to modify a `const` item + --> $DIR/lint-const-item-mutation.rs:17:5 + | +LL | MY_STRUCT.inner_array[0] = 'b'; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: each usage of a `const` item creates a new temporary - the original `const` item will not be modified +note: `const` item defined here + --> $DIR/lint-const-item-mutation.rs:12:1 + | +LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: taking a mutable reference to a `const` item + --> $DIR/lint-const-item-mutation.rs:18:5 + | +LL | MY_STRUCT.use_mut(); + | ^^^^^^^^^^^^^^^^^^^ + | + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: mutable reference created due to call to this method + --> $DIR/lint-const-item-mutation.rs:8:5 + | +LL | fn use_mut(&mut self) {} + | ^^^^^^^^^^^^^^^^^^^^^ +note: `const` item defined here + --> $DIR/lint-const-item-mutation.rs:12:1 + | +LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: taking a mutable reference to a `const` item + --> $DIR/lint-const-item-mutation.rs:19:5 + | +LL | &mut MY_STRUCT; + | ^^^^^^^^^^^^^^ + | + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: `const` item defined here + --> $DIR/lint-const-item-mutation.rs:12:1 + | +LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: taking a mutable reference to a `const` item + --> $DIR/lint-const-item-mutation.rs:20:5 + | +LL | (&mut MY_STRUCT).use_mut(); + | ^^^^^^^^^^^^^^^^ + | + = note: each usage of a `const` item creates a new temporary + = note: the mutable reference will refer to this temporary, not the original `const` item +note: `const` item defined here + --> $DIR/lint-const-item-mutation.rs:12:1 + | +LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'] }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +warning: 6 warnings emitted + From 4434e8cefbec96e6928ea23769eb1a83d0f198b5 Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 8 Sep 2020 17:59:43 -0400 Subject: [PATCH 2/2] Adjust Clippy for CONST_ITEM_MUTATION lint We no longer lint assignments to const item fields in the `temporary_assignment` lint, since this is now covered by the `CONST_ITEM_MUTATION` lint. Additionally, we `#![allow(const_item_mutation)]` in the `borrow_interior_mutable_const.rs` test. Clippy UI tests are run with `-D warnings`, which seems to cause builtin lints to prevent Clippy lints from running. --- .../clippy_lints/src/temporary_assignment.rs | 4 +-- .../tests/ui/borrow_interior_mutable_const.rs | 1 + .../ui/borrow_interior_mutable_const.stderr | 32 ++++++++--------- .../clippy/tests/ui/temporary_assignment.rs | 1 + .../tests/ui/temporary_assignment.stderr | 34 +++---------------- 5 files changed, 24 insertions(+), 48 deletions(-) diff --git a/src/tools/clippy/clippy_lints/src/temporary_assignment.rs b/src/tools/clippy/clippy_lints/src/temporary_assignment.rs index 1aeff1baa362e..2b6ddadd4c112 100644 --- a/src/tools/clippy/clippy_lints/src/temporary_assignment.rs +++ b/src/tools/clippy/clippy_lints/src/temporary_assignment.rs @@ -1,5 +1,4 @@ use crate::utils::{is_adjusted, span_lint}; -use rustc_hir::def::{DefKind, Res}; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -22,10 +21,9 @@ declare_clippy_lint! { "assignments to temporaries" } -fn is_temporary(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { +fn is_temporary(_cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { match &expr.kind { ExprKind::Struct(..) | ExprKind::Tup(..) => true, - ExprKind::Path(qpath) => matches!(cx.qpath_res(qpath, expr.hir_id), Res::Def(DefKind::Const, ..)), _ => false, } } diff --git a/src/tools/clippy/tests/ui/borrow_interior_mutable_const.rs b/src/tools/clippy/tests/ui/borrow_interior_mutable_const.rs index a414832bcd362..39f8751054850 100644 --- a/src/tools/clippy/tests/ui/borrow_interior_mutable_const.rs +++ b/src/tools/clippy/tests/ui/borrow_interior_mutable_const.rs @@ -1,5 +1,6 @@ #![warn(clippy::borrow_interior_mutable_const)] #![allow(clippy::declare_interior_mutable_const, clippy::ref_in_deref)] +#![allow(const_item_mutation)] use std::borrow::Cow; use std::cell::{Cell, UnsafeCell}; diff --git a/src/tools/clippy/tests/ui/borrow_interior_mutable_const.stderr b/src/tools/clippy/tests/ui/borrow_interior_mutable_const.stderr index 1e0b3e4d20a52..5800af7e960f4 100644 --- a/src/tools/clippy/tests/ui/borrow_interior_mutable_const.stderr +++ b/src/tools/clippy/tests/ui/borrow_interior_mutable_const.stderr @@ -1,5 +1,5 @@ error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:65:5 + --> $DIR/borrow_interior_mutable_const.rs:66:5 | LL | ATOMIC.store(1, Ordering::SeqCst); //~ ERROR interior mutability | ^^^^^^ @@ -8,7 +8,7 @@ LL | ATOMIC.store(1, Ordering::SeqCst); //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:66:16 + --> $DIR/borrow_interior_mutable_const.rs:67:16 | LL | assert_eq!(ATOMIC.load(Ordering::SeqCst), 5); //~ ERROR interior mutability | ^^^^^^ @@ -16,7 +16,7 @@ LL | assert_eq!(ATOMIC.load(Ordering::SeqCst), 5); //~ ERROR interior mutabi = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:69:22 + --> $DIR/borrow_interior_mutable_const.rs:70:22 | LL | let _once_ref = &ONCE_INIT; //~ ERROR interior mutability | ^^^^^^^^^ @@ -24,7 +24,7 @@ LL | let _once_ref = &ONCE_INIT; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:70:25 + --> $DIR/borrow_interior_mutable_const.rs:71:25 | LL | let _once_ref_2 = &&ONCE_INIT; //~ ERROR interior mutability | ^^^^^^^^^ @@ -32,7 +32,7 @@ LL | let _once_ref_2 = &&ONCE_INIT; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:71:27 + --> $DIR/borrow_interior_mutable_const.rs:72:27 | LL | let _once_ref_4 = &&&&ONCE_INIT; //~ ERROR interior mutability | ^^^^^^^^^ @@ -40,7 +40,7 @@ LL | let _once_ref_4 = &&&&ONCE_INIT; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:72:26 + --> $DIR/borrow_interior_mutable_const.rs:73:26 | LL | let _once_mut = &mut ONCE_INIT; //~ ERROR interior mutability | ^^^^^^^^^ @@ -48,7 +48,7 @@ LL | let _once_mut = &mut ONCE_INIT; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:83:14 + --> $DIR/borrow_interior_mutable_const.rs:84:14 | LL | let _ = &ATOMIC_TUPLE; //~ ERROR interior mutability | ^^^^^^^^^^^^ @@ -56,7 +56,7 @@ LL | let _ = &ATOMIC_TUPLE; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:84:14 + --> $DIR/borrow_interior_mutable_const.rs:85:14 | LL | let _ = &ATOMIC_TUPLE.0; //~ ERROR interior mutability | ^^^^^^^^^^^^ @@ -64,7 +64,7 @@ LL | let _ = &ATOMIC_TUPLE.0; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:85:19 + --> $DIR/borrow_interior_mutable_const.rs:86:19 | LL | let _ = &(&&&&ATOMIC_TUPLE).0; //~ ERROR interior mutability | ^^^^^^^^^^^^ @@ -72,7 +72,7 @@ LL | let _ = &(&&&&ATOMIC_TUPLE).0; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:86:14 + --> $DIR/borrow_interior_mutable_const.rs:87:14 | LL | let _ = &ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability | ^^^^^^^^^^^^ @@ -80,7 +80,7 @@ LL | let _ = &ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:87:13 + --> $DIR/borrow_interior_mutable_const.rs:88:13 | LL | let _ = ATOMIC_TUPLE.0[0].load(Ordering::SeqCst); //~ ERROR interior mutability | ^^^^^^^^^^^^ @@ -88,7 +88,7 @@ LL | let _ = ATOMIC_TUPLE.0[0].load(Ordering::SeqCst); //~ ERROR interior mu = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:93:13 + --> $DIR/borrow_interior_mutable_const.rs:94:13 | LL | let _ = ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability | ^^^^^^^^^^^^ @@ -96,7 +96,7 @@ LL | let _ = ATOMIC_TUPLE.0[0]; //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:98:5 + --> $DIR/borrow_interior_mutable_const.rs:99:5 | LL | CELL.set(2); //~ ERROR interior mutability | ^^^^ @@ -104,7 +104,7 @@ LL | CELL.set(2); //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:99:16 + --> $DIR/borrow_interior_mutable_const.rs:100:16 | LL | assert_eq!(CELL.get(), 6); //~ ERROR interior mutability | ^^^^ @@ -112,7 +112,7 @@ LL | assert_eq!(CELL.get(), 6); //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:112:5 + --> $DIR/borrow_interior_mutable_const.rs:113:5 | LL | u64::ATOMIC.store(5, Ordering::SeqCst); //~ ERROR interior mutability | ^^^^^^^^^^^ @@ -120,7 +120,7 @@ LL | u64::ATOMIC.store(5, Ordering::SeqCst); //~ ERROR interior mutability = help: assign this const to a local or static variable, and use the variable here error: a `const` item with interior mutability should not be borrowed - --> $DIR/borrow_interior_mutable_const.rs:113:16 + --> $DIR/borrow_interior_mutable_const.rs:114:16 | LL | assert_eq!(u64::ATOMIC.load(Ordering::SeqCst), 9); //~ ERROR interior mutability | ^^^^^^^^^^^ diff --git a/src/tools/clippy/tests/ui/temporary_assignment.rs b/src/tools/clippy/tests/ui/temporary_assignment.rs index c6c315d5fab5d..d6f56d40c5d4e 100644 --- a/src/tools/clippy/tests/ui/temporary_assignment.rs +++ b/src/tools/clippy/tests/ui/temporary_assignment.rs @@ -1,4 +1,5 @@ #![warn(clippy::temporary_assignment)] +#![allow(const_item_mutation)] use std::ops::{Deref, DerefMut}; diff --git a/src/tools/clippy/tests/ui/temporary_assignment.stderr b/src/tools/clippy/tests/ui/temporary_assignment.stderr index 4efe2d4bb6713..4cc32c79f05ce 100644 --- a/src/tools/clippy/tests/ui/temporary_assignment.stderr +++ b/src/tools/clippy/tests/ui/temporary_assignment.stderr @@ -1,5 +1,5 @@ error: assignment to temporary - --> $DIR/temporary_assignment.rs:47:5 + --> $DIR/temporary_assignment.rs:48:5 | LL | Struct { field: 0 }.field = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -7,7 +7,7 @@ LL | Struct { field: 0 }.field = 1; = note: `-D clippy::temporary-assignment` implied by `-D warnings` error: assignment to temporary - --> $DIR/temporary_assignment.rs:48:5 + --> $DIR/temporary_assignment.rs:49:5 | LL | / MultiStruct { LL | | structure: Struct { field: 0 }, @@ -17,40 +17,16 @@ LL | | .field = 1; | |______________^ error: assignment to temporary - --> $DIR/temporary_assignment.rs:53:5 + --> $DIR/temporary_assignment.rs:54:5 | LL | ArrayStruct { array: [0] }.array[0] = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: assignment to temporary - --> $DIR/temporary_assignment.rs:54:5 + --> $DIR/temporary_assignment.rs:55:5 | LL | (0, 0).0 = 1; | ^^^^^^^^^^^^ -error: assignment to temporary - --> $DIR/temporary_assignment.rs:56:5 - | -LL | A.0 = 2; - | ^^^^^^^ - -error: assignment to temporary - --> $DIR/temporary_assignment.rs:57:5 - | -LL | B.field = 2; - | ^^^^^^^^^^^ - -error: assignment to temporary - --> $DIR/temporary_assignment.rs:58:5 - | -LL | C.structure.field = 2; - | ^^^^^^^^^^^^^^^^^^^^^ - -error: assignment to temporary - --> $DIR/temporary_assignment.rs:59:5 - | -LL | D.array[0] = 2; - | ^^^^^^^^^^^^^^ - -error: aborting due to 8 previous errors +error: aborting due to 4 previous errors