[Underspecified semantics] Type check defaults at declaration. #46785

leoyvens · 2017-12-17T13:24:33Z

Fixes #46669. See the test for code that compiles on stable but will no longer compile. This falls under a "Underspecified language semantics" fix. Needs crater.

On type and trait declarations, we currently allow anything that name checks as a type parameter default. That allows the user to write a default that can never be applied, or even a default that may conditionally be applied depending on the type of another parameter. Mostly this just defers the error to use sites, but also allows clever hacks such as Foo<T, U = <T as Iterator>::Item> where U will be able to apply it's default only when T: Iterator. Maybe that means this bug is a feature, but it's a fiddly behaviour that seems undesirable.

This PR validates defaults at declaration sites by ensuring all predicates on the parameter are valid for the default. With the exception of Self: Sized which we don't want to check to allow things like trait Add<RHS = Self>.

rust-highfive · 2017-12-17T13:24:38Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

leoyvens · 2017-12-17T13:25:11Z

r? @nikomatsakis

nikomatsakis · 2017-12-19T21:11:14Z

src/librustc_typeck/check/wfcheck.rs


            fcx.impl_implied_bounds(item_def_id, item.span)
        });
    }

+    /// Checks where clauses and inline bounds.


Nit: maybe better to say "...and inline bounds that are declared on def_id."

nikomatsakis · 2017-12-19T21:13:15Z

src/librustc_typeck/check/wfcheck.rs

+        // Defaults must be well-formed.
+        for d in defaults {
+            fcx.register_wf_obligation(fcx.tcx.type_of(d), fcx.tcx.def_span(d), self.code.clone());
+        }


This part seems unambiguously good.

Though we likely want to do a crater run and decide if we need forwards compat warnings.

nikomatsakis · 2017-12-19T21:22:48Z

src/librustc_typeck/check/wfcheck.rs

+        }
+
+        // Check that each default fulfills the bounds on it's parameter.
+        // We go over each predicate and duplicate it, substituting defaults in the self type.


This part seems less obviously like something we want. It's a bit tricky, since of course there can be "interconnections" between defaults.

I guess that to start i'd like to state more clearly what property we are trying to enforce here. For example, are we checking that each default, considered individually, would not cause things to stop being well-formed?

Let's take struct Foo<T=u32, U=i32> where C (where C is some set of where-clauses) as our running example. We are already checking that WF(C), which basically says that forall T: C => WF(C).

I guess you are then trying to check that forall T: [i32/U] C => [i32/U] WF(C)? That is, assuming that U is i32, but holding T abstract, we check that the predicates C are well-formed? (And then we do a similar thing for T?)

(Does that make sense? Am I confused (or being confusing)?)

leoyvens · 2017-12-20T20:02:01Z

This part seems less obviously like something we want. It's a bit tricky, since of course there can be "interconnections" between defaults.
I guess that to start i'd like to state more clearly what property we are trying to enforce here. For example, are we checking that each default, considered individually, would not cause things to stop being well-formed?
Let's take struct Foo<T=u32, U=i32> where C (where C is some set of where-clauses) as our running example. We are already checking that WF(C), which basically says that forall T: C => WF(C).
I guess you are then trying to check that forall T: [i32/U] C => [i32/U] WF(C)? That is, assuming that U is i32, but holding T abstract, we check that the predicates C are well-formed? (And then we do a similar thing for T?)

Interesting. That sounds like the correct thing to do. What I implemented checks only [u32/T, i32/U] C => [u32/T, i32/U] WF(C). Which is insufficient, because in the following example:

trait Trait {}
struct Foo<T, U>(T, U);
impl Trait for Foo<i32, i32> {}

struct Bogus<T = i32, U = i32>(Foo<T, U>) where Foo<T, U> : Trait;

It does not detect that the defaults in Bogus are not really well formed, since Foo<T> does not compile for a T different from i32. It seems straightforward to implement the smarter version, given we actually want this.

nikomatsakis · 2017-12-20T21:22:38Z

@leodasvacas

What I implemented checks only [u32/T, i32/U] C => [u32/T, i32/U] WF(C)

OK, I agree that's not sufficient. Also, it seems like you wound up with quite a lot of code for doing that check, and -- as you said -- calls to skip-binder and so forth. That is kind of a bad sign. I feel like we should just be able to do some substitutions, kind of like what was written above. Am I overlooking something?

leoyvens · 2017-12-21T15:53:48Z

Ah, Predicate is TypeFoldable. Leveraging that and making the workaround for Self: Trait less clever simplified the code a lot. No more skip_binder. I've implemented substituting each default individually.

leoyvens · 2017-12-21T17:19:36Z

The failure boils down to:

