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

"Add missing case clauses" only inserts first case for switch pattern statements #52180

Closed
DanTup opened this issue Apr 26, 2023 · 8 comments
Closed
Labels
analyzer-quick-fix analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DanTup
Copy link
Collaborator

DanTup commented Apr 26, 2023

With language version 2.19 this works correctly:

Apr-26-2023 14-31-49

But for 3.0 it only inserts one case:

Apr-26-2023 14-32-23

This missing test might be related, though it seems to be looking for a different fix to the one being produced (perhaps it's intended for pre-3.0 code, but probably it shouldn't be failing if that's the case).

@FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/49759')

@DanTup DanTup added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. analyzer-server analyzer-quick-fix labels Apr 26, 2023
@DanTup DanTup changed the title "Add missing case clauses" does not work correctly for switch pattern statements "Add missing case clauses" only inserts first case for switch pattern statements Apr 26, 2023
@bwilkerson
Copy link
Member

Unfortunately, that behavior is the best we can do at the moment. With the switch to patterns we had to write a much more general algorithm to compute exhaustiveness, and that algorithm currently only finds the first case that isn't covered. There have been discussions about extending the algorithm to find all of the missing cases so that we can do a better job, but I don't know what the state of that work is.

@scheglov

@bwilkerson bwilkerson added the P3 A lower priority bug or feature request label May 5, 2023
@deimantasa
Copy link

Any updates to this? It feels like a serious bummer, especially when having several (5+) enum cases to deal with. Auto-fill used to save tons of time.

P.S. Android Studio (Flamingo) does not even generate one case. Only by hovering, first case is hinted.
Screenshot 2023-05-21 at 7 25 19 AM
Screenshot 2023-05-21 at 7 25 50 AM

@pq
Copy link
Member

pq commented Jul 6, 2023

There have been discussions about extending the algorithm to find all of the missing cases so that we can do a better job, but I don't know what the state of that work is.

@scheglov?

@lhengl
Copy link

lhengl commented Jul 12, 2023

Just upgraded to dart 3 and stumbled into this problem. There used to be fixes available for exhaustiveness, now there isn't. With the prevalent use of enum and switch statements, I'm surprised this issue doesn't have more upvotes.

@AbdulazizRasulbek
Copy link

same issue for sealed classes

@bwilkerson
Copy link
Member

bwilkerson commented Jun 13, 2024

This has been fixed by https://dart-review.googlesource.com/c/sdk/+/371062 and should be available in the next stable release.

@DanTup
Copy link
Collaborator Author

DanTup commented Jun 13, 2024

@bwilkerson I tested this and it seems to work great 🥳 but I noticed the test I linked up top is still marked @FailingTest:

@FailingTest(issue: 'https://github.com/dart-lang/sdk/issues/49759')

It fails because of "Unnecessary error filter" (a test bug), but if I remove the WithFilter, it fails with:

Expected to find fix dart.fix.add.missingEnumCaseClauses in
Fix(kind=dart.fix.add.missingSwitchCases, change={"message":"Add missing switch cases","edits":[{"file":"C:\home\test\lib\test.dart","fileStamp":0,"edits":[{"offset":49,"length":0,"replacement":" case E.a:\r\n // TODO: Handle this case.\r\n case E.b:\r\n // TODO: Handle this case.\r\n case E.c:\r\n // TODO: Handle this case.\r\n"}]}],"linkedEditGroups":[],"id":"dart.fix.add.missingSwitchCases"})

It looks like the fix provided is dart.fix.add.missingSwitchCases but this test is for dart.fix.add.missingEnumCaseClauses. It's not clear to me what the difference is here (I thought maybe one is for the original switch statement parsing and the other is since switch expressions were added, but I don't think that explains how any of other other tests in this class would be passing).

Oh, as I was about to send this I noticed all the non-failing tests in that class are marked // @dart=2.19.

So.. should all of the @FailingTests in the class AddMissingEnumCaseClausesTest now be moved into AddMissingSwitchCasesTest_SwitchStatement since it seems for >2.19 they'll all be handled by that fix instead?

@bwilkerson
Copy link
Member

The fix kind DartFixKind.ADD_MISSING_ENUM_CASE_CLAUSES is associated with AddMissingEnumCaseClauses, which is associated with StaticWarningCode.MISSING_ENUM_CONSTANT_IN_SWITCH. That warning code is produced only in pre-pattern-enabled code.

The fix kind DartFixKind.ADD_MISSING_SWITCH_CASES is associated with AddMissingSwitchCases, which is associated with both CompileTimeErrorCode.NON_EXHAUSTIVE_SWITCH_EXPRESSION and CompileTimeErrorCode.NON_EXHAUSTIVE_SWITCH_STATEMENT, both of which are only produced in pattern-enabled code.

... should all of the @FailingTests in the class AddMissingEnumCaseClausesTest now be moved into AddMissingSwitchCasesTest since it seems for >2.19 they'll all be handled by that fix instead?

If the tests are intended to be run with patterns enabled, then yes, they should be moved because they will produce the CompileTimeErrorCode.NON_EXHAUSTIVE_SWITCH_EXPRESSION and CompileTimeErrorCode.NON_EXHAUSTIVE_SWITCH_STATEMENT diagnostics, and AddMissingSwitchCasesTest is where those tests should live.

copybara-service bot pushed a commit that referenced this issue Jun 14, 2024
…ClausesTest` to `AddMissingSwitchCasesTest_SwitchStatement`

The tests in this class were duplicated when pattern support was added, one set
of tests for <= 2.19 and one for > 2.19. The> 2.19 tests were marked as Failing
because this functionality didn't work.

This functionality has now been implemented, but because the error code and fix
are different, these tests continued to fail. This change moves the tests to
the equivalent pattern-enabled fixes tests and removes `@FailingTest` for
those that can now pass.

There are still a few marked `@FailingTest` because of differences in the new
implementation.

See #52180 (comment)

Change-Id: Ie1dfa5bc5c608d38a191507f5f0403d20ba2ef31
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/371684
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Sam Rawlins <srawlins@google.com>
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-quick-fix analyzer-server area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

7 participants