Unions: Align implementation with recent design changes around case types and default nullability - #83161
Conversation
This reverts commit f67de25.
…ypes and default nullability See: - dotnet/csharplang#10092 - dotnet/csharplang#10098
|
@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler Please review |
|
@RikkiGibson, @jjonescz, @dotnet/roslyn-compiler Please review |
RikkiGibson
left a comment
RikkiGibson
left a comment
There was a problem hiding this comment.
I didn't see test coverage for the following scenario mentioned in dotnet/csharplang#10092:
When a union creation member is called (explicitly or through a union conversion), the new union's
Valueproperty gets the null state of the incoming value.
It's possible I missed it, or, it is meant to be implemented in a later PR.
There is no change in the quoted rule. The diff is somewhat confusing, a new bullet was added before it. The rule was implemented earlier with relevant tests and remains implemented now (this PR didn't make any changes around it). |
|
@jjonescz, @dotnet/roslyn-compiler For a second review |
| if (NamedTypeSymbol.IsSuitableUnionConstructor(ctor)) | ||
| { | ||
| var parameter = ctor.Parameters[0]; | ||
| result = result.Join(GetParameterState(parameter.TypeWithAnnotations, parameter.FlowAnalysisAnnotations).State); |
There was a problem hiding this comment.
Do we have tests where the parameter has nullability annotations via attributes like [NotNull]? #Resolved
There was a problem hiding this comment.
Do we have tests where the parameter has nullability annotations via attributes like
[NotNull]?
There is a test - NullableAnalysis_04_State_From_Default_PostCondition.
And there is an open design question whether the spec should be adjusted for cases like this.
There was a problem hiding this comment.
I would expect some test like that to be changed by this PR though since we added this new code path.
There was a problem hiding this comment.
Since we are not paying attention to the attributes (per spec) there is no change in behavior. We didn't look at them and we continue not looking at the attributes now. NullableAnalysis_04_State_From_Default_PostCondition demonstrates that.
| // For union types where none of the case types are nullable, the default state for Value is "not null" rather than "maybe null". | ||
| var result = NullableFlowState.NotNull; | ||
|
|
||
| foreach (var ctor in unionType.InstanceConstructors) |
There was a problem hiding this comment.
Do we need to handle IUnionMembers factory methods too? #Resolved
There was a problem hiding this comment.
Do we need to handle IUnionMembers factory methods too?
Yes we need to handle them too, but I am not planning to do that in this PR. At the moment support for factory methods is limited to calculation of case types. Other consumption scenarios are not implemented
3 participants
See:
TryGetValue(Nullable<T>)methods for a pattern that implies checking for a specific typeTcsharplang#10098The first commit is a revert of #82538.
Microsoft Reviewers: Open in CodeFlow