CODEWITHSUNDEEP
c#inheritancewarningsnew-operator
Guy
Guy

Reputation: 67300

Use new keyword if hiding was intended

I have the following snippet of code that's generating the "Use new keyword if hiding was intended" warning in VS2008:

public double Foo(double param)
{
   return base.Foo(param);
}

The Foo() function in the base class is protected and I want to expose it to a unit test by putting it in wrapper class solely for the purpose of unit testing. I.e. the wrapper class will not be used for anything else. So one question I have is: is this accepted practice?

Back to the new warning. Why would I have to new the overriding function in this scenario?

Upvotes: 56

Views: 72106

Answers (4)

Perret
Perret

Reputation: 189

You'll always find some tricky situations where the new keyword can be used for hiding while it can be avoided most of the times.

However, recently I really needed this keyword, mainly because the language lacks some other proper synthax features to complete an existing accessor for instance:

If you consider an old-fashioned class like: 

KeyedCollection<TKey, TItem>

You will notice that the accesor for acessing the items trough index is: 

TItem this[Int32 index] { get; set; }

Has both { get; set; } and they are of course mandatory due to the inheritance regarding ICollection<T> and Collection<T>, but there is only one { get; } for acessing the items through their keys (I have some guesses about this design and there is plenty of reasons for that, so please note that I picked up the KeyedCollection<TKey, TItem>) just for illustrations purposes).

Anyway so there is only one getter for the keys access:

TItem this[TKey key] { get; }

But what about if I want to add the { set; } support, technically speaking it's not that stupid especially if you keep reasoning from the former definition of the propery, it's just a method... the only way is to implement explicitly another dummy interface but when you want to make implicit you have to come up with the new keyword, I'm hiding the accessor definition, keeping the get; base definition and just add a set stuffed with some personal things to make it work.

I think for this very specific scenario, this keyword is perfecly applicable, in particular in regards to a context where there is no brought to the { get; } part.

  public new TItem this[TKey key]
  { 
      get { return base... }
      set { ... }
  }

That's pretty much the only trick to avoid this sort of warning cause the compiler is suggesting you that you're maybe hiding without realizing what you are doing.

Upvotes: 1

Marc Gravell
Marc Gravell

Reputation: 1063013

The new just makes it absolutely clear that you know you are stomping over an existing method. Since the existing code was protected, it isn't as big a deal - you can safely add the new to stop it moaning.

The difference comes when your method does something different; any variable that references the derived class and calls Foo() would do something different (even with the same object) as one that references the base class and calls Foo():

SomeDerived obj = new SomeDerived();
obj.Foo(); // runs the new code
SomeBase objBase = obj; // still the same object
objBase.Foo(); // runs the old code

This could obviously have an impact on any existing code that knows about SomeDerived and calls Foo() - i.e. it is now running a completely different method.

Also, note that you could mark it protected internal, and use [InternalsVisibleTo] to provide access to your unit test (this is the most common use of [InternalsVisibleTo]; then your unit-tests can access it directly without the derived class.

Upvotes: 70

Jon Skeet
Jon Skeet

Reputation: 1501003

The key is that you're not overriding the method. You're hiding it. If you were overriding it, you'd need the override keyword (at which point, unless it's virtual, the compiler would complain because you can't override a non-virtual method).

You use the new keyword to tell both the compiler and anyone reading the code, "It's okay, I know this is only hiding the base method and not overriding it - that's what I meant to do."

Frankly I think it's rarely a good idea to hide methods - I'd use a different method name, like Craig suggested - but that's a different discussion.

Upvotes: 40

Craig Stuntz
Craig Stuntz

Reputation: 126557

You're changing the visibility without the name. Call your function TestFoo and it will work. Yes, IMHO it's acceptable to subclass for this reason.

Upvotes: 8

Related Questions