New Design Guideline: Virtual Members


As you might guess from my last few posts, I have been doing some thinking about virtual members.  Here are the updated guidelines.    Please let me know if you have any questions or comments.  

As always, you can check out the base design guidelines and my incremental updates.

 

 

Do not call virtual members from constructors. Calling virtual methods during construction will cause the most derived override to be called, even though the most derived constructor has not been fully run yet.  FxCop Rule: ConstructorsShouldNotCallBaseClassVirtualMethods

Consider the example below which prints out “value != 42, what is wrong” when a new instance of Derived is created.  The implementer of the derived class assumes that the value will be set to 42 before anyone can call Method1().  But that is not true because the base class’s constructor is called before the Derived class’s constructor finishes, so any calls it makes to Method1() causes the problem.  

 

public class Base

{

    public Base()

    {

        Method1();

    }

    public virtual void Method1() {

        Console.WriteLine(“In Base’s Method1()”);

    }

}

public class Derived: Base

{

    private int value;

    public Derived()

    {

        value = 42;

    }

 

    public override void Method1()

    {

        if (value == 42) Console.WriteLine(“value == 42, all is good”);

        else Console.WriteLine(“value != 42, what is wrong?”);

    }

}

 

When a virtual method is called, the actual type that executes the method is not selected until runtime. When a constructor calls a virtual method there is a chance that the constructor for the instance that invokes the method has not executed.

Annotation : In unmanaged C++, the vtable is updated during the construction so that a call to a virtual function during construction only calls to the level of the object hierarchy that has been constructed.It’s been my experience that as many programmers are confused by the C++ behavior as the managed behavior. The fact is that most programmers don’t think about the semantics of virtual calls during construction / destruction – until they have just finished debugging a failure related to this.

Either behavior is appropriate for some programs and inappropriate for others. Both behaviors can be logically defended. For the CLR, the decision is ultimately based on our desire to support extremely fast object creation.

Annotation: Within a “program” a developer can deal with the behavior that is implemented. If there are different policies chosen in different “programs” interop becomes a serious issue. Therefore any API between separate “programs” (apps, add-ins) should not depend on behavior of virtual methods. C++ direct calls are another manifestation of this.

 

 

Do not call virtual members from destructors. Calling virtual members will result in possibly surprising behavior because the most derived implementation of the virtual method will be run. 
FxCop Rule:  TBD (XXX)

Consider the example below, for an instance of derived a C++ developer would expect Base::DoCleanUp() to run with the base class’s finalizer is run however because the runtime virtualizes the call to the runtime type of the instance is used and Derived::DoCleanUp() is called.  Notice this issue can be worked around my making DoCleanUp() no virtual or requiring overrides to insert a call to the base classes implementation.  

 

public class Base

{

    public virtual void DoCleanUp() {

        Console.WriteLine(“Do Base’s Cleanup”);

    }

    ~Base()

    {

        DoCleanUp();

    }

}

public class Derived: Base

{

 

    public override void DoCleanUp()

    {

        Console.WriteLine(“Do Derived Cleanup”);

    }

    ~Derived()

    {

        DoCleanUp();

    }

}

 

 

 

