typeck: always expose repeat count `AnonConst`s' parent in `generics_of`. #70452

eddyb · 2020-03-27T00:54:02Z

This should reduce some of the confusion around #43408, although, if you look at the changed test outputs (for the last commit), they all hit #68436, so nothing new will start compiling.

We can let counts of "repeat expressions" (N in [x; N]) always have the correct generics parenting, because they're always in a body, so nothing in the where clauses or impl trait/type of the parent can use it, and therefore no query cycles can occur.

Other potential candidates we might want to apply the same approach to, are:

~~(easy) enum discriminants (see also Should enum discriminants have generics in scope? #70453)~~ opened typeck: always expose explicit enum discriminant AnonConsts' parent in generics_of. #70825
(trickier) array type (not expression) lengths nested in:
- bodies
- types of (associated or not) const/static
- RHS of type aliases and associated types
- fn signatures

We should've done so from the start, the only reason we haven't is because I was squeamish about blacklisting some of the cases, but if we whitelist instead we should be fine.
Also, lazy normalization is taking forever 😞.

There's also 5 other commits here:

"typeck: track any errors injected during writeback and taint tables appropriately." - fixes Closure type error during const-eval (array len context) #66706, as the next commit would otherwise trigger an ICE again
"typeck: workaround WF hole in to_const." - its purpose is to emulate most of WF-check all ty::Const's, not just array lengths. #70107's direct effect, at least in the case of repeat expressions, where the count always goes through to_const
- this is the reason no new code can really compile, as the WF checks require There's currently no way to specify bounds requiring constants in types to be well-formed #68436 to bypass
- however, this has more test changes than I hoped, so it should be reviewed separately, and maybe even landed separately (as WF-check all ty::Const's, not just array lengths. #70107 might take a while, as it's blocked on a few of my PRs)
"ty: erase lifetimes early in ty::Const::eval." - first attempt at fixing MIR type-checking ICE ("is not a subtype of") from lifetime parameter in scope of AnonConst. #70773
- still useful, I believe the new approach is less likely to cause issues long-term
- I could take this out or move it into another PR if desired or someone else could take over (cc @Skinny121)
"traits/query/normalize: add some debug! logging for the result." - debugging aid for MIR type-checking ICE ("is not a subtype of") from lifetime parameter in scope of AnonConst. #70773
"borrow_check/type_check: normalize Aggregate and Call operands." - actually fixes MIR type-checking ICE ("is not a subtype of") from lifetime parameter in scope of AnonConst. #70773

r? @nikomatsakis cc @pnkfelix @varkor @yodaldevoid @oli-obk @estebank

eddyb · 2020-03-27T09:42:09Z

@bors try @rust-timer queue (we'll also want check-only crater, but only if perf isn't terrible)

rust-timer · 2020-03-27T09:42:10Z

Awaiting bors try build completion

bors · 2020-03-27T09:42:20Z

⌛ Trying commit 295003105405c3d12d74f6d1ab7da612cb898350 with merge 97830c559b7e0548f2bde4aea5bfa7277197ac14...

src/librustc_typeck/check/writeback.rs

estebank · 2020-03-27T16:54:50Z

src/test/ui/const-generics/issues/issue-62456.stderr

+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


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.

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

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.

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

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

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

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

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.

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

Opened #71142 for this subthread.

eddyb · 2020-04-04T16:20:00Z

Got back to this and it looks like the build failure (#70452 (comment)) is a pre-existing bug in the MIR type-checker, for which I've opened #70773 and will now attempt to fix in this PR.

eddyb · 2020-04-04T18:46:54Z

src/librustc_middle/ty/sty.rs

    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 e.g. inference vars.
-            if param_env_and_substs.has_local_value() {
-                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 e.g. 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.has_local_value() {
+                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 e.g. 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.has_local_value() {
-                    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
        }
    }


@nikomatsakis NB: this is the entirety of the commit that I mentioned in the PR description as being a false start for fixing #70773 and doesn't have to be in this PR. Let me know what you prefer.

I don't really understand what this commit is doing exactly. It's erasing lifetimes -- why exactly? And why we do want to do it before checking for inference variables?

It's erasing lifetimes that would be erased later, just to increase the chances of the inference variable check passing (and doing fully specific evaluation instead of the polymorphic evaluation attempt with identity substs).

How could it lead to the inference variable check passing when it wouldn't have passed otherwise?

By erasing lifetime inference variables?

Oh, I see, duh.

OK, makes sense I guess. Might as well keep it.

eddyb · 2020-04-04T18:48:33Z

Now that git2-rs compiles fine with this PR:

@bors try @rust-timer queue (we'll also want check-only crater, but only if perf isn't terrible)

rust-timer · 2020-04-04T18:48:35Z

Awaiting bors try build completion

…of`.

eddyb · 2020-04-14T18:03:44Z

@bors r=nikomatsakis rollup=never

bors · 2020-04-14T18:03:45Z

📌 Commit 8bb7b7b has been approved by nikomatsakis

Dylan-DPC-zz · 2020-04-14T18:39:11Z

@bors p=1

bors · 2020-04-14T21:23:46Z

⌛ Testing commit 8bb7b7b with merge d12f030...

bors · 2020-04-15T00:39:08Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing d12f030 to master...

@nikomatsakis

…ent, r=nikomatsakis typeck: always expose explicit enum discriminant `AnonConst`s' parent in `generics_of`. This is similar to rust-lang#70452 but for explicit `enum` discriminant constant expressions. However, unlike rust-lang#70452, this PR should have no effect on stable code, as while it alleviates rust-lang#43408 errors, there is no way to actually compile an `enum` with generic parameters *and* explicit discriminants, without `#![feature(arbitrary_enum_discriminant)]`, as explicit discriminant expression don't count as uses of parameters (if they did, they would count as invariant uses). <hr/> There's also 2 other commits here, both related to rust-lang#70453: * "ty: use `delay_span_bug` in `ty::AdtDef::eval_explicit_discr`." - hides the ICEs demonstrated on rust-lang#70453, when there are other errors (which the next commit adds) * "typeck/wfcheck: require that explicit enum discriminants const-evaluate succesfully." - closes rust-lang#70453 by picking alternative "2", i.e. erroring when a discriminant doesn't fully const-evaluate from the perspective of the `enum` definition In the future, it might be possible to allow `enum` discriminants to actually depend on parameters, but that will likely require rust-lang#68436 + some way to restrict the values so no two variants can end up with overlapping discriminants. As this PR would close rust-lang#70453, it shouldn't be merged until a decision is reached there. r? @nikomatsakis

@eddyb

Add regression test for issue-66768 Fixes rust-lang#66768 This is fixed by rust-lang#70452 (in particular, https://github.com/rust-lang/rust/pull/70452/files#diff-53aef089a36a8e2ed07627fc8915fe63R1763) and I'm not sure it's worth to add this test (i.e. the tests in rust-lang#70452 are enough), so r? @eddyb to confirm it.

@eddyb

Add regression test for issue-66768 Fixes rust-lang#66768 This is fixed by rust-lang#70452 (in particular, https://github.com/rust-lang/rust/pull/70452/files#diff-53aef089a36a8e2ed07627fc8915fe63R1763) and I'm not sure it's worth to add this test (i.e. the tests in rust-lang#70452 are enough), so r? @eddyb to confirm it.

rust-highfive assigned nikomatsakis Mar 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2020