-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Report missing diagnostic for typeless expression in binary operation #69727
Conversation
{ | ||
if (((SyntaxKind)node.OperatorToken.RawKind == SyntaxKind.PlusEqualsToken || (SyntaxKind)node.OperatorToken.RawKind == SyntaxKind.MinusEqualsToken) && | ||
if (IsTypelessExpressionAllowedInBinaryOperator(kind, left, right) && |
There was a problem hiding this comment.
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, orIsExpressionAllowedInBinaryOperator
(with semantics unchanged)?
Also, what happened when this check wasn't here? The GenerateImplicitConversionError
failed? #Resolved
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Fred Silberberg <[email protected]>
Fixes #69579
For context, see the other location that uses
IsTypelessExpressionAllowedInBinaryOperator
(inBinaryOperatorOverloadResolution
) and the comments inIsTypelessExpressionAllowedInBinaryOperator
.I'm not exactly sure why we chose this behavior and it seems odd, but I chose to keep the existing behavior (described in comment) and just fix our error reporting.