Bypass const_item_mutation if const's type has Drop impl

[dtolnay]

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.

struct Log { msg: &'static str }
const LOG: Log = Log { msg: "" };
impl Drop for Log {
    fn drop(&mut self) { println!("{}", self.msg); }
}

LOG.msg = "wow";  // prints "wow"

r? @Aaron1011

[Aaron1011]

I'm not convinced that this is a good idea. The point of const_item_mutation is to warn on counterintuitive code which appears to have a side effect but actually doesn't.

If anything, LOG.msg = "wow" seems even more counterintuitive than the 'typical' code that const_item_mutation warns against - it has a side effect that you wouldn't expect, and doesn't have the side effect that you would expect.

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.

[dtolnay]

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. CFG.header_include_prefix = "path/to". It isn't so much about println-like side effects, rather the assignment actually being very much the intended effect. We just don't expose the whole *MUTEX.lock().unwrap() rigmarole that would usually follow with a static config.

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);
}

If anything, LOG.msg = "wow"CFG.include_prefix = "path/to" seems even more counterintuitive than the 'typical' code that const_item_mutation warns against - it has a side effect that you wouldn't expect, and doesn't have the side effect that you would expect.

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.

[Aaron1011]

Wow, that's quite the API 😄

It looks like the lint is firing for the implicit DerefMut call, rather than for a direct assignment. In general, I think this is correct - writing to a field via a const item's DerefMut impl will almost never have the desired effect.

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 #[const_mutation_allowed] pub const CFG fit your use case?

[dtolnay]

Wow, that's quite the API 😄

It has been beneficial for this library because it very deliberately triggers "okay I am writing to a global (though safely)" neurons, which ::set_whatever(...) does not, in my experience, at least not nearly as strongly or reliably.

Would something like #[const_mutation_allowed] pub const CFG fit your use case?

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).

[Aaron1011]

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.

[dtolnay]

I'm worried that a blanket suppression of this lint for all types with drop glue will cause many false negatives.

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
}

[bugaevc]

Would a warning still get emitted in the Vec case?

const VEC: Vec<i32> = Vec::new();

fn main() {
    VEC.push(42);
    println!("{:?}", VEC);
}

[dtolnay]

@bugaevc: Vec has a drop impl so it would not warn.

rust/library/alloc/src/vec.rs

Line 2631 in db7ee7c

unsafe impl<#[may_dangle] T> Drop for Vec<T> {

[bugaevc]

What I'm saying is it would be unfortunate to lose the warning in the Vec case; this is specifically the case where I've seen people get confused.

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 Drop impl?

[dtolnay]

• Rebased to resolve conflict with Don't fire const_item_mutation lint on writes through a pointer #77324.

[dtolnay]

@bugaevc: I updated the PR to still warn in the case of a &mut self parameter of a method attempting to mutate a const even if there is a destructor, such as in VEC.push, and added a test.

[Aaron1011]

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?

[Aaron1011]

@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.

[dtolnay]

@bors r=Aaron1011

[bors]

📌 Commit 804d159 has been approved by Aaron1011