Lint for using a type literal in a switch case

[munificent]

Consider:

switch (obj) {
  case int: print('int');
}

In Dart both before 3.0 and in 3.0 with patterns, this case does not match integers, it matches only the type object int itself. Prior to 3.0, there was no way to do a type test in a switch, so users would rarely ever think to write the name of a type and expect it to type test.

With patterns, we expect type tests to be common in switches. There are two idiomatic ways to write them:

switch (obj) {
  case int(): print('int');

  case int _: print('int');
}

(The first, an object pattern, is useful when you want to do further destructuring on the value after testing its type. It's likely the idiomatic style to use in algebraic datatype style code. The latter, a wildcard variable declaration, allows you to match any possible type annotation. Object patterns only support named types, but not more complex type annotations like records, functions, nullable types, etc.)

Because type tests are common in patterns, we expect that users will sometimes write a type literal when they meant to do a type test. This is valid code (though in some cases it will lead to non-exhaustive switch errors), but it probably doesn't do what the user wants.

We tried hard to tweak the syntax to avoid this footgun, but every other approach we came up with seemed to be worse overall. We also considered making it an error to have a constant in a pattern that resolves to a type literal. This would force users away from the footgun... but it's also a breaking change. There is code in the wild today that deliberately uses type literals in switch cases to test for type objects.

Given that, I think it would help the user experience if we added a lint that reports a type literal being used as a constant in a switch case (in a Dart 3.0 library). Instead, it should suggest that users use either an object pattern or variable pattern if they meant to do a type test, or an explicit == pattern if they really did mean to compare for equality to a type literal.

The heuristic I'd suggest is:

1. If the matched value type is Type, then suggest they change the case from case SomeType to case == SomeType. That has the same semantics, but makes it clearer to the reader that a type equality test is intended (and silences the lint).

2. Otherwise, if the matched value type is anything else, suggest that they use case SomeType _ if they indended to do a type test and == SomeType otherwise.

This lint will be useful whenever it arrives, but nothing is blocked in 3.0 around having it. It should just help users avoid what's likely a mistake.

[scheglov]

@pq @bwilkerson Do we want to make it s lint, or a warning inside the analyzer?

[bwilkerson]

It isn't wrong to use a type literal in a switch case, so I see this as enforcing a coding style, which makes it a lint.

[pq]

I'm all for this and agree with Brian that a lint is the right place to implement.

[munificent]

Yes, if it were up to me (and it's not, this is really for the tooling teams to decide), I would make it a lint too. For me, warnings all represent some kind of dead code: Code you can remove or simplify and the result is a program with the same semantics.

[bwilkerson]

The other major use for warnings is for code that's guaranteed to be wrong, but for reasons that the type system isn't able to express.

[munificent]

Can you give me an example of the latter?

[bwilkerson]

We have a warning telling you that the callback can't be used as an error handler:

void f(Future<int> future, Future<int> Function({Object a}) callback) {
  future.catchError(callback);
}

The type system can't tell us that because the type of the parameter is Function.

[munificent]

Oh, interesting. So there's some ad hoc overloading support in the analyzer effectively?

[bwilkerson]

I'm not sure I'd call it overloading, we're just doing some type checking that isn't (can't be?) specified in the declaration of catchError.

[eernstg]

Note that there is a connection to https://github.com/dart-lang/language/issues/2894, about 'case constants that will never be == to the matched value type':

An identifier which is a type literal is known to have primitive equality. This makes it possible to know at compile-time that its operator == will never return true for an argument whose static type is not a subtype and not a supertype of Type (it's probably enough to check for 'not a supertype, including Type itself').

In other words, this lint could recognize case T where T is an identifier pattern which is a type literal, but it could also recognize other syntactic forms. We already have a warning for this kind of situation, and it does already emit a warning in some cases:

class A {}

void main() {
  switch (A()) {
    case int: break; // Warning.
    case const (List<int>): break; // Warning.
    case == Map<int, String>: break; // No warning, seems like it should be included.
    case int(hashCode: int): break; // Warning.
  }
  switch (1 as dynamic) {
    case int: break;
    case const (List<int>): break;
    case == Map<int, String>: break;
    case int(hashCode: int): break; // Warning.
  }
}

I think the work requested in this issue could presumably be done most easily by generalizing that warning a bit. It is true that we wouldn't get a warning for a very general matched value type (including dynamic). However, those situations could be false positives anyway, and it's a nice property that the warning doesn't have those.

[scheglov]

About == Map<int, String> and no warning. The specification speaks only about constant patterns: "A constant pattern's constant has primitive equality and is matched against a type that it can never be equal to, like matching a String against the constant pattern 3."

Not as an excuse (unlike the above), but we also have unrelated_type_equality_checks lint, that is reported at that case.

[eernstg]

About == Map<int, String> and no warning.

Ah, my bad, it won't work in that case: The matched value gets to run operator == in that case, and we can't know that it will not return true.

By the way, I think this means that it is not going to be a safe rule to replace case T: by case == T:: The former will match the same reified type and no other object, the latter will match the same reified type, but also any object that wants to be equal to T.

[munificent]

By the way, I think this means that it is not going to be a safe rule to replace case T: by case == T:: The former will match the same reified type and no other object, the latter will match the same reified type, but also any object that wants to be equal to T.

Fair. I'm fine with the lint suggesting users replace T with const T instead then.

[scheglov]

Do you mean const (T)? AFAIK const T is not a valid constant pattern.

[scheglov]

Well, I thought some more about this lint, and start doubting how useful it is.

Matching Type vs. Type seems not incorrect, so I'm not sure why we want to report a lint here. Converting case int into case const (int) does not look as a beneficial code change.

@eernstg example shows that we already report a warning for case int when the matched type is not a supertype of Type. So, types for which we will not report the warning are dynamic, Object?, Object and T (as a type parameter). We probably should not report the lint here for anything else. But these listed, do we think that these will happen often? Maybe.

[munificent]

I didn't realize that we already report lint errors for places where a type literal won't match the static type of the matched value. Given that, I think that's sufficient for now.

[lrhn]

Just relying on type is not sufficient. Take:

Map<String, dynamic> json = ...;
if (json case {"type": "value", "value": int}) {  
    print("An integer value");
} else {
    print("Not an integer value");
}

The plain int pattern is woefully dangerous, and I do want a warning if it's ever used. The only exception would be if the matched value type is precisely Type, or just maybe FutureOr<Type> or Type?, but definitely not if it's Object or any supertype of Object.

(I really want to make a plain type literal as a pattern an error now, to push people towards either const (T) or == T for comparing Type objects, and int _/int() for matching values. It's too big a foot-gun as it is now. Everybody shoots their feet, even those who know about the problem already, because doing "value": int just looks right. I do it. And then, possibly, in the far future, allow a pattern of just int to actually match integers, like everybody thinks it does today.)

[munificent]

After @mit-mit made this mistake several times, we talked about it more in the language meeting. Since this is a long comment, here's the summary up front:

• It seems that users almost never actually want switches that test if objects are equal to a given type literal. Even of the existing switch statements I found that use type literals deliberately, most would be better served using type tests.

• It is a serious footgun that a type literal in a switch case means "see if equal to that type object". But they will often want type tests and it's very easy to accidentally write a type literal thinking you'll get that.

• Existing errors, warnings, and lints don't catch many of these mistakes.

We can't change the pattern syntax (and even if we could, we don't know what we'd change it to), so this seems like a good candidate for a lint. Discussing it at the meeting this morning, I think there was fairly broad agreement that the lint should report all uses of identifier patterns that resolve to type literals, even in cases where a type literal might be what you want.

So it would fire in all of:

// Supertype of Type:
switch (obj as Object) { case int: ... }

// Type:
switch (obj as Type) { case int: ... }

// Nullable Type:
switch (obj as Type?) { case int: ... }

// Dynamic:
switch (obj as dynamic) { case int: ... }

Basically treat typed literals as scorched Earth. When the lint fires on a type literal, it should suggest a fix. Based on my analysis, by far the most likely thing a user meant is to write a type test. So it should first suggest something like, "If you meant to test if the object has type Foo, instead write Foo _." (We could also suggest Foo(), but the former works with all types not just simple named types.)

Then something like "If you do mean to test that the matched value is equal to the type literal Foo, then this lint can be silenced using const (Foo)".

We could also suggest == Foo instead, but that has slightly different semantics. It's arguably more readable, but this is such a rare use case in practice that it seems safest to stick with the syntax that does exactly what a type literal does today.

This lint doesn't need to be (and at this point, won't be) in 3.0, but it would be helpful whenever it can become available. We'll probably put it in the recommended lint set, possibly even in core.

Here's the rest of the context...

Looking at type literals in existing switches

I looked into existing switch statements to try to get a feel for how often type literals appear. This should tell us how important of a use case that is and how often writing a type literal in a case is deliberate.

My analysis is pretty rough because I can't precisely tell if an identifier is a type literal or not without doing resolution. Instead, I just assume that it's a type literal if the name is one of the built in lowercase types (int, etc.) or a capitalized name. This overcounts since some constants happen to have capitalized named. But based on that, I see:

-- Switch (7991 total) --
   7923 ( 99.149%): No type cases  =============================================
     68 (  0.851%): Has type case  =

So it's very rare for a user to deliberately use a type literal in a switch in order to see if a value is equal to some type object. Looking through several of the switches that do, most are like:

switch (drawable.runtimeType) {
  case Pixel:
    Pixel p = drawable as Pixel;
    putPixel(p);
    break;
  case Line:
    Line l = drawable as Line;
    drawLine(l);
    break;
  case WireFrameTriangle:
    WireFrameTriangle wt = drawable as WireFrameTriangle;
    drawWireframeTriangle(wt);
    break;
  case FilledTriangle:
    FilledTriangle ft = drawable as FilledTriangle;
    drawFilledTriangle(ft);
    break;
  // ...
  default:
    debugPrint('can\'t draw : unknown type: ?');
}

So they're calling runtimeType on a value and then switching on the result as a poor man's multiway type test. (In fact, many of the examples I saw would break if they happened to be passed an object that was a subtype of one of the types they care about.) Almost all of these should be migrated to type test patterns, like:

switch (drawable) {
  case Pixel p:
    putPixel(p);
  case Line l:
    drawLine(l);
  case WireFrameTriangle wt:
    drawWireframeTriangle(wt);
  case FilledTriangle ft:
    drawFilledTriangle(ft);
  // ...
  default:
    debugPrint('can\'t draw : unknown type: ?');
}

So, if nothing else, that left me feeling good that patterns will make it easier to write type testing switches correctly and compactly.

There are a handful of switches I saw that are switching on a type parameter, like:

switch (T) {
  case Uri:
    return Uri.parse(v) as T;
  case DateTime:
    return DateTime.fromMicrosecondsSinceEpoch((v * 1000 * 1000).round())
        as T;
  case Duration:
    return Duration(microseconds: (v * 1000 * 1000).round()) as T;
  case String:
  case num:
  case bool:
    return v;
  default:
    return factory == null ? v : factory(v);
}

Those are a more reasonable use of a type literal since you don't have an instance to type test.

Overall, the conclusion I see is that a type literal in a switch case or pattern is very rarely deliberate. Going forward, when a user writes one, they will almost certainly be intended to write a type test.

So allowing type literals in patterns and having them mean "equivalent to the type object" is very likely a serious footgun.

Existing errors, warnings, and lints

The existing static analysis may catch some of these mistakes incidentally. For example, if you write:

test(num n) {
  switch (n) {
    case int: print('integer');
    case double: print('float');
  }
}

The analyzer reports:

  error • temp.dart:2:3 • The type 'num' is not exhaustively matched by the switch cases since it doesn't match
          'double()'. Try adding a default case or cases that match 'double()'. • non_exhaustive_switch_statement
warning • temp.dart:3:10 • The matched value type 'num' can never be equal to this constant of type 'Type'. Try a
          constant of the same type as the matched value type. • constant_pattern_never_matches_value_type
warning • temp.dart:4:10 • The matched value type 'num' can never be equal to this constant of type 'Type'. Try a
          constant of the same type as the matched value type. • constant_pattern_never_matches_value_type

But the error here is only because num happens to be a sealed type. And the warnings are only because I happened to switch on num which is known to the compiler to be a sealed type with primitive equality. There are no errors in, say:

test(Map<String, dynamic> json) {
  switch (json) {
    case {'id': int}: print('integer id');
  }
}

[mit-mit]

Great analysis!

While we're at it, I find this lint message hard to read. Anyone has suggestions for how to potentially improve it?

warning • temp.dart:3:10 • The matched value type 'num' can never be equal to this constant of type 'Type'. Try a
          constant of the same type as the matched value type. • constant_pattern_never_matches_value_type

[munificent]

I think if we have a lint that fires on all type literals and routes them to what they most likely intend to write, a type test, then almost no one will ever see that warning message.

[fishythefish]

I'm catching up here after reading Erik's language meeting notes. @munificent: You have the following example of code migrated to use type test patterns:

switch (drawable.runtimeType) {
  case Pixel p:
    putPixel(p);
  case Line l:
    drawLine(l);
  case WireFrameTriangle wt:
    drawWireframeTriangle(wt);
  case FilledTriangle ft:
    drawFilledTriangle(ft);
  // ...
  default:
    debugPrint('can\'t draw : unknown type: ?');
}

Should .runtimeType also be dropped from the scrutinee? (I didn't want to edit it without confirming.)

[munificent]

Should .runtimeType also be dropped from the scrutinee? (I didn't want to edit it without confirming.)

Oops, yes, fixed. Thanks!

[mit-mit]

@johnpryan tells me that this issue seems to be mentioned as a common footgun on socials.

@scheglov @bwilkerson @pq thoughts on implementing the lint @munificent describes above?

[scheglov]

Sure, we already have this lint, I can make it more aggressive. We already have a fix to use type check Foo _. We don't have a fix to use const (Foo).

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/305420 for 'Convert to constant pattern' quick fix.

[pq]

Nice!

[mit-mit]

Note: a cherry pick discussion for this is happening in the comments of https://github.com/dart-lang/linter/pull/4358

[pq]

Cherry-pick prepared: https://github.com/dart-lang/sdk/issues/52516

[pq]

🦅 Cherry pick landed (but not yet in a release).