trait Trait<T> {}
struct Foo<U, V=i32>(U, V) 
    where U: Trait<V>;
______________________^ the trait `Trait<i32>` is not implemented for `U`

This wasn't happening before because I wasn't substituting the rhs of trait clauses.

It seems I'm not substituting in enough places, what I should be checking is in Foo<U, i32>, U: Trait<i32>. Rather than in Foo<U, V>, U: Trait<i32>. Maybe I should just instantiate Foo<U, i32> and check that for well-formedness. That would mostly work (it doesn't, see edit below), except for traits, trying to check a trait type will probably go wrong because trait types can only exist behind a reference. Should I instantiate a &Trait and check that? Then the error messages will not look good. Or maybe we could just not check traits for now.

EDIT: I've been experimenting with "just substituting" in this branch and the exact same problem remains, even if I'm checking in Foo<U, i32>, U: Trait<i32> it's still not true for an identity substitution of U. So I guess we really need the complicated hacks to substitute only in the LHS that I was doing originally. That or give up on checking clauses at all.

leoyvens · 2017-12-23T17:50:18Z

This now substitute defaults only in the LHS of trait predicates. The code is looking better. There is a skip_binder left, but I maybe it's correct because I put things back in a binder right afterwards?

alexcrichton · 2018-01-04T16:10:48Z

ping @nikomatsakis, I think this may be ready for another look!

eddyb · 2018-01-04T18:46:11Z

@leodasvacas Binder has map_bound and map_bound_ref methods you might want to use.

shepmaster · 2018-01-13T00:46:12Z

It's been over 6 days since we last heard from reviewer @nikomatsakis. Perhaps someone else from @rust-lang/compiler could take a look at this?

nikomatsakis · 2018-01-19T21:53:18Z

@shepmaster I got it

nikomatsakis

Left some nits and also some comments. I need to dig a bit more but I am beginning to have second thoughts -- in particular, the check that says "all predicates must be WF with each default substituted independently" feels a bit strict to me.

Consider the example I gave: either default is fine, so long as one of the two parameters is overriden to String. It is not ok to override neither. Should we really error out on this case?

It seems a bit inconsistent with some of the direcitons we've been going, which is too allow for more "unsatisfiable" things but make it an error only when they wind up being used.

I feel like we need to consult the feelings of the @rust-lang/lang team on this question, and perhaps would want an RFC to get broader feedback (e.g., @sgrif might have an opinion).

nikomatsakis · 2018-01-19T22:55:46Z

src/librustc_typeck/check/wfcheck.rs

