Type literals in switch cases are confusing.

[lrhn]

When you start switching over sealed type hierarchies, it's incredibly easy to write:

   case TheType: ....

when you mean to do a type check for TheType. Instead it compares to the Type object for the type literal TheType, and fails. With luck, you're told that the switch isn't exhaustive, but you still have to figure out why. (I'm guessing that we'll get warnings about unrelated types eventually, which will make it easier to spot the problem.)

I did this, as recently as yesterday.
Others do it too, like a stack-overflow question.

Very little switch-pattern code has been written, and it already crops up, so I fear it might be serious useabilty stumbler.

Maybe we should protect people against this by disallowing type literals as constant patterns without a leading const.
At least that will lead people to do either case TheType _:/case TheType(): or case const TheType:, depending on which one they want.

The big issue is that then existing switches over Type objects will need migration. I don't know how many such switches are, but probably non-zero.

(We could also change the meaning of case TheType: to be case TheType _:, but that risks being confusing in the other direction. If we just prevents the syntax now, it be possible to give it back later in either case.)

[rubenferreira97]

I am already familiar with the syntax case TheType _: / case TheType(): and still occasionally make this mistake. I don't think newcomers will easily understand this behavior:

Instead it compares to the Type object for the type literal TheType, and fails.

I am almost certain that this use case will rarely be used in "client" code compared to type checking, and because of this, I am in favor of Dart changing the meaning of case TheType: to case TheType _:, or providing a more explicit type check operation and prohibiting case TheType: altogether (maybe allow case const TheType: like @lrhn said). This change will save a lot of headaches, Stack Overflow questions, and GitHub issues. In my opinion, it really hurts the developer experience.

Here are some syntax alternatives:

num number = 3.0;
switch (number) {
    case is Int:
    case is Double:
}

// if issue dart-lang/linter#2090 I like this for checking the Type object
Type<num> type = int;
switch (type) {
    case is Type<int>:
    case is Type<double>:
}

// or without explicit syntax (is) for type checking

num number = 3.0;
switch (number) {
    case int:
    case double:
}

Type<num> type = int;
switch (type) {
    case Type<int>:
    case Type<double>:
}

dart-lang/linter#2090

[lrhn]

Disallowing case Foo: is the minimal step needed to avoid problems. (And adding an error message telling you which options you can change to.)

I don't think there is any need to add new syntax, we already have two ways to do either operation:
case Foo _: or case Foo(): for the type check, and
case const Foo: or case == Foo: for the Type-object comparison.

Adding a third syntax for type check isn't adding much.

I'd love for case Foo: to do a type check, but that's dangerous because it's not backwards compatible.
You used to be able to write case Foo: to actually compare to the Type object, and keeping the existing syntax valid, to minimize migration, was one of the reasons for the current design.

But maybe the problem will go away if we get a good warning for unrelated-type patterns,
possibly with an extra special message if the unrelated type is Type.
It's the silently accepting case Foo: today that really hurts.

[munificent]

Very little switch-pattern code has been written, and it already crops up, so I fear it might be serious useabilty stumbler.

