implement: invalid_case_patterns

[munificent]

☂️ Tasks

• initial implementation (for testing and quick-fix hook up)
  • new lint: invalid_case_patterns #4047
• improve error messages for invalid_case_patterns #4172
• Update invalid_case_patterns docs when there's a dart.dev doc link #4055

---

Description

The new patterns language feature in Dart is a breaking change to a very small number of switch cases. To help users migrate and mitigate the breakage, I think it would help to have a lint that warns on switch cases in libraries whose language version is 2.19 or lower if the case will become an error or have its semantics change when the library is upgraded to 3.0.

Almost all existing switch cases are fine as far as I can tell. If the case is one of these constant expressions:

• Identifier (qualified or not), not named _
• Number literal
• String literal (possibly with interpolations)
• null
• as expression containing any of the above

Then it behaves exactly the same with patterns as it did before. There’s no breaking and nothing to migrate. Looking at a corpus of open source code, that covers >99.9% of all switch cases.

Some valid switch cases today will become compile errors in Dart 3.0:

• Set literals
• Parenthesized expressions
• Calls to identical().
• Unary operator expressions !, -, or ~ (except for - before an integer literal, which is a valid pattern and is fine)
• Binary operator expressions !=, ==, &, |, ^, ~/, >>, >>>, <<, +, -, *, /, %, <, <=, >, >=, ??.
• Conditional operator ?:
• .length calls on strings
• is and is! expressions

Examples of all of them:

switch (obj) {
  case {1}: // Set literal.
  case (1): // Parenthesized expression.
  case identical(1, 2): // `identical()` call.
  case -pi: // Unary operator.
  case 1 + 2: // Binary operator.
  case true ? 1 : 2: // Conditional operator.
  case 'hi'.length: // .length call.
  case i is int: // is expression.
}

Some valid switch cases today are also syntactically valid patterns, but the pattern matching behavior may be different from the current constant equality behavior. They are:

• List and map literals. A list or map literal can appear as a constant in a case:

  switch (obj) {
    case [1, 2]: ...
    case {'k': 'v'}: ...
  }

  Currently, the case will only match if the incoming value has the same identity as the constant. So:

  test(List<int> list) {
    switch (list) {
      case [1, 2]: print('Matched'); break;
      default: print('Did not match'); break;
    }
  }
  
  main() {
    test(const [1, 2]); // Prints "Matched".
    test([1, 2]); // Prints "Did not match".
  }

  With patterns, a list or map literal becomes a list or map pattern. The pattern destructures the incoming object and matches if the subpatterns all match. In other words, list and map pattern match using something more like deep equality.

  With Dart 3.0, the above program prints "Matched" twice.

• Constant constructor calls. Similar to collections, you can construct a constant instance of a class in a case:

  class Point {
    final int x;
    final int y;
    const Point({this.x, this.y});
  }
  
  test(Point p) {
    switch (p) {
      case Point(x: 1, y: 2): print('Matched'); break;
      default: print('Did not match'); break;
    }
  }
  
  main() {
    test(const Point(1, 2)); // Prints "Matched".
    test(Point(1, 2)); // Prints "Did not match".
  }

  Again, like collections, the case currently only matches if the incoming value has the same identity. With patterns, the Point(...) syntax becomes an object pattern that destructures the incoming point, calls the x and y getters on it and then matches the results of those against the corresponding subpatterns.

  In this example, it will print "Matched" twice.

  Note that object patterns only support named fields. So any constant constructor in a case today that has positional arguments will become a compile-time error when parsed as a pattern. A constant constructor call with no arguments is a valid object pattern and only does a type test:

  class Thing {
    const Thing();
  }
  
  test(Thing t) {
    switch (t) {
      case Thing(): print('Matched'); break;
      default: print('Did not match'); break;
    }
  }
  
  main() {
    test(const Thing()); // Prints "Matched".
    test(Thing()); // Prints "Did not match".
  }

  When interpreted as a pattern, this prints "Matched" twice.

