Unnecessary Cast detected when removing it would cause an error.

[gspencergoog]

[Edit eernstg: I transferred this issue to the linter repo because it is yet another example showing that the advice given by 'unnecessary_cast' may be confusing.]

If I have the following code:

class Job {}
class Worker extends Job {}
class Group extends Job {}

void main() {
  List<Worker> workers = [Worker()];
  List<Group> groups = [Group()];

  Iterable <Job> all = (groups as Iterable<Job>).followedBy(workers);
  print(all);
}

Then on the line defining all, there will be a lint warning for an unnecessary cast, but if that cast is removed, then there is an error:

The argument type 'List<Worker>' can't be assigned to the parameter type 'Iterable<Group>'.

Because it's not exactly the right type.

This seems like the unnecessary cast lint is being too strict here, and should not fire in this case.

The code is probably better written like below, so perhaps there should be a separate lint to recommend it:

  Iterable <Job> all = groups.cast<Job>().followedBy(workers);

[eernstg]

@gspencergoog, I don't think this topic has been organized in a very strict manner, but it would probably be helpful if you file a similar issue in the linter repo at https://github.com/dart-lang/sdk/issues/new and label it as a false-positive. It is not very likely that this lint can (realistically) be changed such that it always avoids flagging a cast if the code has an error when the cast is removed, but it is useful to keep track of false positives that are encountered in real life because it makes us think about it. ;-)

... if that cast is removed, then there is an error

Right, but with the cast there is a run-time failure!

Uncaught Error: TypeError: Instance of 'JSArray<Worker>': type 'JSArray<Worker>' is not a subtype of type 'Iterable<Group>'

The reason for this is that groups has a run-time type that implements List<Group>, and this implies that its followedBy method expects an actual argument of type List<Group>, and the argument which is actually provided is a List<Worker>. Check out dart-lang/language#524 to see how this kind of failure can be detected at compile time rather than run time (you could even vote for it if you wish to support this feature ;-).

The variant that uses cast<Job> does not have this run-time failure, but it is in general unsafe to use cast<T> on a List<S> when T is a supertype of S, because that can create yet another situation where a dynamic covariance check fails:

void main() {
  var xs = <int>[1].cast<num>();
  xs.add(1.5); // Throws.
}

Here's a safe way to do it:

class Job {}
class Worker extends Job {}
class Group extends Job {}

void main() {
  List<Worker> workers = [Worker()];
  List<Group> groups = [Group()];

  Iterable<Job> all = [...groups, ...workers];
  print(all);
}

This is safe because the covariant requirement is enforced (that is, if we have an expression of type List<T> then the value of that expression is guaranteed to be an object whose run-time type implements List<S> such that S is a subtype of T). This ensures that the new list (created by [....]) will have a type argument which is a supertype of the run-time type of every element in the resulting list, without exceptions and without run-time checks. This type argument is chosen at compile time (with a declared type Iterable<Job> for the variable all it will be Job, and it would also be Job if we had used var all = ... and used type inference to obtain that type argument).

In any case, it is both syntactically and semantically useful to use the list literal and the spread operator (...), because it is quite readable, and methods like cast and followedBy have the potential to cause run-time type failures.

[gspencergoog]

Wow, thanks for the excellent explanation.

The safe way also seems pretty clear, even clearer than the cast function.

Is there already an analyzer lint for cast<T>() to recommend the spread operator instead?

I'm happy to also file a separate issue, but I don't have the access to add labels, so it would basically just be a duplicate of this one. :-)

[eernstg]

Thanks for the kind words!

Is there already an analyzer lint for cast<T>() to recommend the spread operator instead?

I don't think it would work very well, it's probably going to be quite difficult to detect the cases where it's useful to use the spread operator rather than an invocation of cast.

The situation where cast works really well is when we have a List<T> that (for some reason which is well-understood and meaningful in the application context, such that we can actually rely on it) only contains elements of type S, where S is a subtype of T (and different from T). If we want to pass that list to a receiver who wants a List<S> and we actually pass the List<T> then it is just going to be a type error (at compile time or run time, depending on the static types).

However, all the elements are OK, so we'd very much like to pretend that the list is a List<S>, because we don't want to create an entirely new list and copy all the elements; cast<S> will handle that.

void wantsAnIntList(List<int> ys) {
  for (int i in ys) print(i);
}

void main() {
  List<num> xs = [1, 2, 3]; // This is a `List<num>`, filled with `int`s.
  // wantsAnIntList(xs); // Compile-time error.
  // wantsAnIntList(xs as dynamic); // Run-time error.
  wantsAnIntList(xs.cast<int>()); // OK, at compile time and at run time.
}

With cast, it is still possible to get a run-time type error (if there are some elements that aren't of type S after all), but we're taking that risk because we have reasons to think that it is safe, and we want to avoid the copy operation.

I'm happy to also file a separate issue, but .. it would basically just be a duplicate

No problem, the important part is that its reported as a lint issue. But I can transfer this one and put on the label.

[srawlins]

What version of Dart are you running, Greg? The analyzer should not be reporting this any longer.

[gspencergoog]

I was using Dartpad, which is at Dart 3.2.3. Indeed, when I'm using 3.3.0-244.0.dev locally, I no longer get the warning.

Sorry, I should have checked the latest build before filing... Even so, I learned something.

Feel free to reopen this if you want it around to track something else.