-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
const_generics: correctly deal with bound variables #98900
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -635,13 +635,18 @@ impl<'a, 'b, 'tcx> TypeFolder<'tcx> for AssocTypeNormalizer<'a, 'b, 'tcx> { | |
|
||
#[instrument(skip(self), level = "debug")] | ||
fn fold_const(&mut self, constant: ty::Const<'tcx>) -> ty::Const<'tcx> { | ||
if self.selcx.tcx().lazy_normalization() || !self.eager_inference_replacement { | ||
let tcx = self.selcx.tcx(); | ||
if tcx.lazy_normalization() { | ||
constant | ||
} else { | ||
let constant = constant.super_fold_with(self); | ||
debug!(?constant); | ||
debug!("self.param_env: {:?}", self.param_env); | ||
constant.eval(self.selcx.tcx(), self.param_env) | ||
debug!(?constant, ?self.param_env); | ||
with_replaced_escaping_bound_vars( | ||
self.selcx.infcx(), | ||
&mut self.universes, | ||
constant, | ||
|constant| constant.eval(tcx, self.param_env), | ||
) | ||
} | ||
} | ||
|
||
|
@@ -671,6 +676,41 @@ pub struct BoundVarReplacer<'me, 'tcx> { | |
universe_indices: &'me mut Vec<Option<ty::UniverseIndex>>, | ||
} | ||
|
||
/// Executes `f` on `value` after replacing all escaping bound variables with placeholders | ||
/// and then replaces these placeholders with the original bound variables in the result. | ||
/// | ||
/// In most places, bound variables should be replaced right when entering a binder, making | ||
/// this function unnecessary. However, normalization currently does not do that, so we have | ||
/// to do this lazily. | ||
/// | ||
/// You should not add any additional uses of this function, at least not without first | ||
/// discussing it with t-types. | ||
/// | ||
/// FIXME(@lcnr): We may even consider experimenting with eagerly replacing bound vars during | ||
/// normalization as well, at which point this function will be unnecessary and can be removed. | ||
pub fn with_replaced_escaping_bound_vars<'a, 'tcx, T: TypeFoldable<'tcx>, R: TypeFoldable<'tcx>>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know how I feel about making this pattern more common. This probably deserves to be split into its own change with its own justification. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment and maybe a FIXME that we should be more careful to not leak placeholders? |
||
infcx: &'a InferCtxt<'a, 'tcx>, | ||
universe_indices: &'a mut Vec<Option<ty::UniverseIndex>>, | ||
value: T, | ||
f: impl FnOnce(T) -> R, | ||
) -> R { | ||
if value.has_escaping_bound_vars() { | ||
let (value, mapped_regions, mapped_types, mapped_consts) = | ||
BoundVarReplacer::replace_bound_vars(infcx, universe_indices, value); | ||
let result = f(value); | ||
PlaceholderReplacer::replace_placeholders( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are going to make the pattern more common, I think we need to be more rigid about checking that we can't have placeholders left around. |
||
infcx, | ||
mapped_regions, | ||
mapped_types, | ||
mapped_consts, | ||
universe_indices, | ||
result, | ||
) | ||
} else { | ||
f(value) | ||
} | ||
} | ||
|
||
impl<'me, 'tcx> BoundVarReplacer<'me, 'tcx> { | ||
/// Returns `Some` if we *were* able to replace bound vars. If there are any bound vars that | ||
/// use a binding level above `universe_indices.len()`, we fail. | ||
|
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.
Curious to have this ICE only for
variant_count
?Though this might be the only exhaustive type match in the interpreter 🤷 it just won't give a lot of coverage.
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.
^^ yeah, I also thought that we would have this in more places. ctfe mostly doesn't care about
ty::Const
. Stuff like type name looks into types, so there bound stuff is allowed, e.g. forfor<'a> fn(&'a ())
'a
should be bound.The only other place I found was
const_to_op
which I've also changed