• Wildcards. Today, you can have a constant named _:

  test(int n) {
    const _ = 3;
    switch (n) {
      case _: print('Matched'); break;
      default: print('Did not match'); break;
    }
  }
  
  main() {
    test(3); // Prints "Matched".
    test(5); // Prints "Did not match".
  }

  With patterns, the identifier _ is treated as a pattern that matches all values, so this prints "Matched" twice.

• Logic operators. The logic operators && and || are valid constant expressions and also valid patterns. As a constant expression, they simply evaluate the expression to a boolean and match if the incoming value is equal to that boolean value. So:

  test(bool b) {
    switch (b) {
      case true && false: print('Matched'); break;
      default: print('Did not match'); break;
    }
  }
  
  main() {
    test(false); // Prints "Matched".
    test(true); // Prints "Did not match".
  }

  With Dart 3.0, these become patterns. The above example prints "Did not match" twice because no boolean value can be both true and false.

Lint

All of the cases whose behavior might change can be detected syntactically. If the constant expression in a case is any of the above expression forms, then the lint should warn the user that the code will become an error or (worse) possibly behave differently in Dart 3.0.

Quick fixes

Many of these can be mechanically changed to something that is valid both in Dart today and valid and means the same in Dart 3.0.

• Parenthesized expressions: Provided the inner expression is one that's not broken in Dart 3.0, just discard the parentheses.

• List literals, map literals, set literals, and constant constructor calls: Put const before the literal or call. This turns it into a constant pattern which preserves the current behavior:

  // Before:
  case [1, 2]:
  case {'k': 'v'}:
  case {1, 2}:
  case Point(1, 2):
  
  // After:
  case const [1, 2]:
  case const {'k': 'v'}:
  case const {1, 2}:
  case const Point(1, 2):

  We'll probably want to tweak the "unnecessary_const" lint to not treat these const modifiers as unnecessary now.

• Wildcards: Rename the constant from _ to something else. Since the name is private, this can be done locally in the library without affecting other code.

• Everything else: For any other invalid expression, you have to hoist the expression out into a new named constant. For example, if you have code like this:

  switch (n) {
    case 1 + 2: ...
  }

  It can be fixed by changing it to:

  const three = 1 + 2;
  
  switch (n) {
    case three: ...
  }

  This might be a little harder to generate an automatic fix for since it will have to conjure up some name for the constant. But expressions like this are very rare, so it's probably sufficient for the lint to just warn and ask the user to manually fix it.

I think it's still useful to add this lint even if it only ships in Dart 3.0. Because even when Dart 3.0 comes out and users upgrade to it, their libraries will still by default be on some language version < 3.0 and the lint will fire.

[pq]

Bike-shedding names w/ @munificent, maybe:

• invalid_case_patterns or
• pattern_safe_switch_cases

Input welcome!

/cc @bwilkerson

[bwilkerson]

Are we planning any 2.19 releases between now and 3.0? If not, then it isn't clear to me when this lint would become available for anyone to use. If we are, then I'd recommend making it a warning rather than a lint because getting users to widely enable a new lint before 3.0 might be difficult.

@keertip For ideas for quick fixes in 3.0 that we should have to help users migrate their pre-3.0 code, regardless of whether we have a lint before 3.0.

[bwilkerson]

Sorry, I somehow missed this comment at the end:

I think it's still useful to add this lint even if it only ships in Dart 3.0. Because even when Dart 3.0 comes out and users upgrade to it, their libraries will still by default be on some language version < 3.0 and the lint will fire.

We might still want to make this a warning and not a lint, but maybe that would be too noisy.

[pq]

maybe that would be too noisy.

That's my worry too. Especially internally.

Maybe this is a candidate for a lint that after some bake-time graduates to a warning?

[srawlins]

That's my worry too. Especially internally.

Can you expand on that? I don't think think there is an issue there.

I think this would be good as a default-on Warning. But they take a lot of work. I have probably regretted most default-on warnings I've implemented. 🤷 maybe a Lint would be more pragmatic in view of our current CI burdens.

[pq]

Can you expand on that?

Just a gut sense. @munificent did some analysis and found that there were in fact some breaking cases in the wild and if they'd be anywhere, I'd expect them internally!

All of that said,

