Support Union conversion to a nullable value type#81932
Conversation
|
@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review |
333fred
left a comment
There was a problem hiding this comment.
Haven't reviewed the tests yet, had a few questions on binding impl.
333fred
left a comment
There was a problem hiding this comment.
Done review pass (commit 3). A couple of test comments/suggestions.
| } | ||
| "; | ||
| var comp = CreateCompilation([src, IUnionSource], options: TestOptions.ReleaseExe); | ||
| CompileAndVerify(comp, expectedOutput: "int {10}").VerifyDiagnostics(); |
There was a problem hiding this comment.
Should we log an open question for LDM to verify that this is the behavior we'd prefer to happen, rather than getting an error that explicit conversion requires a cast? #Resolved
There was a problem hiding this comment.
Should we log an open question for LDM to verify that this is the behavior we'd prefer to happen, rather than getting an error that explicit conversion requires a cast?
I don't think so. This behavior was already discussed by the working group and was tentatively approved. The question is already in the speclet, but I am planning to update the "Converions" section as well soon.
|
@RikkiGibson, @333fred, @dotnet/roslyn-compiler Please review |
|
@RikkiGibson, @dotnet/roslyn-compiler For a second review |
1 similar comment
|
@RikkiGibson, @dotnet/roslyn-compiler For a second review |
|
Taking a look now. |
| #endif | ||
| foreach (var ctor in toType.InstanceConstructors) | ||
| { | ||
| if (ctor.OriginalDefinition == (object)constructor.OriginalDefinition) |
There was a problem hiding this comment.
Why is there a need to search for a constructor at this point, instead of using the one from the analysis.Operator?
When is a constructor's OriginalDefinition != this? They can't have type parameters, right? #Resolved
There was a problem hiding this comment.
Why is there a need to search for a constructor at this point, instead of using the one from the
analysis.Operator?
Type arguments of the type could be updated with nullability and that might have effect on the constructor's signature.
When is a constructor's
OriginalDefinition != this?
Always, this is not a method symbol.
They can't have type parameters, right?
The containing type might.
There was a problem hiding this comment.
Always,
thisis not a method symbol.
I was intending to refer to a case where, for example, (object)ctor.OriginalDefinition != ctor. It looks like ctor is a method symbol in that case.
It feels like if the goal is to ensure that the constructor is updated based on a type argument reinference of the toType, then, something like constructor = (MethodSymbol)AsMemberOfType(toType, constructor); would more closely resemble how we accomplish that in other places in NullableWalker.
| static S1? Test2() | ||
| { | ||
| System.Console.Write(""2-""); | ||
| return (S1?)default; |
There was a problem hiding this comment.
nit: I think it would be good if the expectedOutput, reflected whether a "null System.Nullable", versus a "non-null System.Nullable, containing S1 with null value", is produced, in cases 2/3/4. I think I see what is happening from the IL baseline, but, some console output would be helpful, in my opinion. This is not blocking. #Resolved
| var s = new S1(x); | ||
| x.Value.ToString(); | ||
| } | ||
| return default; |
There was a problem hiding this comment.
Should there be a warning here? It feels like if T is non-nullable reference type, then, the caller wouldn't be warned that the returned union may have a null Value, when switching on it without a null case, for example. It's possible I misunderstood something, or, that this kind of gap is expected, and we aren't able to add the desired "smarts" for a union struct type.
#Pending
There was a problem hiding this comment.
I don't think the spec requires a warning here. I'll capture this as a design question.
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM, with some non-blocking comments.
Related to dotnet/csharplang#9902