new lint: invalid_case_patterns

[pq]

See: #4044.

WIP but feedback very welcome. Especially for docs.

Because there's a lot to say here, perhaps we could link out to something in the core dev docs?

@MaryaBelanger: thoughts?

Minimally here though, I think the correction message and description could use tightening up. (@bwilkerson? @munificent?)

As for the code, it's a WIP and possibly over-reporting. Needless to say, input welcome there!

[coveralls]

Coverage: 95.664% (+0.02%) from 95.641% when pulling 442dbd0 on invalid_case_patterns into cdc2c6c on main.

[bwilkerson]

I'm guessing that you mean "can we link directly to some doc other than the one generated from the _details. In which case the answer is that it isn't currently supported.

I think the link to the other doc should be in the _details. There will automatically be a link in the IDE to the doc page generated from the _details. That might seem like a bit of an indirection, but it's also more consistent with the way the rest of the IDE links work, and I think that consistency trumps the extra indirection.

[bwilkerson]

First, that's a false dichotomy; we could have both.

But I think the answer to the question of whether to have more details here depends on how well the doc we should link to (so "yes" to the second question) does of explaining the behavior of this lint. If the doc provides important details about switch statements, but doesn't directly address the breaking changes, then we need details here. If the doc is dedicated to the breaking changes, then we might not need any extra details here.

But either way, we need to provide the kind of before and after examples that we have in the other diagnostic documentation in order to help users figure out how to migrate their code.

[MaryaBelanger]

I'm not sure if this is the kind of input you're looking for @pq, but hopefully it helps.

I'm imagining that within the umbrella of patterns documentation in dart.dev, there will be a reference on switch cases, what works, what doesn't work, the relationship/significance of switch for patterns, etc.

Or, that the language tour reference on switch will document the new valid and invalid case patterns, agnostic of patterns, but that points to a page about the relationship between switch and patterns.

If the doc provides important details about switch statements, but doesn't directly address the breaking changes, then we need details here.

@bwilkerson I'm not sure what "directly addressing the breaking changes" on dart.dev would mean, like explicitly calling out that this is effective from 3.0+? Or that the result of these invalid cases is a compile time error?

...depends on how well the doc we should link to...does of explaining the behavior of this lint.

If I understand correctly, "explaining the behavior of the lint" is a combination of:

• explaining the relationship between patterns and switch (why)
• the advent of that change, i.e. Dart 3 (when)
• showing everything that's invalid (what)
• alongside what is valid/what should be done instead (how).

Basically documenting #4044 in dart.dev. Would that satisfy documenting the lint "well"?

I'm not against more detail in the lint message, but I am biased towards more complete dart.dev content. So, if fleshing it out on dart.dev solves the problem of making this lint message really long, then I'm happy to make sure the doc it links to instead is a sufficient resource.

Also important to consider....

Patterns documentation will definitely be up by Dart 3, but I currently don't have a timeline more granular than that, AND it won't actually be published to the website before release day. So that's something to consider, if you need a working link before then. I could prioritize that work, or at the very least create the page and section to link to if the cutoff is very soon.

[bwilkerson]

I'm not sure what "directly addressing the breaking changes" on dart.dev would mean ...

The lint is designed to catch expressions in pre-3.0 switch case clauses whose semantics will change, without a diagnostic, when the code is migrated to 3.0. Those are the breaking changes I'm referring to.

If I understand correctly, "explaining the behavior of the lint" is a combination of: ...

That's exactly what I meant by "directly addressing" the breaking changes.

Would that satisfy documenting the lint "well"?

Yes, that would be perfect.

Patterns documentation will definitely be up by Dart 3 ...

The lint will, hopefully, ship with the next beta, so we'll want to have documentation explaining what the lint is for before we ship 3.0. We can always change the docs later to be reduced and to point to the dart.dev page, but I think it's important for users to understand how this helps them prepare for 3.0 sooner than the release of 3.0.

[pq]

Thanks @MaryaBelanger! In the short term, I agree w/ @bwilkerson that we should make sure that the lint has docs that are complete enough that folks who try it out aren't totally lost. In the medium term, it would be great to link out to a more complete doc on dart.dev (and what you describe sounds perfect). The good news is the linter docs are auto-generated so it's easy for us to update and refresh them once the doc we want to link to goes live.

[MaryaBelanger]

Aha, with that timeline it seems like the lint docs are the best place to hold all that info (I think you both already knew that, I'm catching up haha). I'm not sure if you were looking for input on the lint itself, but now that I'm thinking about it...

I might be missing something, but since the correction message says "Try refactoring" you'd want to provide them with a solution or explanation for each potential case right? Unless you don't typically explain lint topics that come with a quick fix (but there's a few expressions that don't have a quick fix so the question still stands for those).

To avoid having to write out every expression's GOOD/BAD example, would it be possible to add a string token to the correction message that's populated based on the expression that the linter flagged? Please excuse my horrifying dart/pseudocode mashup, but something like:

class InvalidCasePatterns extends LintRule {
  static const LintCode code = LintCode('invalid_case_patterns',
      "This expression is not valid in a 'case' clause in Dart 3.0.",
      correctionMessage: 'Try refactoring the expression to be valid in 3.0:' token);
...
    } else if (expression is ParenthesizedExpression) {
      token = "discard parenthesis";
      rule.reportLint(expression);
    }

Otherwise I'd imagine the lint details could just list out all of the affected expressions, maybe splitting them up by "compile time error/syntactically breaking" and "behavior/semantically breaking". Again, a messy mashup but just for reference, something like:

BAD:

switch (obj) {
// syntactic error
  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.

// semantic error
  case [1, 2]: // list literal
  case {'k': 'v'}:  // map literal
  case Point(1, 2): // constant constructor call
  case _: // wildcard
  case true && false: // logic operators
}

GOOD:

switch (obj) {

// List literals, map literals, set literals, and constant constructor calls:
// Put const before the literal or call:

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

// ... etc. for the rest

Maybe you've already considered all of this 😅 I'm still not 100% sure how to read a lint rule's source. Anyway, I'll let you know when the Dart 3 docs start coming together and where you can link to once the content is available.

[pq]

I might be missing something, but since the correction message says "Try refactoring" you'd want to provide them with a solution or explanation for each potential case right?

You're not missing anything. Where we can, I think we should provide a more specific message. (Often we do and here we should for sure.) TL;DR you're 💯 spot on.

would it be possible to add a string token to the correction message that's populated based on the expression that the linter flagged?

Yep. Possible!

I'm not sure if you were looking for input on the lint itself, but now that I'm thinking about it...

Always. So thank you!

[munificent]

One thing then it looks to me like the tests are covering everything. Thank you for the quick turnaround!