+                }, |def, _| {
+                    let identity_substs = fcx.tcx.mk_param_from_def(def);
+                    if def.index != param_def.index {
+                        identity_substs


This name is odd; normally a substs is a set of substitutions, but here this is a single type. How about something like param_ty or identity_ty?

nikomatsakis · 2018-01-19T23:06:26Z

src/librustc_typeck/check/wfcheck.rs

+                                             .filter(|def| def.has_default &&
+                                                     def.index >= generics.parent_count() as u32);
+        for param_def in defaulted_params {
+            // Defaults must be well-formed.


Nit: I always think it helps to see concrete examples. So here we might extend the comment to say:

This parameter has a default value. Check that this default value is well-formed. This would rule out something like this (see test XXX.rs):

struct Foo<T = Vec<[u32]>> { .. } // Vec<[u32]> is not WF because `[u32]: Sized` does not hold.

nikomatsakis · 2018-01-19T23:07:17Z

src/librustc_typeck/check/wfcheck.rs

+        for param_def in defaulted_params {
+            // Defaults must be well-formed.
+            let d = param_def.def_id;
+            fcx.register_wf_obligation(fcx.tcx.type_of(d), fcx.tcx.def_span(d), self.code.clone());


Nit: some whitespace after this line would make the code more readable, I think. In particular, this for loop below is a kind of separate thing than the register_wf_obligation call.

nikomatsakis · 2018-01-19T23:07:57Z

src/librustc_typeck/check/wfcheck.rs

+            fcx.register_wf_obligation(fcx.tcx.type_of(d), fcx.tcx.def_span(d), self.code.clone());
+            // Check the clauses are well-formed when the param is substituted by it's default.
+            // In trait definitions, the predicate `Self: Trait` is problematic.
+            // Therefore we skip such predicates. This means we check less than we could.


Here too I'd like to see an example.

As an example, consider something like this:

struct Foo<T = u32, U = i32> where T: Bar { } trait Bar where Self: Baz { } trait Baz { } impl Baz<String> for u32 { } impl Baz<i32> for String { }

Here, when processing Foo, we would attempt to prove the following:

forall<T, U> { // i.e., with T, U in scope (T: Bar) => { // and with `T: Bar` in our environment WF(T: Bar), WF(u32: Bar), // substituting the default for T WF(T: Baz<i32>), } }

This would fail because

WF(u32: Bar) requires that u32: Baz, which is not implemented.

WF(T: Bar<i32>) requires that T: Baz<i32>, which is not implemented.

Actually, before I go too much further, let me verify again to be sure I am remembering correctly... that is the intention of this PR, right?

nikomatsakis · 2018-01-19T23:08:12Z

src/librustc_typeck/check/wfcheck.rs

+            let d = param_def.def_id;
+            fcx.register_wf_obligation(fcx.tcx.type_of(d), fcx.tcx.def_span(d), self.code.clone());
+            // Check the clauses are well-formed when the param is substituted by it's default.
+            // In trait definitions, the predicate `Self: Trait` is problematic.


Can you say a bit more here -- remind me -- what makes Self: Trait problematic exactly?

nikomatsakis · 2018-01-19T23:09:36Z

src/librustc_typeck/check/wfcheck.rs

+            // Check the clauses are well-formed when the param is substituted by it's default.
+            // In trait definitions, the predicate `Self: Trait` is problematic.
+            // Therefore we skip such predicates. This means we check less than we could.
+            for pred in predicates.predicates.iter().filter(|p| !(is_trait && p.has_self_ty())) {


This condition (p.has_self_ty()) seems pretty imprecise. Like, wouldn't that mean that code like this is accepted (which I am not sure we would have wanted to accept?)

trait Foo<T=String>: Bar<T> { } trait Bar<T: Copy> { }

That code in particular is rejected as expected, I suppose we generate a T: Copy predicate for the Foo<T=String> declaration, I added that as a test. See comment below on p.has_self_ty() being coarse.

leoyvens · 2018-01-20T15:49:15Z

Can you say a bit more here -- remind me -- what makes Self: Trait problematic exactly?

Edit: The alternatives proposed below side step this issue and that check because Self cannot have a default.

That comment was badly worded, it now says what I meant: In trait Trait: Super, checking Self: Trait or Self: Super is problematic. I don't really understand why but those predicates caused issues so I worked around them with the p.has_self_ty() check. That check is coarse, in particular ma we could check Self on the RHS of a trait predicate, for example in:

trait Foo<T = i32> where Self: Sized, T: Into<Self> {}

Maybe could check that i32 : Into<Self> but for the sake of code simplicity I didn't try.

leoyvens · 2018-01-20T17:19:21Z

Edit: My current preference is not the proposal in this comment but rather alternative 3 discussed below.

Responding to this inline comment which made me rethink things:

The intention of the PR is to move "error on use" to "error on declaration", and I'm particularly concerned with situations where a library goes from pub struct Foo<T> to pub struct Foo<T, U = Dflt> where ..<constraints on U> and it happens to pass all the tests but it's not really backwards compatible because you are constraining U based on T in a way that Dflt only applies for some values of T. So what I really want to check is that none of those constraints on the new parameter U are accidentally breaking existing uses of Foo<T>.

If the previous paragraph makes sense and we agree it's a problem, the current approach is sometimes too strict and sometimes insufficient.

What about: Given a trait predicate, if all params appearing in the LHS have defaults then it should be a backwards compatible predicate. We verify that by checking the WF of predicate with all defaults substituted simultaneously.

So we would no longer substitute defaults individually as that is to strict, we would substitute in the RHS but an example like this is skipped because there is a non-defaulted in the LHS.

Does this motivation and solution make more sense?

leoyvens · 2018-01-21T16:11:40Z

I agree that checking the predicates substituted with defaults is aggressive in many cases, in which it should be a lint instead of an error. Is it viable to make this a lint? Alternatives:

Figure out how to make it into a lint, preferably a clippy lint so we use a more aggressive form.
Do the compromise described in the previous comment.
A less aggressive compromise: Simultaneously substitute all defaults only in predicates that don't contain non-defaulted params. Checks that defaults work together, disallows this.
Most conservative: Check only predicates that contain a single, defaulted type param, if this check fails then the default can never be applied anyways, e.g. struct Foo<T: Copy = String> { .. }, allows this.

The last commit implements 2 for trait predicates, if it's what we want then I'll implement it for projection predicates as well.

bors · 2018-03-01T07:03:31Z

💔 Test failed - status-appveyor

kennytm · 2018-03-01T08:37:14Z

@bors retry #40474

[02:00:58] warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
[02:01:56] error: unable to get packages from source
[02:01:56] 
[02:01:56] Caused by:
[02:01:56]   [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
[02:01:56] thread 'main' panicked at 'tests failed for https://github.com/dzamlo/treeify', tools\cargotest\main.rs:100:9
[02:01:56] note: Run with `RUST_BACKTRACE=1` for a backtrace.

bors · 2018-03-01T13:19:25Z

⌛ Testing commit 3e84aed with merge 3eeb5a6...

…, r=nikomatsakis [Underspecified semantics] Type check defaults at declaration. Fixes #46669. See the test for code that compiles on stable but will no longer compile. This falls under a "Underspecified language semantics" fix. **Needs crater**. On type and trait declarations, we currently allow anything that name checks as a type parameter default. That allows the user to write a default that can never be applied, or even a default that may conditionally be applied depending on the type of another parameter. Mostly this just defers the error to use sites, but also allows clever hacks such as `Foo<T, U = <T as Iterator>::Item>` where `U` will be able to apply it's default only when `T: Iterator`. Maybe that means this bug is a feature, but it's a fiddly behaviour that seems undesirable. This PR validates defaults at declaration sites by ensuring all predicates on the parameter are valid for the default. With the exception of `Self: Sized` which we don't want to check to allow things like `trait Add<RHS = Self>`.

bors · 2018-03-01T16:20:13Z

💔 Test failed - status-appveyor

Manishearth · 2018-03-01T16:21:55Z

@bors retry

appveyor timeout

bors · 2018-03-01T17:21:44Z

⌛ Testing commit 3e84aed with merge 040c66fa39dfb84beb413d74c06eec2887aa6b0a...

alexcrichton · 2018-03-01T17:26:05Z

The previous build succeeded all dist jobs, merging manually

bors · 2018-03-01T21:21:44Z

💥 Test timed out

kennytm · 2018-03-03T07:39:35Z

@bors clean retry r-

rust-highfive assigned eddyb Dec 17, 2017

rust-highfive assigned nikomatsakis and unassigned eddyb Dec 17, 2017

leoyvens force-pushed the type-check-defaults-at-declaration branch from 02d1cd9 to cb9da17 Compare December 17, 2017 18:48

nikomatsakis reviewed Dec 19, 2017

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2017

leoyvens force-pushed the type-check-defaults-at-declaration branch from 19415dc to e09082c Compare December 21, 2017 15:51

shepmaster added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 13, 2018

leoyvens force-pushed the type-check-defaults-at-declaration branch 3 times, most recently from 4f33c30 to b0973cc Compare January 18, 2018 16:29

nikomatsakis requested changes Jan 19, 2018

View reviewed changes

leoyvens force-pushed the type-check-defaults-at-declaration branch 3 times, most recently from 8055bdd to b1d4d42 Compare January 21, 2018 16:10

leoyvens force-pushed the type-check-defaults-at-declaration branch from cb35fcb to 197bea4 Compare January 22, 2018 15:46

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2018

alexcrichton merged commit 3e84aed into rust-lang:master Mar 1, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 1, 2018

leoyvens deleted the type-check-defaults-at-declaration branch March 7, 2018 10:52

nikomatsakis mentioned this pull request Apr 5, 2018

Regression in interaction between lifetime parameter and defaulted type parameter #49344

Closed

pietroalbini mentioned this pull request May 1, 2018

Regression in 5 crates in 1.26: the trait bound is not satisfied #49932

Closed

nikomatsakis mentioned this pull request Jul 11, 2019

Self as default type isnt typechecked #61631

Closed

[Underspecified semantics] Type check defaults at declaration. #46785

[Underspecified semantics] Type check defaults at declaration. #46785

Conversation

leoyvens commented Dec 17, 2017 • edited Loading

rust-highfive commented Dec 17, 2017

leoyvens commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoyvens commented Dec 20, 2017

nikomatsakis commented Dec 20, 2017

leoyvens commented Dec 21, 2017

leoyvens commented Dec 21, 2017 • edited Loading

leoyvens commented Dec 23, 2017

alexcrichton commented Jan 4, 2018

eddyb commented Jan 4, 2018

shepmaster commented Jan 13, 2018

nikomatsakis commented Jan 19, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoyvens commented Jan 20, 2018 • edited Loading

leoyvens commented Jan 20, 2018 • edited Loading

leoyvens commented Jan 21, 2018

bors commented Mar 1, 2018

kennytm commented Mar 1, 2018

bors commented Mar 1, 2018

bors commented Mar 1, 2018

Manishearth commented Mar 1, 2018

bors commented Mar 1, 2018

alexcrichton commented Mar 1, 2018

bors commented Mar 1, 2018

kennytm commented Mar 3, 2018

leoyvens commented Dec 17, 2017 •

edited

Loading

leoyvens commented Dec 21, 2017 •

edited

Loading

leoyvens commented Jan 20, 2018 •

edited

Loading

leoyvens commented Jan 20, 2018 •

edited

Loading