Misc. union changes#82538
Conversation
Align implementation with recent design changes: - Case types are not nullable types (constructor parameter types are unwrapped) - Default nullability of a `Value` property no longer inferred from constructors. Annotations on the property declaration itself are used as for any regular property.
|
@RikkiGibson, @dotnet/roslyn-compiler For a second review |
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM with a comment and question.
| @@ -22,7 +23,8 @@ public UnionTypeTypeUnionValueSetFactory(NamedTypeSymbol unionType) | |||
|
|
|||
| private ImmutableArray<TypeSymbol> AdjustedTypesInUnion() | |||
There was a problem hiding this comment.
Not blocking: it feels reasonable to remove this helper if we don't end up needing to actually do an adjustment here.
| { | ||
| public S1(int x) => throw null!; | ||
| public S1(bool? x) => throw null!; | ||
| public S1(bool x) => throw null!; |
There was a problem hiding this comment.
Do we have a test where we try to convert null to a union like this?
I'm trying to understand the consequences of using a System.Nullable<T> constructor parameter versus using a T now. It looks like it no longer affects the case types of the union, but it might affect whether null can be converted to the union type? Is that right?
There was a problem hiding this comment.
Do we have a test where we try to convert
nullto a union like this?
I assume you are referring to a union with Nullable<T> as constructor parameter. It doesn't look like we have a test like that, I will add one in another PR.
I'm trying to understand the consequences of using a
System.Nullable<T>constructor parameter versus using aTnow. It looks like it no longer affects the case types of the union, but it might affect whethernullcan be converted to the union type? Is that right?
That is correct.
This reverts commit f67de25.
Align implementation with recent design changes:
Valueproperty is no longer inferred from constructors. Annotations on the property declaration itself are used as for any regular property.