Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expressions of type void should be allowed in switch expressions #52191

Closed
munificent opened this issue Apr 26, 2023 · 10 comments
Closed

Expressions of type void should be allowed in switch expressions #52191

munificent opened this issue Apr 26, 2023 · 10 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. area-front-end Use area-front-end for front end / CFE / kernel format related issues. P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone

Comments

@munificent
Copy link
Member

The language has some fairly ad hoc rules about where expressions of type void are and aren't allowed. The patterns specification makes no mention of disallowing expressions of type void anywhere so I assumed that, by default, they would be allowed. However:

void printBugsSwitch(int n) => switch (n) {
      0 => print('no bugs'),
      1 => print('one bug'),
      _ => print('$n bugs'),
    };

void printBugsConditional(int n) => n == 0
    ? print('no bugs')
    : n == 1
        ? print('one bug')
        : print('$n bugs');

Currently, this produces on analyzer:

Analyzing temp.dart...                 0.5s

  error • temp.dart:2:12 • This expression has a type of 'void' so its value can't be used. Try checking to see if
          you're using the correct API; there might be a function or call that returns void you didn't expect. Also check type
          parameters and variables which might also be void. • use_of_void_result
  error • temp.dart:3:12 • This expression has a type of 'void' so its value can't be used. Try checking to see if
          you're using the correct API; there might be a function or call that returns void you didn't expect. Also check type
          parameters and variables which might also be void. • use_of_void_result
  error • temp.dart:4:12 • This expression has a type of 'void' so its value can't be used. Try checking to see if
          you're using the correct API; there might be a function or call that returns void you didn't expect. Also check type
          parameters and variables which might also be void. • use_of_void_result

And if you try to run it:

temp.dart:2:12: Error: This expression has type 'void' and can't be used.
      0 => print('no bugs'),
           ^
temp.dart:3:12: Error: This expression has type 'void' and can't be used.
      1 => print('one bug'),
           ^
temp.dart:4:12: Error: This expression has type 'void' and can't be used.
      _ => print('$n bugs'),
           ^

Note that there are no errors in printBugsConditional(), which has similar behavior using conditional expressions. I think switch expressions should behave similarly to conditional expressions.

I'm going to put this in the stable milestone so that triaging folks take a look at it. This is an annoying restriction (at least two users have already noticed it), but I don't know if it's worth trying to cherry pick a fix for it. Since fixing this bug is removing a restriction, it's a non-breaking change that we could do in 3.0.1 or 3.1 if needed. Personally, I would lean towards not trying to cherry pick a fix, but I'm happy to defer to implementers (who know how hard/risky this is to fix) and leads (who know how much risk we want to accept).

Related language issue: dart-lang/language#2907

cc @dart-lang/language-team

@munificent munificent added this to the Dart 3 stable milestone Apr 26, 2023
@munificent
Copy link
Member Author

cc @johnniwinther @scheglov

@munificent munificent added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-front-end Use area-front-end for front end / CFE / kernel format related issues. area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. labels Apr 26, 2023
@johnniwinther
Copy link
Member

I'll take a look at the CFE side of this.

@mit-mit
Copy link
Member

mit-mit commented Apr 27, 2023

Is this different from dart-lang/language#2907 ?

@lrhn
Copy link
Member

lrhn commented Apr 27, 2023

Yes. Or it's a subset, but not the most important subset.

dart-lang/language#2907 mentions expression in new syntactic locations, like:

  • The switch expression itself (switch (<here>) ...), for the new expression switch.
  • The pattern constants values case <here>, case const (<here>), case == <here>.

These are new syntactic locations for expressions where the value is pretty certainly used, so they should likely not be allowed to be expressions with static type void.

It didn't mention the expression switch case result expressions, which it probably should, but they are still within our current framework, we just need to say "yes" to those, like we usually do for expressions in tail position (the value of this expression becomes the value of the surrounding expression, without anyone looking at the value on the way).

The current void-prevention scheme is about which expression positions are allowed to have type void.
Then dart-lang/language#2907 points to another issue, where a value is produced by something which is not an expression like case Foo(bar: var x) where Foo.bar has static type void, but matchedValue.bar is not an expression anywhere in that program.

We don't have an existing framework to put that into, so that's the real meat of the issue.
We'll need to introduce rules for when a matched value type is allowed to be void.
(Suggested answer, only for non-checking, non-binding patterns like _).

@scheglov
Copy link
Contributor

@scheglov scheglov added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Apr 27, 2023
copybara-service bot pushed a commit that referenced this issue Apr 27, 2023
…tchExpressionCase.

Bug: #52191
Change-Id: I2cea3260ad63a0c59399f02dadb99bb09a944623
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/299200
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@scheglov
Copy link
Contributor

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

@munificent
Copy link
Member Author

I consider #270 to be the issue to discuss whether we want to change the specification of which expression contexts related to patterns allow void.

I filed this issue to track the fact that the implementations currently disallow void expressions in a context where the spec doesn't forbid it.

copybara-service bot pushed a commit that referenced this issue Apr 28, 2023
This allows void typed switch expressions by allowed void typed
expressions in all subexpressions inferred through the shared type
analysis.

This might lead to allowing void in intended places but the current
approach employed by the CFE doesn't really scale so we need to revisit
it anyway.

In response to #52191

Change-Id: Iee53670d3c316764cfc1309dc76cf37bb3b03629
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/299020
Reviewed-by: Chloe Stefantsova <cstefantsova@google.com>
Commit-Queue: Johnni Winther <johnniwinther@google.com>
@johnniwinther
Copy link
Member

CFE fix landed in https://dart-review.googlesource.com/c/sdk/+/299020

@scheglov
Copy link
Contributor

CP request for the analyzer.
#52218

@leafpetersen
Copy link
Member

I'll prepare a CP request for the CFE fix. Note that we have decided to push the cherry pick to the first hot fix release, since the fix will be non-breaking to land.

copybara-service bot pushed a commit that referenced this issue May 15, 2023
Fixes #52234

Bug:#52191
Change-Id: I74d749115ec8cab0a89416ca905aa9c4e0d83ccc
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/299020
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/303020
Commit-Queue: Leaf Petersen <leafp@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>
copybara-service bot pushed a commit that referenced this issue May 15, 2023
…xpressionCase.

Bug: #52191
Change-Id: I2cea3260ad63a0c59399f02dadb99bb09a944623
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/299200
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/302880
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. area-fe-analyzer-shared Assigned by engineers; when triaging, prefer either area-front-end or area-analyzer. area-front-end Use area-front-end for front end / CFE / kernel format related issues. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

6 participants