Is There A Good Reason To Hide Inherited Members?

22 commentsWritten on January 17th, 2010 by
Categories: C#

A mistake that i used to see a lot, and sometimes still do, is that developers hide inherited members in derived types.  For those of you who don’t know what that means, check out the following class:

    public class MyClass

    {

        public void DoSomething()

        {

            Console.WriteLine("i'm doing something important");

        }

    }

 

As you can see, the DoSomething method does something important, so you as a developer should consider its behavior important as well.   In some cases, you might want to add something extra to this behavior in a derived class.  Some developers would do that like this:

    public class MyDerivedClass : MyClass

    {

        public void DoSomething()

        {

            base.DoSomething();

            Console.WriteLine("(i'm just better at it)");

        }

    }

 

When they compile this, they’ll get the following warning:

warning CS0108: 'HidingMethods.MyDerivedClass.DoSomething()' hides inherited member 'HidingMethods.MyClass.DoSomething()'. Use the new keyword if hiding was intended.

When this occurs, either one of 4 things can happen:

  1. The developer doesn’t bother to read compiler warnings (an offense worthy of a bitchslap) and is not aware of a possible problem
  2. The developer sees the warning and just adds the ‘new’ keyword to the method.
  3. The developer reads the documentation to figure what this really means, and hopefully realizes his mistake.  If he does, he combines this option with option 4.  If he doesn’t, he goes the option 2 route.
  4. The developer realizes his mistake and either makes the base method virtual and adds the override keyword to the method in the derived class, or when that’s not possible, either renames the method or thinks of a different approach.

If you’re unlucky, you either end up with no modification or the method will now look like this:

        new public void DoSomething()

        {

            base.DoSomething();

            Console.WriteLine("(i'm just better at it)");

        }

 

Great, no more compiler warning! All is well in the world now, right?

Err… not really.

The DoSomething method of MyDerivedClass actually hides the original DoSomething method. 

The following code will always produce the expected behavior:

            new MyClass().DoSomething();

            new MyDerivedClass().DoSomething();

 

That is, when the DoSomething method of an instance of MyClass is called, it will obviously execute the original DoSomething method.  And if the DoSomething method of an instance of MyDerivedClass class is called through a reference of MyDerivedClass then it will call the ‘new’ DoSomething method.

The following code however, would not produce the expected behavior:

        static void DoIt(MyClass subject)

        {

            subject.DoSomething();

        }

 

        static void Main(string[] args)

        {

            DoIt(new MyClass());   

            DoIt(new MyDerivedClass());

 

            Console.ReadLine();

        }

 

In this example, the DoSomething method is always called through a reference of MyClass.  When the DoIt method receives an instance of MyClass, the original DoSomething method will obviously be executed.  What some (many?) people unfortunately aren’t aware of is that when an instance of MyDerivedClass is passed into the DoIt method, the ‘new’ DoSomething method will not be executed but only the original one will be executed.  The reason is because methods that hide inherited members can only be called through references of the type, or derived types of that, that hides the inherited method.  Doesn’t really sound like a fun situation to debug, right?

So anyways, back to my original question: is there any valid reason why you would want to do this? Or better yet, can you share a situation where you had to resort to hiding an inherited member and if so, are you happy with the solution or did you consider it a hack?  And are you aware that it is essentially a bug waiting to happen?

So far, i have never actually seen a valid reason for doing this.  The only reason i’ve seen so far for occurrences of hidden members was because the developers that used it simply didn’t know any better.  In some cases, either me or someone else wasted debugging time on this when a piece of code that used a reference of a base class suddenly didn’t do what was expected when it was passed an instance of a derived class which hid the original method.  

