Skip to content

fix const_item_mutation lint to use needs_drop instead of has_dtor #160097

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rachit2323 wants to merge 1 commit into
base: main
Choose a base branch
from
+47 −46
Open

fix const_item_mutation lint to use needs_drop instead of has_dtor #160097

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions compiler/rustc_mir_transform/src/check_const_item_mutation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use rustc_hir::HirId;
use rustc_lint_defs::builtin::CONST_ITEM_MUTATION;
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::*;
use rustc_middle::ty::TyCtxt;
use rustc_middle::ty::{self, TyCtxt, TypeVisitableExt};
use rustc_span::Span;
use rustc_span::def_id::DefId;

Expand Down Expand Up @@ -35,8 +35,9 @@ impl<'tcx> ConstMutationChecker<'_, 'tcx> {
fn is_const_item_without_destructor(&self, local: Local) -> Option<DefId> {
let def_id = self.is_const_item(local)?;

// We avoid linting mutation of a const item if the const's type has a
// Drop impl. The Drop logic observes the mutation which was performed.
// We avoid linting mutation of a const item if the const's type needs
// drop. Any drop logic (including that of fields) may observe the
// mutation which was performed.
//
// pub struct Log { msg: &'static str }
// pub const LOG: Log = Log { msg: "" };
Expand All @@ -46,21 +47,30 @@ impl<'tcx> ConstMutationChecker<'_, 'tcx> {
//
// LOG.msg = "wow"; // prints "wow"
//
// Likewise, if a field of the const type has its own Drop impl, that
// drop logic may also observe the mutation:
//
// struct Inner { val: u32 }
// impl Drop for Inner { fn drop(&mut self) { println!("{}", self.val); } }
// struct Outer { inner: Inner }
// const O: Outer = Outer { inner: Inner { val: 0 } };
//
// O.inner.val = 42; // Inner::drop prints "42"
//
// FIXME(https://github.com/rust-lang/rust/issues/77425):
// Drop this exception once there is a stable attribute to suppress the
// const item mutation lint for a single specific const only. Something
// equivalent to:
//
// #[const_mutation_allowed]
// pub const LOG: Log = Log { msg: "" };
// FIXME: this should not be checking for `Drop` impls,
// but whether it or any field has a Drop impl (`needs_drop`)
// as fields' Drop impls may make this observable, too.
match self.tcx.type_of(def_id).skip_binder().ty_adt_def().map(|adt| adt.has_dtor(self.tcx))
{
Some(true) => None,
Some(false) | None => Some(def_id),
// const item mutation lint for a single specific const only.
let ty = self.tcx.type_of(def_id).instantiate_identity().skip_norm_wip();
// `needs_drop` is overly conservative for types that contain type
// parameters (e.g. `Self` in a trait associated const): it always
// returns `true` because the parameter *might* implement Drop, even
// when the concrete type at the call site does not. In that case we
// cannot suppress the lint, so fall through and warn.
if ty.has_param() {
return Some(def_id);
}
let typing_env = ty::TypingEnv::non_body_analysis(self.tcx, def_id);
if ty.needs_drop(self.tcx, typing_env) { None } else { Some(def_id) }
}

/// If we should lint on this usage, return the [`HirId`], source [`Span`]
Expand Down
10 changes: 7 additions & 3 deletions tests/ui/lint/lint-const-item-mutation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,19 @@ impl Drop for Mutable {
}
}

