S1185: False positive

[pa...@paulomorgado.info]

Hi,

The rule states that:

Overriding a member just to call the same member from the base class without performing any other actions is useless and misleading. The only time this is justified is in sealed overriding methods, where the effect is to lock in the parent class behavior.

However, the way virtual calls work in C# is that, at compile-time, a virtual call is issued to the most derived implementation version of the overridden method in the base class, not to the declaration of the virtual method.

That means that if a type C on a different assembly derives from the one being analyzed (B), if the type being analyzed does not override the virtual method (M), on the derived type (C), a call to base.M will be a virtual call to the most derived implementation bypassing B. If later on B implements M, without recompilation, C won't call B.M.

I have a simple demonstration of this: https://github.com/paulomorgado/CSharpVirtualCallGotchas

The correct rule should recommend implementing every virtual method in non sealed classes, not the other way around.

--

Cheers,

Paulo Morgado

[valeri....@sonarsource.com]

Hi Paulo,

I researched a bit and I couldn't find an article on the Internet that advises to override all possible virtual methods just in case the base class changes without recompilation. To be fair, I couldn't find the opposite too, but also I never saw code that does that as well and it certainly does not seem like a good practice :)

The rule is meant to keep your code clean and simple, and show you the methods and properties that just call base and do nothing really meaningful.

Best regards,

Valeri

[pa...@paulomorgado.info]

According to the C# spec (§7.6.8):

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). Thus, within an override of a virtual function member, a base-access can be used to invoke the inherited implementation of the function member. If the function member referenced by a base-access is abstract, a binding-time error occurs.

Eric Lippert has a good write up about the issue here: https://blogs.msdn.microsoft.com/ericlippert/2010/03/29/putting-a-base-in-the-middle/

Keeping code clean and simple shouldn't be achieved at the expense of correctness and run time unexpected failures.

Imagine I have publish a library that follows your proposed rules and choose to pull some code form a derived class to a base class in a way that's functionally compatible. If someone takes a dependency on another library that has a dependency on this one and updates this one because of a bug fix, that someone will have her code break at run time while still compiling. 

I've updated my repo with a demo of what happens if an override is removed (tes2.cmd).

The rule should be that every non sealed class should override all virtual members, even if just to call the base.

[valeri....@sonarsource.com]

Hi Paulo,
 
I completely agree with the article you sent. But as far as I can say, Eric Lippert did not recommend to always override all virtual methods. Instead I think he suggests:
- if you are a library developer, you should be extra careful when you are adding new overrides, because they could be breaking changes and you should update the version of your library accordingly, so that your consumers know they need to recompile and retest.
- If you are an application developer, you should
   - test your application thoroughly and see if it breaks when the dependencies are changed, or
   - simplify your dependency chain, or use different way to reuse and organize code, not inheritance

I think this scenario is an excellent candidate for a binary analysis rule on compiled assemblies (which we cannot do yet, unfortunately) – to check if the code in an assembly is skipping virtual methods in the hierarchy chain. This way you will be absolutely certain that something is not right and you need to take the needed measures (e.g. recompile and upgrade dependencies).
 
Lately we are trying to reduce the noise from our rules to the maximum, so that our customers will see only actual problems, not potential caveats. Forcing to override every possible virtual method will generate a lot of noise, because in the majority of the cases the developers don’t have to care about their dependencies in the middle adding overrides.

All the best,

Valeri

[pa...@paulomorgado.info]

Hi Valeri,

Let's assume that the change I'm making is removing all code from my overload other than the call to the base. Following that rule, I would be removing the overload altogether.

Let's assume that I'm the developer of Newtonsoft.Josn (Json.NET) and let's also realize that 7 years have gone by and we're no longer in 2010.

Being the most downloaded package on NuGet (and probably the most depended on) and adding the fact that upon install a binding redirect of all versions to the current one is configured, the potential for a world wide disaster of epical proportions would be huge.

Wouldn't you agree?

Cheers,

Paulo Morgado

[valeri....@sonarsource.com]

Hi Paulo,

The rule does not take into account the previous state of the code and cannot know whether the overridden method was doing something or not before. However, the developer of the library could easily mark the method as ignored from this rule by adding a documentation comment:

/// <summary>

/// Keep this override for backwards compatibility. See https://blogs.msdn.microsoft.com/ericlippert/2010/03/29/putting-a-base-in-the-middle/

/// </Summary>

public override void Foo()

{

    base.Foo();

}

An empty documentation comment would be enough, but I would put the actual reason for this method being empty, in case some day I forget (which happens more often than expected).

I added this note to the rule description, as it was apparently missing...

All the best,

Valeri