I have been worried about this exact footgun for basically the entire time I've been working on the proposal. I know you and I have talked about it and I've had long conversations about it with Leaf. I was never able to come up with a great solution. :( Other languages sometimes address this by having different naming rules for types and values, so they can say a capitalized identifier is always a type test. But we are cursed with int, num, bool, and double.

Maybe we should protect people against this by disallowing type literals as constant patterns without a leading const.

Instead of const, I'd suggest they use == patterns, since I think it's a little clearer then that you're matching for equality on the type object:

switch (someType) {
  case == int: print('int type');
  case == String: print('String type');
}

Either way, I like this idea.

The big issue is that then existing switches over Type objects will need migration. I don't know how many such switches are, but probably non-zero.

I believe I've heard anecdotally that switches on type objects are a fairly common pattern in some kind of code? Maybe something to do with tests or factories? I can't recall.

(We could also change the meaning of case TheType: to be case TheType _:, but that risks being confusing in the other direction. If we just prevents the syntax now, it be possible to give it back later in either case.)

We've discussed this. The problem is that it doesn't compose. We could make case int: work, but then case (int, String): would not do what you expect, unless we do something really weird and special case record literals containing type literals too. But then also, you can't write case List<int>. In general, collapsing the type annotation and expression grammars is just fraught with peril.

How about we add a lint for using a type literal as a switch case with a quick fix that puts == before it? Thoughts, @pq?

One thing that's somewhat helpful: If you're trying to type test on a sealed type and you accidentally do a type literal instead of a type test, you'll get an error that the switch isn't exhaustive.

[lrhn]

If we disallow type literals as patterns, then the record pattern (int, String) would be disallowed too, because of its two nested type literals patterns.

If we give precedence to parsing a type as a pattern over parsing an expressions, then int would be a pattern mathcing int, and so would (int, String) be a pattern matching an int-string pair.

Treating a type literal as TheType _ would turn the record pattern (int, String) into (int _, String _).

If we do nothing, then int is a type literal, and (int, String) is a record pattern matching two type literals, just like (1, "2") would match two specific other values.
And I'll make the mistake again.

dart-lang/linter#2393!

[pq]

I worry about this footgun a bit too. Code-completion could be possibly get tuned to nudge people away from the mistake. Currently we make this way too easy:

(fyi @bwilkerson)

But even that is only small measure. Diagnostics seem like a good way to go.

How about we add a lint for using a type literal as a switch case with a quick fix that puts == before it? Thoughts, @pq?

On the surface this sounds like a good idea to me.

Is this correct?

BAD

  switch(o) {
    case int:
    case String:  
    case (String, int):
  }

GOOD

  switch(o) {
    case == int:
    case == String:  
    case == (String, int):
  }

[munificent]

I think we've settled on leaving the feature as specified and trying to route around the footgun with lints: dart-lang/sdk#59087

[lrhn]

We haven't actually created the lint, so I think it's too soon to close this out.

It's a significant foot-gun in our syntax.
People, consistently, write patterns like {"foo": int} or ("A", int), and do not understand why it doesn't do what they think it should do.
A lint is the minimum needed to help those people, before they turn to stack overflow and asks "why does this pattern not match".

I actually think it should be an on-by-default warning instead of a lint.

[rrousselGit]

Shouldn't a lint for checking unrelated types work?

Like:

switch ("str") {
  case 42: // dead code
}

The root of the issue is that we're effectively checking that a Type is == to a non-Type object.

[lrhn]

Every type is related to dynamic and every top type, and most are related to Object and its supertypes.

Map<String, dynamic> json = ...;
if (json case {"kind": "value", "value": int}) print("is integer value");

Pattern matching JSON structures will probably happen a lot. Most values are typed as dynamic there.

Same for handling other untyped messages, like isolate port events:

port.handler = (message) {
  switch (message) {
    case ("number", int): 
    case ("number", double):
       // Might actually fail here if `message` is promoted to `(String, Type)`.
      handleNumbers(message.$1);
    // ...
  }
};

Pattern-based switching is a way to figure out the type of an untyped object. Using "unrelated type" to catch bad patterns just doesn't work in that case.

[rrousselGit]

    case ("number", double):
       // Might actually fail here if `message` is promoted to `(String, Type)`.
      handleNumbers(message.$1);

They'd get a compilation error since Type is not a number, so I'm not sure that's an issue.

---

I don't think Object/dynamic being converted into Type is an issue.
I doubt using pattern matching for decoding JSON is going to be very common. To begin with, metaprogramming is WIP and is supposed to also solve decoding through code generation.

I'd expect most use cases to have an interface other than Object/dynamic as input (typically a sealed class). In which case an unrelated type check would work.

[lrhn]

Have you tried it?

In DartPad, master branch, running:

void main() {
  var message = ("number", 42) as dynamic;
  switch (message) {
    case ("number", int): 
    case ("number", double):
       // static type of `message` is `dynamic`.
      handleNumbers(message.$1);
    // ...
  }
}

void handleNumbers(num value) {
  print(value);
}

I get zero warnings, zero errors. It just doesn't match.

If we could promote to (String, Type), as I was originally hoping, that would be nice, but constant patterns do not promote (because they're based on == which isn't guaranteed to only accept something of the same type), so writing int instead of int _ also prevents you from getting a better type that will make you realize the mistake.

So the static type of message inside that match case is still dynamic, which again means no warnings or errors.
Exactly the worst-case scenario for this foot-gun. But I don't expect it to be that rare.

Arguably, the biggest problem is starting with dynamic. If you avoid that, most type-mistakes become compile-time errors.

JSON is almost always typed as dynamic. Metaprogramming won't be done for some time yet.
I do expect people to use pattern matching to match JSON.
In fact, the recent change to map patterns, to not require them to match all entries, was partly based on the assumption that you do parse JSON-like maps using patterns.

My bet is we'll see a lot of questions of "why doesn't this match" from people trying to match JSON, and using type literal constant patterns where they intended a typed variable or object pattern.
Pattern matching against something of an unknown type is a core part of pattern matching functionality.
It's perfectly suited for handling JSON, so I'd be very surprised if nobody does that.

I could be wrong, and nobody runs into this issue. I hope I am.
But I think we should be ready to help people more than just relying on static type errors or unrelated types warnings, which won't cover any of these very concrete use-cases.

[mit-mit]

I've his this twice while writing sample code just the past week. Both times I was super confused about what was happening...

[rrousselGit]

I get zero warnings, zero errors. It just doesn't match.

@lrhn I do get an error. But upon further look, it seems to be caused by strict-casts/inference/raw-types.
Which is fine by me. That's what I'd expect those options to be for.

I'd probably end up refactoring the code to:

var message = ('number', 42) as dynamic;
switch (message) {
  case ('number', num second):
    handleNumbers(second);
  // ...
}

handleNumbers(num nbr) {...}

Although this isn't quite the same behavior as what you shared.

I originally naively thought we could do:

switch (message) {
  case ('number', int second):
  case ('number', double second):
   // "second" is downcasted to "num"
}

[rrousselGit]

Adding to that, I'm surprised avoid_dynamic_calls doesn't trigger on the message.$1 within the case block by the way.

I guess that's because the pattern match means the property is known. But then why is message still dynamic within the case block? Both behaviors sound conflicting.

I'd expect that if message isn't upcasted to a tuple within the case block, then avoid_dynamic_calls should still trigger on message.$1.
But maybe that's unrelated and just a bug in the lint?

[munificent]

Adding to that, I'm surprised avoid_dynamic_calls doesn't trigger on the message.$1 within the case block by the way.

There's no dynamic call when destructuring the fields in the pattern because the record pattern has already type tested that the value is a record first. It's almost as if every pattern binds a new hidden variable whose type is the pattern's tested type, and then further destructuring operates on that hidden variable.

I guess that's because the pattern match means the property is known. But then why is message still dynamic within the case block? Both behaviors sound conflicting.

We don't actually promote variables of type dynamic to another type because, in general, type promotion doesn't cause new errors to appear. If you have a variable of type dynamic and promote it to some other type, it means that some method calls on the variable which were valid (because it's dynamic) could then become errors (because the method isn't defined on the type you promoted it to).

[lrhn]

We actually do promote from dynamic:

void main() {
  dynamic d = 1;
  if (d is int) {
    var l = [d];
    print(l.runtimeType); // JSArray<int>, so `d` has static type `int` in the line above.
  }
}

It's the pattern which promotes to (dynamic, dynamic):

 dynamic d = (1, "A");
 if (d case (1, "A")) {
    // Assumes pattern matched, so `d` is promoted to `(X, Y)` 
    // where `X` is the type promoted by `d.$1 case 1` and `Y` by `d.$2 case "A"`. 
    // But `d.$1` has type dynamic and `case 1`, or any const pattern, doesn't promote
    //  so result is `(dynamic, dynamic)`.
    var l = [d];
    print(l.runtimeType); // JSArray<(dynamic, dynamic)>
 }

Same for if (d case (int, String)) which also doesn't promote.

So the .$1 is not a dynamic invocation because the type is (dynamic, dynamic).

[rrousselGit]

I guess what confuses me is: why does .$1 return dynamic instead of whatever the pattern is matched against?

After checking more, I don't understand why the following doesn't work:

Object? a = (42, "a");
if (a case (42, "a")) {
  String second = a.$2;
}

That's probably the root of my confusion when discussing the impact of the type vs type() mistake.

I'd naively think that doing case (42, "a") would be identical to case (int(), String()) && it.$1 == 42 && it.$2 == "a"

Is the issue that objects could somehow override ==, such that a non-string could be "equal" to "a"? (like how 42.0 and 42 are equal)?
But in the case of a primitive type, although obj == "a" could be true, "a" == obj couldn't be, so I'd expect this to break the rules of ==.

Of course, this wouldn't work for classes as they could have a getter instead of a field. But for records, I'd expect this to work. Although I'm likely missing something.

[lrhn]

I guess what confuses me is: why does .$1 return dynamic instead of whatever the pattern is matched against?

Because the pattern matching doesn't guarantee anything about the type of the matched value, in general, because pattern matching uses == which can accept value. (So yes, that is the issue).

All we know about a.$1 is that "a" == a.$1 is true.

While we can actually know something in this case, because "a" has primitive equality and we know what it does, we can't do that in general, so pattern matching which uses == just doesn't promote.
There is no special casing for equalities where we could know more.

As you mention yourself, even (42, "a") doesn't tell us the type of a.$1. It can be both int and double, so we'd have to deduce num. It's special cases all the way down, only works for constant objects where we know the == implementation at language specification time, and case c: doesn't work the same as case == c:, where we can't assume anything.

So an if (a case (42, "a")) { ... } is closer to equivalent to if (a is (dynamic, dynamic) && 42 == a.$1 && "a" == a.$2) { ... }. There is no type checks and no promotion, and if I do:

class Wat {
  const Wat();
  bool operator(Object other) => true; // Everyone's equal!
}

// ...
  if (a case (const Wat(), const Wat())) { ... }

then the pattern check succeeds, but I know absolutely nothing about the type of a.$1 and a.$2 other than that they are typed dynamic, because a.$1 has type dynamic.

TL;DR: It could possibly work, for constant patterns only, when the constant has primitive equality, and we'd have to specify the behavior for every such type in the language itself.
At that point, we should expect if ("" == s) to promote s to String too.

We haven't attempted to do that.

[rrousselGit]

That's fair.

I guess my intuition comes from typescript, which does handle cases such as if ("" == s) where s gets promoted to the constant value "" – although it has various unsound cases so they are cheating a bit.

Still, with strict-casts & variants enabled, are there unsafe cases?
From my tests, they handle pretty much all cases.
Dealing with dynamics has always been quite funky anyway. I personally use Object? instead of dynamic everywhere, enable strict-casts + related lints, and recommend my users to do the same.

Maybe the issue could be solved with a bit of education on how to deal with dynamic?
Like having flutter create enable strict-casts by default, or having flutter_lints enable avoid_dynamic_calls

[scheglov]

You might want to enable the type_literal_in_constant_pattern lint.

And when we know types, the analyzer reports warnings.

[lrhn]

Will close this issue when the lint is released in the lints package.

[escamoteur]

What I don't understand why this case const is even necessary if I really want to switch over a generic Type T or a Type variable like here:

    switch (T) {
      case const (String):
        await prefs.setString(key, value as String);
        break;
      case const (int):
        await prefs.setInt(key, value as int);
        break;

when I suddenly got the linter error after upgrading Flutter I was quire annoyed because I didn't see any benefit of adding the additional const

[munificent]

Will close this issue when the lint is released in the lints package.

I think this is done now, so I'm going to close this.

What I don't understand why this case const is even necessary if I really want to switch over a generic Type T or a Type variable like here:

    switch (T) {
      case const (String):
        await prefs.setString(key, value as String);
        break;
      case const (int):
        await prefs.setInt(key, value as int);
        break;

when I suddenly got the linter error after upgrading Flutter I was quire annoyed because I didn't see any benefit of adding the additional const

It's not necessary. It's just that the code is—to some readers at least—more confusing without it. If you just do case int:, then it really looks like you are matching objects that are *instances of int, which is also much more likely to be what you want to do. But it doesn't do that. Without the lint enabled, if we silently allow that, the result is often code that looks like it does something but will never actually match anything.

If we had a time machine, we would have designed Dart around patterns initially and avoided ever having type literals or allowing them as constant value patterns. But we don't have a time machine. In the absence of that, the best we can do is try to route users away from existing but confusing syntax.