Static DIM reabstractions don't need to be final?

[MichalStrehovsky]

@BrzVlad pointed me to this passing test:

runtime/src/tests/Loader/classloader/StaticVirtualMethods/DiamondShape/svm_diamondshape.il

Lines 235 to 239 in 201aae9

	.method private hidebysig virtual abstract
	static int32 Func(int32 a) cil managed
	{
	.override method int32 I1::Func(int32)
	} // end of I4Reabstract::Func

Notice that this is an .override marked static abstract virtual. Notably there's no final. This means we create a new slot that someone could individually override (on top of the method we're overriding). Is this intentional?

If this was a non-static default interface methods we enforce that these are always marked final. If they aren't, the runtime will throw TypeLoadException "Method implementation on an interface 'X' from assembly 'Y' must be a final method.".

FWIW, attempting to actually override this method currently throws a MissingMethodException (for a method that exists, whatever that means), and Roslyn does emit these as final, so it should be safe to enforce it here too, unless it's intentional.

Cc @davidwrighton @trylek

[SamMonoRT]

cc @lambdageek for visibility

[lambdageek]

Yea, @BrzVlad and I discussed this in the context of Mono's DIM implementation - I think the test is wrong - possibly there was some iterations between the runtime and Roslyn and earlier non-final methods were allowed.

Reading ECMA-335 augments https://github.com/dotnet/runtime/blob/main/docs/design/specs/Ecma-335-Augments.md#proposed-specification-change-5

Section "II.12.1 Implementing interfaces" is extended to say all virtual instance methods defined on an interface must be abstract, be marked with newslot and not have an associated MethodImpl which uses the method as its Impl, or final without newslot and with a MethodImpl that uses the method as its Impl entry.

My reading of this is that since reabstraction uses .override to add a MethodImpl, the reabstracted-method must be final. The test is wrong.

[MichalStrehovsky]

My reading of this is that since reabstraction uses .override to add a MethodImpl

This section was added for instance default interface methods and wasn't updated for statics - "all virtual instance methods defined on an interface must be".

It would really help if @davidwrighton or @trylek could clarify whether we should update the wording to apply to static methods as well and fix the runtime (to throw) and test (to be final).

[MichalStrehovsky]

@mangod9 I moved this to 8.0, this is blocking implementations in other runtimes.

[trylek]

Hmm. I have recently fixed this bit to put it in sync with what Roslyn emits in

#80987

I was under the impression that requiring such overridden methods to be final was a temporary limitation for the sake of code simplicity where we originally didn't allow multiple overrides in derived classes. For the reabstraction case the final modifier doesn't make much sense to me as I believe the sole purpose of the reabstraction is to require an implementing class to override the method in question (basically as if the interface with the reabstraction declaration was the one declaring the original abstract virtual method). According to Vlad's explanation the Mono implementation is quite different than the CoreCLR version so I can easily understand these flags may have additional subtle meanings I'm not used to from CoreCLR, I'll be happy to make sure we can continue making progress in all runtimes we currently support.

[mangod9]

@MichalStrehovsky @trylek does this need any fixing in 8?

[MichalStrehovsky]

@MichalStrehovsky @trylek does this need any fixing in 8?

If we want to block static reabstractions that are not marked final, we better do it earlier than later. I don't think non-final reabstractions actually work with CoreCLR today - the VM does something completely unexpected if I try to actually override the non-final slot.

I believe non-final reabstractions should be blocked by the type loader, but I can't get David to make a statement on this (I tried here, over email, etc.). I'm not in Redmond, so this is my best. Last time I checked, this was blocking the Mono team from making progress on their implementation.

[mangod9]

Moving this to 9 at this point.