Sdk: "Runtime check on non-ground type T may throw StrongModeError"

[Hixie]

I got the strong-mode analyzer message:
Runtime check on non-ground type T may throw StrongModeError

...but what does it mean? When may it throw that? Why? How do I fix it?

[vsmenon]

This is primarily relevant to DDC generated JS code, but it is a general soundness issue in Dart.

What this means is that we can't guarantee that the is check is sound. E.g., the following:

var list = ["hello", "world"];
print(list is List<int>);

prints true. list is actually considered a List<int> per the spec. Function type checks have similar problems.

In general, type checks on String or List are fine, ones on List<Foo> are problematic and usually a bug in the program.

For type parameters (T is one here?), we don't necessarily know at compile time which one it is. Do you have a pointer to your code?

[Hixie]

Isn't that a List<dynamic>?

Here are the actual errors I get, with pointers to the code online:

[hint] Runtime check on non-ground type T may throw StrongModeError (https://github.com/flutter/engine/tree/master/sky/packages/sky/lib/src/widgets/drag_target.dart, line 158, col 9)
[hint] Runtime check on non-ground type ValueKey<T> may throw StrongModeError (https://github.com/flutter/engine/tree/master/sky/packages/sky/lib/src/widgets/framework.dart, line 34, col 9)
[hint] Runtime check on non-ground type StatefulComponentElement<dynamic, T> may throw StrongModeError (https://github.com/flutter/engine/tree/master/sky/packages/sky/lib/src/widgets/framework.dart, line 113, col 9)
[hint] Runtime check on non-ground type T may throw StrongModeError (https://github.com/flutter/engine/tree/master/sky/packages/sky/lib/src/widgets/framework.dart, line 331, col 39)

[vsmenon]

Yes, but List<dynamic> can act as any List type.

So, it's both a List<dynamic> and a List<int> per the Dart spec (as far as is checks go). As well as a List<String> or a List<Map<Foo, Bar>>.

In general, list is List or list is List<dynamic> is a meaningful check. list is List<int> is usually a bug.

To take the first example on your list, if you do something like:

var state = new DragTargetState<List<int>>();
var result = state.didEnter(["hello", "world"]);

then didEnter may accept the argument and return true. The is check on line 158 will return true.

[Hixie]

Is the runtimeType of a List<dynamic> the same as the runtimeType of a List<int>? Or is it a superclass of List<int>? I'm trying to figure out what the relationship is. Is is special-casing , or is it just walking a class hierarchy that I can't see?

I guess I'm saying it depends on what the definition of is is.

It sounds to me like what you're saying is that the bug is using [] without explicitly specifying the type. I'd certainly support a warning that flagged any code that implicitly typed something using dynamic.

[vsmenon]

The actual runtimeType is different. The is check, however, is a subtype check - e.g., x is Object asks if x's type is a subtype of Object (which is alway true in Dart).

The funny thing about Dart's subtype relationship is that it is cyclic in a few corners. List<int> is a subtype of List<dynamic> _and_ List<dynamic> is a subtype of List<int>. Strong mode is basically disallowing the latter.

A no implicit dynamic would be useful, but perhaps not really usable until we have generic methods. (E.g., Iterable.map always returns Iterable<dynamic>.)

Another (complementary) possibility is to annotate classes like DragTargetState that effectively say the they can only be instantiated on 'safe' types (what we're opaquely calling ground types). So, no error on x is T inside the class. Instead, warn on new DragTargetState<List<int>>() but allow new DragTargetState<List>().

[Hixie]

What I'd ideally want is to allow DragTargetState to differentiate Foo<int> from Foo<String>, and disallow Foo<dynamic>. Is there a way to do that?

[vsmenon]

We might be able to make that work. To do it statically, we may need to annotate both DragTargetState and Foo to denote that DragTargetState requires a 'safe' type param and that Foo is always safe (no dynamics inside the runtime type). The latter might be something like:

@noDynamic
class Foo<T> { ... }

which would allow new Foo<int>() but disallow new Foo() or new Foo<dynamic>().

[Hixie]

DragTargetState can't know about Foo, and Foo can't know about DragTargetState. Foo could be List, for instance. Or some generic class from some random pub package.

I'm fine with waiting for generic methods if it would allow us to warn on implicit dynamics.

[Hixie]

Anyway, it sounds like this error message is actually useful (even outside strict mode). The bug is that it's very unclear what it means or what action should be taken to address it.

[jmesserly]

I believe @munificent recently fixed this?

[munificent]

I don't think so. The underlying behavior is unchanged as far as I know. I definitely think we can improve the error message, and I'd be happy to do that.

[jmesserly]

merging https://github.com/dart-lang/sdk/issues/26100, CC @hermanbergwerf

[jmesserly]

Now that more folks are hitting this, I wonder if we can add a document somewhere to describe why as List<int> and as int are treated differently. It's definitely something folks wonder about.

@hermanbergwerf -- the way to handle that in your example would be something like:

factory fromData(Map<String, dynamic> data) {
  List<int> field;

  if(data.containsKey('field') && data['field'] is List/*OLD: <int>*/) {
    // OLD: field = data['field'] as List<int>;
    field = new List<int>.from(data['field']); // NEW
  } else {
    throw new ArgumentError.value(data, 'data', "no valid data field for 'field'");
  }

  return new MyClass(field);
}

... the copy will ensure that each item is checked that it's actually an integer.

An alternative would be to supply a checked wrapper of some sort, e.g. new CheckedList<int>(list), there's another bug requesting that feature.

[munificent]

An alternative would be to supply a checked wrapper of some sort, e.g. new CheckedList(list), there's another bug requesting that feature.

I think @nex3 is working on a wrapper class like that for the collections package.

[nex3]

https://codereview.chromium.org/1840573002/

[eernstg]

I think it's worth considering where to put the blame. Vijay said

In general, list is List or list is List<dynamic> is a meaningful check. list is List<int> is usually a bug.

The underlying issue is that a reference of type List<int> xs justifies conclusions like "xs[0] has type int" (unless xs == null || xs.length < 1, of course, but then we'll be thrown out of the context where that question is relevant).

But if xs is allowed to refer to an instance of List<dynamic> then there is no such guarantee. The spec subtype rule for dynamic and type argument covariance conspire to allow this, presumably in order to let dynamic have an effect that may be described as "just make the type checker stop complaining". ;-)

From the point of view of soundness this is an expensive design choice. In particular, navigation in the object graph is unsound even in checked mode if we encounter an object whose runtime type is a reified-dynamic type (that is, a generic class whose type arguments include dynamic at any level, e.g., List<List<dynamic>>).

I'd like to be able to guarantee that such objects just don't exist, in which case xs is List<int> is not a bug, it's a perfectly appropriate runtime check which will justify conclusions like "xs[0] has type int". This means that generic types can be recovered (re-discovered) at runtime as needed, just like non-generic types (because, luckily, we don't have any objects whose runtime type is dynamic, so nothing will claim to have a non-generic type like MyClass unless it is an instance of a class that actually implements/extends MyClass).

Strong mode in the analyzer uses a different approach, where reified-dynamic type instances are allowed to exist, and dynamic type checks for instantiations of generic classes are frowned upon. This means, of course, that a List<dynamic> may be accessible via references of type List<int> xs and List<Object> ys at the same time, so when getting an element from xs it may be a String that someone added via ys:

List<Object> f(List<int> xs) => xs;

main() {
  List<int> xs = [];
  List<Object> ys = f(xs);
  ys.add("Hello, world!");
  int x = xs[0];
  print("$x");
}

This program has 'No issues' in strong mode analysis, and still it stops with a runtime error in checked mode.

It's because of this kind of phenomenon that strong mode cannot have exactly the same semantics as the one that the spec describes. I'd love to see strong mode and the spec converge, but it is not trivial (especially as long as we allow all these reified-dynamic instances to exist ;-).

[vsmenon]

@eernstg Just a quick note:

For soundness, strong mode still requires a runtime type check on generic class method arguments where the parameter type is a type parameter (or based on the type parameter).

In DDC,

ys.add("Hello, world!");

would fail at runtime. The alternative is to disallow covariance on generics, but that's a bigger change.

[eernstg]

Indeed, but you're not being entirely explicit about what is going on here:

If you want a runtime check to fail at ys.add("Hello, world!") when ys is an instance of List<dynamic> then you can't use the _actual_ value of the type argument to make that decision: A String certainly does have type dynamic. So you're not just referring to a normal checked mode check. It's not a check based on the static type, either: If you perform the dynamic check based on the type annotation of ys then we will simply require that the String is an Object -- and succeed.

So the only way you can get the run time failure you mention is by making sure that the given list is an instance of something else than List<dynamic> in the first place, and this relies on inference from the declaration of xs. So you'll let List<int> xs = [] mean List<int> xs = <int>[], and then you'll have an int list at the invocation of add.

This is the kind of thing I was referring to when I said 'cannot have exactly the same semantics'.

[Hixie]

For what it's worth, over in Flutter land, we don't really care that <dynamic>["Hello"] matches List<int>. (Now that we understand what's going on. I didn't when I first wrote this bug, but I have since been educated in the ways of generics in Dart.)

We're designing our framework to handle two cases:

• people who care very much about types and want everything to be typed and type-checked.
• people who don't care about types at all.

For people in the first category, we have configured the analyzer to complain about any missing types, and (once the analyzer supports this) this will include complaining about untyped literals. So there will never _be_ a List<dynamic> to conflict with unless the author is making a very specific choice, which can be an informed choice whose effects we would document.

For people in the second category, all their lists are <dynamic>. It's their job to make sure that they don't send the wrong kind of list to the our APIs. They are explicitly saying that they want to trade source-level brevity for greater responsibility.

We try to write our code to handle both. This means we don't do checks like "is this a List<int> or a List<String>" when we could be getting a List<dynamic>.

As a side note, for people in the first bucket, I'm totally fine with the analyzer and the runtime using types on the LHS to imply types on the RHS or vice versa. (I think I'd prefer explicit RHS types implying LHS types, but that's another story.) For people in the second bucket, though, there's no types at all in the source, and it's not ok to turn this:

   var xs = [0];

...into a List<int>. There's not enough information there to say if it's a List<int>, List<Object>, or List<num>. (Or indeed, a List<double> where someone has assumed that 0 is compatible with double, which is something we see a lot. But that's also another story.)

[vsmenon]

@Hixie could you please file a separate bug on this? There is a general question of how strong (magical?) inference should be for generic methods and generic class constructors (and list literals are just a case of the latter).

For people in the second bucket, though, there's no types at all in the source, and it's not ok to turn this:

    var xs = [0];

...into a List<int>. There's not enough information there to say if it's a List<int>, List<Object>, or List<num>. (Or indeed, a List<double> where someone has assumed that 0 is compatible with double, which is something we see a lot. But that's also another story.)

Edit: I believe the above - what you don't want - is our current behavior.

[Hixie]

Happy to file bugs -- can you clarify which of my various passive aggressive complaints in my last comment you'd like bugs for? :-)

[vsmenon]

We currently infer / instantiate:

var xs = [0];

as List<int>.

That means you'll get a static error if follow that with:

xs.add("hello");

I think you're objecting. :-)

[Hixie]

In strong mode? That certainly doesn't cause any problems today. :-)

[nex3]

Type-asserting wrapper methods have landed in collection 1.5.0.

[jmesserly]

Duping this against https://github.com/dart-lang/sdk/issues/28988 ... but the good news is, we're going to remove this error.

[jmesserly]

(also, that bug is about DDC runtime behavior, the analyzer message was removed quite a while ago)