Fix error recovery for various nested conversion scenarios #74089

RikkiGibson · 2024-06-20T22:38:59Z

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2088611

We had various places where we visited the underlying conversions of a conversion, but did not call MarkUnderlyingConversionsChecked. This caused assert failures in various nested conversion scenarios (exercised in new tests). It seems like this method is just a sanity check to ensure we actually traversed the whole conversion "tree" and should be called whenever we are applying the nested conversions to the corresponding subexpressions. See pre-existing call sites of the method for comparison.
We were not calling BindToNaturalType for the operand of a user-defined conversion which was not valid e.g. due to ambiguity. This results in unexpected unconverted target-typed expressions in the bound tree in later phases.
Increased uniformity of NullableWalker assertions for switch expressions versus conditional operators. This fixed assertion failures in some of the new tests.
NullableWalker would report nullability warnings for user-defined conversion arguments, when the conversion was not valid in initial binding anyway. Note that a user-defined conversion may exist but still not be valid, when it is ambiguous which user-defined conversion method should be used. Entering this code path in such cases was causing an assertion to fail and is not needed.

RikkiGibson · 2024-06-21T17:11:43Z

@dotnet/roslyn-compiler for review

AlekseyTs · 2024-06-24T13:05:18Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -683,6 +683,7 @@ private static void CheckInlineArrayTypeIsSupported(SyntaxNode syntax, TypeSymbo
                }

                var elementConversions = conversion.UnderlyingConversions;
+                conversion.MarkUnderlyingConversionsChecked();


conversion.MarkUnderlyingConversionsChecked();

Consider moving this after the work of checking the underlying conversions is done. I assume that is after the loop below. #Closed

BTW, it is not clear to me what is this operation going to change. It look like we are mutating a local value of a struct, which is not returned or stored anywhere.

it is not clear to me what is this operation going to change

I guess this operation is mutating a class instance referenced by the struct, therefore, the mutation is preserved even when the struct value is not preserved.
Is this change backed by a test?

If we do not make this call, then newly added test Tuple_UserDefinedConversion_CollectionExpression_NotAmbiguous will fail in AssertUnderlyingConversionsChecked.

