unnecessary_cast quick-fix unexpected result in switch expression

[FMorschel]

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'),
  };
}