Add friendlier error message on an explicit implementation when the return type is wrong

[stbau04]

Fixes #60742

[333fred]

Needs tests added for this.

[333fred]

I like the idea, but let's try to preference the correct match with less work; do the full check first, and if that doesn't work, do the relaxed check. This is how we do overload resolution as well.

Also, sloppy isn't a great name. Let's be specific: "ExplicitImplementationWithoutReturnTypeComparer". Longer, but then we don't have to read the implementation to understand what is "sloppy" about it.

[stbau04]

Needs tests added for this.

What kinds of tests would you expect for that changes? Are some unit tests within the Microsoft.CodeAnalysis.CSharp.Symbol.UnitTests where the diagnostic is produced enough, or should tests be added as done for the original CS0539 (which is referenced in over 250 tests; that seems like a lot)?

[333fred]

Are some unit tests within the Microsoft.CodeAnalysis.CSharp.Symbol.UnitTests where the diagnostic is produced enough

That should be enough. So long as we have good scenario coverage.

[RikkiGibson]

It feels like it would be reasonable to report a specific error message for return type mismatch for properties and events also. The diagnostics for bad return types on overrides would be a useful reference here, maybe both how they are written and how they are reported.

See .NET Lab sample for reference.

[stbau04]

@RikkiGibson I modified the pr, so that i perform the type check manually (avoids calling the MemberSignatureComparer twice). Is that what you meant by "looking at how the overrides are written"? I could not figure anything else that can be copied from those checks

[RikkiGibson]

What I meant by how they are written was: the phrasing of the diagnostic message.

[stbau04]

Oh, i definitely misunderstood that one.. Anyways, now that i already implemented it, should i continue working on it with or without 3f95508?

[RikkiGibson]

Your current solution seems reasonable to me as it looks like you just extracted the way that MemberSignatureComparer.Equals itself compares return types.

We do need tests to demonstrate the new diagnostic though. It also looks like some existing tests are failing due to this change. That seems expected as we are now reporting a different diagnostic in certain conditions. See example. In some cases, you should be able to just copy the new diagnostics from the test failure message, into the source code, as long as the new diagnostics look reasonable/expected.

I am unsure whether we need to defend against cases of same name/signature but different return types. Possibly we do, in which case your solution looks reasonable. It would be welcome if you could add a test which uses an interface like the following in IL, to check whether that is the case, or whether the compiler gives some "bogus type" error when attempting to implement the interface at all. There is a CreateCompilationWithIL() method to help with doing this, and existing tests using it which would serve as a starting point.

.class interface public auto ansi abstract beforefieldinit I
{
    // Methods
    .method public hidebysig newslot abstract virtual 
        instance int32 M () cil managed 
    {
    } // end of method I::M

    .method public hidebysig newslot abstract virtual 
        instance string M () cil managed 
    {
    } // end of method I::M
} // end of class I

[stbau04]

New pr #80376 as Github apparently behaves different in comparison to Bitbucket when a branch with pr is deleted and pushed again (sorry for that, learned my lesson)