Closed gafter closed 4 years ago
I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.
@lambdageek @jaredpar @janvorli This runtime bug will affect covariant returns.
cc: @davidwrighton
It is possible that methodimpl entries for class overrides are not actually supported before netcore 5. If that is the case, this is not a bug (but would be a bug if these symptoms were exhibited in netcore 5). However, note that existing compilers do produce methodimpl entries for overrides of the object destructor.
From ECMA-335 4th edition (June 2006):
9.10 Explicit method overrides
A type, be it generic or non-generic, can implement particular virtual methods (whether the method was introduced in an interface or base class) using an explicit override. (See §10.3.2 and §15.1.4.)
10.3.2 The .override directive
The .override directive specifies that a virtual method shall be implemented (overridden), in this type, by a virtual method with a different name, but with the same signature. This directive can be used to provide an implementation for a virtual method inherited from a base class, or a virtual method specified in an interface implemented by this type. The .override directive specifies a Method Implementation (MethodImpl) in the metadata (§15.1.4).
15.1.4 Method implementations
A MethodImpl, or method implementation, supplies the executable body for an existing virtual method. It associates a Method (representing the body) with a MethodDecl or Method (representing the virtual method). A MethodImpl is used to provide an implementation for an inherited virtual method or a virtual method from an interface when the default mechanism (matching by name and signature) would not provide the correct result. See §22.27.
22.27 MethodImpl : 0x19
MethodImpl tables let a compiler override the default inheritance rules provided by the CLI. Their original use was to allow a class C, that inherited method M from both interfaces I and J, to provide implementations for both methods (rather than have only one slot for M in its vtable). However, MethodImpls can be used for other reasons too, limited only by the compiler writer’s ingenuity within the constraints defined in the Validation rules below.
So this is indeed a bug.
I am no expert on the type system intricacies, but I wonder, looking at the override:
.override method instance void class Base`2<int32,int32>::Method(
class [mscorlib]System.Collections.Generic.List`1<!0>&,
class [mscorlib]System.Collections.Generic.List`1<!1>&)
There is no [Out] at any of these parameters in the IL code. So how should it choose whether to override the (ref, out) or (out, ref) flavor?
The first parameter is of type
List`1<!0>&
and the second of type
List`1<!1>&
indicating a list of the first type parameter or the second type parameter in the original type declaration where the method was introduced. These are different type tokens. They would have to be reversed in the parameter list for the other method.
I guess I am missing something. The !0 and !1 are the generic arguments of the Base type (!0 is int32, !1 is int32) in the override above, it is not clear to me how would they be related to whether the method arguments that become Listout
or ref
.
It has nothing to do with whether they are out
or ref
. The two methods are distinguished by their signature (type) where originally declared, not as substituted. The method tokens do not substitute generics in the arguments or return part of the signature (otherwise you would see int32
instead of !0
). You see the same thing in the call instructions.
It seems we are each talking about something different. I guess my question was not clear. Let me try to rephrase it. The Base<T, U> class has the following two methods:
.method public hidebysig newslot virtual instance void Method(
[out] class [mscorlib]System.Collections.Generic.List`1<!U>& y,
class [mscorlib]System.Collections.Generic.List`1<!T>& x) cil managed
.method public hidebysig newslot virtual instance void Method(
class [mscorlib]System.Collections.Generic.List`1<!T>& x,
[out] class [mscorlib]System.Collections.Generic.List`1<!U>& y) cil managed
Then the Derived class is derived from the instantiation with both T and U being int32
And then we have this override:
.override method instance void class Base`2<int32,int32>::Method(
class [mscorlib]System.Collections.Generic.List`1<!0>&,
class [mscorlib]System.Collections.Generic.List`1<!1>&)
In the C#, it is clear which one we are overriding, as the overriding method signature has ref
at the first parameter and out
at the second.
But in the IL, the [Out] is not on either of those. So which one of those should it override? If you omit the [Out], those two are the same.
The point is that
Method(
class [mscorlib]System.Collections.Generic.List`1<!0>&,
class [mscorlib]System.Collections.Generic.List`1<!1>&)
is not identical to
Method(
class [mscorlib]System.Collections.Generic.List`1<!1>&,
class [mscorlib]System.Collections.Generic.List`1<!0>&)
so the runtime can distinguish them. The type parameter references are different. The former clearly only matches the second method declared in source, and the latter clearly only matches the first method declared in source.
Ah, now I got it, thanks for bearing with me!
mono/mono produces the correct output
$ mono --version
Mono JIT compiler version 6.12.0.74 (2020-02/e9d3af508e4 Fri May 15 10:10:32 EDT 2020)
...
$ ilasm foo.il
Assembling 'foo.il' , no listing file, to exe --> 'foo.exe'
Operation completed successfully
$ mono foo.exe
Derived.Method(ref List<int> a, out List<int> b)
Base<T, U>.Method(out List<U> y, ref List<T> x)
Derived.Method(ref List<int> a)
Base2<A, B>.Method(out List<A> x)
Derived.Method(ref List<int> a, out List<int> b)
Base<T, U>.Method(out List<U> y, ref List<T> x)
Derived.Method(ref List<int> a)
Base2<A, B>.Method(out List<A> x)
Derived.Method(ref List<int> a, out List<int> b)
Base<T, U>.Method(out List<U> y, ref List<T> x)
Derived.Method(ref List<int> a)
I will check whether my WIP PR #37516 does the right thing too.
Update based on the comment below also tried the same thing with the order of Method
definitions in Base
reordered. Same (correct) output.
Please also check whether mono produces the correct result when the order of the methods in Base are changed.
Let me look into this. I believe the rules that Neal is using for handling of MethodImpl's aren't quite what is specified in the latest version of the Ecma 335 spec. Those rules were determined to be incorrect some years ago and fixed in more recent editions of Ecma 335. See II.10.3.4 in https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-335.pdf and the other MethodImpl spec contents. I will take a deeper look here and attempt to determine if the runtime is behaving incorrectly here, or if your reading of the spec in this area is incorrect.
Ah, I misunderstood the issue here. This isn't about the multi-level override problem which is deeply problematic with MethodImpls, and for which we've added a new attribute that will need to be used by these covariant overrides, but instead, this is a signature comparison problem.
I agree that this looks like a runtime bug, although one of frightful long-standing. The issue here is that the runtime is applying the rules for substitutions in a way which is incompatible with specifying the exact override in this case. I believe this bug is in the substitutions setup for the comparisons. (Substitutions are used and non-exact signatures are considered to match in order to handle the scenario where a base type's virtual method is moved to its base type.)
Fixed with PR #38310
The runtime incorrectly interprets methodimpl table entries for overrides of class methods. Since they are used for generating code for covariant returns, we will need the runtime to obey them in order to correctly execute code using covariant returns. However, the problem can be illustrated without covariant returns using a compiler from Roslyn's covariant return feature branch.
The following program produces the IL shown below it. Running the program produces the (incorrect) output shown afterwards. The first two lines in each group of output lines are wrong.
Output produced by .net 3.1:
The correct output should be: