experiment: remove value-based reasoning for interior mutability #122789
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
rustbot
added
S-waiting-on-review
|
Some changes occurred in exhaustiveness checking cc @Nadrieril Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
RalfJung
force-pushed
the
interior-mut-experiment
branch
from
9051c32
to
3660dd9
Compare
RalfJung
removed
the
S-waiting-on-review
|
Sorry for the pings, I forgot to mark this a draft. (Why does github make that so annoying? Just give me a checkbox, instead of remember this decision for the next PR...) |
|
@bors try |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Mar 20, 2024
…<try> experiment: remove value-based reasoning for interior mutability This also stabilizes `const_refs_to_cell` as it's just a crater experiment anyway and that reduces the amount of regressions.
Nadrieril
reviewed
Mar 20, 2024
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| #![allow(rustc::untranslatable_diagnostic)] | |||
| #![allow(rustc::diagnostic_outside_of_impl)] | |||
| #![feature(freeze)] | |||
There was a problem hiding this comment.
If this actually gets merged, please set this only when feature = "rustc". This crate needs to build on stable for rust-analyzer.
There was a problem hiding this comment.
is it needed just for this?
There was a problem hiding this comment.
This is an experiment, it won't get merged. Also the crate can't build on stable any more with this rustc change.
There was a problem hiding this comment.
is it needed just for this?
Yes. That previously got promoted even though other enum variants have interior mutability, which is not entirely a sound thing to do (Cc rust-lang/unsafe-code-guidelines#493).
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
the
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Well I think that answers the question. ;) |
Noratrieb
added a commit
to Noratrieb/rust
that referenced
this pull request
Jun 4, 2024
…ference-self, r=BoxyUwU
Item bounds can reference self projections and still be object safe
### Background
Currently, we have some interesting rules about where `Self` is allowed to be mentioned in objects. Specifically, we allow mentioning `Self` behind associated types (e.g. `fn foo(&self) -> Self::Assoc`) only if that `Self` type comes from the trait we're defining or its supertraits:
```
trait Foo {
fn good() -> Self::Assoc; // GOOD :)
fn bad() -> <Self as OtherTrait>::Assoc; // BAD!
}
```
And more specifically, these `Self::Assoc` projections are *only* allowed to show up in:
* (A1) Method signatures
* (A2) Where clauses on traits, GATs and methods
But `Self::Assoc` projections are **not** allowed to show up in:
* (B1) Supertrait bounds (specifically: all *super-predicates*, which includes the projections that come from elaboration, and not just the traits themselves).
* (B2) Item bounds of associated types
The reason for (B1) is interesting: specifically, it arises from the fact that we currently eagerly elaborate all projection predicates into the object, so if we had the following code:
```
trait Sub<Assoc = Self::SuperAssoc> {}
trait Super {
type SuperAssoc;
}
```
Then given `dyn Sub<SuperAssoc = i32>` we would need to have a type that is substituted into itself an infinite number of times[^1], like `dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <... as Super>::SuperAssoc> as Super>::SuperAssoc> as Super>::SuperAssoc>`, i.e. the fixed-point of: `type T = dyn Sub<SuperAssoc = i32, Assoc = <T as Super>::SuperAssoc>`.
Similarly for (B2), we restrict mentioning `Self::Assoc` in associated type item bounds, which is the cause for rust-lang#122798. However, there is **no reason** for us to do so, since item bounds never show up structurally in the `dyn Trait` object type.
#### What?
This PR relaxes the check for item bounds so that `Self` may be mentioned behind associated types in the same cases that they currently work for method signatures (A1) and where clauses (A2).
#### Why?
Fixes rust-lang#122798. Removes a subtle and confusing inconsistency for the code mentioned in that issue.
This is sound because we only assemble alias bounds for rigid projections, and all projections coming from an object self type are not rigid, since all associated types should be specified by the type.
This is also desirable because we can do this via supertraits already. In rust-lang#122789, it is noted that an item bound of `Eq` already works, just not `PartialEq` because of the default item bound. This is weird and should be fixed.
#### Future work
We could make the check for `Self` in super-predicates more sophisticated as well, only erroring if `Self` shows up in a projection super-predicate.
[^1]: This could be fixed by some sort of structural replacement or eager normalization, but I don't think it's necessary currently.
Noratrieb
added a commit
to Noratrieb/rust
that referenced
this pull request
Jun 4, 2024
…ference-self, r=BoxyUwU
Item bounds can reference self projections and still be object safe
### Background
Currently, we have some interesting rules about where `Self` is allowed to be mentioned in objects. Specifically, we allow mentioning `Self` behind associated types (e.g. `fn foo(&self) -> Self::Assoc`) only if that `Self` type comes from the trait we're defining or its supertraits:
```
trait Foo {
fn good() -> Self::Assoc; // GOOD :)
fn bad() -> <Self as OtherTrait>::Assoc; // BAD!
}
```
And more specifically, these `Self::Assoc` projections are *only* allowed to show up in:
* (A1) Method signatures
* (A2) Where clauses on traits, GATs and methods
But `Self::Assoc` projections are **not** allowed to show up in:
* (B1) Supertrait bounds (specifically: all *super-predicates*, which includes the projections that come from elaboration, and not just the traits themselves).
* (B2) Item bounds of associated types
The reason for (B1) is interesting: specifically, it arises from the fact that we currently eagerly elaborate all projection predicates into the object, so if we had the following code:
```
trait Sub<Assoc = Self::SuperAssoc> {}
trait Super {
type SuperAssoc;
}
```
Then given `dyn Sub<SuperAssoc = i32>` we would need to have a type that is substituted into itself an infinite number of times[^1], like `dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <dyn Sub<SuperAssoc = i32, Assoc = <... as Super>::SuperAssoc> as Super>::SuperAssoc> as Super>::SuperAssoc>`, i.e. the fixed-point of: `type T = dyn Sub<SuperAssoc = i32, Assoc = <T as Super>::SuperAssoc>`.
Similarly for (B2), we restrict mentioning `Self::Assoc` in associated type item bounds, which is the cause for rust-lang#122798. However, there is **no reason** for us to do so, since item bounds never show up structurally in the `dyn Trait` object type.
#### What?
This PR relaxes the check for item bounds so that `Self` may be mentioned behind associated types in the same cases that they currently work for method signatures (A1) and where clauses (A2).
#### Why?
Fixes rust-lang#122798. Removes a subtle and confusing inconsistency for the code mentioned in that issue.
This is sound because we only assemble alias bounds for rigid projections, and all projections coming from an object self type are not rigid, since all associated types should be specified by the type.
This is also desirable because we can do this via supertraits already. In rust-lang#122789, it is noted that an item bound of `Eq` already works, just not `PartialEq` because of the default item bound. This is weird and should be fixed.
#### Future work
We could make the check for `Self` in super-predicates more sophisticated as well, only erroring if `Self` shows up in a projection super-predicate.
[^1]: This could be fixed by some sort of structural replacement or eager normalization, but I don't think it's necessary currently.
|
A lot of the regressions come from the |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 14, 2024
…dead const-eval interning: accept interior mutable pointers in final value …but keep rejecting mutable references This fixes rust-lang#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#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#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#122153 by removing the lint. Cc `@rust-lang/opsem` `@rust-lang/lang`
github-actions bot
pushed a commit
to rust-lang/miri
that referenced
this pull request
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This also stabilizes
const_refs_to_cellas it's just a crater experiment anyway and that reduces the amount of regressions.