-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Postpone the evaluation of constant expressions that depend on inference variables #90023
Conversation
Some changes occured to the CTFE / Miri engine cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
effd201
to
afa34ce
Compare
@@ -1588,6 +1589,15 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { | |||
span: Option<Span>, | |||
) -> EvalToConstValueResult<'tcx> { | |||
let mut original_values = OriginalQueryValues::default(); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be erasing lifetimes in param_env
and substs
at the start of this fn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
y, with this PR we can erase lifetimes and don't have to canonicalize anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll change that. Can you explain why we should do this? Is it just that we don't need any lifetime information anymore during codegen (which the evaluation of constants loosely speaking is)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it just that we don't need any lifetime information anymore during codegen
yes, const eval and codegen (which should pretty much behave the same) do not depend on lifetime information so it is fine to erase them. That also improves cashing as all lifetime params are always 'erased
.
src/test/ui/const-generics/generic_const_exprs/const_eval_resolve_canonical.rs
Outdated
Show resolved
Hide resolved
afa34ce
to
42695c2
Compare
forgot to write this when I first reviewed this: the change looks good to me but I think we should not land this if we ever intend to revert this PR, as in that case it simply masks bugs we will have to fix then. My only concern here is that anonymous constants can refer to unused generic arguments, so by not trying to evaluate them we will probably prevent some really rare cases from compiling. Need to take some time to figure out the greater picture here, so it might take a bit until this PR gets merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with the change below
let param_env = self.tcx.erase_regions(param_env); | ||
let mut substs = unevaluated.substs(self.tcx); | ||
substs = self.tcx.erase_regions(substs); | ||
substs = self.resolve_vars_if_possible(substs); | ||
|
||
// Postpone the evaluation of constants whose substs depend on inference | ||
// variables | ||
if substs.has_infer_types_or_consts() { | ||
return Err(ErrorHandled::TooGeneric); | ||
} | ||
|
||
let unevaluated = ty::Unevaluated { | ||
def: unevaluated.def, | ||
substs_: Some(substs), | ||
promoted: unevaluated.promoted, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we re-order this as follows:
let param_env = self.tcx.erase_regions(param_env); | |
let mut substs = unevaluated.substs(self.tcx); | |
substs = self.tcx.erase_regions(substs); | |
substs = self.resolve_vars_if_possible(substs); | |
// Postpone the evaluation of constants whose substs depend on inference | |
// variables | |
if substs.has_infer_types_or_consts() { | |
return Err(ErrorHandled::TooGeneric); | |
} | |
let unevaluated = ty::Unevaluated { | |
def: unevaluated.def, | |
substs_: Some(substs), | |
promoted: unevaluated.promoted, | |
}; | |
let mut substs = unevaluated.substs(self.tcx); | |
substs = self.resolve_vars_if_possible(substs); | |
// Postpone the evaluation of constants whose substs depend on inference | |
// variables | |
if substs.has_infer_types_or_consts() { | |
return Err(ErrorHandled::TooGeneric); | |
} | |
let param_env_erased = self.tcx.erase_regions(param_env); | |
let substs_erased = self.tcx.erase_regions(substs); | |
let unevaluated = ty::Unevaluated { | |
def: unevaluated.def, | |
substs_: Some(substs_erased), | |
promoted: unevaluated.promoted, | |
}; | |
// The return value is the evaluated value which doesn't contain any reference to inference | |
// variables, thus we don't need to substitute back the original values. | |
self.tcx.const_eval_resolve(param_env, unevaluated, span) |
It is necessary to resolve variables before erasing because an inference variable may resolve to an &T
of some kind.
This comment has been minimized.
This comment has been minimized.
effbfa8
to
3a7af1b
Compare
This comment has been minimized.
This comment has been minimized.
3a7af1b
to
37ed2db
Compare
@bors r=nikomatsakis |
📌 Commit 37ed2db has been approved by |
…askrgr Rollup of 6 pull requests Successful merges: - rust-lang#89642 (environ on macos uses directly libc which has the correct signature.) - rust-lang#90022 (Explain why `Self` is invalid in generic parameters) - rust-lang#90023 (Postpone the evaluation of constant expressions that depend on inference variables) - rust-lang#91215 (Implement VecDeque::retain_mut) - rust-lang#91355 (std: Stabilize the `thread_local_const_init` feature) - rust-lang#91528 (LLVM support .insn directive) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The following also look to be fixed by this PR 🎉
I haven't closed or marked any as needs-test, since I'm not sure which would be considered duplicates |
@b-naber any chance I can enlist you to go through the above issues and decide which are "duplicates" and which need a test? |
Of course, I'll do that tomorrow. |
…al, r=jackh726 Add tests fixed by rust-lang#90023 The following issues were fixed by rust-lang#90023 Fixes rust-lang#79674 Fixes rust-lang#83765 Fixes rust-lang#86033 Fixes rust-lang#90318 Fixes rust-lang#88468 The following issues were duplicates of rust-lang#90654 Fixes rust-lang#86850 Fixes rust-lang#89022 r? `@jackh726`
…al, r=jackh726 Add tests fixed by rust-lang#90023 The following issues were fixed by rust-lang#90023 Fixes rust-lang#79674 Fixes rust-lang#83765 Fixes rust-lang#86033 Fixes rust-lang#90318 Fixes rust-lang#88468 The following issues were duplicates of rust-lang#90654 Fixes rust-lang#86850 Fixes rust-lang#89022 r? ``@jackh726``
…al, r=jackh726 Add tests fixed by rust-lang#90023 The following issues were fixed by rust-lang#90023 Fixes rust-lang#79674 Fixes rust-lang#83765 Fixes rust-lang#86033 Fixes rust-lang#90318 Fixes rust-lang#88468 The following issues were duplicates of rust-lang#90654 Fixes rust-lang#86850 Fixes rust-lang#89022 r? ```@jackh726```
…al, r=jackh726 Add tests fixed by rust-lang#90023 The following issues were fixed by rust-lang#90023 Fixes rust-lang#79674 Fixes rust-lang#83765 Fixes rust-lang#86033 Fixes rust-lang#90318 Fixes rust-lang#88468 The following issues were duplicates of rust-lang#90654 Fixes rust-lang#86850 Fixes rust-lang#89022 r? ````@jackh726````
…askrgr Rollup of 11 pull requests Successful merges: - rust-lang#91668 (Remove the match on `ErrorKind::Other`) - rust-lang#91678 (Add tests fixed by rust-lang#90023) - rust-lang#91679 (Move core/stream/stream/mod.rs to core/stream/stream.rs) - rust-lang#91681 (fix typo in `intrinsics::raw_eq` docs) - rust-lang#91686 (Fix `Vec::reserve_exact` documentation) - rust-lang#91697 (Delete Utf8Lossy::from_str) - rust-lang#91706 (Add unstable book entries for parts of asm that are not being stabilized) - rust-lang#91709 (Replace iterator-based set construction by *Set::From<[T; N]>) - rust-lang#91716 (Improve x.py logging and defaults a bit more) - rust-lang#91747 (Add pierwill to .mailmap) - rust-lang#91755 (Fix since attribute for const_linked_list_new feature) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
…ce, r=lcnr Directly use ConstValue for single literals in blocks Addresses the minimal repro in rust-lang#92186, but doesn't fix the underlying problem (which would be solved by solving the anon subst problem afaict). I do, however, think that it makes sense in general to treat single literals in anon blocks as const values directly, especially in light of the problem that the issue refers to (anon const evaluation being postponed until infer variables in substs can be resolved, which was introduced by rust-lang#90023), i.e. while we do get warnings for those unnecessary braces, we should try to avoid errors caused by those braces if possible.
Previously
delay_span_bug
calls were triggered once an inference variable was included in the substs of a constant that was to be evaluated. Some of these would merely have resulted in trait candidates being rejected, hence no real error was ever encountered, but the triggering of thedelay_span_bug
then caused an ICE in later stages of the compiler due to no error ever occurring.We now postpone the evaluation of these constants, so any trait obligation fulfillment will simply stall on this constant and the existing type inference machinery of the compiler handles any type errors if present.
Fixes #89320
Fixes #89146
Fixes #87964
Fixes #87470
Fixes #83288
Fixes #83249
Fixes #90654
I want to thank @BoxyUwU for cooperating on this and for providing some help.
r? @lcnr maybe?