proposal: `unrelated_pattern_type`

[stereotype441]

unrelated_pattern_type

(Note: I'm having trouble thinking of a good name for this lint; feel free to replace this with something better).

Description

The pattern can only match values of type T1, but the matched value type is T2, which is unrelated.

Details

Usually, the required type of a pattern is either the same as the matched value type, or a subtype of it. For example:

f(FileSystemEntity e) {
  switch (e) {
    case Directory():
  }
}

It's rarer for the required type of a pattern to be a supertype of the matched value type, but it still works:

f(List<int> list) {
  switch (list) {
    case [int? i]:
  }
}

(For the subpattern int? i, the required type is int?, but the matched value type is int. This is fine, though the ? is superfluous.)

But it's probably a mistake if there isn't a supertype or subtype relation between the required type and the matched value type. For example:

class Base1 {}

class Derived1 extends Base1 {}

class Base2 {}

class Derived2 extends Base2 {}

f(Base1 b) {
  switch (b) {
    case Derived2(): // Oops!  Should be `Derived1`.
  }
}

(Note that technically it's possible for Derived2 to match, because there might be some class somewhere is a subtype of both Base1 and Derived2. But this is rare; it's much more likely a mistake.)

The lint catches mistakes like this by firing whenever a pattern's required type is neither a subtype or a supertype of the matched value type.

Kind

This lint guards against errors.

Bad Examples

See above.

Good Examples

See above.

Discussion

I was inspired to suggest this lint by my own work on a side project that uses patterns to consume analyzer AST nodes. I had two functions, one to handle statements and one to handle expressions. I accidentally inserted a handler for a particular type of expression in the function for handling statements. There were no static errors or warnings (since none of the types involved are sealed), but my handler was never reached. This lint would have instantly alerted me to my mistake.

Discussion checklist

• [ ] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• [ ] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
• [ ] If there's any prior art (e.g., in other linters), please add references here.
• [ ] If this proposal corresponds to Effective Dart or Flutter Style Guide advice, please call it out. (If there isn't any corresponding advice, should there be?)
• [x] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[srawlins]

I think I like this proposal, but I am worried about amount of false positives. We don't have a similar rule for is- or as-expressions. It could have decent benefit if we did have a similar rule for is- and as-expressions, especially as these expressions do not promote the type of a local variable without a subtype/supertype relationship.

I'll note that there are currently guards against using final types in this way. Each of these raises a warning (like "The matched value type 'Foo' can never match the required type 'String'."):

class Foo {}

void f(String b) {
  switch (b) { case Foo(): }
}

void g(Foo b) {
  switch (b) { case String(): }
}

But there are cases that are not covered. For example, in the following code, there is no subtype of Bar which can also be a subtype of Foo, since Bar is sealed and all of it's (singular) subtypes are final. But there is no warning

f(Bar b) { switch (b) { case Foo(): } }

class Foo {}

sealed class Bar {}

final class Bar2 {}

[srawlins]

CC @scheglov

[bwilkerson]

I'm also concerned about the number of false positives. We've had problems in the past with assuming that it's rare to have a subtype of two seemingly unrelated types. And in the past the number of provably unrelated types has been very small.

With the class modifiers feature, it's possible that the number of provably unrelated types is now (or soon will be) high enough to justify the cost. But if that's true, then I have to wonder: should this be a language-level error? And if it were, would there be enough cases left over to justify a lint?

[scheglov]

@srawlins I guess you mean final class Bar2 extends Bar {}, although this does not change the fact that we should have reported the hint. No subtypes is even better than one final subtype :-) I will take a look at improving the check.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/295801

[srawlins]

IIRC, the one intersection type that we see in the wild a lot is with protos: For example a function will accept a GeneratedMessage, and then check is Map, as some protos implement Map.

[stereotype441]

I'm also concerned about the number of false positives. We've had problems in the past with assuming that it's rare to have a subtype of two seemingly unrelated types. And in the past the number of provably unrelated types has been very small.

With the class modifiers feature, it's possible that the number of provably unrelated types is now (or soon will be) high enough to justify the cost. But if that's true, then I have to wonder: should this be a language-level error? And if it were, would there be enough cases left over to justify a lint?

I don't know. I can definitely understand the issue with false positives. The lint I'm proposing definitely wouldn't be perfect.

On the other hand, I'm also concerned because this lint was inspired by a real mistake I made while trying out the patterns feature. I accidentally inserted a case for handling a particular type of expression into a switch statement switching on statements. So although I understand that, theoretically, it is possible that in some hidden part of my code (or part of some other code that depends on mine), there's some class that is a subtype of both statement and expression, in practice I know that this never happens. So in practice the switch case was unreachable, and if a lint had warned me that it was unreachable, I would have noticed my mistake and it would have saved me a bunch of annoying debugging.

[srawlins]

Here's the related issue for unrelated types in is-expressions. https://github.com/dart-lang/sdk/issues/58275