Dart should warn on guaranteed cast errors

[matanlurey]

(I think this would significantly help Flutter, Angular, and all Dart2 users)

Best explained through an example:

void giveMeListOfString(List<String> names) {}

void main() {
  // "Effectively" final List<dynamic>
  var names = [];
  if (someFlag) {
    names.add('Matan');
  }

  // Statically does *not* accept List<dynamic>
  giveMeListOfStrings(names);
}

This line:

giveMeListOfStrings(names);

... will never succeed, statically. It is a runtime error in Dart2, however, because of implicit downcasts. That's a much harder problem, and might require additional flags and such. I think we could catch a good number of "local" downcast errors in the analyzer:

1. If the variable is definitively a Generic<T> where T is dynamic
2. And is passed in somewhere expecting T != dynamic

... issue a warning or lint or whatever:

// LINT: local_downcast_will_fail_at_runtime
giveMeListOfStrings(names);

This might remind you of @Hixie's bugs around List<Widget>. That is intentional :)

... in conjunction with a "quick fix", it could suggest rewriting names as <String> [].

[matanlurey]

This could also be a --strict addition, i.e. --strict-static-cast-errors, or related.

I totally understand why Dart2, the specification, does not allow giveMeListOfStrings(names) to be an error, because you need to be able to write code like:

try {
  giveMeListOfStrings(names);
} on CastError {
  // ...
}

For example, for:

void main() {
  var x = [];
  useListOfString(x);
}

void useListOfString(List<String> _) {
  print('Yay!');
}

Dart2JS emits, roughly:

main: function() {
  H.assertSubtype([], "$isList", [P.String], "$asList");
  H.printString("Yay!");
}

But given that I think 99.99%+ of users do not want a CastError (or TypeError), I wish we could emit a warning/info/lint when we know one will be generated.

[eernstg]

Note #33307, 'Exact types': A situation where a cast is guaranteed to fail is also often a situation where some relevant expression may be given an exact type.

[matanlurey]

Thanks @eernstg. To be clear, I hope this isn't gated on changing the specification :)

(Call it a "warning" or a "hint" or whatever nomenclature lets me have it sooner!)

[eernstg]

I can see the point, but if we don't know which types will be considered exact then we may not know how to deal with some concrete situations. Say, how about the return type of a local function declared as foo() => <num>[];, should that make List<int> x = foo(); an error? Considering that updates from Dart 2 and onwards should be non-breaking (for quite a while, at least ;) it might be a good idea to handle this in a lint.

[matanlurey]

I can see the point, but if we don't know which types will be considered exact then we may not know how to deal with some concrete situations.

Honestly for most cases I see users hit this, just considering the precise/exact/simple cases (i.e. doesn't require global inference) is a huuuuuuuuge benefit. I'd hate to try and solve 100%.

[eernstg]

We can't hope for completeness because the underlying problem is undecidable, but we must be able to tell precisely which expressions have an exact type. Focusing on literals (including function literals) at first would make sense, but constant expressions have an exact type as well, even when accessed via one or more declared names, so it isn't even obvious which cases are "easy" to be included in a first round. In any case, any new error will conflict with the intention to stay away from breaking changes for quite a while, so it would have to be a lint.

[matanlurey]

Yes, the request was for a lint or hint or warning not an error 👍

[denniskaselow]

Thanks @matanlurey for pointing me to the correct issue.

I just want to add an example, where this caused a runtime error because the analyzer did not complain:
When this change was made in package coverage the code changed from void mergeHitmaps(Map newMap, Map result) to void mergeHitmaps( Map<String, Map<int, int>> newMap, Map<String, Map<int, int>> result).
Meanwhile, in dart_coverage it was used like this:

Map<String, dynamic> hitmap = {};
mergeHitmaps(createHitmap(reportFile), hitmap);

resulting in a runtime exception of type '_InternalLinkedHashMap<String, dynamic>' is not a subtype of type 'Map<String, Map<int, int>>'. And this in turn was one of the reasons why for example the linter package apparently didn't get any coverage reports for a month (the error also wasn't reported, because the linter build doesn't have the flags on dart_coveralls set to output this error; so it wasn't even obvious why it failed).

In my opinion this should be reported by the analyzer by default as it fails on runtime anyway and can never be successful so it should not require any flags. I can get a warning by using

strong-mode:
    implicit-casts: false

but this will also add warnings to places that at least have a chance to succeed -> all paces where the parameter itself is dynamic and not one of the type parameters.

[eernstg]

@denniskaselow, note that this situation could not give rise to a 'guaranteed cast error':

Map<String, dynamic> hitmap = {};
mergeHitmaps(createHitmap(reportFile), hitmap);

Surely, the problem could have arisen because hitmap used to work just fine as a Map<String, Map<int, int>> (in Dart 1) when it was created as a Map<dynamic, dynamic>, but when dynamic is a top type and it gets created as a Map<String, dynamic> (as in Dart 2), we get the run-time error that you mention. So I can see how this problem would arise as a surprise during the switch from Dart 1 to Dart 2, and why it might be difficult to track down.

However, it cannot be a 'guaranteed cast error'; it's just a regular downcast, and unless you want to flag all downcasts (cf. --no-implicit-casts and/or #31410), there is no particular reason why this downcast should be guaranteed to fail—in particular, hitmap is mutable so we might have assigned it as follows at some point: hitmap = <String, Map<int, int>>{};, and the cast would not fail.

Maybe the cure would have two steps: (1) detect that hitmap may be constant such that the developer can make it so; (2) detect that the downcast is guaranteed to fail (because that's true when it is constant and the type can hence be exact). It may not be possible to do (1) automatically because there would be way to many false positives or false negatives to make any such lint useful, but it could perhaps be done semi-automatically by searching for empty maps or somesuch, and otherwise just by making more declarations const whenever possible.

[matanlurey]

@eernstg:

but it could perhaps be done semi-automatically by searching for empty maps or somesuch, and otherwise just by making more declarations const whenever possible.

I don't think you need to go that far. For any final local variable, it's safe to assume right now.

For any mutable (var), you'd have to do (local) flow-analysis to assure that the variable is "effectively" final. That might be cumbersome, and maybe the answer there is to ask users to make all effectively final local variables final to get extra static-checking.

[eernstg]

For any final local variable, it's safe to assume right now.

Ah, that's right: If a final variable (local or not) with no type annotation is initialized by an expression whose type has some kind of exactness, the variable itself can be given the same type including the exactness. But if it's constant then we always know the actual value, which in particular means that its type is exact.

For the 'effectively final' stuff, I hope we can introduce something like x := e which would declare x as a final local variable whose type is the type of e, initializing it with the value of e. Maybe we can then persuade developers to use a final local rather than a mutable one whenever possible. ;-)

[asashour]

Is this issue still valid?

The below triggers argument_type_not_assignable hint.

Should a fix be offered to have it as <String>[]?

void giveMeListOfStrings(List<String> names) {}

void f(bool b) {
  // "Effectively" final List<dynamic>
  var names = [];
  if (b) {
    names.add('Matan');
  }

  // Statically does *not* accept List<dynamic>
  giveMeListOfStrings(names);
}

[eernstg]

I'll close this issue: The original example relies on implicit downcasts, and they have not been present in the language for any other source type than dynamic for a while (so dynamic d = 1; int i = d; is allowed, but List<dynamic> list = []; List<String> strings = list; is a compile-time error, as you mention).

@matanlurey, please reopen or create a new issue if you think there a substantial part of this issue that hasn't already been handled by more recent language updates.