compile-time evaluation: detect writes through immutable pointers by RalfJung · Pull Request #118324 · rust-lang/rust
rustbot
added
S-waiting-on-review
labels
Nov 26, 2023bors added a commit to rust-lang-ci/rust that referenced this pull request
Nov 26, 2023…<try> compile-time evaluation: detect writes through immutable pointers This has two motivations: - it unblocks rust-lang#116745 (and therefore takes a big step towards `const_mut_refs` stabilization), because we can now detect if the memory that we find in `const` can be interned as "immutable" - it would detect the UB that was uncovered in rust-lang#117905, which was caused by accidental stabilization of `copy` functions in `const` that can only be called with UB When UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with [the const-UB RFC](https://rust-lang.github.io/rfcs/3016-const-ub.html), meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without `const_mut_refs` -- the accidentally stabilized `copy` functions are the only way this can happen, so the crates that popped up in rust-lang#117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that uncovered have all been fixed in their respective crates already. I just hope perf works out.
rustbot
added
A-testsuite
labels
Nov 26, 2023This was referenced
Dec 7, 2023
RalfJung
deleted the
ctfe-read-only-pointers
branch
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request
Dec 19, 2023…saethlin compile-time evaluation: detect writes through immutable pointers This has two motivations: - it unblocks rust-lang#116745 (and therefore takes a big step towards `const_mut_refs` stabilization), because we can now detect if the memory that we find in `const` can be interned as "immutable" - it would detect the UB that was uncovered in rust-lang#117905, which was caused by accidental stabilization of `copy` functions in `const` that can only be called with UB When UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with [the const-UB RFC](https://rust-lang.github.io/rfcs/3016-const-ub.html), meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without `const_mut_refs` -- the accidentally stabilized `copy` functions are the only way this can happen, so the crates that popped up in rust-lang#117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that we know about have all been fixed in their respective crates already. The way this is implemented is by making use of the fact that our interpreter is already generic over the notion of provenance. For CTFE we now use the new `CtfeProvenance` type which is conceptually an `AllocId` plus a boolean `immutable` flag (but packed for a more efficient representation). This means we can mark a pointer as immutable when it is created as a shared reference. The flag will be propagated to all pointers derived from this one. We can then check the immutable flag on each write to reject writes through immutable pointers. I just hope perf works out.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Jan 17, 2024…-obk const-eval interning: get rid of type-driven traversal This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global `tcx` memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for `static` and `const`, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.) We can do this now that rust-lang#118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not. The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for `const` and pomoted and non-interior-mutable `static`; all other allocations are interned as immutable for `static`, `const`, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB: - for promoteds, we rely on the analysis that does promotion to ensure that this is sound. - for `const` and `static`, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via `&<expr>` at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable. Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules. I also extended our type-driven const validity checks to ensure that `&mut T` in the final value of a const points to mutable memory, at least if `T` is not zero-sized. This catches cases of people turning `&i32` into `&mut i32` (which would still be considered a read-only pointer). Similarly, when these checks encounter an `UnsafeCell`, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer. This PR does have the immediate effect of allowing some new code on stable, for instance: ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a `Vec<i32>`), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway. This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.) `@rust-lang/wg-const-eval` I hope you are okay with this plan. :) `@rust-lang/lang` paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Jan 23, 2024…-obk const-eval interning: get rid of type-driven traversal This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global `tcx` memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for `static` and `const`, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.) We can do this now that rust-lang#118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not. The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for `const` and pomoted and non-interior-mutable `static`; all other allocations are interned as immutable for `static`, `const`, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB: - for promoteds, we rely on the analysis that does promotion to ensure that this is sound. - for `const` and `static`, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via `&<expr>` at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable. Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules. I also extended our type-driven const validity checks to ensure that `&mut T` in the final value of a const points to mutable memory, at least if `T` is not zero-sized. This catches cases of people turning `&i32` into `&mut i32` (which would still be considered a read-only pointer). Similarly, when these checks encounter an `UnsafeCell`, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer. This PR does have the immediate effect of allowing some new code on stable, for instance: ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a `Vec<i32>`), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway. This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.) `@rust-lang/wg-const-eval` I hope you are okay with this plan. :) `@rust-lang/lang` paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
Jan 24, 2024const-eval interning: get rid of type-driven traversal This entirely replaces our const-eval interner, i.e. the code that takes the final result of a constant evaluation from the local memory of the const-eval machine to the global `tcx` memory. The main goal of this change is to ensure that we can detect mutable references that sneak into this final value -- this is something we want to reject for `static` and `const`, and while const-checking performs some static analysis to ensure this, I would be much more comfortable stabilizing const_mut_refs if we had a dynamic check that sanitizes the final value. (This is generally the approach we have been using on const-eval: do a static check to give nice errors upfront, and then do a dynamic check to be really sure that the properties we need for soundness, actually hold.) We can do this now that rust-lang/rust#118324 landed and each pointer comes with a bit (completely independent of its type) storing whether mutation is permitted through this pointer or not. The new interner is a lot simpler than the old one: previously we did a complete type-driven traversal to determine the mutability of all memory we see, and then a second pass to intern any leftover raw pointers. The new interner simply recursively traverses the allocation holding the final result, and all allocations reachable from it (which can be determined from the raw bytes of the result, without knowing anything about types), and ensures they all get interned. The initial allocation is interned as immutable for `const` and pomoted and non-interior-mutable `static`; all other allocations are interned as immutable for `static`, `const`, and promoted. The main subtlety is justifying that those inner allocations may indeed be interned immutably, i.e., that mutating them later would anyway already be UB: - for promoteds, we rely on the analysis that does promotion to ensure that this is sound. - for `const` and `static`, we check that all pointers in the final result that point to things that are new (i.e., part of this const evaluation) are immutable, i.e., were created via `&<expr>` at a non-interior-mutable type. Mutation through immutable pointers is UB so we are free to intern that memory as immutable. Interning raises an error if it encounters a dangling pointer or a mutable pointer that violates the above rules. I also extended our type-driven const validity checks to ensure that `&mut T` in the final value of a const points to mutable memory, at least if `T` is not zero-sized. This catches cases of people turning `&i32` into `&mut i32` (which would still be considered a read-only pointer). Similarly, when these checks encounter an `UnsafeCell`, they are checking that it lives in mutable memory. (Both of these only traverse the newly created values; if those point to other consts/promoteds, the check stops there. But that's okay, we don't have to catch all the UB.) I co-developed this with the stricter interner changes but I can split it out into a separate PR if you prefer. This PR does have the immediate effect of allowing some new code on stable, for instance: ```rust const CONST_RAW: *const Vec<i32> = &Vec::new() as *const _; ``` Previously that code got rejected since the type-based interner didn't know what to do with that pointer. It's a raw pointer, we cannot trust its type. The new interner does not care about types so it sees no issue with this code; there's an immutable pointer pointing to some read-only memory (storing a `Vec<i32>`), all is good. Accepting this code pretty much commits us to non-type-based interning, but I think that's the better strategy anyway. This PR also leads to slightly worse error messages when the final value of a const contains a dangling reference. Previously we would complete interning and then the type-based validation would detect this dangling reference and show a nice error saying where in the value (i.e., in which field) the dangling reference is located. However, the new interner cannot distinguish dangling references from dangling raw pointers, so it must throw an error when it encounters either of them. It doesn't have an understanding of the value structure so all it can say is "somewhere in this constant there's a dangling pointer". (Later parts of the compiler don't like dangling pointers/references so we have to reject them either during interning or during validation.) This could potentially be improved by doing validation before interning, but that's a larger change that I have not attempted yet. (It's also subtle since we do want validation to use the final mutability bits of all involved allocations, and currently it is interning that marks a bunch of allocations as immutable -- that would have to still happen before validation.) `@rust-lang/wg-const-eval` I hope you are okay with this plan. :) `@rust-lang/lang` paging you in since this accepts new code on stable as explained above. Please let me know if you think FCP is necessary.
ojeda
mentioned this pull request
48 tasks
GuillaumeGomez pushed a commit to GuillaumeGomez/rust that referenced this pull request
Feb 21, 2024…saethlin compile-time evaluation: detect writes through immutable pointers This has two motivations: - it unblocks rust-lang#116745 (and therefore takes a big step towards `const_mut_refs` stabilization), because we can now detect if the memory that we find in `const` can be interned as "immutable" - it would detect the UB that was uncovered in rust-lang#117905, which was caused by accidental stabilization of `copy` functions in `const` that can only be called with UB When UB is detected, we emit a future-compat warn-by-default lint. This is not a breaking change, so completely in line with [the const-UB RFC](https://rust-lang.github.io/rfcs/3016-const-ub.html), meaning we don't need t-lang FCP here. I made the lint immediately show up for dependencies since it is nearly impossible to even trigger this lint without `const_mut_refs` -- the accidentally stabilized `copy` functions are the only way this can happen, so the crates that popped up in rust-lang#117905 are the only causes of such UB (in the code that crater covers), and the three cases of UB that we know about have all been fixed in their respective crates already. The way this is implemented is by making use of the fact that our interpreter is already generic over the notion of provenance. For CTFE we now use the new `CtfeProvenance` type which is conceptually an `AllocId` plus a boolean `immutable` flag (but packed for a more efficient representation). This means we can mark a pointer as immutable when it is created as a shared reference. The flag will be propagated to all pointers derived from this one. We can then check the immutable flag on each write to reject writes through immutable pointers. I just hope perf works out.
bors added a commit to rust-lang-ci/rust that referenced this pull request
Aug 17, 2024…inter, r=<try> make writes_through_immutable_pointer a hard error This turns the lint added in rust-lang#118324 into a hard error. This has been reported in cargo's future-compat reports since Rust 1.76 (released in February). Given that const_mut_refs is still unstable, it should be impossible to even hit this error on stable: we did accidentally stabilize some functions that can cause this error, but that got reverted in rust-lang#117905. Still, let's do a crater run just to be sure. Given that this should only affect unstable code, I don't think it needs an FCP, but let's Cc `@rust-lang/lang` anyway -- any objection to making this unambiguous UB into a hard error during const-eval? This can be viewed as part of rust-lang#129195 which is already nominated for discussion.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Aug 24, 2024…pointer, r=compiler-errors make writes_through_immutable_pointer a hard error This turns the lint added in rust-lang#118324 into a hard error. This has been reported in cargo's future-compat reports since Rust 1.76 (released in February). Given that const_mut_refs is still unstable, it should be impossible to even hit this error on stable: we did accidentally stabilize some functions that can cause this error, but that got reverted in rust-lang#117905. Still, let's do a crater run just to be sure. Given that this should only affect unstable code, I don't think it needs an FCP, but let's Cc `@rust-lang/lang` anyway -- any objection to making this unambiguous UB into a hard error during const-eval? This can be viewed as part of rust-lang#129195 which is already nominated for discussion.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
Aug 24, 2024…pointer, r=compiler-errors make writes_through_immutable_pointer a hard error This turns the lint added in rust-lang#118324 into a hard error. This has been reported in cargo's future-compat reports since Rust 1.76 (released in February). Given that const_mut_refs is still unstable, it should be impossible to even hit this error on stable: we did accidentally stabilize some functions that can cause this error, but that got reverted in rust-lang#117905. Still, let's do a crater run just to be sure. Given that this should only affect unstable code, I don't think it needs an FCP, but let's Cc ``@rust-lang/lang`` anyway -- any objection to making this unambiguous UB into a hard error during const-eval? This can be viewed as part of rust-lang#129195 which is already nominated for discussion.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Aug 25, 2024Rollup merge of rust-lang#129199 - RalfJung:writes_through_immutable_pointer, r=compiler-errors make writes_through_immutable_pointer a hard error This turns the lint added in rust-lang#118324 into a hard error. This has been reported in cargo's future-compat reports since Rust 1.76 (released in February). Given that const_mut_refs is still unstable, it should be impossible to even hit this error on stable: we did accidentally stabilize some functions that can cause this error, but that got reverted in rust-lang#117905. Still, let's do a crater run just to be sure. Given that this should only affect unstable code, I don't think it needs an FCP, but let's Cc ``@rust-lang/lang`` anyway -- any objection to making this unambiguous UB into a hard error during const-eval? This can be viewed as part of rust-lang#129195 which is already nominated for discussion.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request
Aug 26, 2024…r=compiler-errors make writes_through_immutable_pointer a hard error This turns the lint added in rust-lang/rust#118324 into a hard error. This has been reported in cargo's future-compat reports since Rust 1.76 (released in February). Given that const_mut_refs is still unstable, it should be impossible to even hit this error on stable: we did accidentally stabilize some functions that can cause this error, but that got reverted in rust-lang/rust#117905. Still, let's do a crater run just to be sure. Given that this should only affect unstable code, I don't think it needs an FCP, but let's Cc ``@rust-lang/lang`` anyway -- any objection to making this unambiguous UB into a hard error during const-eval? This can be viewed as part of rust-lang/rust#129195 which is already nominated for discussion.
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, 2024const-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`
Kobzol pushed a commit to Kobzol/portable-simd that referenced this pull request
Feb 3, 2026const-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`
ojeda
mentioned this pull request
57 tasks
This file contains hidden or 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