Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error recovery for various nested conversion scenarios #74089

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Fix error recovery for various nested conversion scenarios #74089

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
99a19ca
Fix error recovery for various nested conversion scenarios
RikkiGibson Jun 19, 2024
c39c918
Address feedback
RikkiGibson Jun 24, 2024
7bc7859
verify output
RikkiGibson Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,7 @@ private BoundCollectionExpression ConvertCollectionExpression(
diagnostics);
builder.Add(convertedElement!);
}
conversion.MarkUnderlyingConversionsChecked();
}

return new BoundCollectionExpression(
Expand Down Expand Up @@ -1644,7 +1645,8 @@ private BoundExpression ConvertConditionalExpression(
{
bool targetTyped = conversionIfTargetTyped is { };
Debug.Assert(targetTyped || destination.IsErrorType() || destination.Equals(source.Type, TypeCompareKind.ConsiderEverything));
ImmutableArray<Conversion> underlyingConversions = conversionIfTargetTyped.GetValueOrDefault().UnderlyingConversions;
var conversion = conversionIfTargetTyped.GetValueOrDefault();
ImmutableArray<Conversion> underlyingConversions = conversion.UnderlyingConversions;
var condition = source.Condition;
hasErrors |= source.HasErrors || destination.IsErrorType();

Expand All @@ -1656,6 +1658,7 @@ private BoundExpression ConvertConditionalExpression(
targetTyped
? CreateConversion(source.Alternative.Syntax, source.Alternative, underlyingConversions[1], isCast: false, conversionGroupOpt: null, destination, diagnostics)
: GenerateConversionForAssignment(destination, source.Alternative, diagnostics);
conversion.MarkUnderlyingConversionsChecked();
var constantValue = FoldConditionalOperator(condition, trueExpr, falseExpr);
hasErrors |= constantValue?.IsBad == true;
if (targetTyped && !destination.IsErrorType() && !Compilation.IsFeatureEnabled(MessageID.IDS_FeatureTargetTypedConditional))
Expand Down Expand Up @@ -1695,6 +1698,7 @@ private BoundExpression ConvertSwitchExpression(BoundUnconvertedSwitchExpression
new BoundSwitchExpressionArm(oldCase.Syntax, oldCase.Locals, oldCase.Pattern, oldCase.WhenClause, newValue, oldCase.Label, oldCase.HasErrors);
builder.Add(newCase);
}
conversion.MarkUnderlyingConversionsChecked();

var newSwitchArms = builder.ToImmutableAndFree();
return new BoundConvertedSwitchExpression(
Expand Down Expand Up @@ -1723,7 +1727,7 @@ private BoundExpression CreateUserDefinedConversion(

return new BoundConversion(
syntax,
source,
BindToNaturalType(source, diagnostics),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

conversion,
CheckOverflowAtRuntime,
explicitCastInCode: isCast,
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7184,7 +7184,7 @@ private void VisitArgumentConversionAndInboundAssignmentsAndPreConditions(
case RefKind.In:
{
// Note: for lambda arguments, they will be converted in the context/state we saved for that argument
if (conversion is { Kind: ConversionKind.ImplicitUserDefined })
if (conversion is { IsValid: true, Kind: ConversionKind.ImplicitUserDefined })
{
var argumentResultType = resultType.Type;
conversion = GenerateConversion(_conversions, argumentNoConversion, argumentResultType, parameterType.Type, fromExplicitCast: false, extensionMethodThisArgument: false, isChecked: conversionOpt?.Checked ?? false);
Expand Down
10 changes: 6 additions & 4 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker_Patterns.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,10 +917,12 @@ private void VisitSwitchExpressionCore(BoundSwitchExpression node, bool inferTyp

if (inferType && inferredType is null)
{
// This can happen when we're inferring the return type of a lambda, or when there are no arms (an error case).
// For this case, we don't need to do any work, as the unconverted switch expression can't contribute info, and
// there is nothing that is being publicly exposed to the semantic model.
Debug.Assert((node is BoundUnconvertedSwitchExpression && _returnTypesOpt is not null)
// This can happen when we're inferring the return type of a lambda or visiting a node without diagnostics like
// BoundConvertedTupleLiteral.SourceTuple. For these cases, we don't need to do any work,
// the unconverted switch expression can't contribute info. The conversion that should be on top of this
// can add or remove nullability, and nested nodes aren't being publicly exposed by the semantic model.
// See also NullableWalker.VisitConditionalOperatorCore for a similar check for conditional operators.
Debug.Assert((node is BoundUnconvertedSwitchExpression && (_returnTypesOpt is not null || _disableDiagnostics))
|| node is BoundSwitchExpression { SwitchArms: { Length: 0 } });
inferredState = default;

Expand Down
Loading
Loading