maybe a Lint would be more pragmatic in view of our current CI burdens.

That's a part of my concession too. In general it seems like ecosystem impact is nearly always more than we anticipated.

[bwilkerson]

I was actually thinking about it potentially being too breaking for external users if we made it a warning. We know there's code that would break, but I don't remember what percentage of code would be impacted. It might be too high, necessitating making it a lint.

[bwilkerson]

Putting aside the lint vs. warning question for a minute, there's another question I want to ask:

This isn't the only breaking change in 3.0, there are at least two others (at least potentially):

• annotations can't have whitespace between the constructor name and the argument list, and
• classes can't be used in a with clause unless the class is marked with mixin.
  There might be others that I'm not thinking of.

Do we want to have a separate check for those cases, or do we want a single 'not_forward_compatible' check that covers all of the cases?

I think we can bulk fix all of the pattern and annotation issues prior to migration. I don't think we can offer a bulk fix for the mixin issue, and in some cases (where adding mixin is the right solution) users can't fix their code manually either until after they've migrated. That might argue for not trying to catch the mixin issue.

[oprypin]

The thing about patterns is that there exists code that will compile both before and after but will mean different things. So, more than just a breaking change. (Is there anything else like that?)
That's why I think there has to be a transition period. First change the syntax to not compiling and then simply "add new syntax".
Internally, we will really want this transition period, and we will also slightly prefer these lints to be separate.

[bwilkerson]

The thing about patterns is that there exists code that will compile both before and after but will mean different things.

Actually, the transition to null-safety was kind of like that. An application can have libraries that opt back to a pre-null safety state. We were/are in a state where int in a pre-null safe library means the same thing as int? in a null-safe library, but is also perfectly valid code in a null-safe library, just with a different meaning. And some code that was valid in a pre-null safe library is no longer valid in a null-safe library (like some chains of ?. where null-shorting makes the ? unnecessary).

For null-safety we handled the problem by defining the semantics of mixed-mode programs. We needed to do that because the effects of the language choice could be seen outside of a library. In this case the effects of the language level are local to a single library.

My ideal is that when a package is migrated to 3.0, users would run dart fix and it would fix the issues, converting any broken code without manual intervention. To the extent that dart fix isn't able to do that, I would expect that users would either migrate the code manually, or, if there's too much broken code, mark the broken libraries with a // @dart=2.19 language comment, and then perform the manual migration process incrementally. Personally, I hope that the value of the lint being discussed is nearly zero because we're able to make dart fix work 100% of the time, but that doesn't mean that we shouldn't implement it to support a different workflow.

I know that my ideal process won't work internally, and I expect that we'll need to also have a plan for how we're going to make the transition internally. Your idea of a transition period might well be the right approach. Whatever approach we take, though, this lint could minimally be part of that discussion by allowing us to measure ahead of time just how much code there is that will be broken.

[oprypin]

I don't see how it's possible to rely on dart fix.
case Point(x: 1, y: 2) in the default language version right now means one correct thing, and in a future to-be-default language version it will mean another correct thing. What is dart fix supposed to do?

[oprypin]

If the lint is available only in a release that coincides with 3.0, then we'll be expecting users to download that, mark every file with language version 2.19, run dart fix, then unmark the files. Then yes, dart fix can work. I'm just saying I wish there was a good way to access this fix well in advance of 3.0

[bwilkerson]

Preserve the current semantics. Always. In this case by adding const before the constructor invocation.

[bwilkerson]

My understanding is that the 3.0 release will allow users to set the minimum language version to any version greater than or equal to 2.12. They won't be required to immediately set the language version to 3.0. The only requirement we're placing is that you can't use a 3.0 (or higher) version of the SDK on code that hasn't yet been migrated to 2.12 (null-safety).

I don't know whether that helps us internally, but it should ease the migration path externally.

[pq]

Thanks for all the feedback! A first cut is done and ready to land and integrate into the SDK so we can do broader testing (and associate quick-fixes).

Pre 3.0, we'll want to update the docs and improve the error messages. (Will add tasks above.)

[pq]

Closing this in favor of sub-issues for the remaining work.