Implement exhaustiveness check for Union types#81442
Conversation
|
@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review #Resolved |
|
@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review #Resolved |
|
@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review #Resolved |
|
I have begun reviewing this, will need till tomorrow to finish. Thanks #Resolved |
|
@333fred, @RikkiGibson, @dotnet/roslyn-compiler Please review #Resolved |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM, only had some nits and a question.
When I proceed with closed class exhaustiveness checking I may try cherry picking the first commit as a starting point, I think it should be possible to add another implementation of ITypeUnionValueSetFactory for closed classes.
| { | ||
| // We don't know for sure that test for T2 is going to match, but if it fails, | ||
| // that means that some previous test must match and we simply cannot get to this test. | ||
| // In other words, this test is either unreachanble or it will succeed. |
There was a problem hiding this comment.
| // In other words, this test is either unreachanble or it will succeed. | |
| // In other words, this test is either unreachable or it will succeed. | |
| ``` #Resolved |
| falseTestImpliesTrueOther = true; | ||
| break; | ||
| case BoundDagTypeTest _: | ||
| case BoundDagTypeTest t: |
There was a problem hiding this comment.
nit: this t looks unused #Resolved
| { | ||
| if (easyOutForLowering != (object)planA && !forLowering && ValueSetFactory.TypeUnionValueSetFactoryForInput(planA.Input) is not null) | ||
| { | ||
| // We need a test about `null` peresent in the Dag to properly handle exhaustiveness |
There was a problem hiding this comment.
| // We need a test about `null` peresent in the Dag to properly handle exhaustiveness | |
| // We need a test about `null` present in the Dag to properly handle exhaustiveness | |
| ``` #Resolved |
|
|
||
| string tryHandleTypeUnionLimits() | ||
| { | ||
| if (evaluations.IsEmpty && ValueSetFactory.TypeUnionValueSetFactoryForInput(input) is { } factory && |
There was a problem hiding this comment.
To check my understanding, and, in case it helps the second reviewer:
It looks like evaluations.IsEmpty ensures that we don't propose one of our unmatched types as an example pattern, in cases where we would move on to find a more suitable example further along in the process, For example, in test Exhaustiveness_04, this allows us to propose pattern false as an example rather than pattern bool. #Resolved
|
|
||
| private static bool AnyTypeFromUnionMightMatch(ImmutableArray<TypeSymbol> typesInUnion, TypeSymbol type, ConversionsBase conversions, ref CompoundUseSiteInfo<AssemblySymbol> useSiteInfo) | ||
| { | ||
| Debug.Assert(!typesInUnion.IsEmpty); |
There was a problem hiding this comment.
I didn't follow how it is that we are certain of this. The other two assertions I understand, but, I didn't follow why it is not possible for a pattern matching against an empty union to make it here. #Resolved
There was a problem hiding this comment.
I didn't follow how it is that we are certain of this. The other two assertions I understand, but, I didn't follow why it is not possible for a pattern matching against an empty union to make it here.
It is not possible because any type test for an empty Union will result in an ERR_PatternWrongType and, since we are forcing a null check first, we end up with an empty value set after it. However, it is better to not create a value set factory for an empty Union type. I am going to make this change.
|
@333fred, @dotnet/roslyn-compiler For a second review |
|
@333fred, @dotnet/roslyn-compiler For a second review |
333fred
left a comment
There was a problem hiding this comment.
Mostly LGTM. Couple of test suggestions and one small question.
| Diagnostic(ErrorCode.ERR_PatternWrongType, "C3").WithArguments("S1", "C3").WithLocation(28, 46), | ||
| // (33,57): hidden CS9335: The pattern is redundant. | ||
| // return u switch { int => 1, I1 => 2, null => 3, C2 => 4 }; | ||
| Diagnostic(ErrorCode.HDN_RedundantPattern, "C2").WithLocation(33, 57) |
There was a problem hiding this comment.
Does the prototype comment in _05 cover this diagnostic as well? #Resolved
There was a problem hiding this comment.
Does the prototype comment in _05 cover this diagnostic as well?
No. This case is a known behavior of the redundancy checker, that is why it is a hidden diagnostic.
It can be observed in the scenario below, for example. Basically the checker often "complains" if a pattern
always matches, suggesting to use _ instead. The case in _05 is different, replacing the reported
pattern with _ wouldn't be semantically equivalent, hence the comment there.
[Fact]
public void Test()
{
var src = @"
struct S1
{
public bool Value => false;
}
class Program
{
int M(S1 x)
{
return x switch {{Value: true} => 1, {Value: false} => 2};
}
}
";
var comp = CreateCompilation(src);
CompileAndVerify(comp).VerifyDiagnostics(
// (11,54): hidden CS9335: The pattern is redundant.
// return x switch {{Value: true} => 1, {Value: false} => 2};
Diagnostic(ErrorCode.HDN_RedundantPattern, "false").WithLocation(11, 54)
);
}
There was a problem hiding this comment.
Interesting. Thanks for the explanation, I would not have assumed this was by-design.
There was a problem hiding this comment.
Makes sense, I forgot about this behavior.
|
@RikkiGibson I think I responded to your feedback/questions. Please let me know if you have additional feedback. Otherwise, I will rebase the branch to prepare it for merging. |
First commit is a change that hopefully could be shared with "Closed Hierarchies" feature.