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

typeck: always expose repeat count AnonConsts' parent in generics_of. #70452

Merged
merged 6 commits into from
Apr 15, 2020
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
58 changes: 28 additions & 30 deletions src/librustc_middle/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2339,43 +2339,41 @@ impl<'tcx> Const<'tcx> {
/// Tries to evaluate the constant if it is `Unevaluated`. If that doesn't succeed, return the
/// unevaluated constant.
pub fn eval(&self, tcx: TyCtxt<'tcx>, param_env: ParamEnv<'tcx>) -> &Const<'tcx> {
let try_const_eval = |did, param_env: ParamEnv<'tcx>, substs, promoted| {
if let ConstKind::Unevaluated(did, substs, promoted) = self.val {
let param_env_and_substs = param_env.with_reveal_all().and(substs);

// Avoid querying `tcx.const_eval(...)` with any inference vars.
if param_env_and_substs.needs_infer() {
return None;
}
// HACK(eddyb) this erases lifetimes even though `const_eval_resolve`
// also does later, but we want to do it before checking for
// inference variables.
let param_env_and_substs = tcx.erase_regions(&param_env_and_substs);

// HACK(eddyb) when the query key would contain inference variables,
// attempt using identity substs and `ParamEnv` instead, that will succeed
// when the expression doesn't depend on any parameters.
// FIXME(eddyb, skinny121) pass `InferCtxt` into here when it's available, so that
// we can call `infcx.const_eval_resolve` which handles inference variables.
let param_env_and_substs = if param_env_and_substs.needs_infer() {
tcx.param_env(did).and(InternalSubsts::identity_for_item(tcx, did))
} else {
param_env_and_substs
};

// FIXME(eddyb) maybe the `const_eval_*` methods should take
// `ty::ParamEnvAnd<SubstsRef>` instead of having them separate.
let (param_env, substs) = param_env_and_substs.into_parts();

// try to resolve e.g. associated constants to their definition on an impl, and then
// evaluate the const.
tcx.const_eval_resolve(param_env, did, substs, promoted, None)
.ok()
.map(|val| Const::from_value(tcx, val, self.ty))
};

match self.val {
ConstKind::Unevaluated(did, substs, promoted) => {
// HACK(eddyb) when substs contain inference variables,
// attempt using identity substs instead, that will succeed
// when the expression doesn't depend on any parameters.
// FIXME(eddyb, skinny121) pass `InferCtxt` into here when it's available, so that
// we can call `infcx.const_eval_resolve` which handles inference variables.
if substs.needs_infer() {
let identity_substs = InternalSubsts::identity_for_item(tcx, did);
// The `ParamEnv` needs to match the `identity_substs`.
let identity_param_env = tcx.param_env(did);
match try_const_eval(did, identity_param_env, identity_substs, promoted) {
Some(ct) => ct.subst(tcx, substs),
None => self,
}
} else {
try_const_eval(did, param_env, substs, promoted).unwrap_or(self)
}
match tcx.const_eval_resolve(param_env, did, substs, promoted, None) {
// NOTE(eddyb) `val` contains no lifetimes/types/consts,
// and we use the original type, so nothing from `substs`
// (which may be identity substs, see above),
// can leak through `val` into the const we return.
Ok(val) => Const::from_value(tcx, val, self.ty),

Err(_) => self,
}
_ => self,
} else {
self
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/librustc_mir/borrow_check/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1760,6 +1760,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
}
for (n, (fn_arg, op_arg)) in sig.inputs().iter().zip(args).enumerate() {
let op_arg_ty = op_arg.ty(body, self.tcx());
let op_arg_ty = self.normalize(op_arg_ty, term_location);
let category = if from_hir_call {
ConstraintCategory::CallArgument
} else {
Expand Down Expand Up @@ -2402,6 +2403,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
}
};
let operand_ty = operand.ty(body, tcx);
let operand_ty = self.normalize(operand_ty, location);

if let Err(terr) = self.sub_types(
operand_ty,
Expand Down
15 changes: 13 additions & 2 deletions src/librustc_trait_selection/traits/query/normalize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,22 @@ impl<'cx, 'tcx> AtExt<'tcx> for At<'cx, 'tcx> {
anon_depth: 0,
};

let value1 = value.fold_with(&mut normalizer);
let result = value.fold_with(&mut normalizer);
debug!(
"normalize::<{}>: result={:?} with {} obligations",
::std::any::type_name::<T>(),
result,
normalizer.obligations.len(),
);
debug!(
"normalize::<{}>: obligations={:?}",
::std::any::type_name::<T>(),
normalizer.obligations,
);
if normalizer.error {
Err(NoSolution)
} else {
Ok(Normalized { value: value1, obligations: normalizer.obligations })
Ok(Normalized { value: result, obligations: normalizer.obligations })
}
}
}
Expand Down
27 changes: 25 additions & 2 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3311,8 +3311,31 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

