Raw types in bounds should be allowed when the raw type bound is ground

[leafpetersen]

If a parameterized type A is used somewhere in a type bound with no type arguments, it is currently treated as an error in strong mode unless the type parameter to A has no bound. This should be relaxed to also allow this in any case where the bound on the type parameter to A does not reference any other type parameters to A. Examples:

class A<T> {}

/// Already allowed, A is treated as A<dynamic>
class B<T extends A> {}

class C<T extends A<int>> {}

/// Currently disallowed, should be allowed, C is treated as C<A<int>>
class D<T extends C> {}

/// Currently disallowed, should be allowed, List<C> is treated as List<C<A<int>>
class E<T extends List<C>> {}

[mit-mit]

This does not block 1.22; tagging 1.23

[eernstg]

As I argued elsewhere, we should consider treating A as A<Object> rather than A<dynamic>: Allowing dynamic invocation of arbitrary members on expressions whose type is the type argument of A is a non-trivial source of unsoundness, so we may wish to ask programmers to request that explicitly—and write the type they want if they are actually not happy about Object.

[scheglov]

I implemented this change, partially.

There is a problem though. If we allow "instantiate to bounds" for bounds, we cannot resolve bounds in arbitrary order, we need to do this in dependency order. This works with the new analysis driver, because we can resynthesize in any order. But not with the old tasks based analysis. And we're going to remove it in the near future, so I think it is not worth making it work with the old implementation.

[dgrove]

Any updates here?

[scheglov]

I'm working on moving DDC to the new analysis driver, so that we could use resynthesized types.
Once this is done, we will be unblocked to implement this feature only in one place.

[dgrove]

Is this likely to make it for 1.23?

[scheglov]

Yes, I think so.

[scheglov]

https://codereview.chromium.org/2762863002

[scheglov]

ef527a9

[scheglov]

Unfortunately, I had to roll the change back. There are problems that I was not able to fix quickly enough. At this point I don't think I will able to make it for 1.23.

[dgrove]

Is this happening for 1.24?

[dgrove]

@leafpetersen is this happening for 1.24? (ie, this week)

[leafpetersen]

Moving to 1.25. @scheglov @bwilkerson is this feasible to implement in the analyzer for 1.25, or should we leave this to the new front end to resolve?

[devoncarew]

@leafpetersen, is this a blocker for alpha2? If no, I'll move it out of the milestone.

[leafpetersen]

Moving it out is fine.

[bwilkerson]

Closing as obsolete. Please re-open if that's not the case.

[leafpetersen]

This still seems to reproduce with bleeding edge analyzer.

[dgrove]

@bwilkerson - can you please move this to Dart2-beta3/beta4/stable, or remove the milestone?

[bwilkerson]

@MichaelRFairhurst Any update?

[MichaelRFairhurst]

I ran into merge issues trying to patch in the reverted CL since its pretty out of date at this point. Didn't get it working by the time I got derailed.

Thanks for the reminder, I'll get back on it.

[MichaelRFairhurst]

Just to give an update on the progress of this, I was able to merge the reverted change and update it to work for function types.

I did however just hit a stack overflow that will be tough to fix. Now the problem is that linking the summaries depends on instantiating to bounds, which requires linking summaries.

a.dart

class A<T extends B> {}

b.dart

class B<T extends A> {}

Since the infinite loop here is caused by typeA.instantiateToBounds() calling typeB.typeArguments calling typeB.instantiateToBounds(), I'm not sure I see a place where I can easily track & break this.

I will continue to work on it but may need to go over it with @scheglov

[eernstg]

In the example, B must have a simple bound (search 'We say' in this document), which is true if A has a simple bound, which is true if B has a simple bound, and it is specifically stated that such a circularity implies that the answer is 'No'. This means that the example has a compile-time error.

So the function that determines whether a given bound is simple should keep a set of bound declarations visited so far, and return false if at any point a bound declaration is visited which is already in that set.

