Different inference with = null, = [null], = {1: null}

[matanlurey]

Forked from an internal discussion between @MichaelRFairhurst and @jonahwilliams.

It looks like this infers x as dynamic:

main() {
  var x = null;

  // In DDC/Dart2 runtime: List<dynamic>
  print([x].runtimeType);
}

But both of these infer x as Null:

main() {
  var x = [null];

  // In DDC/Dart2 runtime: List<Null>
  print(x.runtimeType);
}

main() {
  var x = {1: null};

  // In DDC/Dart2 runtime: Map<int, Null>
  print(x.runtimeType);
}

Our guess is this is likely never what the user intends, however. @leafpetersen - thoughts?

(@MichaelRFairhurst also brought up var x = (() => null)();)

[matanlurey]

To make it clear, this is with DDC, so it is possible the CFE is different.

[matanlurey]

EDIT: Jonah clarified this occurs on the standalone VM with --preview-dart2 as well.

[jonahwilliams]

This is also on the dart vm with --preview-dart-2.

[matanlurey]

To be fair, what about:

class Container<T> {}

main() {
  // I assume we expect this to print `Container<Null>`, right?
  print(new Container(null));
}

[jonahwilliams]

The issue is that to me, and probably a lot of other users, the value null is only loosely connected to the type Null. Setting something to null says "this has no useful value right now". Typing something as Null says "this will never have a useful value". And in the case where it is not explicit, inferring the narrowest possible type doesn't seem to be useful.

[eernstg]

tl;dr We could lint mutable lists/maps with implicit type argument Null rd;lt

One case to keep in mind is this one:

void foo<T>([List<T> xs = const []]) {}

where the default value const [] cannot be a List<T> (because that is not a compile-time constant expression), but it can (and will) be a List<Null>. Being empty and immutable, it is not a problem that the list cannot contain anything else than null and we could even have used a bottom type—in Scala they call it Nothing, and that's exactly what it is: nothing has that type, and that's OK because there isn't anything in that list.

So the type argument Null can actually be the right choice for some empty/default/trivial collections, and they do come up in practice.

However, it would make sense to have a lint for a mutable list/map literal whose type argument is inferred to be Null. Basically, when there's nothing other than null in a list (or among the values in a map) the developer ought to choose the element type explicitly, now that the developer has refused to give any useful information via the initial contents of that list (map).

It wouldn't be hard to take another route and make the inferred type argument dynamic in that specific case, but that looks like a quirky exception to me, and it might very well cause problems later on if that object is passed on as a List<S> (or Map<..., S>) for some S which is not a top type.

[matanlurey]

@eernstg That's a good point about mutable/immutable lists/maps, except:

// Now we have a mutable List<Null> again :)
final list = const [null].toList();

[jonahwilliams]

@eernstg inferring dynamic doesn't seem that surprising to me, but that could just be my Dart 1 bias. It seems more consistent with inference for fields/locals. In cases where dynamic is unwanted, I think the no-implicit-dynamic lint works well, not sure if we want a no-implicit-Null too.

[eernstg]

tl;dr Could again be: We might lint mutable lists/maps with implicit type argument Null rd;lt ;-)

@matanlurey, with

final list = const [null].toList();

I just can't help thinking: "It's not unreasonable for an inference procedure to decide that you want a List<Null> here". ;-)

But I guess you're saying that it would be more useful if mutable collections with inferred type argument Null would in general have that adjusted to dynamic? Or we would use dynamic in all situations where Null has been inferred as a type argument of a fresh object, and that object is mutable in some sense? Even that might not help for the list above, though, because toList does not take a type argument. Maybe we could add .reType<dynamic>() to the initializing expression via some kind of desugaring, but how would the language know that toList creates a new object, and it's mutable?

Moreover, it's a two-edged sword: Such a List<dynamic> would accept all kinds of objects (so it would "just work" as a data sink), but it wouldn't work as a List<T> for any T which is not a top type (so it would not pass as a suitable data source for any consumer who has any non-trivial expectations for the kind of data delivered).

It's really not obvious to me that you always want the List<dynamic> in this kind of situation, it's more like we want a way to warn developers that they just don't want inference in this situation, because they're actively setting up inference to produce a less-than-perfect result!

[eernstg]

@jonahwilliams, I agree that we don't want a --no-implicit-X for all X. ;-)

[matanlurey]

@eernstg I think --no-implicit-dynamic could include Null: https://github.com/dart-lang/sdk/issues/33119

(Of course, the flag name needs to be better, i.e. --strict-implicit-types).

[MichaelRFairhurst]

Hmm, looking at this ticket again, I think it could use a better explanation of the fallout from picking Null.

For instance, if we take:

var x = [];
for (var y in list) {
  x.add(y);
}

then having a "no-implicit-null" is a matter of getting better error localization. Currently you'll get an error on line 3 about some type not being assignable to null, and that gets to @jonahwilliams's point about Null being a fairly unintuitive type. A new user will probably have an issue figuring out that they need to change line 1. So, using "no-implicit-null" (or some other flag that includes that behavior) will point the user in a better direction in this case, and hopefully they can figure out that they need to write <Foo>[]. Unfortunately, I'm not sure either error case is great for users.

In a more complicated case it can get worse though without implicit null.

List<...> getList() {
  var x = [];
  if (...) return x;
  return ...;
}

void useList() {
  var x = getList()..add(...);
}

I'll first make a case for why this is extra bad. In this example, we have a case for making generics invariant. List<Null> will be a valid subtype (even at runtime) of List<...>, even though its used in this case contravariantly for some condition. Users will here not encounter any error, of any locality, until runtime in that condition, and then they'll get an error "... isn't a subtype of Null" with even less ability to figure out why you got a List<Null> in the first place. Assuming a world where --no-implicit-null doesn't exist, or isn't turned on, this should have been caught where the List<Null> was returned as a List<...>. Then we would be back to having a compile time error which could have had better locality, but it won't have as terrible of locality and it won't be a runtime error.

I will now say though why its not necessarily a big deal: most people would never write

var x = [];
...
return x;

unless they had some branch in which they first put items in x. Assuming they don't put items in x, you'd be much much much more likely to write return []. In that case, I do believe type inference would give you a List<...> and not a List<Null>. And if there is a branch where items are added to x, then we once again get a static error.

var x = [];
if/for/while (...) {
  ...
  x.add(...); // static error
}

So I think its important to frame this problem in terms of its end effect. And I think the end effect will in the vast majority of the time be limited to "that error could have been better" rather than "I can't catch a certain class of bugs." FWIW, there are other ways to make the error better which maybe don't depend on a flag. One hypothetical error message:

Foo is not assignable to the inferred type Null (inferred on line 1 because list was empty. Add a <type> declaration to override inferred type).

I think @jmesserly has done work to provide these types of error messages, though it would require changes to the IDE experience (because they often take multiple lines) and I imagine that POC was not implemented in the CFE.

I welcome other examples of how inferring Null can kind of snowball into a bigger issue, because if I'm missing cases where it can, then that would help us figure out what the best solutions are and how important it is that we find such solutions!

[matanlurey]

@MichaelRFairhurst I think https://github.com/dart-lang/sdk/issues/33119 is the actual problem here