pub fn to_const(&self, ast_c: &hir::AnonConst) -> &'tcx ty::Const<'tcx> {
let c = self.tcx.hir().local_def_id(ast_c.hir_id).expect_local();
ty::Const::from_anon_const(self.tcx, c)
let const_def_id = self.tcx.hir().local_def_id(ast_c.hir_id).expect_local();
let c = ty::Const::from_anon_const(self.tcx, const_def_id);

// HACK(eddyb) emulate what a `WellFormedConst` obligation would do.
// This code should be replaced with the proper WF handling ASAP.
if let ty::ConstKind::Unevaluated(def_id, substs, promoted) = c.val {
assert!(promoted.is_none());

// HACK(eddyb) let's hope these are always empty.
// let obligations = self.nominal_obligations(def_id, substs);
// self.out.extend(obligations);

let cause = traits::ObligationCause::new(
self.tcx.def_span(const_def_id.to_def_id()),
self.body_id,
traits::MiscObligation,
);
self.register_predicate(traits::Obligation::new(
cause,
self.param_env,
ty::Predicate::ConstEvaluatable(def_id, substs),
));
}

c
}

// If the type given by the user has free regions, save it for later, since
Expand Down
20 changes: 16 additions & 4 deletions src/librustc_typeck/check/writeback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
wbcx.tables.upvar_list =
mem::replace(&mut self.tables.borrow_mut().upvar_list, Default::default());

wbcx.tables.tainted_by_errors = self.is_tainted_by_errors();
wbcx.tables.tainted_by_errors |= self.is_tainted_by_errors();

debug!("writeback: tables for {:?} are {:#?}", item_def_id, wbcx.tables);

Expand Down Expand Up @@ -578,14 +578,21 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
}
}

fn resolve<T>(&self, x: &T, span: &dyn Locatable) -> T
fn resolve<T>(&mut self, x: &T, span: &dyn Locatable) -> T
where
T: TypeFoldable<'tcx>,
{
let x = x.fold_with(&mut Resolver::new(self.fcx, span, self.body));
let mut resolver = Resolver::new(self.fcx, span, self.body);
let x = x.fold_with(&mut resolver);
if cfg!(debug_assertions) && x.needs_infer() {
span_bug!(span.to_span(self.fcx.tcx), "writeback: `{:?}` has inference variables", x);
}

// We may have introduced e.g. `ty::Error`, if inference failed, make sure
// to mark the `TypeckTables` as tainted in that case, so that downstream
// users of the tables don't produce extra errors, or worse, ICEs.
self.tables.tainted_by_errors |= resolver.replaced_with_error;

x
}
}
Expand Down Expand Up @@ -613,6 +620,9 @@ struct Resolver<'cx, 'tcx> {
infcx: &'cx InferCtxt<'cx, 'tcx>,
span: &'cx dyn Locatable,
body: &'tcx hir::Body<'tcx>,

/// Set to `true` if any `Ty` or `ty::Const` had to be replaced with an `Error`.
replaced_with_error: bool,
}

impl<'cx, 'tcx> Resolver<'cx, 'tcx> {
Expand All @@ -621,7 +631,7 @@ impl<'cx, 'tcx> Resolver<'cx, 'tcx> {
span: &'cx dyn Locatable,
body: &'tcx hir::Body<'tcx>,
) -> Resolver<'cx, 'tcx> {
Resolver { tcx: fcx.tcx, infcx: fcx, span, body }
Resolver { tcx: fcx.tcx, infcx: fcx, span, body, replaced_with_error: false }
}

fn report_error(&self, t: Ty<'tcx>) {
Expand All @@ -644,6 +654,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for Resolver<'cx, 'tcx> {
Err(_) => {
debug!("Resolver::fold_ty: input type `{:?}` not fully resolvable", t);
self.report_error(t);
self.replaced_with_error = true;
self.tcx().types.err
}
}
Expand All @@ -661,6 +672,7 @@ impl<'cx, 'tcx> TypeFolder<'tcx> for Resolver<'cx, 'tcx> {
debug!("Resolver::fold_const: input const `{:?}` not fully resolvable", ct);
// FIXME: we'd like to use `self.report_error`, but it doesn't yet
// accept a &'tcx ty::Const.
self.replaced_with_error = true;
eddyb marked this conversation as resolved.
Show resolved Hide resolved
self.tcx().consts.err
}
}
Expand Down
20 changes: 17 additions & 3 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1170,14 +1170,28 @@ fn generics_of(tcx: TyCtxt<'_>, def_id: DefId) -> &ty::Generics {
}
// FIXME(#43408) enable this always when we get lazy normalization.
Node::AnonConst(_) => {
let parent_id = tcx.hir().get_parent_item(hir_id);
let parent_def_id = tcx.hir().local_def_id(parent_id);

// HACK(eddyb) this provides the correct generics when
// `feature(const_generics)` is enabled, so that const expressions
// used with const generics, e.g. `Foo<{N+1}>`, can work at all.
if tcx.features().const_generics {
let parent_id = tcx.hir().get_parent_item(hir_id);
Some(tcx.hir().local_def_id(parent_id))
Some(parent_def_id)
} else {
None
let parent_node = tcx.hir().get(tcx.hir().get_parent_node(hir_id));
match parent_node {
// HACK(eddyb) this provides the correct generics for repeat
// expressions' count (i.e. `N` in `[x; N]`), as they shouldn't
// be able to cause query cycle errors.
Node::Expr(&Expr { kind: ExprKind::Repeat(_, ref constant), .. })
if constant.hir_id == hir_id =>
{
Some(parent_def_id)
}

_ => None,
}
}
}
Node::Expr(&hir::Expr { kind: hir::ExprKind::Closure(..), .. }) => {
Expand Down
5 changes: 5 additions & 0 deletions src/test/compile-fail/issue-52443.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,9 @@ fn main() {
//~| WARN denote infinite loops with
[(); { for _ in 0usize.. {}; 0}];
//~^ ERROR `for` is not allowed in a `const`
//~| ERROR calls in constants are limited to constant functions
//~| ERROR references in constants may only refer to immutable values
//~| ERROR calls in constants are limited to constant functions
//~| ERROR constant contains unimplemented expression type
//~| ERROR evaluation of constant value failed
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ impl Foo for Def {

pub fn test<A: Foo, B: Foo>() {
let _array = [4; <A as Foo>::Y];
//~^ ERROR the trait bound `A: Foo` is not satisfied [E0277]
//~^ ERROR constant expression depends on a generic parameter
}

fn main() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,17 +1,10 @@
error[E0277]: the trait bound `A: Foo` is not satisfied
error: constant expression depends on a generic parameter
--> $DIR/associated-const-type-parameter-arrays-2.rs:16:22
|
LL | const Y: usize;
| --------------- required by `Foo::Y`
...
LL | let _array = [4; <A as Foo>::Y];
| ^^^^^^^^^^^^^ the trait `Foo` is not implemented for `A`
| ^^^^^^^^^^^^^
|
help: consider further restricting this bound
|
LL | pub fn test<A: Foo + Foo, B: Foo>() {
| ^^^^^
= note: this may fail depending on what value the parameter takes

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
3 changes: 1 addition & 2 deletions src/test/ui/const-generics/issues/issue-62456.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
#![feature(const_generics)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash

// build-pass

fn foo<const N: usize>() {
let _ = [0u64; N + 1];
//~^ ERROR constant expression depends on a generic parameter
}

fn main() {}
10 changes: 9 additions & 1 deletion src/test/ui/const-generics/issues/issue-62456.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,13 @@ LL | #![feature(const_generics)]
|
= note: `#[warn(incomplete_features)]` on by default

warning: 1 warning emitted
error: constant expression depends on a generic parameter
--> $DIR/issue-62456.rs:5:20
|
LL | let _ = [0u64; N + 1];
| ^^^^^
|
= note: this may fail depending on what value the parameter takes
Comment on lines +9 to +15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should improve the wording of the error. In my eyes I would prefer something along the lines of

error: constant expression can't depend on generic parameter
  --> $DIR/issue-62456.rs:5:20
   |
LL |     let _ = [0u64; N + 1];
   |                    ^^^^^ depends on a generic parameter
   |
   = note: this will be supported in the future, but isn't now

preferably with a link to documentation, tracking issue or its own error code so that we explain why we don't support it yet and when it might be done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is indeed misleading, because what you're suggesting isn't what the error means.

The error is trying to say that evaluating the constant expression may only succeed for some "values" of the generic parameter.
The missing piece of the puzzle is a way to put that expression in a where clause to force the caller to evaluate it, ruling out post-monomorphization errors (#68436).

Note that expressions using generics can still evaluate just fine while remaining polymorphic.

cc @varkor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this diagnostic could be improved, especially as it was misinterpreted. How about something like:

error: constant expression may not depend on a generic parameter
  --> $DIR/issue-62456.rs:5:20
   |
LL |     let _ = [0u64; N + 1];
   |                    ^^^^^
   |
   = note: this expression may only be valid for some values of `N`, which `N` is guaranteed to take

This is a bit long, though this sort of thing hopefully gets the intention across better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"constant expression may not depend on a generic parameter"? (emphasis mine)

Copy link
Contributor

@Centril Centril Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

   = note: this will be supported in the future, but isn't now

(Let's not make promises. ^^)

"constant expression may not depend on a generic parameter"? (emphasis mine)

I think we tend to prefer cannot over may not. The former is more definitive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe is not allowed to depend, which makes clear whether the constant expression is supposed to or not.

Copy link
Member Author

@eddyb eddyb Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it can depend on generic parameters, the only problem is we have no guarantor for it evaluating successfully.

So perhaps a better phrasing for the error itself might be "cannot prove/determine/guarantee/etc. that constant expression will evaluate successfully" and then we can search its Substs for type/const parameters and list them out to give the more helpful information.

We should also link https://github.com/rust-lang/rust/issues/68436, in some sort of help messaging along the lines of "there is no way to currently write the necessary where clause, watch this space".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I suppose I meant that it could not depend on an "unrestricted" generic parameter (or "guaranteed well-formed"). Yes, it's quite difficult to use language that's both intuitive, and doesn't take up a lot of space. Linking to the issue would be good.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on linking to that issue. I would say I've come around to the position that reducing verbosity is a goal, but much lower than correct understandable wording.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #71142 for this subthread.


error: aborting due to previous error; 1 warning emitted

1 change: 1 addition & 0 deletions src/test/ui/const-generics/issues/issue-62504.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ impl<const X: usize> ArrayHolder<X> {
pub const fn new() -> Self {
ArrayHolder([0; Self::SIZE])
//~^ ERROR: mismatched types
//~| ERROR constant expression depends on a generic parameter
}
}

Expand Down
10 changes: 9 additions & 1 deletion src/test/ui/const-generics/issues/issue-62504.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ LL | ArrayHolder([0; Self::SIZE])
= note: expected array `[u32; _]`
found array `[u32; _]`

error: aborting due to previous error
error: constant expression depends on a generic parameter
--> $DIR/issue-62504.rs:18:25
|
LL | ArrayHolder([0; Self::SIZE])
| ^^^^^^^^^^
|
= note: this may fail depending on what value the parameter takes

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
3 changes: 1 addition & 2 deletions src/test/ui/const-generics/issues/issue-66205.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// check-pass

#![allow(incomplete_features, dead_code, unconditional_recursion)]
#![feature(const_generics)]

fn fact<const N: usize>() {
fact::<{ N - 1 }>();
//~^ ERROR constant expression depends on a generic parameter
}

fn main() {}
10 changes: 10 additions & 0 deletions src/test/ui/const-generics/issues/issue-66205.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: constant expression depends on a generic parameter
--> $DIR/issue-66205.rs:5:12
|
LL | fact::<{ N - 1 }>();
| ^^^^^^^^^
|
= note: this may fail depending on what value the parameter takes

error: aborting due to previous error

3 changes: 1 addition & 2 deletions src/test/ui/const-generics/issues/issue-67739.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// Regression test for #67739

// check-pass

#![allow(incomplete_features)]
#![feature(const_generics)]

Expand All @@ -12,6 +10,7 @@ pub trait Trait {

fn associated_size(&self) -> usize {
[0u8; mem::size_of::<Self::Associated>()];
//~^ ERROR constant expression depends on a generic parameter
0
}
}
Expand Down
10 changes: 10 additions & 0 deletions src/test/ui/const-generics/issues/issue-67739.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: constant expression depends on a generic parameter
--> $DIR/issue-67739.rs:12:15
|
LL | [0u8; mem::size_of::<Self::Associated>()];
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: this may fail depending on what value the parameter takes

error: aborting due to previous error

2 changes: 2 additions & 0 deletions src/test/ui/consts/const-eval/issue-52442.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
fn main() {
[(); { &loop { break } as *const _ as usize } ];
//~^ ERROR `loop` is not allowed in a `const`
//~| ERROR casting pointers to integers in constants is unstable
//~| ERROR evaluation of constant value failed
}
Loading