AlekseyTs · 2024-06-24T13:38:08Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1645,6 +1646,7 @@ internal bool TryGetCollectionIterationType(SyntaxNode syntax, TypeSymbol collec
            bool targetTyped = conversionIfTargetTyped is { };
            Debug.Assert(targetTyped || destination.IsErrorType() || destination.Equals(source.Type, TypeCompareKind.ConsiderEverything));
            ImmutableArray<Conversion> underlyingConversions = conversionIfTargetTyped.GetValueOrDefault().UnderlyingConversions;
+            conversionIfTargetTyped.GetValueOrDefault().MarkUnderlyingConversionsChecked();


conversionIfTargetTyped.GetValueOrDefault()

Consider extracting a local and reusing. #Closed

AlekseyTs · 2024-06-24T13:38:50Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1645,6 +1646,7 @@ internal bool TryGetCollectionIterationType(SyntaxNode syntax, TypeSymbol collec
            bool targetTyped = conversionIfTargetTyped is { };
            Debug.Assert(targetTyped || destination.IsErrorType() || destination.Equals(source.Type, TypeCompareKind.ConsiderEverything));
            ImmutableArray<Conversion> underlyingConversions = conversionIfTargetTyped.GetValueOrDefault().UnderlyingConversions;
+            conversionIfTargetTyped.GetValueOrDefault().MarkUnderlyingConversionsChecked();


MarkUnderlyingConversionsChecked

Consider doing this after the conversions are actually checked. #Closed

Is this change backed by a test?

If we do not make this call, then newly added test Tuple_UserDefinedConversion_ConditionalExpression_NotAmbiguous will fail in AssertUnderlyingConversionsChecked.

AlekseyTs · 2024-06-24T13:39:59Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1682,6 +1684,7 @@ private BoundExpression ConvertSwitchExpression(BoundUnconvertedSwitchExpression
            Conversion conversion = conversionIfTargetTyped ?? Conversion.Identity;
            Debug.Assert(targetTyped || destination.IsErrorType() || destination.Equals(source.Type, TypeCompareKind.ConsiderEverything));
            ImmutableArray<Conversion> underlyingConversions = conversion.UnderlyingConversions;
+            conversion.MarkUnderlyingConversionsChecked();


MarkUnderlyingConversionsChecked

Consider doing this after the conversions are actually checked. #Closed

Is this change backed by a test

If we do not make this call, then newly added test Tuple_UserDefinedConversion_SwitchExpression_NotAmbiguous will fail in AssertUnderlyingConversionsChecked.

AlekseyTs · 2024-06-24T13:52:17Z

Done with review pass (commit 1)

RikkiGibson · 2024-06-24T23:21:30Z

It looks like some test runs are timing out. I would be surprised if just the changes in c39c918 were causing this. Following up with infraswat.

src/Compilers/CSharp/Test/Semantic/Semantics/UserDefinedConversionTests.cs

AlekseyTs

LGTM (commit 3)

RikkiGibson · 2024-06-25T19:12:15Z

@dotnet/roslyn-compiler for a second review

RikkiGibson · 2024-06-26T16:14:30Z

@dotnet/roslyn-compiler for a second review

333fred · 2024-06-26T21:34:37Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1723,7 +1727,7 @@ private BoundExpression ConvertSwitchExpression(BoundUnconvertedSwitchExpression

                return new BoundConversion(
                    syntax,
-                    source,
+                    BindToNaturalType(source, diagnostics),


Should we be calling BindToTypeForErrorRecovery instead? It seems like the main difference is around error reporting; any errors from BindToNaturalType will be put into diagnostics, while BindToTypeForErrorRecovery will discard them. Do we already have a diagnostic at this point?

We will definitely have some diagnostic about this expression, because of the preceding if (!hasErrors) GenerateImplicitConversionError(...).

re: the difference in practice. If we used BindToTypeForErrorRecovery here, then test Repro_VsFeedback_2088611 wouldn't report ERR_ImplicitObjectCreationNoTargetType. It would only report ERR_ConversionNotTupleCompatible. I feel like in this particular case, it's maybe better to have a squiggle on the new() because it happens to be related to the ambiguity. But, ideally, we would be able to report some kind of user-defined operator overload resolution error here, I think.

Fix error recovery for various nested conversion scenarios

99a19ca

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 20, 2024

RikkiGibson marked this pull request as ready for review June 21, 2024 00:10

RikkiGibson requested a review from a team as a code owner June 21, 2024 00:10

AlekseyTs reviewed Jun 24, 2024

View reviewed changes

Address feedback

c39c918

RikkiGibson requested a review from AlekseyTs June 24, 2024 21:38

RikkiGibson commented Jun 24, 2024

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/UserDefinedConversionTests.cs Outdated Show resolved Hide resolved

verify output

7bc7859

AlekseyTs approved these changes Jun 25, 2024

View reviewed changes

RikkiGibson requested a review from a team June 25, 2024 15:17

333fred reviewed Jun 26, 2024

View reviewed changes

333fred approved these changes Jun 27, 2024

View reviewed changes

RikkiGibson merged commit 9951ceb into dotnet:main Jun 27, 2024
24 checks passed

RikkiGibson deleted the vsfeedback-2088611 branch June 27, 2024 00:41

dotnet-policy-service bot added this to the Next milestone Jun 27, 2024

This was referenced Jul 4, 2024

[Automated] PRs inserted in VS build main-35104.04 #74270

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35108.50 #74296

Closed

RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error recovery for various nested conversion scenarios #74089

Fix error recovery for various nested conversion scenarios #74089

RikkiGibson commented Jun 20, 2024

RikkiGibson commented Jun 21, 2024

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024

AlekseyTs Jun 24, 2024

RikkiGibson Jun 24, 2024

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024

RikkiGibson Jun 24, 2024

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024

RikkiGibson Jun 24, 2024

AlekseyTs commented Jun 24, 2024

RikkiGibson commented Jun 24, 2024

AlekseyTs left a comment

RikkiGibson commented Jun 25, 2024

RikkiGibson commented Jun 26, 2024

333fred Jun 26, 2024

RikkiGibson Jun 27, 2024

Fix error recovery for various nested conversion scenarios #74089

Fix error recovery for various nested conversion scenarios #74089

Conversation

RikkiGibson commented Jun 20, 2024

RikkiGibson commented Jun 21, 2024

AlekseyTs Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Jun 24, 2024

RikkiGibson commented Jun 24, 2024

AlekseyTs left a comment

Choose a reason for hiding this comment

RikkiGibson commented Jun 25, 2024

RikkiGibson commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024 •

edited

Loading

AlekseyTs Jun 24, 2024 •

edited

Loading