Eliminate Warning CS1957 (runtime ambiguous override) by generating correct code

[gafter]

The compiler sometimes reports CS1957:

class Base<TInt>
{
    //these signatures differ only in ref/out
    public virtual void Method(int @in, ref int @ref) { }
    public virtual void Method(TInt @in, out TInt @out) { @out = @in; }
}

class Derived : Base<int>
{
    public override void Method(int @in, ref int @ref) { } // HERE
}

warning CS1957: Member 'Derived.Method(int, ref int)' overrides 'Base<int>.Method(int, ref int)'. There are multiple override candidates at run-time. It is implementation dependent which method will be called.

The compiler is capable of emitting a methodimpl record which will disambiguate the situation for the runtime. If we do that, there will never be any need to produce this warning. Perhaps more importantly, the compile-time behavior and the runtime behavior will agree.

[gafter]

The simplest fix is to modify (and rename) GetFirstRuntimeOverriddenMethodIgnoringNewSlot to return a method only if it is the only runtime overridden method (otherwise return null).

[AlekseyTs]

At the moment the removal of the warning doesn't look like the right thing to do due to dotnet/runtime#38119.

[gafter]

Not sure if we intend to have the C# compiler produce warnings for runtime-version-specific bugs. If so, the warning should be reworded to identify the bug and suggest upgrading the runtime.

[AlekseyTs]

The wording of the warning is correct no matter what version of the runtime is used: "Member '{1}' overrides '{0}'. There are multiple override candidates at run-time. It is implementation dependent which method will be called." What method is called is indeed implementation dependent, even when the "right" one is called.

[gafter]

The CLI specification is not ambiguous about which method will be called. There is only one candidate at runtime. The only reason it is implementation-dependent is that some implementations are incorrect.

[AlekseyTs]

The only reason it is implementation-dependent is that some implementations are incorrect.

The actual reason makes absolutely no difference in my opinion. We are not trying to state it in the warning.

[gafter]

Sure, if we have a policy of producing warnings when we generate code that might be incorrect on some implementation because of runtime bugs.

[AlekseyTs]

We could emit explicit override long time ago, we already did in some scenarios. Why do you think we didn't do that and instead kept reporting the warning?

[gafter]

Why do you think we didn't do that and instead kept reporting the warning?

I think we didn't do that because that portion of the Roslyn compiler was transcribed from a similar part of the native compiler, which only added the use of a class .override for a very specific scenario not related to methods that are declared override, not realizing it could be used for other scenarios as well. We have comments in the code incorrectly indicating that in IL there is no way to express the override precisely. It is only recently that the compiler team has become aware of this application of .override in metadata.

[AlekseyTs]

It is only recently that the compiler team has become aware of this application of .override in metadata.

That is definitely not the case. We, even in the native compiler time, knew about that and even used it in some scenarios in classes. But not tried to use it for these scenarios. My guess, we new it wouldn't help.

[gafter]

@AlekseyTs I'm working from evidence rather than guessing.

The native and Roslyn compilers never used methodimpl entries for methods declared override. The comments in the compiler indicate that there is no way to express the override relationship precisely in metadata. That comment is, of course, incorrect and shows that the author of the relevant compiler code misunderstood the expressiveness of the metadata. The comment did not say that there is a runtime bug preventing it from working. The runtime team was unaware of the bug until this week. It seems unlikely that the compiler team knew about the bug but never bothered to report it to the runtime team.

In any case, we do not have a policy of reporting warnings on source that might encounter bugs specific to some version of the runtime.

[gafter]

The actual reason makes absolutely no difference in my opinion. We are not trying to state it in the warning.

The warning does state a reason. It says that there are multiple candidates at runtime. Since there is one candidate at runtime, I prefer to reword of remove the diagnostic.

[AlekseyTs]

The native and Roslyn compilers never used methodimpl entries for methods declared override.

That is definitely not the case. Please look at the implementation and usage of

        internal static bool RequiresExplicitOverride(this MethodSymbol method)
        {
            if (method.IsOverride)
            {
                MethodSymbol csharpOverriddenMethod = method.OverriddenMethod;
                if ((object)csharpOverriddenMethod == null)
                {
                    return false;
                }
                // We can ignore newslot, since we checked IsOverride.
                // We can ignore interface implementation changes since the method is already metadata virtual (since override).
                // TODO: do we want to add more sophisticated handling for the case where there are multiple runtime-overridden methods?
                MethodSymbol runtimeOverriddenMethod = method.GetFirstRuntimeOverriddenMethodIgnoringNewSlot(ignoreInterfaceImplementationChanges: true);
                return csharpOverriddenMethod != runtimeOverriddenMethod &&
                    method.IsAccessor() != runtimeOverriddenMethod.IsAccessor();
            }

            return false;
        }

Similar method exists in VB compiler and it uses "methodimpl entries for methods declared override" even in more scenarios.

[gafter]

I stand corrected. More precisely, the compilers did not produce a methodimpl in order to disambiguate multiple candidate overridden methods.

[AlekseyTs]

So, do you still think we we didn't know that methidimpl can be used with overrides in classes?

[gafter]

Yes, I still think the compiler team documented that there was no mechanism to disambiguate runtime ambiguous overrides (unlike the situation with the code quoted in the compiler that uses methodimpl to correct runtime unambiguous but incorrect overrides).

[AlekseyTs]

You are answering the question that wasn't asked and do not answer the question that was asked. I wonder why.

[AlekseyTs]

compiler team documented that there was no mechanism to disambiguate runtime ambiguous overrides

This, by the way, is still correct. There is no mechanism to disambiguate runtime ambiguous overrides
today. Your tests confirm that.

[gafter]

You are answering the question that wasn't asked and do not answer the question that was asked. I wonder why.

Perhaps because I assumed you had read my previous comment which answered your question and clarified what I think.

There is no mechanism to disambiguate runtime ambiguous overrides today.

That wasn't what I claimed. I claimed that the compiler team thought (incorrectly, as it turns out) that there was no mechanism to communicate a disambiguation to the runtime.

[gafter]

The belief by the compiler team that there was no such mechanism to express it was incorrect. The mechanism is documented in the ECMA specification, though some runtimes do not implement it correctly yet. I am aware of no evidence anybody was aware of that bug before this week. We expect this to be implemented correctly in .net 5.

[AlekseyTs]

I am aware of no evidence anybody was aware of that bug before this week.

I am that evidence. I was aware of the way runtime behaves since the native compiler times. I new that emitting explicit override doesn't help, at least in some scenarios. That is why I made the comment on the PR that tried to eliminate the warning and asked you to properly test your assumptions and see for yourself.

We expect this to be implemented correctly in .net 5.

I definitely support fixing this long standing bug in runtime. And will be happy to return to discussion about the warning once that happened. Until then, I think, doing anything is premature.

[gafter]

Fixed in #44025