DiagnosticDescription arguments do not match those from DiagnosticInfo

[leppie]

Example: Diagnostics output:

// (6,74): error CS1715: 'C.Event3': type must be 'EventDelegate<UnavailableClass[]>' to match overridden member 'ClassEvents.Event3'
//     public override event CSharpErrors.EventDelegate<UnavailableClass[]> Event3 { add { } remove { } }

The existing verify diag desc:

Diagnostic(ErrorCode.ERR_CantChangeTypeOnOverride, "Event3").WithArguments("C.Event3", "CSharpErrors.ClassEvents.Event3", "CSharpErrors.EventDelegate<UnavailableClass[]>")

As one can see, one need to provide additional info (this case, namespace) to make it passable.

Related code: https://github.com/dotnet/roslyn/blob/master/src%2FCompilers%2FCore%2FPortable%2FDiagnostic%2FDiagnosticInfo.cs#L347 https://github.com/dotnet/roslyn/blob/master/src%2FTest%2FUtilities%2FShared%2FDiagnostics%2FDiagnosticDescription.cs#L48

I corrected the code locally to try match description to info, but it breaks a gazillion tests, mainly due to namespaces included in the DiagnosticDescription arguments.

[leppie]

Looking at the tests, it sure looks like namespaces were emitted at some stage, but this is not case for a while now. Namespaces are also emitted in the legacy compiler.

Update: It was changed with https://github.com/dotnet/roslyn/commit/c2dcf6d870898e85d764c3f0c034ff5392a0bed6 but due to the issue described here, many test cases were not updated.

[leppie]

cc @heejaechang

[heejaechang]

@leppie can you provide test cases to see what behavior is changed? I am not sure what you are trying to change.

about the omitting the namespace, that is a intentional behavior change of Roslyn compiler against legacy C#/VB compiler. most of cases, we no longer put namespace in error message.

arguments in DiagnosticInfo can point to symbol directly. it is when they are converted to DiagnosticDescription those arguments are converted to string with right format (without namespace).

[leppie]

@heejaechang That is the problem, when converted in DiagnosticDescription, ISymbol specifically is not handled the same and outputs a typename with the namespace.

Example: https://github.com/dotnet/roslyn/blob/master/src%2FCompilers%2FCSharp%2FTest%2FSemantic%2FSemantics%2FSemanticErrorTests.cs#L9489

Both diagnostics have wrong output in the comments, and the actual construction still requires a type with a namespace for the test to pass.

Applying my fix (https://github.com/leppie/roslyn/commit/9310774fbdd77e104f7986af9beb3c9b8b780125) causes Roslyn.Compilers.CSharp.Semantic.UnitTests.dll alone to have 185 test failures due to namespaces.

Another problem due to this issue is having to have tests like: https://github.com/dotnet/roslyn/pull/7203/files#diff-c32e4d4a7fd7539039ec082e460dcb42R2256

[heejaechang]

@leppie yes, some cases, we still put namespaces. I dont remember which cases we decide to still put namespaces @srivatsn do you remember?

but sure there can be a bug where we left namespace when we should have removed.

by the way, I still don't following. are you talking about DiagnosticDescriptor? or DiagnosticDescription? DiagnosticDescription is something for testing.

[leppie]

@heejaechang DiagnosticDescription, hence the failing tests. It is not a bug by showing or not showing the namespace. It is about how DiagnosticDescription handles it, which is different from DiagnosticInfo. And subsequently a bunch of tests that is handled in a different (and non-intuitive) way.

[heejaechang]

@leppie ah, now I understand. I see. so test wasn't creating right arguments. so what test tests and what actually shown to users are actually different.

[leppie]

@heejaechang Yes, and I am pretty much fine with that, except that specific case that cannot be handled correctly now. #7203

[leppie]

As a compromise, one could let DiagnosticDescription allow a partial 'ends-with' match on the arguments if a namespace-like string is present. That would at least not break existing tests.

Update: This does not work completely. Still 52 failing.

[leppie]

@heejaechang Here is a specific C# example that I noted you fixed for the stringly tests, that is subsequently bad again.

https://github.com/dotnet/roslyn/blob/master/src%2FCompilers%2FCSharp%2FTest%2FSemantic%2FSemantics%2FSemanticErrorTests.cs#L10628

https://github.com/dotnet/roslyn/commit/c2dcf6d870898e85d764c3f0c034ff5392a0bed6#diff-b7d01038b50b0fadd91ad2b8f4ed3826R10341

[heejaechang]

@leppie ya, we should fix this. thank you for reporting the issue.

[leppie]

@heejaechang Not urgent, but I will try make some Roslyn tool to adjust the Roslyn tests :) This is way too tedious to do by hand (I tried a bit, but gave up after 20 or so fixes, too demoralizing).

[heejaechang]

moving to update 3. need to take care of more urgent update 2 issues.

[heejaechang]

sorry, need to move one more milestone.