Recover statics better #125555
Open
Recover statics better #125555
+118
−26
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
The const interner uses the body to intern static allocations. This means that an allocation will be mutable if the resulting type contains interior mutability. Const eval contains an assertion that types that are `Freeze` do not have mutable allocations, which was failing for cases where the type was a type error. An alternative fix for this would be to avoid the assertion in const eval for type errors, allowing those to both be mutable and immutable. This is implemented in a follow-up commit as well.
rustbot
added
S-waiting-on-review
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
Noratrieb
force-pushed
the
recover-statics-better
branch
from
333296f to
4d97ffb
Compare
This comment has been minimized.
This comment has been minimized.
|
Does the first commit actually have user-visible changes anywhere? |
When the type is an inferred placeholder or an error, we used to infer the type for an error message and then return `TyKind::Error`. Now we return the proper type, which helps follow-up code do more checking. The primary motivation for this was fixing the const-eval mutability assertion bug, because this commit forwards the inferred type to that assertion, but it is also just nicer in general.
We do not know whether a type error was supposed to be mutable or not.
|
The only change in the test suite is that it adds that one cycle error. |
Noratrieb
force-pushed
the
recover-statics-better
branch
from
4d97ffb to
44a576c
Compare
RalfJung
reviewed
May 26, 2024
| if let Some((_, alloc)) = ecx.memory.alloc_map.get(alloc_id) { | ||
| if let Some((_, alloc)) = ecx.memory.alloc_map.get(alloc_id) | ||
| // For type errors, we do not know whether they are supposed to be mutable or not. | ||
| && !ty.references_error() |
There was a problem hiding this comment.
It is a bad sign that this check is needed. We should never even evaluate a static whose type references an error. So something still seems wrong here.
oli-obk
reviewed
May 28, 2024
| // Assoc consts can reference generic lifetimes from the parent generics, but treating them | ||
| // as static is unlikely to cause issues. | ||
| let ty = tcx.fold_regions(ty, |region, _| match region.kind() { | ||
| ty::ReErased => tcx.lifetimes.re_static, |
There was a problem hiding this comment.
Suggested change
| ty::ReErased => tcx.lifetimes.re_static, | |
| ty::ReErased => ty::Region::new_error(tcx, guar), |
would also fix this issue without requiring any of the other changes, because then the error type is tainted, without replacing any of the types. So you'd still get the suggestions, but const eval won't attempt to run.
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #127358) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Noratrieb ping from triage - can you post your status on this PR? This PR has not received an update in a few months. |
alex-semenyuk
added
S-waiting-on-author
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.
The const interner uses the body to intern static allocations.
This means that an allocation will be mutable if the resulting type contains interior mutability.
Const eval contains an assertion that types that are
Freezedo not have mutable allocations, which was failing for cases where the type was a type error.The first two commits fix this by forwarding the inferred type through
type_of, while the last commit disables the assertion for remaining cases where type errors are involved.Note that the erased region mapping is not fully correct, and causes this spurious error now:
I deem that acceptable, that you may disagree.
note: the commit messages contain more information
fixes #124164
r? oli-obk