Comments (16)

  1. Is this a bad enough practice that it is worthy of being elevated to a compiler error rather than an FxCop error? Or are there plenty of cases where this is useful and needed?

  2. Stuart says:

    I’d consider making both of them compiler warnings, rather than errors.

    Jeroen Frijters once (http://weblog.ikvm.net/PermaLink.aspx?guid=EC330149-7DA0-473A-B693-1C51EB03496A and previous entries) made a pretty good case for not allowing destructors at all except in sealed classes. Basically, the argument was that destructors (as opposed to IDisposable) should only be used when wrapping unmanaged resources, and in that case the class that does the wrapping should always be sealed and simply implement the required methods for communicating with the unmanaged resource. Then a second class (which may not be sealed) would implement the public API by delegation to the sealed class.

    The only case I can think of where this *might* not be appropriate (and I’m not even sure about this) is if the native resource itself was, eg, implemented in C++ and had subclasses of its own, which might want to be wrapped. Not sure how you would handle that case.

  3. Stuart says:

    I’d consider making both of them compiler warnings, rather than errors.

    Jeroen Frijters once (http://weblog.ikvm.net/PermaLink.aspx?guid=EC330149-7DA0-473A-B693-1C51EB03496A and previous entries) made a pretty good case for not allowing destructors at all except in sealed classes. Basically, the argument was that destructors (as opposed to IDisposable) should only be used when wrapping unmanaged resources, and in that case the class that does the wrapping should always be sealed and simply implement the required methods for communicating with the unmanaged resource. Then a second class (which may not be sealed) would implement the public API by delegation to the sealed class.

    The only case I can think of where this *might* not be appropriate (and I’m not even sure about this) is if the native resource itself was, eg, implemented in C++ and had subclasses of its own, which might want to be wrapped. Not sure how you would handle that case.

  4. Stuart says:

    Sorry for the double post, I got a YSOD the first time.

  5. Philip Rieck says:

    Making the finalizer one an FXCop rule or compile error will make anyone who’s used the recommended Dispose pattern for unmanaged resources a bit unhappy.

    That involves having a virtual dispose method and a finalizer that calls it. See http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpgenref/html/cpconFinalizeDispose.asp

    Personally, I’d like to see this page updated so that a private member is called from both the finalizer and dispose for the "just in case" when the derived class doesn’t call base.dispose.

  6. Mike says:

    So would it be more appropriate to do something like:

    public class Base

    {

    public Base()

    {

    Method1();

    }

    public void Method1() {

    Console.WriteLine("In Base’s Method1()");

    }

    }

    public class Derived: Base

    {

    private int value;

    public Derived()

    {

    value = 42;

    Method1();

    }

    new public void Method1()

    {

    if (value == 42) Console.WriteLine("value == 42, all is good");

    else Console.WriteLine("value != 42, what is wrong?");

    }

    }

    ?

  7. Kevin Dente says:

    What’s the recommendation for handling cases like this?

    http://weblogs.asp.net/asmith/archive/2003/08/20/24765.aspx

  8. Stuart says:

    How about:

    public class MyUglyButton {

    private Color uglyColor = Color.Green;

    public override Color Color {

    get {return uglyColor;}

    set {uglyColor = base.Color = value;}

    }

    }

  9. Stuart says:

    On second thoughts, even that’s overcomplicated. There’s no reason to set base.Color as far as I can see.

    I wonder if you can safely call base.Foo() from a constructor (where Foo is virtual, but it doesn’t matter because you’re calling it non-virtually)?

  10. Matthew W. Jackson says:

    I had always been under the impression that calling base.Foo() in any method other than an overriding Foo results in a virtual call, and is exactly the same as this.Foo(). However, upon looking at the specs and at some disassembly, it is indeed possible to call base.Foo() from another method Bar().

    I’m trying to think of any real use for calling base.Foo() outside of the overridden this.Foo(). I suppose that base.Foo() could be used in a constructor, since there is no way that any derived version will run.

    However, a derived class may assume that base.Foo() will never be called directly (it’s a bad assumption, I know, considering C++ and IL’s ability to call instead of callvirt, and VB’s ability to use MyClass).

  11. Hi,

    maybe a bit contraversial opinion, but isn’t this expected behaviour? if I make something virtual, I EXPECT it to be overriden.

    Btw isn’t Dispose pattern conflicting with this? One of it’s part is calling Dispose(false) from finalizer, where Dispose IS supposed to be virtual…

    Kuba

  12. Well, to some folks it is suprising as C++ (unmanaged) does not work this way.

    Yes, you are right about the Dispose pattern… that is an exception to this rule