I have read of only one valid reason to do this, and that is if a (either virtual or non-virtual) method with the same signature is introduced outside of your control in a base class that you can’t modify (like say, a class in the .NET framework or some other 3rd party assembly that you depend on) and you want to get rid of the compiler error that you got when recompiling against the newer version of the assembly.  In that case, it does make sense though i’d still say it’s going to be a future source of confusion sooner or later, and quite likely lead to a future bug as well.  In that situation, i’d much rather rename the member in my class.  Even if it means that consumers of my code will be forced to deal with the breaking change.  After all, a simple rename will always be less work than having to debug an issue because of a hidden member.

  • JeroenH

    If I were asked to remove one feature from the C# language, it would be the new keyword. There are a few use cases for it, but the potential for errors like you describe is indeed very real.

    For an explanation and a few reasonable use cases, see this post on Eric Lippert’s blog.

  • http://geekswithblogs.net/alternativedotnet Michel Grootjans

    Funny, I never really looked at it this way. Although I’m well aware of the ‘new’ keyword and its effects, I never used this feature. I never even considered why you’d want to use this feature, and I have absolutely no idea what the original intent was.

  • Dalibor Carapic

    I’ve used it in derived Windows Forms Controls in order to override the standard control property but provide different DefaultValue and DesignerSerializationVisibility attributes. The new property just calls the old property so it can not break the code.
    Other then that I have not seen it.
    While we are on the subject of weird possibilities that C# gives you, why is it not possible to declare virtual interface members which are explicitly implemented?
    (explitit implementation of interface member:
    void IInterface.DoSomething(string bla) { })
    This one is really anoying to me.

  • http://blog.maartenballiauw.be Maarten Balliauw

    Here’s a use case for the new keyword I used in the past few days:


    public class BaseSomething
    {
    public object Model { get; set; }
    }

    public class BaseSomething
    {
    public new TModel Model
    {
    get { return (TModel)base.Model; }
    set { base.Model = value; }
    }
    }

    It should not break anyting, but it provides an extra useful generic layer.

  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #519

  • Damien

    You say that in the case of a breaking change in a base class out of your control, you’d do a rename on your method and force your consumers to update. But consider – if you’re only becoming aware of the breaking change due to a compiler warning, how do you expect your consumers to discover *your* breaking change? When they recompile, they’ll now get the base class implementation, and no warning.

  • http://davybrion.com Davy Brion

    @Damien

    good point :)

  • http://dgoyani.blogspot.com/ Dhananjay Goyani

    So, do you want all methods to be virtual by default in .NET unless specified otherwise?

    Just today morning, I discussed virtual vs non-virtual with my team and here I read your entry. Passed this to my team and they have enjoyed it very much.

  • http://davybrion.com Davy Brion

    @Dhananjay

    i would prefer virtual-by-default but i do understand their decision to make non-virtual the default.

  • http://simpleprogrammer.com John Sonmez

    Yet another reason why inheritance is evil:
    http://simpleprogrammer.com/2010/01/15/inheritance-is-inherently-evil/

    Composition solves most of the problems with concrete class inheritance, because it makes everything explicit. If you implement an interface and provide a base implementation for that interface, all methods in the interface must be explicitly dealt with.

  • http://mynkow.blogpost.com mynkow

    Well there is one good reason to use the new keyword. At the moment I have nested entities which I want to display in a combobox. I inherit from Combobox and then write my own methods for DataSource, ValueMember, DisplayMember, Items etc.

  • http://davybrion.com Davy Brion

    @Mynkow

    well, once you pass instances of your derived Combobox to code that requires a Combobox reference and accesses one of those methods, you’ll run into the issues i mentioned.

  • http://mynkow.blogpost.com mynkow

    I want to hide the base class methods because they are useless. I do not want to use them ever. And I use the new keyword because the base classes are from .net framework.

  • http://www.cameronfletcher.com Cameron Fletcher

    I’ve used it on occasion to change the scope of an inherited member – eg. make a protected member public – whilst not altering the implemetation. I believe this is a valid use of the new keyword.

  • James Hare

    @Cameron while that may be valid syntactically, it would be a very dire thing to make a protected field public in a derived class. You’re then very strongly coupling the subclass and in essence violating the original class.

  • Thomas G. Mayfield

    @James I use “new” to change scope with framework (vendor, OSS libraries, whatever) code that’s marked “protected internal” in the base implementation. When I’m replacing a group of classes that form a single piece of functionality (the same reason the original code vendor marked it internal), I have no qualms about it having the same scope as it originally had (so “new protected internal”), but in my own project that extends it.

    @Davy My most common use case for “new” is on interfaces, where I want to have both a generic and non-generic version. But “new” on interfaces (where an implementing class is required to provide both versions) doesn’t often lead to the kind of bugs you’re describing.

  • Thomas G. Mayfield

    Side note: I’d forgotten about this, but I actually needed “new” yesterday to work around a problem with the Spark View Engine. It doesn’t create a strongly-typed HtmlHelper in its view classes: http://groups.google.com/group/spark-dev/browse_thread/thread/d59e8675d9a78916/747c170bd7932064#747c170bd7932064 But that’s another case like Maarten’s above, where code that uses a reference to the base class still functions the same.

  • Richard J Foster

    The place I’ve used it is very similar to Maarten Balliauw’s.

    Specifically, a base class provided a method returning ISomeType. An inheriting class always provided an ISomeOtherType (where ISomeOtherType inherits from ISomeType). If a reference to the inheriting class was used, I used the new keyword to ensure that when the method was called a reference to an ISomeOtherType was used. This meant it was not necessary for any caller to know that the value returned could be cast to ISomeOtherType.

  • Dathan Bennett

    As with Richard and Maarten, I use new to retype base class properties and methods to a type that’s appropriate to the derived class, but I always to do in a way that plays nice. The only potential concern would be the following:


    class BaseClass
    {
    public Model TheModel { get; set; }
    }

    class DerivedClass
    {
    public new DerivedModel TheModel
    {
    get { return (DerivedModel)base.Model; }
    set { base.Model = value; }
    }
    }

    DerivedClass instance = new DerivedClass() { TheModel = new DerivedModel }; // fine
    ((BaseClass)instance).TheModel = new IncompatibleModel(); // will pass because IncompatibleModel inherits from Model, but trouble is brewing
    Console.WriteLine(instance.TheModel); // typecast failure in the getter

    But I control for that in code, so I feel safe using it.

    I’ve also used it in slightly less safe context to convert the value of CheckBox.Checked to Nullable. (If Checked = null is set, we set the display to render the indeterminate checked state, and set “false” to the underlying Checked property). So long as I always reference Checked by a derived class reference or through reflection (e.g., data binding), everything’s fine, but I feel less sanguine about this solution.

  • Peter

    I use it to hide static generic factory methods; Thanks for the article -very to the point-

    public abstract class A
    {
    public static T Create() where T : A, new() { return new T(); }
    }

    public abstract class B : A
    {
    protected string _BSpecificParameter;

    public new static T Create() where T : B, new()
    { return Create(null); }

    public static T Create(string BSpecificParameter)
    where T : B, new()
    {
    T t = A.Create();
    t._BSpecificParameter = BSpecificParameter;
    return t;
    }
    }

  • Peter

    wrong code, srry (posted too early)

    errata:

    public abstract class A
    {
    public static T Create() where T : A, new() { return new T(); }
    }

    public abstract class B : A
    {
    protected string _BSpecificParameter;

    public new static T Create() where T : B, new()
    { return Create(null); }

    public static T Create(string BSpecificParameter)
    where T : B, new()
    {
    T t = A.Create();
    t._BSpecificParameter = BSpecificParameter;
    return t;
    }
    }

  • Pingback: Brain Twist: .NET MVC 3, Entity Framework 4.1, and TDD