Base virtual member invocation rules

[paulomorgado]

As far as I understand, the invocation of a base member is made by a call to the closest known implementation at compile time.

Given:

// Assembly A
public class A
{
    public virtual string GetInfo() => nameof(A);
}

// Assembly B
public class B : A
{
}

// Assembly C
public class C : B
{
}

// Program
class Program
{
    static void Main(string[] args)
    {
        var c = new C();

        System.Console.WriteLine(c.GetInfo());
    }
}

Running the program will produce the following output:

A

IF the following change is made to the C assembly without recompiling any of the other assemblies:

public class C : B
{
    public override string GetInfo() => $"{base.GetInfo()}->{nameof(C)}";
}

Running the program will produce the following output:

A->C

IF the following change is made to the B assembly without recompiling any of the other assemblies:

public class B : A
{
    public override string GetInfo() => $"{base.GetInfo()}->{nameof(B)}";
}

Running the program will produce the following output:

A->C

That's because the base.GetInfo() call in C.GetInfo was compiled to be a call to A.GetInfo().

Is this right? Is this documented anywhere?

[paulomorgado]

Here is a test for this: https://github.com/paulomorgado/CSharpVirtualCallGotchas

[HaloFour]

It's mentioned in 7.6.8 of the C# spec:

When a base-access references a virtual function member (a method, property, or indexer), the determination of which function member to invoke at run-time (§7.5.4) is changed. The function member that is invoked is determined by finding the most derived implementation (§10.6.3) of the function member with respect to B (instead of with respect to the run-time type of this, as would be usual in a non-base access).

My understanding is that this might be born out of IL limitations. The C# compiler can't use callvirt to invoke the method, as that would result in invoking the most derived implementation of that method. So the compiler has to resort to using call and specifying the exact method that is to be invoked. B::GetInfo doesn't exist so the compiler must choose A::GetInfo.

[jveselka]

Eric Lippert blogged about it in 2010 and argued this is not a bug per se.
https://blogs.msdn.microsoft.com/ericlippert/2010/03/29/putting-a-base-in-the-middle/

[paulomorgado]

I was under the impression that Visual Basic did it different but I did the same test for Visual Basic and had the same results.

[paulomorgado]

Most static analysis tools (like SonarQube) recommend that overrides that just call the base should not exist.

Because of this, I think that all non sealed classes should override all virtual members, even if just to call the base.

[Mafii]

@paulomorgado that's just a hassle. It's what abstract is for, not virtual. I would dislike to do this every time. Or should the Compiler do this? Anyway, once this would be in effect, older code couldn't be used with the new compiler. It would either behave differently OR not even compile anymore. I see where you're coming from, but I disagree with what this would imply/change.

[paulomorgado]

@Mafii, I'm not discussing or proposing any change to the language and/or compiler.

I'm trying to assert what the exact behavior is and what should be the best practice.

[sharwell]

Because of this, I think that all non sealed classes should override all virtual members, even if just to call the base.

You can narrow this down substantially, since the situation can only occur if it's possible to override the method from a different assembly:

1. The method must be exposed (public, protected, protected internal).
2. The containing type must be exposed.
3. The containing type must have an exposed constructor.

💭 I lean in favor of the rule that the empty overrides should be avoided. Doing so provides a performance advantage, and it's easy enough to use tools like the Public API analyzer to avoid making changes that could potentially break external code in this manner.

[HaloFour]

@paulomorgado

Because of this, I think that all non sealed classes should override all virtual members, even if just to call the base.

Won't do you much good if the class you're deriving from exists in someone else's assembly, particularly one not written using this newer version of C#. Also, the JIT doesn't inline virtual methods so such would certainly cause an impact on performance.

This issue has existed since C# 1.0 and it's quite rare that it actually causes problems. I don't really think that it's worth addressing.

[paulomorgado]

@HaloFour

Won't do you much good if the class you're deriving from exists in someone else's assembly, particularly one not written using this newer version of C#.

If you make it a practice to always override everything, there won't be any deriving class on another assembly.

Also, the JIT doesn't inline virtual methods so such would certainly cause an impact on performance.

I'm aware of this, but once you go there, the decision to have an inheritance hierarchy and which members are virtual has already made.

I just made a quick search on Json.NET and found this on the JTokenWriter:

        public override void Close()
        {
            base.Close();
        }

I don't expect that many code out there to be extending JTokenWriter, but given that A LOT of applications out there depend directly or indirectly on Json.NET and that this will be on every config file:

<dependentAssembly>
    <assemblyIdentity name="Newtonsoft.Json" publicKeyToken="30ad4fe6b2a6aeed" culture="neutral" />
    <bindingRedirect oldVersion="0.0.0.0-10.0.0.0" newVersion="10.0.0.0" />
</dependentAssembly>

removing that would be nothing short of a catastrophe of global proportions.

[HaloFour]

@paulomorgado

If you make it a practice to always override everything, there won't be any deriving class on another assembly.

I'm talking about deriving from a class hierarchy that already partially exists in another assembly. For example, JTokenWriter already derives from JsonWriter. If you wanted to derive from JTokenWriter the C# compiler couldn't do anything about the virtual members of JsonWriter not overridden in JTokenWriter.

I'm aware of this, but once you go there, the decision to have an inheritance hierarchy and which members are virtual has already made.

Yes, but the cost of invoking those members now increases for every level of hierarchy regardless of whether or not those members have been overridden.

removing that would be nothing short of a catastrophe of global proportions.

As would removing any accessible member. That constitutes a breaking change. This is probably why that empty overrides exists; it didn't used to be empty, but they no longer need that logic there and removing the member would break compatibility.

[jnm2]

Wow, I would have expected removing an override to be in the disallowed list, not in the allowed list: https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md#members

[paulomorgado]

@jnm2, and they forgot the problem of removing an override.

[paulomorgado]

@HaloFour,

My point is that if you're designing your class hierarchy having in mind that it will be derived by others you have to take that into account and pay the price upfront.

I'm talking here about correctness and resilience over raw performance.

[jveselka]

Why would removing an override be a breaking change? call instruction would fallback to grandparent in that case. If you bothered to read article I linked before, great part of this discussion wouldn't need to happen. :-)

[jnm2]

It's really too bad there isn't a first-class opcode meaning "constrain this call to the nearest available superclass implementation," since that's every developer's mental model and the behavior at compile time.

[paulomorgado]

@zippec, have you tried my demo?

[HaloFour]

@jnm2

IIRC there really is no mechanism to do that. The instance has a single vtable slot for that virtual method and it points directly at the most overridden version. Short of reflection (or some variant thereof by the JIT compiler) there really isn't any easy way to determine that there are any other overrides of that method in the inheritance chain.

@zippec

That does appear to be the case. If you derived from JTokenWriter and overrode Close calling base.Close(), that would embed a call instance JTokenWriter::Close(). If someone later removed JTokenWriter.Close() the CLR would correctly call JsonWriter.Close() instead. So removal of a member is less of a concern.

Where it could still cause an issue at runtime is if the grandfather method is abstract.

[paulomorgado]

@HaloFour,

That does appear to be the case. If you derived from JTokenWriter and overrode Close calling base.Close(), that would embed a call instance JTokenWriter::Close(). If someone later removed JTokenWriter.Close() the CLR would correctly call JsonWriter.Close() instead. So removal of a member is less of a concern.

Wouldn't that throw a TypeLoadException?

[HaloFour]

@paulomorgado

Not when I tried it locally.

[paulomorgado]

With my repo? test2.cmd

[jveselka]

@HaloFour

Where it could still cause an issue at runtime is if the grandfather method is abstract.

Removing an implementation for abstract method forcing all childs to implement something they didn't need to implement before is no doubt a breaking change anyway.

But there is a problematic case when parent removes overriding method and introduces private or internal method with the same name. Then child is basically compiled with call to inaccessible member of another class which fails in runtime.

[paulomorgado]

Did anyone ran the tests in my repo?

[paulomorgado]

My test2.cmd had a few typos that totally sent me in the wrong direction.

Although C.GetInfo is still calling B.GetInfo after line #10:

.method public hidebysig virtual instance string 
        GetInfo() cil managed
{
  // Code size       22 (0x16)
  .maxstack  8
  IL_0000:  ldstr      "{0}->{1}"
  IL_0005:  ldarg.0
  IL_0006:  call       instance string [B]B::GetInfo()
  IL_000b:  ldstr      "C"
  IL_0010:  call       string [mscorlib]System.String::Format(string,
                                                              object,
                                                              object)
  IL_0015:  ret
} // end of method C::GetInfo

the runtime is cleaver enough to call A.GetInfo.

Tools like .NET Reflector still complain that B.GetInfo is missing, though.

It's still bad to remove an overload because, if you later need to reintroduce it, you'll get into to the case of adding an overload. But my premisse was all wrong.