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

Tracking issue for future-incompatibility lint const_eval_mutable_ptr_in_final_value #122153

Closed
pnkfelix opened this issue Mar 7, 2024 · 0 comments · Fixed by #128543
Closed
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Mar 7, 2024

This is a tracking issue for the CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE lint, which is being added in #122204. The lint detects cases where a const expression evaluates to a value that holds a pointer to mutable state, which is undefined behavior and may be rejected by a future version of the compiler.

Example

This lint will trigger for code like this:

use std::cell::Cell;

pub enum JsValue {
    Undefined,
    Object(Cell<bool>),
}

impl ::std::ops::Drop for JsValue {
    fn drop(&mut self) {}
}

const UNDEFINED: &JsValue = &JsValue::Undefined;

fn main() {
}

Cc #121610

@pnkfelix pnkfelix added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC C-future-incompatibility Category: Future-incompatibility lints T-lang Relevant to the language team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-const-eval Area: Constant evaluation (MIR interpretation) labels Mar 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 14, 2024
…ng-ptr-in-final-to-future-incompat-lint, r=wesleywiser

Downgrade const eval dangling ptr in final to future incompat lint

Short term band-aid for issue rust-lang#121610, downgrading the prior hard error to a future-incompat lint (tracked in issue rust-lang#122153).

Note we should not mark rust-lang#121610 as resolved until after this (or something analogous) is beta backported.
@fmease fmease changed the title Tracking Issue for CONST_EVAL_MUTABLE_PTR_IN_FINAL_VALUE Tracking issue for future-incompatibility lint const_eval_mutable_ptr_in_final_value Sep 14, 2024
@fmease fmease added the A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. label Sep 14, 2024
@bors bors closed this as completed in 9b72238 Sep 14, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 16, 2024
const-eval interning: accept interior mutable pointers in final value

…but keep rejecting mutable references

This fixes rust-lang/rust#121610 by no longer firing the lint when there is a pointer with interior mutability in the final value of the constant. On stable, such pointers can be created with code like:
```rust
pub enum JsValue {
    Undefined,
    Object(Cell<bool>),
}
impl Drop for JsValue {
    fn drop(&mut self) {}
}
// This does *not* get promoted since `JsValue` has a destructor.
// However, the outer scope rule applies, still giving this 'static lifetime.
const UNDEFINED: &JsValue = &JsValue::Undefined;
```
It's not great to accept such values since people *might* think that it is legal to mutate them with unsafe code. (This is related to how "infectious" `UnsafeCell` is, which is a [wide open question](rust-lang/unsafe-code-guidelines#236).) However, we [explicitly document](https://doc.rust-lang.org/reference/behavior-considered-undefined.html) that things created by `const` are immutable. Furthermore, we also accept the following even more questionable code without any lint today:
```rust
let x: &'static Option<Cell<i32>> = &None;
```
This is even more questionable since it does *not* involve a `const`, and yet still puts the data into immutable memory. We could view this as promotion [potentially introducing UB](rust-lang/unsafe-code-guidelines#493). However, we've accepted this since ~forever and it's [too late to reject this now](rust-lang/rust#122789); the pattern is just too useful.

So basically, if you think that `UnsafeCell` should be tracked fully precisely, then you should want the lint we currently emit to be removed, which this PR does. If you think `UnsafeCell` should "infect" surrounding `enum`s, the big problem is really rust-lang/unsafe-code-guidelines#493 which does not trigger the lint -- the cases the lint triggers on are actually the "harmless" ones as there is an explicit surrounding `const` explaining why things end up being immutable.

What all this goes to show is that the hard error added in rust-lang/rust#118324 (later turned into the future-compat lint that I am now suggesting we remove) was based on some wrong assumptions, at least insofar as it concerns shared references. Furthermore, that lint does not help at all for the most problematic case here where the potential UB is completely implicit. (In fact, the lint is actively in the way of [my preferred long-term strategy](rust-lang/unsafe-code-guidelines#493 (comment)) for dealing with this UB.) So I think we should go back to square one and remove that error/lint for shared references. For mutable references, it does seem to work as intended, so we can keep it. Here it serves as a safety net in case the static checks that try to contain mutable references to the inside of a const initializer are not working as intended; I therefore made the check ICE to encourage users to tell us if that safety net is triggered.

Closes rust-lang/rust#122153 by removing the lint.

Cc `@rust-lang/opsem` `@rust-lang/lang`
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) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants