Skip to content
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

Merged
merged 4 commits into from
Sep 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,9 @@ pub(crate) fn eval_nullary_intrinsic<'tcx>(
ty::Projection(_)
| ty::Opaque(_, _)
| ty::Param(_)
| ty::Bound(_, _)
| ty::Placeholder(_)
| ty::Infer(_) => throw_inval!(TooGeneric),
ty::Bound(_, _) => bug!("bound ty during ctfe"),
Copy link
Member

@RalfJung RalfJung Jul 4, 2022

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.

Copy link
Contributor Author

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. for for<'a> fn(&'a ()) 'ashould be bound.

The only other place I found was const_to_op which I've also changed

ty::Bool
| ty::Char
| ty::Int(_)
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -559,15 +559,15 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
layout: Option<TyAndLayout<'tcx>>,
) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
match c.kind() {
ty::ConstKind::Param(_) | ty::ConstKind::Bound(..) => throw_inval!(TooGeneric),
ty::ConstKind::Param(_) | ty::ConstKind::Placeholder(..) => throw_inval!(TooGeneric),
ty::ConstKind::Error(DelaySpanBugEmitted { reported, .. }) => {
throw_inval!(AlreadyReported(reported))
}
ty::ConstKind::Unevaluated(uv) => {
let instance = self.resolve(uv.def, uv.substs)?;
Ok(self.eval_to_allocation(GlobalId { instance, promoted: uv.promoted })?.into())
}
ty::ConstKind::Infer(..) | ty::ConstKind::Placeholder(..) => {
ty::ConstKind::Bound(..) | ty::ConstKind::Infer(..) => {
span_bug!(self.cur_span(), "const_to_op: Unexpected ConstKind {:?}", c)
}
ty::ConstKind::Value(valtree) => {
Expand Down
20 changes: 8 additions & 12 deletions compiler/rustc_infer/src/infer/combine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ struct Generalizer<'cx, 'tcx> {

param_env: ty::ParamEnv<'tcx>,

cache: SsoHashMap<Ty<'tcx>, RelateResult<'tcx, Ty<'tcx>>>,
cache: SsoHashMap<Ty<'tcx>, Ty<'tcx>>,
}

/// Result from a generalization operation. This includes
Expand Down Expand Up @@ -593,8 +593,8 @@ impl<'tcx> TypeRelation<'tcx> for Generalizer<'_, 'tcx> {
fn tys(&mut self, t: Ty<'tcx>, t2: Ty<'tcx>) -> RelateResult<'tcx, Ty<'tcx>> {
assert_eq!(t, t2); // we are abusing TypeRelation here; both LHS and RHS ought to be ==

if let Some(result) = self.cache.get(&t) {
return result.clone();
if let Some(&result) = self.cache.get(&t) {
return Ok(result);
}
debug!("generalize: t={:?}", t);

Expand Down Expand Up @@ -664,10 +664,10 @@ impl<'tcx> TypeRelation<'tcx> for Generalizer<'_, 'tcx> {
Ok(t)
}
_ => relate::super_relate_tys(self, t, t),
};
}?;

self.cache.insert(t, result.clone());
return result;
self.cache.insert(t, result);
Ok(result)
}

fn regions(
Expand Down Expand Up @@ -743,9 +743,7 @@ impl<'tcx> TypeRelation<'tcx> for Generalizer<'_, 'tcx> {
}
}
}
ty::ConstKind::Unevaluated(ty::Unevaluated { def, substs, promoted })
if self.tcx().lazy_normalization() =>
{
ty::ConstKind::Unevaluated(ty::Unevaluated { def, substs, promoted }) => {
assert_eq!(promoted, None);
let substs = self.relate_with_variance(
ty::Variance::Invariant,
Expand Down Expand Up @@ -967,9 +965,7 @@ impl<'tcx> TypeRelation<'tcx> for ConstInferUnifier<'_, 'tcx> {
}
}
}
ty::ConstKind::Unevaluated(ty::Unevaluated { def, substs, promoted })
if self.tcx().lazy_normalization() =>
{
ty::ConstKind::Unevaluated(ty::Unevaluated { def, substs, promoted }) => {
assert_eq!(promoted, None);
let substs = self.relate_with_variance(
ty::Variance::Invariant,
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/ty/consts/kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ impl<'tcx> ConstKind<'tcx> {
param_env: ParamEnv<'tcx>,
eval_mode: EvalMode,
) -> Option<Result<EvalResult<'tcx>, ErrorGuaranteed>> {
assert!(!self.has_escaping_bound_vars(), "escaping vars in {self:?}");
if let ConstKind::Unevaluated(unevaluated) = self {
use crate::mir::interpret::ErrorHandled;

Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_middle/src/ty/relate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -594,10 +594,6 @@ pub fn super_relate_consts<'tcx, R: TypeRelation<'tcx>>(
);
}

let eagerly_eval = |x: ty::Const<'tcx>| x.eval(tcx, relation.param_env());
let a = eagerly_eval(a);
let b = eagerly_eval(b);

// Currently, the values that can be unified are primitive types,
// and those that derive both `PartialEq` and `Eq`, corresponding
// to structural-match types.
Expand Down
48 changes: 44 additions & 4 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
)
}
}

Expand Down Expand Up @@ -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>>(
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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(
Copy link
Member

Choose a reason for hiding this comment

The 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.
Expand Down
19 changes: 10 additions & 9 deletions compiler/rustc_trait_selection/src/traits/query/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::infer::at::At;
use crate::infer::canonical::OriginalQueryValues;
use crate::infer::{InferCtxt, InferOk};
use crate::traits::error_reporting::InferCtxtExt;
use crate::traits::project::needs_normalization;
use crate::traits::project::{needs_normalization, BoundVarReplacer, PlaceholderReplacer};
use crate::traits::{Obligation, ObligationCause, PredicateObligation, Reveal};
use rustc_data_structures::sso::SsoHashMap;
use rustc_data_structures::stack::ensure_sufficient_stack;
Expand Down Expand Up @@ -283,11 +283,7 @@ impl<'cx, 'tcx> FallibleTypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
let tcx = self.infcx.tcx;
let infcx = self.infcx;
let (data, mapped_regions, mapped_types, mapped_consts) =
crate::traits::project::BoundVarReplacer::replace_bound_vars(
infcx,
&mut self.universes,
data,
);
BoundVarReplacer::replace_bound_vars(infcx, &mut self.universes, data);
let data = data.try_fold_with(self)?;

let mut orig_values = OriginalQueryValues::default();
Expand All @@ -313,8 +309,7 @@ impl<'cx, 'tcx> FallibleTypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
debug!("QueryNormalizer: result = {:#?}", result);
debug!("QueryNormalizer: obligations = {:#?}", obligations);
self.obligations.extend(obligations);

let res = crate::traits::project::PlaceholderReplacer::replace_placeholders(
let res = PlaceholderReplacer::replace_placeholders(
infcx,
mapped_regions,
mapped_types,
Expand Down Expand Up @@ -343,7 +338,13 @@ impl<'cx, 'tcx> FallibleTypeFolder<'tcx> for QueryNormalizer<'cx, 'tcx> {
constant: ty::Const<'tcx>,
) -> Result<ty::Const<'tcx>, Self::Error> {
let constant = constant.try_super_fold_with(self)?;
Ok(constant.eval(self.infcx.tcx, self.param_env))
debug!(?constant, ?self.param_env);
Ok(crate::traits::project::with_replaced_escaping_bound_vars(
self.infcx,
&mut self.universes,
constant,
|constant| constant.eval(self.infcx.tcx, self.param_env),
))
}

fn try_fold_mir_const(
Expand Down
9 changes: 3 additions & 6 deletions compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -495,13 +495,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

pub fn to_const(&self, ast_c: &hir::AnonConst) -> ty::Const<'tcx> {
let const_def_id = self.tcx.hir().local_def_id(ast_c.hir_id);
let span = self.tcx.hir().span(ast_c.hir_id);
let c = ty::Const::from_anon_const(self.tcx, const_def_id);
self.register_wf_obligation(
c.into(),
self.tcx.hir().span(ast_c.hir_id),
ObligationCauseCode::WellFormed(None),
);
c
self.register_wf_obligation(c.into(), span, ObligationCauseCode::WellFormed(None));
self.normalize_associated_types_in(span, c)
}

pub fn const_arg_to_const(
Expand Down
1 change: 0 additions & 1 deletion src/test/ui/closures/issue-52437.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,4 @@ fn main() {
[(); &(&'static: loop { |x| {}; }) as *const _ as usize]
//~^ ERROR: invalid label name `'static`
//~| ERROR: type annotations needed
//~| ERROR mismatched types
}
13 changes: 2 additions & 11 deletions src/test/ui/closures/issue-52437.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,6 @@ help: consider giving this closure parameter an explicit type
LL | [(); &(&'static: loop { |x: _| {}; }) as *const _ as usize]
| +++

error[E0308]: mismatched types
--> $DIR/issue-52437.rs:2:5
|
LL | fn main() {
| - expected `()` because of default return type
LL | [(); &(&'static: loop { |x| {}; }) as *const _ as usize]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found array `[(); _]`

error: aborting due to 3 previous errors
error: aborting due to 2 previous errors

Some errors have detailed explanations: E0282, E0308.
For more information about an error, try `rustc --explain E0282`.
For more information about this error, try `rustc --explain E0282`.
3 changes: 0 additions & 3 deletions src/test/ui/issues/issue-66706.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ fn a() {
[0; [|_: _ &_| ()].len()]
//~^ ERROR expected `,`, found `&`
//~| ERROR type annotations needed
//~| ERROR mismatched types
}

fn b() {
Expand All @@ -13,13 +12,11 @@ fn b() {
fn c() {
[0; [|&_: _ &_| {}; 0 ].len()]
//~^ ERROR expected `,`, found `&`
//~| ERROR mismatched types
}

fn d() {
[0; match [|f @ &ref _| () ] {} ]
//~^ ERROR expected identifier, found reserved identifier `_`
//~| ERROR mismatched types
}

fn main() {}
35 changes: 5 additions & 30 deletions src/test/ui/issues/issue-66706.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,21 @@ LL | [0; [|_: _ &_| ()].len()]
| help: missing `,`

error: expected identifier, found reserved identifier `_`
--> $DIR/issue-66706.rs:9:20
--> $DIR/issue-66706.rs:8:20
|
LL | [0; [|f @ &ref _| {} ; 0 ].len() ];
| ^ expected identifier, found reserved identifier

error: expected `,`, found `&`
--> $DIR/issue-66706.rs:14:17
--> $DIR/issue-66706.rs:13:17
|
LL | [0; [|&_: _ &_| {}; 0 ].len()]
| -^ expected `,`
| |
| help: missing `,`

error: expected identifier, found reserved identifier `_`
--> $DIR/issue-66706.rs:20:26
--> $DIR/issue-66706.rs:18:26
|
LL | [0; match [|f @ &ref _| () ] {} ]
| ^ expected identifier, found reserved identifier
Expand All @@ -32,31 +32,6 @@ error[E0282]: type annotations needed
LL | [0; [|_: _ &_| ()].len()]
| ^ cannot infer type

error[E0308]: mismatched types
--> $DIR/issue-66706.rs:2:5
|
LL | fn a() {
| - help: try adding a return type: `-> [i32; _]`
LL | [0; [|_: _ &_| ()].len()]
| ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found array `[{integer}; _]`

error[E0308]: mismatched types
--> $DIR/issue-66706.rs:14:5
|
LL | fn c() {
| - help: try adding a return type: `-> [i32; _]`
LL | [0; [|&_: _ &_| {}; 0 ].len()]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found array `[{integer}; _]`

error[E0308]: mismatched types
--> $DIR/issue-66706.rs:20:5
|
LL | fn d() {
| - help: try adding a return type: `-> [i32; _]`
LL | [0; match [|f @ &ref _| () ] {} ]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found array `[{integer}; _]`

error: aborting due to 8 previous errors
error: aborting due to 5 previous errors

Some errors have detailed explanations: E0282, E0308.
For more information about an error, try `rustc --explain E0282`.
For more information about this error, try `rustc --explain E0282`.