Skip to content

Convert operand of true/false operator invoked as part of &&/||operator evaluation #78629

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.

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

Merged
merged 1 commit into from
May 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
50 changes: 48 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1156,17 +1156,36 @@ private BoundExpression BindConditionalLogicalOperator(BinaryExpressionSyntax no
CheckConstraintLanguageVersionAndRuntimeSupportForOperator(node, kind == BinaryOperatorKind.LogicalAnd ? falseOperator : trueOperator,
isUnsignedRightShift: false, signature.ConstrainedToTypeOpt, diagnostics);

var operandPlaceholder = new BoundValuePlaceholder(resultLeft.Syntax, resultLeft.Type).MakeCompilerGenerated();
var trueFalseParameterType = (resultKind.Operator() == BinaryOperatorKind.And ? falseOperator : trueOperator).Parameters[0].Type;
CompoundUseSiteInfo<AssemblySymbol> useSiteInfo = GetNewCompoundUseSiteInfo(diagnostics);
var conversion = this.Conversions.ClassifyConversionFromType(resultLeft.Type, trueFalseParameterType, isChecked: CheckOverflowAtRuntime, ref useSiteInfo);
Debug.Assert(conversion.IsImplicit);

BoundExpression operandConversion = CreateConversion(
resultLeft.Syntax,
operandPlaceholder,
conversion,
isCast: false,
conversionGroupOpt: null,
trueFalseParameterType,
diagnostics);

diagnostics.Add(operandConversion.Syntax, useSiteInfo);

return new BoundUserDefinedConditionalLogicalOperator(
node,
resultKind,
resultLeft,
resultRight,
signature.Method,
trueOperator,
falseOperator,
operandPlaceholder,
operandConversion,
signature.ConstrainedToTypeOpt,
lookupResult,
originalUserDefinedOperators,
resultLeft,
resultRight,
signature.ReturnType);
}
else
Expand Down Expand Up @@ -1234,8 +1253,35 @@ private bool IsValidDynamicCondition(BoundExpression left, bool isNegative, Bind
return false;
}

// Strictly speaking, we probably should be digging through nullable type here to look for an operator
// declared by the underlying type. However, it doesn't look like dynamic binder is able to deal with
// nullable types while evaluating logical binary operators. Exceptions that it throws look like:
//
// Microsoft.CSharp.RuntimeBinder.RuntimeBinderException : The type ('S1?') must contain declarations of operator true and operator false
// Stack Trace:
// at CallSite.Target(Closure, CallSite, Object, Object)
// at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1)
//
// Microsoft.CSharp.RuntimeBinder.RuntimeBinderException : Operator '&&' cannot be applied to operands of type 'S1' and 'S1?'
// Stack Trace:
// at CallSite.Target(Closure, CallSite, Object, Nullable`1)
// at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1)
var namedType = type as NamedTypeSymbol;
var result = HasApplicableBooleanOperator(namedType, isNegative ? WellKnownMemberNames.FalseOperatorName : WellKnownMemberNames.TrueOperatorName, type, ref useSiteInfo, out userDefinedOperator);

if (result)
{
Debug.Assert(userDefinedOperator is not null);

var operandPlaceholder = new BoundValuePlaceholder(left.Syntax, left.Type).MakeCompilerGenerated();
TypeSymbol parameterType = userDefinedOperator.Parameters[0].Type;

implicitConversion = this.Conversions.ClassifyConversionFromType(operandPlaceholder.Type, parameterType, isChecked: CheckOverflowAtRuntime, ref useSiteInfo);
Debug.Assert(implicitConversion.IsImplicit);

CreateConversion(left.Syntax, operandPlaceholder, implicitConversion, isCast: false, conversionGroupOpt: null, parameterType, diagnostics);
}

diagnostics.Add(left.Syntax, useSiteInfo);

return result;
Expand Down
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,15 @@
<Field Name="LogicalOperator" Type="MethodSymbol"/>
<Field Name="TrueOperator" Type="MethodSymbol"/>
<Field Name="FalseOperator" Type="MethodSymbol"/>

<!-- Used as an operand to bind TrueFalseOperandConversion -->
<Field Name="TrueFalseOperandPlaceholder" Type="BoundValuePlaceholder?" SkipInVisitor="true" Null="allow"/>
<!--
Could be a BoundConversion or, in some cases of an identity conversion, the OperandPlaceholder, or null for no conversion case
If the node survives lowering, this property in it must be null.
Copy link
Member

@jcouv jcouv May 19, 2025

Choose a reason for hiding this comment

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

Consider asserting that in LocalRewritingValidator #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider asserting that in LocalRewritingValidator

This is an FYI, not a contract or a correctness requirement

-->
<Field Name="TrueFalseOperandConversion" Type="BoundExpression?" SkipInVisitor="true" Null="allow"/>

<Field Name="ConstrainedToTypeOpt" Type="TypeSymbol?" Null="allow"/>
<Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
<!--The set of method symbols from which this operator's method was chosen.
Expand Down
33 changes: 0 additions & 33 deletions src/Compilers/CSharp/Portable/BoundTree/Constructors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -454,39 +454,6 @@ public BoundBinaryOperator Update(UncommonData uncommonData)
}
}

internal sealed partial class BoundUserDefinedConditionalLogicalOperator
{
public BoundUserDefinedConditionalLogicalOperator(
SyntaxNode syntax,
BinaryOperatorKind operatorKind,
BoundExpression left,
BoundExpression right,
MethodSymbol logicalOperator,
MethodSymbol trueOperator,
MethodSymbol falseOperator,
TypeSymbol? constrainedToTypeOpt,
LookupResultKind resultKind,
ImmutableArray<MethodSymbol> originalUserDefinedOperatorsOpt,
TypeSymbol type,
bool hasErrors = false)
: this(
syntax,
operatorKind,
logicalOperator,
trueOperator,
falseOperator,
constrainedToTypeOpt,
resultKind,
originalUserDefinedOperatorsOpt,
left,
right,
type,
hasErrors)
{
Debug.Assert(operatorKind.IsUserDefined() && operatorKind.IsLogical());
}
}

internal sealed partial class BoundParameter
{
public BoundParameter(SyntaxNode syntax, ParameterSymbol parameterSymbol, bool hasErrors = false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ private BoundNode VisitBinaryOperatorBase(BoundBinaryOperatorBase binaryOperator
right,
type!),
// https://github.com/dotnet/roslyn/issues/35031: We'll need to update logical.LogicalOperator
BoundUserDefinedConditionalLogicalOperator logical => logical.Update(logical.OperatorKind, logical.LogicalOperator, logical.TrueOperator, logical.FalseOperator, logical.ConstrainedToTypeOpt, logical.ResultKind, logical.OriginalUserDefinedOperatorsOpt, leftChild, right, type!),
BoundUserDefinedConditionalLogicalOperator logical => logical.Update(logical.OperatorKind, logical.LogicalOperator, logical.TrueOperator, logical.FalseOperator, logical.TrueFalseOperandPlaceholder, logical.TrueFalseOperandConversion, logical.ConstrainedToTypeOpt, logical.ResultKind, logical.OriginalUserDefinedOperatorsOpt, leftChild, right, type!),
_ => throw ExceptionUtilities.UnexpectedValue(currentBinary.Kind),
};

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading