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.

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

Clarify diagnostic when constant pattern has non-constant conversion to input type #64741

Conversation

ArcadeMode
Copy link
Contributor

@ArcadeMode ArcadeMode commented Oct 14, 2022

This proposes a fix for #63476
The newly introduced error message goes for example: error CS9096: Cannot implicitly convert type 'string' to type 'XName' in constant pattern using non-constant conversion error CS9098: Cannot implicitly convert type 'string' to type 'XName' using user-defined conversion in pattern expression

@ArcadeMode ArcadeMode requested a review from a team as a code owner October 14, 2022 19:46
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Oct 14, 2022
@dnfadmin
Copy link

dnfadmin commented Oct 14, 2022

CLA assistant check
All CLA requirements met.

@ArcadeMode ArcadeMode changed the title Clarify diagnostics Clarify diagnostic when constant pattern has non-constant conversion to input type Oct 14, 2022
@AlekseyTs
Copy link
Contributor

It looks like there are no tests added or modified.

var conversion = convertedExpression as BoundConversion;
if (conversion != null && conversion.ConversionKind == ConversionKind.ImplicitUserDefined)
{
diagnostics.Add(ErrorCode.ERR_NonConstantConversionInConstantPattern, patternExpression.Location, conversion.Operand.Type!, conversion.Type);
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 14, 2022

Choose a reason for hiding this comment

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

conversion.Operand

I think in some cases conversion.Operand might not correspond to the syntactic operand for the conversion, Therefore, the type could be surprising and the error message would not make sense because, for example, there is a conversion that supports constant folding between the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated this to use expression instead - this should be the expression being bound and as I understand that does makes sense. Stepping through a few unit tests confirmed my beliefs. This change however, caused errors when the expression was a null literal so I have written a case to show ErrorCode.ERR_ValueCantBeNull when null literals are attempted to be converted to a non-nullable type.

For the following snipped:
static bool M1(Span<char> chars) => chars is null;
This yields the following message:
// (5,50): error CS0037: Cannot convert null to 'Span<char>' because it is a non-nullable value type
instead of
// (5,50): error CS0150: A constant value is expected

Affected test cases were adjusted to this change.

Looking forward to hear your feedback on this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of conversion is actually used for Span<char> x = null?

Copy link
Contributor Author

@ArcadeMode ArcadeMode Oct 26, 2022

Choose a reason for hiding this comment

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

That appears to be an ImplicitReference conversion.

This was wrong -- its an implicit user defined conversion. I have reverted the changes with respect to the null literal changes as that was not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The approach using expression did not work when explicit casts were involved, then the message would effectively turn into "cannot convert 'A' to 'A'". I have now used the SymbolDistinguisher with conversion.Operand as I have seen being used here and there when reporting 'no possible conversion' errors.

I did read up a little on constant folding and I do wonder, are there actually situations where constant folding results in type conversions?

Copy link
Contributor Author

@ArcadeMode ArcadeMode Oct 27, 2022

Choose a reason for hiding this comment

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

Now, the error message in a pattern which attempts to convert null to Span would become CS9098: Cannot implicitly convert type 'char[]' to type 'Span<char>' using user-defined conversion in pattern expression.

Im feeling conflicted about this, one one hand its technically correct but it could be confusing. Should there perhaps be a special message for null?

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 6).

@ArcadeMode ArcadeMode requested a review from AlekseyTs October 16, 2022 18:01
@RikkiGibson RikkiGibson self-assigned this Oct 20, 2022
@RikkiGibson
Copy link
Contributor

Thanks for opening the PR. Will take a look as soon as I am able.

@ArcadeMode ArcadeMode force-pushed the clarify-diagnostic-non-constant-conversion-in-constant-pattern branch from fabef2c to 2500017 Compare October 21, 2022 17:40
@RikkiGibson
Copy link
Contributor

It looks like some test baselines needs to be updated. https://dev.azure.com/dnceng-public/public/_build/results?buildId=214143&view=ms.vss-test-web.build-test-results-tab

For example:

xUnit.net 00:00:24.38]     Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.SwitchTests.SwitchOnNullableWithNonConstant [FAIL]
  Failed Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.SwitchTests.SwitchOnNullableWithNonConstant [18 ms]
  Error Message:
   
Expected:
                Diagnostic(ErrorCode.ERR_ConstantExpected, "i").WithLocation(8, 18)
Actual:
                // (8,18): error CS9135: A constant value of type 'int?' is expected
                //             case i:
                Diagnostic(ErrorCode.ERR_ConstantValueOfTypeExpected, "i").WithArguments("int?").WithLocation(8, 18)
Diff:
++>                 Diagnostic(ErrorCode.ERR_ConstantValueOfTypeExpected, "i").WithArguments("int?").WithLocation(8, 18)
-->                 Diagnostic(ErrorCode.ERR_ConstantExpected, "i").WithLocation(8, 18)
Expected: True
Actual:   False
  Stack Trace:
     at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify(IEnumerable`1 actual, DiagnosticDescription[] expected, Boolean errorCodeOnly) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 99
   at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify(IEnumerable`1 actual, DiagnosticDescription[] expected) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 48
   at Microsoft.CodeAnalysis.DiagnosticExtensions.Verify(ImmutableArray`1 actual, DiagnosticDescription[] expected) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 63
   at Microsoft.CodeAnalysis.DiagnosticExtensions.VerifyDiagnostics[TCompilation](TCompilation c, DiagnosticDescription[] expected) in /_/src/Compilers/Test/Core/Diagnostics/DiagnosticExtensions.cs:line 109
   at Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.SwitchTests.SwitchOnNullableWithNonConstant() in /_/src/Compilers/CSharp/Test/Emit/CodeGen/SwitchTests.cs:line 2625
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

@ArcadeMode
Copy link
Contributor Author

I've updated the tests (I hope I got all of them). Oddly enough The failure reported here I couldn't reproduce locally as the baseline was already set to expect the new error code.. any clue what might be going on there?

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 37)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 40)

@ArcadeMode
Copy link
Contributor Author

addressed feedback

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 43)

@AlekseyTs
Copy link
Contributor

@RikkiGibson For the second review

@RikkiGibson RikkiGibson merged commit 15898f5 into dotnet:main Mar 28, 2023
@ghost ghost added this to the Next milestone Mar 28, 2023
@RikkiGibson
Copy link
Contributor

Thanks for the contribution @ArcadeMode!

@allisonchou allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants