Ensure initial binding always binds to natural type #48205
Conversation
|
@dotnet/roslyn-compiler for reviews please. |
src/Compilers/CSharp/Test/Semantic/Semantics/ConditionalOperatorTests.cs
Show resolved
Hide resolved
|
@dotnet/roslyn-compiler for a second review please. |
I think @gafter added an assert somewhere in flow analysis to catch all the nodes that haven't been properly adjusted. I am curious if these cases are caught by that assert. If they are not, perhaps it would be useful to understand why and adjust the assert accordingly. #Closed |
| @@ -3165,6 +3165,10 @@ private BoundExpression BindArrayDimension(ExpressionSyntax dimension, Diagnosti | |||
| hasErrors = true; | |||
| } | |||
| } | |||
| else | |||
| { | |||
| size = BindToNaturalType(size, diagnostics); | |||
There was a problem hiding this comment.
BindToNaturalType [](start = 27, length = 17)
BindToTypeForErrorRecovery #Closed
| @@ -3165,6 +3165,10 @@ private BoundExpression BindArrayDimension(ExpressionSyntax dimension, Diagnosti | |||
| hasErrors = true; | |||
| } | |||
| } | |||
| else | |||
| { | |||
| size = BindToNaturalType(size, diagnostics); | |||
There was a problem hiding this comment.
diagnostics [](start = 51, length = 11)
I don't think we want to collect diagnostics on this code path. #Closed
|
Done with review pass (iteration 4) #Closed |
Running the added tests without my fix triggers that assert. It looks like we didn't have test coverage for this path. It also looks like the path is missing checking for unconverted address of operators (it was added in Neal's branch, which was in development at the same time as mine, so the address of operator was never added). I'll add that and a test here, as that would also crash. |
Actually, I'm going to do this change on master. It looks like it's all handled appropriately currently. |
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1198816/, which is about
CSharpOperationFactory.CreateInternalbeing passed a node it does not expect. I looked through all the places in the binder where we useBindValueand ensured the results were being correctly bound to a natural type, even if there was some error. I found 2 such cases:ifconditions were not being bound to their natural type if they had some error. This is the case that results in the specific stack pattern from the watson, where aBoundConversionsat on top of aBoundUnconvertedObjectCreationExpression.In both of these cases, unconverted results were passed to the factory, which cannot handle nodes of these types and threw.