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

Not handling all combinations of named enum values and booleans should give a CS8509 warning regardless of order, even if warnings are disabled for CS8524 #64698

Closed
olatoft opened this issue Oct 13, 2022 · 4 comments
Assignees
Labels
Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@olatoft
Copy link

olatoft commented Oct 13, 2022

[Summary(jcouv):] The ask is to report exhaustiveness problems using named enum values (over unnamed ones) where possible.

Version Used:
dotnet version 6.0.110
Microsoft (R) Build Engine version 17.0.1+b177f8fa7 for .NET

Steps to Reproduce:

A CS8524 warning is given if we forget to handle an unnamed enum value, for instance (Values)2. I want to disable warnings for CS8524, in order to get a warning, namely CS8509, if and only if I forget to handle a named enum value, or a combination of named enum values and booleans.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net6.0</TargetFramework>
    <RootNamespace>test_exhaustive</RootNamespace>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <NoWarn>CS8524</NoWarn>
  </PropertyGroup>

</Project>

The following, with Value1 at the top, gives "warning CS8509: The switch expression does not handle all possible values of its input type (it is not exhaustive). For example, the pattern '(Values.Value1, false)' is not covered.", as I expect:

var test1 = (Values.Value0, true) switch
{
    (Values.Value1, true) => true,
    (Values.Value0, true) => true,
    (Values.Value0, false) => true,
};

enum Values
{
    Value0,
    Value1
}

The following, with the boolean first, also gives the same warning, as I expect:

var test2 = (true, Values.Value0) switch
{
    (true, Values.Value0) => true,
    (false, Values.Value0) => true,
    (true, Values.Value1) => true,
};

However, the following, with Value1 at the bottom and the boolean last, gives no warnings, which I do not expect:

var test3 = (Values.Value0, true) switch
{
    (Values.Value0, true) => true,
    (Values.Value0, false) => true,
    (Values.Value1, true) => true,
};

If I do handle some unnamed enum values without handling (Values.Value1, false), regardless of whether warnings for CS8524 are disabled or not, I do get the same warning as in test1 and test2, as I expect:

var test4 = (Values.Value0, true) switch
{
    (Values.Value0, true) => true,
    (Values.Value0, false) => true,
    (Values.Value1, true) => true,
    (_, true) => true,
};

Expected Behavior:
A CS8509 warning telling me that I forgot to handle the pattern (Values.Value1, false) for all of the above examples, regardless of order.

Actual Behavior:
No warning for test3, even if not all combinations of named enum values and booleans are handled.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 13, 2022
@jcouv jcouv added the Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. label Nov 9, 2022
@jcouv jcouv self-assigned this Nov 9, 2022
@jcouv jcouv added 4 - In Review A fix for the issue is submitted for review. and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 9, 2022
@jcouv jcouv added this to the 17.5 milestone Nov 9, 2022
@jcouv jcouv added Resolution-By Design The behavior reported in the issue matches the current design and removed 4 - In Review A fix for the issue is submitted for review. labels Nov 9, 2022
@jcouv
Copy link
Member

jcouv commented Nov 9, 2022

From discussion with team members, this is by design. The compiler's commitment is to provide a diagnostic, but exactly which diagnostic is a question that's reserved and we keep flexibility on that.
I'll go ahead and close as by-design.

@jcouv jcouv closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2022
@olatoft
Copy link
Author

olatoft commented Nov 10, 2022

Thanks for looking into the issue :) I'm sad to see it closed by design, because this is a feature I would love to have. However, I understand your arguments about complexity and being careful about adding strong guarantees.

Is there any other way in C# to get a warning, or ideally an error, if I forget to handle all cases of a discriminated union and combinations of discriminated unions? From what I now know, it doesn't seem to be possible with named enum values and booleans, unless I manually make sure to put them in the right order, and it doesn't seem to be possible with subclasses, as described in #64497. Would it be possible to implement such a feature in Roslyn? Or would it require a change in the C# language design? Or is it simply not realistic to expect such a feature in C#?

@jcouv
Copy link
Member

jcouv commented Nov 10, 2022

We are actively discussing some design options around closed type hierarchies and discriminated unions. One of the proposals is to add a [Closed] on a type hierarchy or enum, which would inform switch and patterns that the set of possible values is restricted.
Working group notes are posted about every other week in https://github.com/dotnet/csharplang/tree/main/meetings/working-groups/discriminated-unions

@olatoft
Copy link
Author

olatoft commented Nov 11, 2022

That's exciting to hear! Thanks for the info, and good luck with the work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants