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

Report missing diagnostic for typeless expression in binary operation #69727

Merged
merged 2 commits into from
Aug 30, 2023
Merged

Report missing diagnostic for typeless expression in binary operation #69727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24baeff
Report missing diagnostic for typeless expression in binary operation
jcouv Aug 25, 2023
46fcddb
Use pattern-matching
jcouv Aug 29, 2023
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
7 changes: 4 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ private BoundExpression BindCompoundAssignment(AssignmentExpressionSyntax node,
BinaryOperatorAnalysisResult best = this.BinaryOperatorOverloadResolution(kind, isChecked: CheckOverflowAtRuntime, left, right, node, diagnostics, out resultKind, out originalUserDefinedOperators);
if (!best.HasValue)
{
ReportAssignmentOperatorError(node, diagnostics, left, right, resultKind);
ReportAssignmentOperatorError(node, kind, diagnostics, left, right, resultKind);
left = BindToTypeForErrorRecovery(left);
right = BindToTypeForErrorRecovery(right);
return new BoundCompoundAssignmentOperator(node, BinaryOperatorSignature.Error, left, right,
Expand Down Expand Up @@ -779,9 +779,10 @@ private static void ReportUnaryOperatorError(CSharpSyntaxNode node, BindingDiagn
Error(diagnostics, errorCode, node, operatorName, operand.Display);
}

private void ReportAssignmentOperatorError(AssignmentExpressionSyntax node, BindingDiagnosticBag diagnostics, BoundExpression left, BoundExpression right, LookupResultKind resultKind)
private void ReportAssignmentOperatorError(AssignmentExpressionSyntax node, BinaryOperatorKind kind, BindingDiagnosticBag diagnostics, BoundExpression left, BoundExpression right, LookupResultKind resultKind)
{
if (((SyntaxKind)node.OperatorToken.RawKind == SyntaxKind.PlusEqualsToken || (SyntaxKind)node.OperatorToken.RawKind == SyntaxKind.MinusEqualsToken) &&
if (IsTypelessExpressionAllowedInBinaryOperator(kind, left, right) &&
Copy link
Member

@jjonescz jjonescz Aug 29, 2023 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Looks like IsTypelessExpressionAllowedInBinaryOperator returns false if the expression is a kind of typeless expression which is disallowed in binary operator and true if it's allowed or not typeless at all, right? The name confused me as it sounds like it would return true only if the expression is typeless. Should it perhaps be renamed to

  • IsDisallowedTypelessExpressionInBinaryOperator and its semantics negated, or
  • IsExpressionAllowedInBinaryOperator (with semantics unchanged)?

Also, what happened when this check wasn't here? The GenerateImplicitConversionError failed? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the current name is appropriate, looking at its usage in BinaryOperatorOverloadResolution:

        private BinaryOperatorAnalysisResult BinaryOperatorOverloadResolution(...)
        {
            if (!IsTypelessExpressionAllowedInBinaryOperator(kind, left, right))
            {
                resultKind = LookupResultKind.OverloadResolutionFailure;
                originalUserDefinedOperators = default;
                return default(BinaryOperatorAnalysisResult);
            }
            ...

Also, what happened when this check wasn't here?

In DEBUG, the program would assert below (Debug.Assert(!conversion.IsImplicit);).
In RETAIL, the program would fail to report an error and would crash during emit phase. I didn't investigate in details, but I think it's just that GenerateImplicitConversionError doesn't handle the case and so didn't report an error.

Copy link
Member

Choose a reason for hiding this comment

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

looking at its usage in BinaryOperatorOverloadResolution

Yeah, I guess there it makes more sense since it's negated.

node.OperatorToken.RawKind is (int)SyntaxKind.PlusEqualsToken or (int)SyntaxKind.MinusEqualsToken &&
(object)left.Type != null && left.Type.TypeKind == TypeKind.Delegate)
{
// Special diagnostic for delegate += and -= about wrong right-hand-side
Expand Down
41 changes: 41 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/TargetTypedDefaultTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3759,5 +3759,46 @@ class C
Diagnostic(ErrorCode.ERR_BadAttributeParamType, "A").WithArguments("y2", "S?").WithLocation(14, 2)
);
}

[Fact]
public void TypelessExpressionInBinaryOperation()
{
string source = """
MyDelegate d = M;
d += M;

d = new MyDelegate(M);
d += new MyDelegate(M);

d = null;
d += null;

d = new(M);
d += new(M); // 1

d = default;
d += default; // 2

bool b = true;
d += b switch { _ => new(M) };
d += b switch { _ => null };
d += b switch { _ => default };

d.Invoke();

void M() { }

public delegate void MyDelegate();
""";
var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (11,1): error CS8310: Operator '+=' cannot be applied to operand 'new(method group)'
// d += new(M); // 1
Diagnostic(ErrorCode.ERR_BadOpOnNullOrDefaultOrNew, "d += new(M)").WithArguments("+=", "new(method group)").WithLocation(11, 1),
// (14,1): error CS8310: Operator '+=' cannot be applied to operand 'default'
// d += default; // 2
Diagnostic(ErrorCode.ERR_BadOpOnNullOrDefaultOrNew, "d += default").WithArguments("+=", "default").WithLocation(14, 1)
);
}
}
}