dart-lang / language

Design of the Dart language
Other
2.65k stars 202 forks source link

[analyzer] Switch cases for destructured records do not promote types #3989

Open PIG208 opened 1 month ago

PIG208 commented 1 month ago

When trying to have switch cases where the record fields each match a certain subtype of a common supertype, it appears that when switching on the destructured variables, the type promotion does not happen. However, as indicated in demoB, it could work without destructuring. Is this the intended behavior?

Example:

class Foo {}

class FooA extends Foo {}

class FooB extends Foo {}

void funA(FooA first, FooA second) {}
void funB(FooB first, FooB second) {}

void demoA(Foo first, Foo second) {
  switch ((first, second)) {
    case ((FooA(), FooA())):
      // The argument type 'Foo' can't be assigned to the parameter type 'FooA'
      funA(first, second);
    case ((FooB(), FooB())):
      // The argument type 'Foo' can't be assigned to the parameter type 'FooB'
      funB(first, second);
  }
}

void demoB(Foo first, Foo second) {
  final record = (first, second);
  switch (record) {
    case ((FooA(), FooA())):
      funA(record.$1, record.$2); // OK
    case ((FooB(), FooB())):
      funB(record.$1, record.$2); // OK
  }
}

Dart info:

- Dart 3.5.0-319.0.dev (dev) (Sun Jun 30 17:06:39 2024 -0700) on "linux_x64"
- on linux / Linux 6.1.0-22-amd64 dart-lang/sdk#1 SMP PREEMPT_DYNAMIC Debian 6.1.94-1 (2024-06-21)

Related:

dart-github-bot commented 1 month ago

Summary: The analyzer incorrectly flags errors when using switch cases with destructured records. Type promotion does not occur for destructured variables, leading to type mismatches even when the case matches the record's type. This behavior is inconsistent with the expected type promotion in non-destructured cases.

srawlins commented 1 month ago

Passing on to language team as I suspect this is specified behavior, and promoting record types according to matching in their fields would be a new language request. I looked for a duplicate and found similar issues, but no perfect duplicates.

lrhn commented 1 month ago

It is working as designed.

You can promote local variables by type-checking an expression consisting just of the variable, but (first, second) is not a variable expression, it's a record value expression. The record's fields may come from variables, but that's a step further removed than what promotion tracks.

Therefore knowing that the record values (first, second) has type (Foo, Foo) does not reflect back on the variables first and second.

Promoting record works, because record is a single local varible, and that's the expression that is being type-checked, so promotion applies.

(Now, could we remember that a record field initialized from a variable is an alias for the variable? Possibly. We'd have to invalidate the aliasing if the variable changes value, without changing the field value. That's a kind of aliasing that could be very general and useful, but also not what we're currently doing.)

gnprice commented 1 month ago

That makes sense, thanks.

I'd like to echo that this would be a very useful feature. Effectively what's desired is a way to switch on two (or more) variables at once, on the lines of demoA above.

The use cases I have in mind would all be equally well served if instead switch grew support for multiple values directly:

  switch (first, second) {
    case (FooA(), FooA()): funA(first, second);
    case (FooB(), FooB()): funB(first, second);
  }

That even looks a bit cleaner, visually, with the double-parentheses gone.

That's probably conceptually less nice than using records, though. The Dart solution for returning multiple values is records, rather than a specific multiple-return feature, so it'd make sense for a solution for switching on multiple values to be records rather than a specific multiway-switch feature.


(Now, could we remember that a record field initialized from a variable is an alias for the variable? Possibly. We'd have to invalidate the aliasing if the variable changes value, without changing the field value. That's a kind of aliasing that could be very general and useful, but also not what we're currently doing.)

With "switch on multiple values" as the use case in mind: I'd be perfectly happy if this aliasing applied only to a record expression (…, …), not to other values of record type. In other words it'd be fine if an example like this:

  final record = (first, second);
  switch (record) {
    case ((FooA(), FooA())):
      funA(first, second); // error

didn't work. I think that limitation will be fairly intuitive for developers to understand, too: after all, first and second aren't there in the expression after switch, so it makes sense they don't get promoted.

Then with that restriction… hmm, I guess that doesn't eliminate the need for invalidation, because one could write something like:

  switch ((first, first = second)) {
    case ((FooA(), FooB()):
      FooA foo = first; // error, and must be error

where the variable is changed by the initializer of a later field in the record expression.

Anyway, if restricting to record expressions does end up making it simpler to spec and implement this feature, I think it'd be a fine restriction to have.

Ing-Brayan-Martinez commented 1 month ago

If you allow me to give my opinion, I think that the correct way in which a pattern match to records should be proposed would be:

void demoB(Foo first, Foo second) {
  final record = (first, second);
  switch (record) {
    case ((FooA(), FooA())):
      funA(record.$1, record.$2); // OK
    case ((FooB(), FooB())):
      funB(record.$1, record.$2); // OK
  }
}

or

void demoB(Foo first, Foo second) {
  final record = (first, second);
  switch (record) {
    (FooA, FooA) x => funA(x.$1, x.$2); // OK
    (FooB, FooB) y => funB(y.$1, y.$2); // OK
  }
}

or

void demoB(Foo first, Foo second) {
  final record = (first, second);
  switch (record) {
    (FooA, FooA) x when x is (FooA, FooA) => funA(x.$1, x.$2); // OK
    (FooB, FooB) y when y is (FooB, FooB) => funB(y.$1, y.$2); // OK
  }
}

It is necessary that the switch statement always maintain a single input parameter to follow the rules of functional programming.

When we try to read this syntax it is easier to understand because it is recognized that we are trying to do a pattern match on an algebraic type

And this design would also be compatible if in the future the Dart language team tried to introduce Tuples it would be completely compatible with this concept that is related to the topic we are discussing

I encourage you to read this article, which was very important to me.

There are possibly more ways to solve this with another pattern matching strategy that is beyond my understanding.