Note that it is not sufficient to check whether you ever get back to the first bound, because you can have eventually-cyclic sequences like 1, 2, 3, 4, 3, 4, 3, 4, ..., where you never revisit 1.

So how does that fit into the current implementation, is there no such bool simpleBound(...)? Does it get lost in modular compilation and summaries and stuff? Otherwise, it should be doable, and hopefully not too costly. In a lot of cases it may be possible to avoid allocating an actual Set, which might be important; maybe it would be useful to call a depth-limited function first (that is naïve in that it ignores cycles), taking an int depth argument and bailing out at a fixed depth (say, 5), and then calling the expensive one that uses a Set if the naïve function couldn't conclude anything.

[MichaelRFairhurst]

Yeah, the problem is that in order to detect cycles, I need to inspect the A's type parameters. Behind the scenes, asking for the type parameters causes the bounds to be instantated to bounds. This causes it to loop infinitely if I ask for the type parameters of either type. There is likely some solution where I peek into which type I'm using, however, I'm not sure that's the best solution..

…

On Fri, Apr 13, 2018, 12:58 AM Erik Ernst ***@***.***> wrote: In the example, B must have a simple bound (search 'We say' in this document <https://github.com/dart-lang/sdk/blob/master/docs/language/informal/instantiate-to-bound.md>), which is true if A has a simple bound, which is true if B has a simple bound, and it is specifically stated that such a circularity implies that the answer is 'No'. So the function that determines whether a given bound is simple should keep a set of bound declarations visited so far, and return false if at any point a bound declaration is visited which is already in that set. So how does that fit into the current implementation, is there on such bool simpleBound(...)? Does it get lost in modular compilation and summaries and stuff? Otherwise, it should be doable, and hopefully not too costly (in a lot of cases it may be possible to avoid allocating an actual Set, which might be important). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28580 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABjWe0Spzz7JYCXkV2DVrlrs-cIFmRE3ks5toFqIgaJpZM4Ly7ot> .

[eernstg]

Before a bound B is instantiated to its bounds there should be a check that B has simple bounds. That should be enough to catch the cycle.

[MichaelRFairhurst]

Right, the problem is that the only way to check if B has simple bounds is to get it's type parameters. In getting it's type parameters, it runs background logic to deserialize from disk. And one step in that process is to instantiate the B's bounds (A in this case) to bounds. This causes A.instantiateToBounds to call A.instantiateToBounds before B.typeParameters returns a value. I could make a new method getTypeParametersWithoutInstantiatingBounds or something like that to get around this I think.

…

On Fri, Apr 13, 2018, 1:24 AM Erik Ernst ***@***.***> wrote: When a bound B is instantiated to its bounds there should be a check that B has simple bounds. That should be enough to catch the cycle. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28580 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABjWe0B8ihcVfxlbbAVKtJe-3PXfNYWIks5toGDYgaJpZM4Ly7ot> .

[eernstg]

I shouldn't try to guess my way through these things because I don't know the CFE implementation details. But it seems reasonable to say that the deserialization should check that bounds are simple, because it's apparently executing the instantiate-to-bound operation that shouldn't even be attempted. Maybe the CFE should attach some kind of metadata to each type variable indicating whether it has or does not have a simple bound, such that the answer comes directly, pre-computed, from the serialized data?

[chloestefantsova]

@eernstg I think @MichaelRFairhurst is talking about the implementation in analyzer, not CFE. Fasta is going to have the check for simple bounds soon (see CL 41264). The work on this check was postponed a while ago because of other higher priority tasks.

FWIW, instantiate-to-bound is run eagerly in fasta, and there's no need to run it whenever we want to access type variables.

[eernstg]

OK, thanks @stefantsov! — and it still stands that I should just stop guessing at implementation details. ;)

[MichaelRFairhurst]

a few hurdles overcome to get to: https://dart-review.googlesource.com/c/sdk/+/52062

[MichaelRFairhurst]

This change has landed, and hopefully it sticks!