Unpredictable missing strong mode errors when analyzing across files.

[leafpetersen]

With the following three files:

// tobounds_a.dart

class S<T> {}
class Remote<T extends S> {
  List<T> get listOfS => [];
  List<S> get listOfS2 => [];
}

//tobounds.dart

import 'tobounds_a.dart';

class Local<T extends S> {
  List<T> get listOfS => [];
  List<S> get listOfS2 => [];
}

class B {
  Remote a;
  Local b;
  void test() {
    List l = [];
    a.listOfS.addAll(l); // should be warning
    b.listOfS.addAll(l); // should be warning
    a.listOfS2.addAll(l); // should be warning
    b.listOfS2.addAll(l); // should be warning
  }
}

//tobounds_client.dart

import 'tobounds_a.dart';
import 'tobounds.dart';

var x = new B();

Analyzing tobounds.dart directly in strong mode yields three of the four expected errors:

[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 14, col 22)
[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 15, col 23)
[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 16, col 23)
3 warnings found.

Analyzing tobounds_client.dart in strong mode yields all four of the expected errors:

[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 13, col 22)
[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 14, col 22)
[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 15, col 23)
[warning] Unsound implicit cast from List<dynamic> to Iterable<S<dynamic>> (/usr/local/google/home/leafp/tmp/tobounds.dart, line 16, col 23)
[hint] Unused import (/usr/local/google/home/leafp/tmp/tobounds_client.dart, line 1, col 8)
4 warnings and 1 hint found.

Removing the unused import in tobounds_client.dart causes the fourth expected error not to be reported again.

[leafpetersen]

I suspect that this is related to strong mode instantiating to the bounds of the generic, rather than to dynamic. That is, in standard dart, the Remote and Local fields would be treated as Remote<dynamic> and Local<dynamic>, whereas in strong mode we instantiate to the bound S instead. It looks like for some reason Remote is still being instantiated to dynamic but Local is being instantiated to S.

When I reproduce this in Intellij, what I see is that the initial analysis does not report the expected error, but if I edit the body of the test method, the re-analysis changes the result and surfaces the error (the intellij reproduction was done with the original source code from which this example was derived).

[leafpetersen]

Originally reproduced in 1.16.0-dev.4.0 , but reproduces in 1.16.0-dev.5.0 .

[scheglov]

When ResolveUnitTypeNamesTask uses TypeResolverVisitor to analyze tobounds.dart and finds Remote without type arguments, it attempts to perform instantiateToBounds. But at this point Remote is not processed by ResolveUnitTypeNamesTask yet. So, T in Remote does not have its bound set yet.

Actually we don't even need to have multiple files.
It's enough to put Local declaration after its usage.

main() {
  Local a;
  List l = [];
  a.listOfS.addAll(l); // should be warning
}

class S<T> {}
class Local<T extends S> {
  List<T> get listOfS => null;
}

Or, in general case, types depend on the order of their declarations.

class A {}
class B<T extends A> {}
class C<T extends B> {}
class R<T extends C> {
  List<T> get listOfS => null;
}

main() {
  R a;
  List l = [];
  a.listOfS.addAll(l); // should be warning
}

Gives [warning] Unsound implicit cast from List<dynamic> to Iterable<C<B<A>>> (/Users/scheglov/dart/test/bin/test.dart, line 12, col 20).

But if we move B below, the types and the error message is different.

class A {}
class C<T extends B> {}
class R<T extends C> {
  List<T> get listOfS => null;
}

main() {
  R a;
  List l = [];
  a.listOfS.addAll(l); // should be warning
}
class B<T extends A> {}

Gives [warning] Unsound implicit cast from List<dynamic> to Iterable<C<B<dynamic>>> (/Users/scheglov/dart/test/bin/test.dart, line 11, col 20).

@bwilkerson
It seems that to perform instantiateToBounds always correctly we need to resolve type bounds in the correct order. I think it might be too expensive to do.

[bwilkerson]

Yep. I discovered that yesterday while looking at #26310, which has the same underlying cause. It (only) happens in strong mode because we're using the bounds for inference.

I was thinking that you were right, that it will be somewhat expensive to fix because it would means that we would have to resolve the type bounds in dependency order. But then I thought of an interesting case that might make even that insufficient. Consider the following:

class A<G extends B> {}
class B<H extends A> {}

What is the inferred type for

A x = null;

If we infer the bounds of A, that would make it A<B>, but if we infer the bounds of B, it becomes A<B<A>>, and we'll never finish. @leafpetersen How do we want to define this case?

But assuming that we define reasonable behavior for this case, we might be able to simplify the implementation by delaying the inference of type arguments until after all the bounds (for every imported unit and the transitive closure of their exports) have been computed.

[leafpetersen]

Oh, that's a fun one! I think it would possible to make it an error and detect it - it would look something like typedef cycle detection. So when you get to A<B<A>> and ask what the expansion of A is, you make it a circular type error.

An alternative would be to say that in bounds, extends A means extends A<dynamic>, which in this case would then generate an error since dynamic does not extend B. This feels a bit ad-hoc though. I wonder if there's something in between the two?

Yet another alternative would be to forbid implicit instantiation in bounds for things with a non-default bound.

I'd suggest filing a separate bug for the language issue, and in the meantime seeing if there's something pragmatic we can do to fix the immediate issue. For example, if it would be possible to implement a version which does the right thing for top level uses, and instantiates all uses in bounds to dynamic, then this would at least put us in a stable position. If doing this is pretty straightforward, I'd suggest doing this for now. If doing even this much requires substantial changes that might have to be backed out if we do something else, then perhaps we should discuss more first.

[leafpetersen]

@scheglov @bwilkerson Can one of you take this and get it to a state that at least unblocks @ochafik ?

I suspect that as long as we get top level uses correct, corner cases like uses of implicitly instantiated parameterized types as upper bounds can be left open for now. Let's be sure and keep a tracking bug open for any missing functionality though.

[scheglov]

I'm working on it.

[leafpetersen]

Per offline discussion, we are going to look into fixing this for occurrences outside of bounds now, while making uses inside of bounds be errors. We will look into how to handle the case for things inside of bounds later.

class C<T extends num> {...}

// the bound in the extends clause here will be treated as C<dynamic>, and hence
// the bound will be treated as erroneous.
class D<T extends C> {

}

void main() {
  C c; // this will be reliably interpreted as C<num> 
}

@ochafik If you need to make progress in the meantime, you can explicitly instantiate instead of relying on strong mode to fill it in:

// strong and regular mode
Rectangle<num>
// strong mode only
Rectangle<dynamic /*=num*/>
// I think this syntax works as well for strong mode only?
Rectangle/*<num>*/

[scheglov]

https://codereview.chromium.org/1927323002

[leafpetersen]

@scheglov is this resolved? Can I close this?

[scheglov]

The problem in analysis is resolved.
I wasn't sure whether I should close it or not though.
We don't perform instantiateToBounds in AST based summaries.