struct Mutable2 { // this one has drop glue but not a Drop impl
struct Mutable2 { // this one has drop glue but not a direct Drop impl
msg: &'static str,
other: String,
}

struct WithFieldDrop { inner: Mutable } // no Drop on this type, but Mutable has one

const ARRAY: [u8; 1] = [25];
const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'], raw_ptr: 2 as *mut u8 };
const RAW_PTR: *mut u8 = 1 as *mut u8;
const MUTABLE: Mutable = Mutable { msg: "" };
const MUTABLE2: Mutable2 = Mutable2 { msg: "", other: String::new() };
const WFD: WithFieldDrop = WithFieldDrop { inner: Mutable { msg: "" } };
const VEC: Vec<i32> = Vec::new();
const PTR: *mut () = 1 as *mut _;
const PTR_TO_ARRAY: *mut [u32; 4] = 0x12345678 as _;
Expand All @@ -50,8 +53,9 @@ fn main() {
*MY_STRUCT.raw_ptr = 0;
}

MUTABLE.msg = "wow"; // no warning, because Drop observes the mutation
MUTABLE2.msg = "wow"; //~ WARN attempting to modify
MUTABLE.msg = "wow"; // no warning — Drop impl observes the mutation
MUTABLE2.msg = "wow"; // no warning — field String has drop glue (needs_drop = true)
WFD.inner.msg = "observed"; // no warning — Mutable's Drop observes the field mutation
VEC.push(0); //~ WARN taking a mutable reference to a `const` item

// Test that we don't warn when converting a raw pointer
Expand Down
43 changes: 15 additions & 28 deletions tests/ui/lint/lint-const-item-mutation.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1,45 @@
warning: attempting to modify a `const` item
--> $DIR/lint-const-item-mutation.rs:37:5
--> $DIR/lint-const-item-mutation.rs:40:5
|
LL | ARRAY[0] = 5;
| ^^^^^^^^^^^^
|
= 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:26:1
--> $DIR/lint-const-item-mutation.rs:28:1
|
LL | const ARRAY: [u8; 1] = [25];
| ^^^^^^^^^^^^^^^^^^^^
= note: `#[warn(const_item_mutation)]` on by default

warning: attempting to modify a `const` item
--> $DIR/lint-const-item-mutation.rs:38:5
--> $DIR/lint-const-item-mutation.rs:41: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:27:1
--> $DIR/lint-const-item-mutation.rs:29:1
|
LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'], raw_ptr: 2 as *mut u8 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: attempting to modify a `const` item
--> $DIR/lint-const-item-mutation.rs:39:5
--> $DIR/lint-const-item-mutation.rs:42: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:27:1
--> $DIR/lint-const-item-mutation.rs:29:1
|
LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'], raw_ptr: 2 as *mut u8 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: taking a mutable reference to a `const` item
--> $DIR/lint-const-item-mutation.rs:40:5
--> $DIR/lint-const-item-mutation.rs:43:5
|
LL | MY_STRUCT.use_mut();
| ^^^^^^^^^^^^^^^^^^^
Expand All @@ -52,27 +52,27 @@ note: mutable reference created due to call to this method
LL | fn use_mut(&mut self) {}
| ^^^^^^^^^^^^^^^^^^^^^
note: `const` item defined here
--> $DIR/lint-const-item-mutation.rs:27:1
--> $DIR/lint-const-item-mutation.rs:29:1
|
LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'], raw_ptr: 2 as *mut u8 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: taking a mutable reference to a `const` item
--> $DIR/lint-const-item-mutation.rs:41:5
--> $DIR/lint-const-item-mutation.rs:44: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:27:1
--> $DIR/lint-const-item-mutation.rs:29:1
|
LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'], raw_ptr: 2 as *mut u8 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: taking a mutable reference to a `const` item
--> $DIR/lint-const-item-mutation.rs:42:5
--> $DIR/lint-const-item-mutation.rs:45:5
|
LL | (&mut MY_STRUCT).use_mut();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -85,26 +85,13 @@ note: mutable reference created due to call to this method
LL | fn use_mut(&mut self) {}
| ^^^^^^^^^^^^^^^^^^^^^
note: `const` item defined here
--> $DIR/lint-const-item-mutation.rs:27:1
--> $DIR/lint-const-item-mutation.rs:29:1
|
LL | const MY_STRUCT: MyStruct = MyStruct { field: true, inner_array: ['a'], raw_ptr: 2 as *mut u8 };
| ^^^^^^^^^^^^^^^^^^^^^^^^^

warning: attempting to modify a `const` item
--> $DIR/lint-const-item-mutation.rs:54:5
|
LL | MUTABLE2.msg = "wow";
| ^^^^^^^^^^^^^^^^^^^^
|
= 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:30:1
|
LL | const MUTABLE2: Mutable2 = Mutable2 { msg: "", other: String::new() };
| ^^^^^^^^^^^^^^^^^^^^^^^^

warning: taking a mutable reference to a `const` item
--> $DIR/lint-const-item-mutation.rs:55:5
--> $DIR/lint-const-item-mutation.rs:59:5
|
LL | VEC.push(0);
| ^^^^^^^^^^^
Expand All @@ -114,10 +101,10 @@ LL | VEC.push(0);
note: mutable reference created due to call to this method
--> $SRC_DIR/alloc/src/vec/mod.rs:LL:COL
note: `const` item defined here
--> $DIR/lint-const-item-mutation.rs:31:1
--> $DIR/lint-const-item-mutation.rs:34:1
|
LL | const VEC: Vec<i32> = Vec::new();
| ^^^^^^^^^^^^^^^^^^^

warning: 8 warnings emitted
warning: 7 warnings emitted

Loading