Skip to content
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.

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

Allow disabling const_item_mutation lint for all uses of a specific const item #77522

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
65 changes: 39 additions & 26 deletions compiler/rustc_mir/src/transform/check_const_item_mutation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ 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_session::lint::Level;
use rustc_span::def_id::DefId;
use rustc_span::sym;

use crate::transform::{MirPass, MirSource};

Expand Down Expand Up @@ -32,31 +34,38 @@ impl<'a, 'tcx> ConstMutationChecker<'a, 'tcx> {
}
}

fn is_const_item_without_destructor(&self, local: Local) -> Option<DefId> {
let def_id = self.is_const_item(local)?;
let mut any_dtor = |_tcx, _def_id| Ok(());

// 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.
//
// pub struct Log { msg: &'static str }
// pub const LOG: Log = Log { msg: "" };
// impl Drop for Log {
// fn drop(&mut self) { println!("{}", self.msg); }
// }
//
// LOG.msg = "wow"; // prints "wow"
//
// 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: "" };
match self.tcx.calculate_dtor(def_id, &mut any_dtor) {
Some(_) => None,
None => Some(def_id),
// In addition to considering lint levels inside of Body as normal, we also
// look at whether the *definition* of the const being mutated is annotated
// with #[allow(const_item_mutation)].
fn is_suppressed_at_definition(&self, const_item: DefId) -> bool {
if let Some(local_const_item) = const_item.as_local() {
let const_item_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_const_item);
if let Some(lint_level_at_definition) = self
.tcx
.lint_levels(const_item.krate)
.level_and_source(CONST_ITEM_MUTATION, const_item_hir_id, self.tcx.sess)
{
let (level, _lint_source) = lint_level_at_definition;
level == Level::Allow
} else {
false
}
} else {
let attrs = self.tcx.get_attrs(const_item);
for attr in attrs {
if attr.name_or_empty() != sym::allow {
continue;
}
for meta in attr.meta_item_list().unwrap_or_else(Vec::new) {
if meta
.ident()
.map_or(false, |ident| ident.as_str() == CONST_ITEM_MUTATION.name_lower())
{
return true;
}
}
}
false
}
}

Expand All @@ -66,6 +75,10 @@ impl<'a, 'tcx> ConstMutationChecker<'a, 'tcx> {
location: Location,
decorate: impl for<'b> FnOnce(LintDiagnosticBuilder<'b>) -> DiagnosticBuilder<'b>,
) {
if self.is_suppressed_at_definition(const_item) {
return;
}

let source_info = self.body.source_info(location);
let lint_root = self.body.source_scopes[source_info.scope]
.local_data
Expand All @@ -88,7 +101,7 @@ impl<'a, 'tcx> Visitor<'tcx> for ConstMutationChecker<'a, 'tcx> {
// 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_without_destructor(lhs.local) {
if let Some(def_id) = self.is_const_item(lhs.local) {
// Don't lint on writes through a pointer
// (e.g. `unsafe { *FOO = 0; *BAR.field = 1; }`)
if !matches!(lhs.projection.last(), Some(PlaceElem::Deref)) {
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/lint/lint-const-item-mutation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ fn main() {
*MY_STRUCT.raw_ptr = 0;
}

MUTABLE.msg = "wow"; // no warning, because Drop observes the mutation
MUTABLE.msg = "wow"; //~ WARN attempting to modify
MUTABLE2.msg = "wow"; //~ WARN attempting to modify
VEC.push(0); //~ WARN taking a mutable reference to a `const` item
}
15 changes: 14 additions & 1 deletion src/test/ui/lint/lint-const-item-mutation.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,19 @@ note: `const` item defined here
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:50:5
|
LL | MUTABLE.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:29:1
|
LL | const MUTABLE: Mutable = Mutable { msg: "" };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: attempting to modify a `const` item
--> $DIR/lint-const-item-mutation.rs:51:5
|
Expand Down Expand Up @@ -123,5 +136,5 @@ note: `const` item defined here
LL | const VEC: Vec<i32> = Vec::new();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: 8 warnings emitted
warning: 9 warnings emitted

83 changes: 83 additions & 0 deletions src/test/ui/lint/sneaky-const-mutation.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// run-pass
// edition:2018

#![feature(once_cell)]
#![deny(const_item_mutation)]

mod library {
use std::lazy::SyncLazy;
use std::ops::{Deref, DerefMut};
use std::ptr;
use std::sync::atomic::{AtomicPtr, Ordering};
use std::sync::Mutex;

pub struct Options {
pub include_prefix: &'static str,
}

static INCLUDE_PREFIX: SyncLazy<Mutex<&'static str>> = SyncLazy::new(Mutex::default);

impl Options {
fn current() -> Self {
Options {
include_prefix: *INCLUDE_PREFIX.lock().unwrap(),
}
}
}

pub struct Cfg {
options: AtomicPtr<Options>,
}

#[allow(const_item_mutation)]
pub const CFG: Cfg = Cfg {
options: AtomicPtr::new(ptr::null_mut()),
};

impl Deref for Cfg {
type Target = Options;
fn deref(&self) -> &Self::Target {
let options = Box::into_raw(Box::new(Options::current()));
let prev = self
.options
.compare_and_swap(ptr::null_mut(), options, Ordering::Relaxed);
if !prev.is_null() {
// compare_and_swap did nothing.
let _ = unsafe { Box::from_raw(options) };
panic!();
}
unsafe { &*options }
}
}

impl DerefMut for Cfg {
fn deref_mut(&mut self) -> &mut Self::Target {
let options = self.options.get_mut();
if !options.is_null() {
panic!();
}
*options = Box::into_raw(Box::new(Options::current()));
unsafe { &mut **options }
}
}

impl Drop for Cfg {
fn drop(&mut self) {
let options = *self.options.get_mut();
if let Some(options) = unsafe { options.as_mut() } {
*INCLUDE_PREFIX.lock().unwrap() = options.include_prefix;
let _ = unsafe { Box::from_raw(options) };
}
}
}
}

use library::CFG;

fn main() {
assert_eq!(CFG.include_prefix, "");

CFG.include_prefix = "path/to";

assert_eq!(CFG.include_prefix, "path/to");
}