Poor ergonomics when a package developer uses a private enumerated value to prevent exhaustive matching.

[stereotype441]

This came up in a discussion with @brianquinlan.

Sometimes a package author wants to prevent clients from exhaustively switching on an enumerated type, so that in the future they can add enumerated values without breaking clients. One way to do this is to create a dummy private enumerated value (e.g. _unknown); this ensures that it's impossible for a client to exhaustively switch on the enum type; they have to use a default clause or a wildcard pattern.

Unfortunately, this approach isn't very ergonomic, because if the client tries to exhaustively match such an enum type:

• The problem message they get is "The type TYPE isn't exhaustively matched by the switch cases since it doesn't match the pattern 'TYPE._unknown'". This seems to imply that the user ought to be able to fix the problem by adding a switch case for TYPE._unknown, but they can't.
• The correction message they get is "Try adding a wildcard pattern or cases that match 'TYPE._unknown'." Again, adding a case that matches TYPE._unknown isn't possible.
• The analysis server offers a quick fix "Add missing switch cases", which does nothing. Instead, a quick fix should be offered that adds a default clause or a wildcard pattern.

[bwilkerson]

I like all of those suggestions. The easiest way to implement it is to add an error code that shares a name with the existing code and to use the new code when the constant has a private name.

There's an open question about what to do if there are multiple unmatched constants, some with public names and some with private names. I'm guessing we want to use the existing code so that users can see the public names, and for "Add missing switch cases" to add explicit cases for the public names and a default for the rest, but maybe not?

Also, adding the default clause means that the user won't be told when there are other public names added, which seems unfortunate. There might not be a solution for that problem, though.

[bwilkerson]

Another possible solution would be to support an annotation or pragma that indicates that the enum isn't intended to be exhaustive.

[FMorschel]

The analysis server offers a quick fix "Add missing switch cases", which does nothing. Instead, a quick fix should be offered that adds a default clause or a wildcard pattern.

I'll take a look at this.

[FMorschel]

Okay, it seems that #60531 already handles this. I've added some tests specifically with the example described above to be sure this isn't regressed.

Here is the CL: https://dart-review.googlesource.com/c/sdk/+/459760. I've added you as a reviewer, @stereotype441, so you can let me know there if I missed anything or if you can still repro this with another example on master, thanks!

Also, adding the default clause means that the user won't be told when there are other public names added, which seems unfortunate. There might not be a solution for that problem, though.

Yes, unfortunately. Maybe we could add a lint for warning when the public names aren't all fully exhaustive? So the user can add // ignore: if they want to dismiss but still get notified if new cases come up?

• The problem message they get is "The type TYPE isn't exhaustively matched by the switch cases since it doesn't match the pattern 'TYPE._unknown'". This seems to imply that the user ought to be able to fix the problem by adding a switch case for TYPE._unknown, but they can't.
• The correction message they get is "Try adding a wildcard pattern or cases that match 'TYPE._unknown'." Again, adding a case that matches TYPE._unknown isn't possible.

I'll take a look and see if I can think of a better message for this.

(Speaking of messages, I've pinged you on Discord about #60093, @stereotype441. Would you mind taking a look at it? Thanks!)

[FMorschel]

I've made an attempt at the other two things at https://dart-review.googlesource.com/c/sdk/+/459780.

[FMorschel]

Since I can't upload files to Gerrit, I'll ask this here.

While following the instructions to run dart --enable-asserts pkg/front_end/test/coverage_suite.dart --tasks=5 --add-and-remove-comments, it ends with:

stdout> Formatted D:\dev\clones\dart-sdk\sdk\pkg\front_end\lib\src\type_inference\type_demotion.dart
readAsBytes':
        return new Future.value(new io.File.fromUri(uri).readAsBytesSync());
                   ^^^^^^


  Expected 100% coverage, but got 97.14285714285714% (34 hits and 1 misses).

  To re-run this test, run:

     dart --enable-asserts pkg/front_end/test/coverage_suite.dart --tasks=5

     Or automatically insert ignore comments via

     dart --enable-asserts pkg/front_end/test/coverage_suite.dart --tasks=5 --add-and-remove-comments

  If that does not work, create a bug report and approve the failure.
Log files written to D:\dev\clones\dart-sdk\sdk\results.json and D:\dev\clones\dart-sdk\sdk\logs.json
Entire run took 0:01:55.251909.
Warning: At least one processes exited with a non-0 exit code.

results.json

CC @johnniwinther

I'm not sure what I should do here. I've locally rebased to 2d2b854 and ran gclient sync too before running the command.

[johnniwinther]

@FMorschel It doesn't seem to work for me either, though I have no info other than Warning: At least one processes exited with a non-0 exit code. as to why.

@jensjoha Do you know what is happening?

[johnniwinther]

I looks like 2d2b854 have made the coverage collection to crash VM. I'll revert it.

[jensjoha]

Was just running the thing, and first looking into the non-0 exit code --- got a "-6" and wondered what that meant 🙄 .

[FMorschel]

I saw you reverted it at 6d53170. Do I need to use a new dart built after this? Or just rebasing should work? I've rebased and still got the same error.

[stereotype441]

I saw you reverted it at 6d53170. Do I need to use a new dart built after this? Or just rebasing should work? I've rebased and still got the same error.

I was able to make it work by rebuilding the SDK and then running the coverage command using my built version of Dart. For example, for me (on Linux/x86), the command was:

out/ReleaseX64/dart-sdk/bin/dart --enable-asserts pkg/front_end/test/coverage_suite.dart --tasks=5 --add-and-remove-comments

Also, FYI, since https://dart-review.googlesource.com/c/sdk/+/461440 has landed, you'll need to rerun the diagnostic message code generator again (sorry!)