`unnecessary_cast` quick-fix unexpected result in switch expression

[FMorschel]

trafficstars

Describe the issue When using the quick-fix for unnecessary_cast in a switch expression the removed type cast is the outer one but should be all the inner ones.

To Reproduce

T a<T>(int x, bool test) {
  return switch (x) {
    0 => 0 as T,
    1 => 1 as T,
    _ => throw UnimplementedError()
  } as T;
}

Expected behaviour

T a<T>(int x, bool test) {
  return switch (x) {
    0 => 0,    // Removed cast here
    1 => 1,    // Removed cast here
    _ => throw UnimplementedError()
  } as T;
}

Additional context This should be taken into account because it is way more verbose to write the cast in every line than at the end.

[FMorschel]

Alternatively, the assist to cast a switch statement to a switch expression could move the casts. If the reviewer agrees, I can create a new issue if necessary.

[srawlins]

This one's pretty tricky. How things are implemented now, the inner two casts are not unnecessary because 0's static type is different from T and 1's static type is different from T. So those casts do something, and they can fail at runtime. Then when we've gotten to the type of the switch expression, it is computed to be T, so that outside cast is unnecessary.

But I think broadly your issue raises a valid UX improvement. A list literal is a similar example:

List<int> f(Object a, Object b) => [a as int, b as int] as List<int>;

The outer most cast is considered unnecessary. So we offer a fix to remove it. But we could offer an additional fix to tidy things up internally as well. A fix to "make the cast necessary" by removing inner casts.

Actually, casting a List with a new type argument is dangerous. Maybe better examples would be:

int g(Object a, Object b) => (1 == 2 ? a as int : b as int) as int;

int h(Object? a, Object b) => (a as int? ?? b as int) as int;

[dart-github-bot]

Summary: The unnecessary_cast quick-fix in switch expressions removes only the outer cast, while it should remove all inner casts, leading to more verbose code. The expected behavior is to remove all unnecessary casts within the switch expression.

[FMorschel]

https://dart-review.googlesource.com/c/sdk/+/389423

[srawlins]

WDYT, @bwilkerson @scheglov ? This one makes me a little squirmy, because it feels unpredictable. If we stick to immediate inner casts, that's a little better for me. But it kind of introduces an inconsistency. Like for conditional expressions, take:

int f() {
  return (1 == 2 ? a as int : 2 == 3 ? b as int : c as int) as int;
}

I imagine we would only offer to un-cast the a as int cast. If we would instead want to remove inner casts that are arbitrarily deep... ooh. Seems very bug-prone, hard-to-define, arbitrary (would we have a set of expression types that we go into, and others we don't?).

An example of the "arbitrary" issue. I'm not sure what immediate expressions we should look into, even not considering going deep (but then the same question applies to going deep, if we want to go deep). So here's another perfectly valid example, like the top one in the issue here, and I think I've seen something like this in real life:

int f<T>(List<T> list) {
  return list.fold(
    list.first as int,
    (value, e) => value + (e as int),
  ) as int;
}

Again the outermost as int is only unnecessary because the other as int casts are present. But one an argument in an argument list, and the other is in a binary expression (+), in an expression function (=>) in an argument.

We don't have to come up with a perfect list of cases before we land a quick fix; it's not a "breaking change" to offer quick fixes in more cases. But in order for the quick fix to carry its weight, I think we need a few cases (definitely more than 2; we've got examples of like 6 so far), and we probably need examples that don't feel super contrived. I think I've seen this list.fold example in real code, but I certainly cannot figure out why I've had code that needed (dangerous) casts like this. Tricky.

[FMorschel]

I imagine we would only offer to un-cast the a as int cast.

I've tested it with the current CL and this is the current behaviour. Both b and c still keep their casts. If the expression was something like:

(1 == 2 ? a as int : (2 == 3 ? b as int : c as int) as int) as int;

With another condition that was casted yet again, the bulk fix (we could discuss removing it as well, I don't actually expect lots of code like this) would make it into:

(1 == 2 ? a : (2 == 3 ? b : c)) as int;

About the list, I'm not sure how we could do something that would handle it but it's probably doable. Althoug I'd lean on teaching the user that something like this is possible (not sure how to do that here but it would be way better):

int f<T>(List<T> list) {
  return switch (list) {
    List<int> list => list.fold(
        list.first,
        (value, e) => value + e,
      ),
    _ => throw ArgumentError('Invalid list type'),
  };
}

[bwilkerson]

For quite a while now I've been leaning toward having smaller more composable pieces of functionality. There is a drawback, in that users often have to take more steps to accomplish their bigger goals, but the composability aspect means that it's often possible to support more user needs with fewer resources.

If I were to apply that perspective to this problem I would say that the quick fix for an unnecessary_cast should only remove the unnecessary cast, but that we should instead have an assist that will take a language structure (a switch expression, conditional expression, list literal, and maybe others) and hoist the type promotions to a higher level.

In the original example, after the outer as is removed, notice that the right-hand side of every case is either cast to T or has type Never, which would allow the cast to be moved outside the switch expression. The same operation would apply to the conditional expression examples.

But I would say that it should work on one level at a time. In the case of the nested conditional expressions it should work only on the innermost conditional expression (that is, it shouldn't try to figure out that it's possible to convert the outermost and innermost simultaneously). If it's applied to the innermost expression then it would become available for the outermost.

This approach would be simple to understand, predictable in the way it works, and allow the greatest amount of flexibility and control for users.