Nullable analysis of conditional operator should use best common type unless target typed #73047
Conversation
… unless target typed
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| hasErrors = true; | ||
| } | ||
| else | ||
| { | ||
| trueExpr = GenerateConversionForAssignment(bestType, trueExpr, diagnostics); | ||
| falseExpr = GenerateConversionForAssignment(bestType, falseExpr, diagnostics); | ||
| hasErrors = trueExpr.HasAnyErrors || falseExpr.HasAnyErrors; | ||
| // If one of the conversions went wrong (e.g. return type of method group being converted | ||
| // didn't match), then we don't want to use bestType because it's not accurate. | ||
| type = hasErrors ? CreateErrorType() : bestType; |
There was a problem hiding this comment.
To fix the bug, it seems sufficient to either take this change (to use the best common type in binding always), or the change in NullableWalker (to use best common in nullable analysis always). But both changes seemed appropriate to me.
There was a problem hiding this comment.
Let me check my understanding.
- In this path, bestType is not null and not ErrorType. If one of the conditional expr operands had an ErrorType, we would have inferred ErrorType as the bestType.
- The removed lines here are checking if there is some error within the operands, which didn't cause them to have an ErrorType, per (1). We previously thought in this case that we need to change the conditional-expr type to ErrorType in this case. Now we are simply keeping the bestType which we already determined in the face of such errors.
Basically I'm not seeing what purpose the original check was serving, it feels like this change is just propagating along reasonable type information, which allows our assumptions about the bound tree shape to be met when we reach nullable analysis.
There was a problem hiding this comment.
The intent, from the comment, seems to be that we are doing a conversion check from the operands to the best type, which can fail, and I also find that a bit confusing. One issue here is that we are not checking specifically if the conversion had errors, we are checking for any kind of errors within the whole operand.
Essentially if one of the operands doesn't convert to the "best type", why was that chosen as the "best type"? Are there some invariants in best type analysis we can rely on in this regard?
There was a problem hiding this comment.
BestTypeInferrer.InferBestTypeForConditionalOperator() and GenerateConversionForAssignment() are both using ClassifyImplicitConversionFromExpression(), so it's not clear when the GenerateConversionForAssignment() calls above would fail if the best common type is not an error type.
| @@ -5867,7 +5867,7 @@ void makeAndAdjustReceiverSlot(BoundExpression receiver) | |||
|
|
|||
| TypeSymbol? resultType; | |||
| bool wasTargetTyped = node is BoundConditionalOperator { WasTargetTyped: true }; | |||
| if (node.HasErrors || wasTargetTyped) | |||
| if (wasTargetTyped) | |||
There was a problem hiding this comment.
Would it make sense to change the condition on 5898 if (!wasTargetTyped) to Debug.Assert(!wasTargetTyped)? #Resolved
There was a problem hiding this comment.
I've added an assert rather than replacing the if.
| IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: ?, IsInvalid, IsImplicit) (Syntax: 'b ? &i1 : &i2') | ||
|
|
||
| Right: | ||
| IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: System.Int32*, IsInvalid, IsImplicit) (Syntax: 'b ? &i1 : &i2') |
There was a problem hiding this comment.
It looks like this is the meaningful change in here. I didn't notice it until after looking at it for a bit.
Perhaps we should also exercise the difference in semanticModel.GetTypeInfo(conditionalExprSyntax) as a result of this change. #Resolved
| hasErrors = true; | ||
| } | ||
| else | ||
| { | ||
| trueExpr = GenerateConversionForAssignment(bestType, trueExpr, diagnostics); | ||
| falseExpr = GenerateConversionForAssignment(bestType, falseExpr, diagnostics); | ||
| hasErrors = trueExpr.HasAnyErrors || falseExpr.HasAnyErrors; | ||
| // If one of the conversions went wrong (e.g. return type of method group being converted | ||
| // didn't match), then we don't want to use bestType because it's not accurate. | ||
| type = hasErrors ? CreateErrorType() : bestType; |
There was a problem hiding this comment.
Let me check my understanding.
- In this path, bestType is not null and not ErrorType. If one of the conditional expr operands had an ErrorType, we would have inferred ErrorType as the bestType.
- The removed lines here are checking if there is some error within the operands, which didn't cause them to have an ErrorType, per (1). We previously thought in this case that we need to change the conditional-expr type to ErrorType in this case. Now we are simply keeping the bestType which we already determined in the face of such errors.
Basically I'm not seeing what purpose the original check was serving, it feels like this change is just propagating along reasonable type information, which allows our assumptions about the bound tree shape to be met when we reach nullable analysis.
|
@dotnet/roslyn-compiler for a second review, thanks. |
| var model = comp.GetSemanticModel(tree); | ||
| var expr = tree.GetRoot().DescendantNodes().OfType<IdentifierNameSyntax>().Last(); | ||
| Assert.Equal("y", expr.ToString()); | ||
| _ = model.GetSymbolInfo(expr); |
There was a problem hiding this comment.
Consider verifying the result of GetSymbolInfo on the conditional expression since the change to BoundConditionalOperator.NaturalTypeOpt is surfaced through .Type from this API #Resolved
Fixes #72898