-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Bypass const_item_mutation if const's type has Drop impl #77251
Conversation
I'm not convinced that this is a good idea. The point of If anything, However, I think it would be reasonable to add a note to the warning message if the type has a destructor, so that the user is away that deleting the statement may cause unexpected changes in behavior. |
Let me clarify the reason I came across this. I am using assignments for a global static configuration of a library, as in e.g. Here is a simplified setup to give you the flavor (actual implementation is somewhat different): pub mod thelibrary {
use lazy_static::lazy_static;
use std::ptr;
use std::sync::atomic::{AtomicPtr, Ordering};
use std::sync::Mutex;
pub struct Options {
pub include_prefix: &'static str,
}
lazy_static! {
static ref INCLUDE_PREFIX: Mutex<&'static str> = Mutex::new(env!("CARGO_CRATE_NAME"));
}
impl Options {
fn current() -> Self {
Options {
include_prefix: *INCLUDE_PREFIX.lock().unwrap(),
}
}
}
use private::Cfg;
pub const CFG: Cfg = Cfg {
options: AtomicPtr::new(ptr::null_mut()),
};
mod private {
use super::*;
pub struct Cfg {
pub(super) options: AtomicPtr<Options>,
}
}
impl std::ops::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 std::ops::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 thelibrary::CFG;
fn main() {
CFG.include_prefix = "path/to";
println!("{:?}", CFG.include_prefix);
}
I think the snippet above shows it has exactly the effect the person should expect. The write is observable to subsequent reads of the const. |
Wow, that's quite the API 😄 It looks like the lint is firing for the implicit I'm worried that a blanket suppression of this lint for all types with drop glue will cause many false negatives. I think it would make more sense to have a per-item opt out, which would indicate that any mutation actually has some side effect. @dtolnay Would something like |
It has been beneficial for this library because it very deliberately triggers "okay I am writing to a global (though safely)" neurons, which
Right now that would be slightly less good for me because it would imply having to do rustc version detection for whether const_mutation_allowed is supported in the current compiler, but it would work, and I sympathize with the concern about false negatives. As a new user-facing attribute I guess that would need to go through FCP (compiler team? lang team?). If so, and if it doesn't make the next beta cutoff which is happening next week, I would ask that we consider the approach in the PR in the interim until const_mutation_allowed is landed (and not feature gated). |
That sounds fine to me. I'll work on an MCP. |
To clarify, as implemented in the PR this is not bypassing for all types with drop glue. It's only for types that specifically have their own Drop impl. Something with compiler-generated drop glue but not a Drop impl would still warn. struct Log { s: String }
const LOG: Log = Log { s: String::new() };
fn main() {
LOG.s = "~~~".to_owned(); //~attempting to modify a `const` item
} |
Would a warning still get emitted in the const VEC: Vec<i32> = Vec::new();
fn main() {
VEC.push(42);
println!("{:?}", VEC);
} |
What I'm saying is it would be unfortunate to lose the warning in the So perhaps there could be a better heuristic to detect cases like yours above, while also keeping a warning for simple cases that happen to have a |
This comment has been minimized.
This comment has been minimized.
adt_destructor by default also validates the Drop impl using dropck::check_drop_impl, which contains an expect_local(). This leads to ICE in check_const_item_mutation if the const's type is not a local type. thread 'rustc' panicked at 'DefId::expect_local: `DefId(5:4805 ~ alloc[d7e9]::vec::{impl#50})` isn't local', compiler/rustc_span/src/def_id.rs:174:43 stack backtrace: 0: rust_begin_unwind 1: rustc_span::def_id::DefId::expect_local::{{closure}} 2: rustc_typeck::check::dropck::check_drop_impl 3: rustc_middle::ty::util::<impl rustc_middle::ty::context::TyCtxt>::calculate_dtor::{{closure}} 4: rustc_middle::ty::trait_def::<impl rustc_middle::ty::context::TyCtxt>::for_each_relevant_impl 5: rustc_middle::ty::util::<impl rustc_middle::ty::context::TyCtxt>::calculate_dtor 6: rustc_typeck::check::adt_destructor 7: rustc_middle::ty::query::<impl rustc_query_system::query::config::QueryAccessors<rustc_middle::ty::context::TyCtxt> for rustc_middle::ty::query::queries::adt_destructor>::compute 8: rustc_query_system::dep_graph::graph::DepGraph<K>::with_task_impl 9: rustc_query_system::query::plumbing::get_query_impl 10: rustc_mir::transform::check_const_item_mutation::ConstMutationChecker::is_const_item_without_destructor
|
@bugaevc: I updated the PR to still warn in the case of a |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open an issue to track removing this (conditional on having a stable attribute to suppress the lint for a particular const
item), and add a FIXME?
@dtolnay: I wasn't able to come up with a good design for an attribute to suppress this. There are similar lints that I'd like to create/uplift in the future (e.g. const items with interior mutability), and it would be annoying to introduce a single-use attribute for each of them. r=me with the comment addressed, so that this can be beta-backported. |
@bors r=Aaron1011 |
📌 Commit 804d159 has been approved by |
…as-schievink Rollup of 8 pull requests Successful merges: - rust-lang#75377 (Fix Debug implementations of some of the HashMap and BTreeMap iterator types) - rust-lang#76107 (Write manifest for MAJOR.MINOR channel to enable rustup convenience) - rust-lang#76745 (Move Wrapping<T> ui tests into library) - rust-lang#77182 (Add missing examples for Fd traits) - rust-lang#77251 (Bypass const_item_mutation if const's type has Drop impl) - rust-lang#77264 (Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. ) - rust-lang#77421 (Revert "resolve: Avoid "self-confirming" import resolutions in one more case") - rust-lang#77452 (Permit ty::Bool in const generics for v0 mangling) Failed merges: r? `@ghost`
Follow-up to #75573. This PR disables the const_item_mutation lint in cases that the const has a Drop impl which observes the mutation.
r? @Aaron1011