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

const checker confuses "pointer to static mut" with "local mutable variable that escapes this scope" #118447

Closed
RalfJung opened this issue Nov 29, 2023 · 1 comment · Fixed by #120932
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-feature-request Category: A feature request, i.e: not implemented / a PR.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 29, 2023

This code gets rejected, and (as far as I can tell) not for good reasons:

#![feature(const_mut_refs)]

#[repr(C)]
struct Foo {
    v: u32,
}

#[repr(C)]
struct Bar {
    foo: *mut Foo,
}

unsafe impl Sync for Bar {}

extern "C" {
    static mut foo: Foo;
}

fn do_it() {
    static BAR: Bar = Bar {
        foo: unsafe { core::ptr::addr_of_mut!(foo)},
    };
}

The error is

error[E0764]: raw mutable references are not allowed in the final value of statics
  --> src/lib.rs:21:23
   |
21 |         foo: unsafe { core::ptr::addr_of_mut!(foo)},
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

This error is meant to catch cases like this

    static BAR: Bar = Bar {
        foo: &mut Foo { v: 0 },
    };

where a temporary (Foo { v: 0 }) escapes into the final value of the constant. However, for the original code there's no temporary, this is a pointer to a static.

Here's an executable pure Rust version (no extern).

It seems to me we could allow mutable pointers to already existing (mutable) allocations to leak out into the final const value. However we should carefully consider the consequences of this. I can't think of anything that would go wrong here right now, but that might be just a sign of sleep deprivation. ;)

Cc @rust-lang/wg-const-eval

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Nov 29, 2023
@RalfJung RalfJung changed the title const checker confuses "pointer to static mut" with "local mutable variables that escapes this scope" const checker confuses "pointer to static mut" with "local mutable variable that escapes this scope" Nov 29, 2023
@RalfJung RalfJung added C-feature-request Category: A feature request, i.e: not implemented / a PR. A-const-eval Area: constant evaluation (mir interpretation) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 21, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Feb 16, 2024

Turns out there is a work-around where code equivalent to the one above already compiles on nightly without #120932:

#![feature(const_mut_refs)]

#[repr(C)]
struct Foo {
    v: u32,
}

#[repr(C)]
struct Bar {
    foo: *mut Foo,
}

unsafe impl Sync for Bar {}

extern "C" {
    static mut foo: Foo;
}

fn do_it() {
    static BAR: Bar = {
        let bar = Bar {
            foo: unsafe { core::ptr::addr_of_mut!(foo)},
        };
        bar
    };
}

You just have to move the use of the foo static out of the trailing expression, into a let-binding.

Nadrieril added a commit to Nadrieril/rust that referenced this issue Feb 17, 2024
const_mut_refs: allow mutable pointers to statics

Fixes rust-lang#118447

Writing this PR became a bit messy, see [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Statics.20pointing.20to.20interior.20mutable.20statics) for some of my journey.^^ Turns out there was a long-standing bug in our qualif logic that led to us incorrectly classifying certain places as "no interior mut" when they actually had interior mut. Due to that the `const_refs_to_cell` feature gate was required far less often than it otherwise would, e.g. in [this code](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=9e0c042c451b3d11d64dd6263679a164). Fixing this however would be a massive breaking change all over libcore and likely the wider ecosystem. So I also changed the const-checking logic to just not require the feature gate for the affected cases. While doing so I discovered a bunch of logic that is not explained and that I could not explain. However I think stabilizing some const-eval feature will make most of that logic inconsequential so I just added some FIXMEs and left it at that.

r? `@oli-obk`
@bors bors closed this as completed in f70f13a Feb 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 17, 2024
Rollup merge of rust-lang#120932 - RalfJung:mut-ptr-to-static, r=oli-obk

const_mut_refs: allow mutable pointers to statics

Fixes rust-lang#118447

Writing this PR became a bit messy, see [Zulip](https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/Statics.20pointing.20to.20interior.20mutable.20statics) for some of my journey.^^ Turns out there was a long-standing bug in our qualif logic that led to us incorrectly classifying certain places as "no interior mut" when they actually had interior mut. Due to that the `const_refs_to_cell` feature gate was required far less often than it otherwise would, e.g. in [this code](https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=9e0c042c451b3d11d64dd6263679a164). Fixing this however would be a massive breaking change all over libcore and likely the wider ecosystem. So I also changed the const-checking logic to just not require the feature gate for the affected cases. While doing so I discovered a bunch of logic that is not explained and that I could not explain. However I think stabilizing some const-eval feature will make most of that logic inconsequential so I just added some FIXMEs and left it at that.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) C-feature-request Category: A feature request, i.e: not implemented / a PR.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants