Not so long ago I was working on a new C# diagnostic - V3119 - for the PVS-Studio static code analyzer. The function of this diagnostic is to detect potentially unsafe constructions in the source code of C#, related to the usage of virtual and overridden events. Let's try to sort out, what's wrong with virtual events in C# - the principle of this diagnostic, and why Microsoft doesn't recommend using virtual and overridden events.
I think our readers are quite aware of what virtual mechanisms in C# are. The simplest example would be an example of virtual methods. In this case, virtuality allows to run the overridden virtual method according to the object's run-time type. I'll give an illustration using a simple example.
class A
{
public virtual void F() { Console.WriteLine("A.F"); }
public void G() { Console.WriteLine("A.G"); }
}
class B : A
{
public override void F() { Console.WriteLine("B.F"); }
public new void G() { Console.WriteLine("B.G"); }
}
static void Main(....)
{
B b = new B();
A a = b;
a.F();
b.F();
a.G();
b.G();
}As a result of execution we will have the following:
B.F
B.F
A.G
B.GEverything is correct. Since both objects a and b have the B type at run-time, then the call of the virtual method F() for both these objects will lead to the call of the overridden method F() of B class. On the other hand, a and b objects differ in the compile time type, having A and B types accordingly. That's why the call of the G() method for each of these objects leads to the call of the corresponding method for A or B class. You can find more details about the usage of the keywords virtual and override here.
Like methods, properties and indicators, events can also be declared as virtual:
public virtual event ....You can do this as for "simple" and for events, explicitly implementing accessors add and remove. So, working with virtual and overridden events in the derived classes, it would be logical to expect behavior similar to the behavior of the virtual methods. But this is not the case. Moreover, MSDN directly say that they do not recommend using virtual and overridden events: "Do not declare virtual events in a base class and override them in a derived class. The C# compiler does not handle these correctly, and it is unpredictable whether a subscriber to the derived event will actually be subscribing to the base class event".
However, we do not give up, so let us try to implement "... declare virtual events in a base class and override them in a derived class".
As the first experiment, let us create a console application, where we will have two virtual events in the base class declared and used (with explicit and implicit implementation of add and remove accessors) and a derived class, overriding these events:
class Base
{
public virtual event Action MyEvent;
public virtual event Action MyCustomEvent
{
add { _myCustomEvent += value; }
remove { _myCustomEvent -= value; }
}
protected Action _myCustomEvent { get; set; }
public void FooBase()
{
MyEvent?.Invoke();
_myCustomEvent?.Invoke();
}
}
class Child : Base
{
public override event Action MyEvent;
public override event Action MyCustomEvent
{
add { _myCustomEvent += value; }
remove { _myCustomEvent -= value; }
}
protected new Action _myCustomEvent { get; set; }
public void FooChild()
{
MyEvent?.Invoke();
_myCustomEvent?.Invoke();
}
}
static void Main(...)
{
Child child = new Child();
child.MyEvent += () =>
Console.WriteLine("child.MyEvent handler");
child.MyCustomEvent += () =>
Console.WriteLine("child.MyCustomEvent handler");
child.FooChild();
child.FooBase();
}The result of the exectution will be:
child.MyEvent handler
child.MyCustomEvent handlerUsing the debugger or a test output, it's easy to make sure that at the time of the child.FooBase() call, the values of both variables MyEvent and _myCustomEvent are null, and the program doesn't crash only because of the conditional access operator upon the attempt to initialize the events MyEvent?.Invoke() and _myCustomEvent?.Invoke().
So, the MSDN warning was not in vain. It really doesn't work. The subscription to the virtual events of an object using the Child run time type, doesn't lead to a simultaneous subscription to the events of the Base class. In the case of implicit implementation of the event, the compiler automatically creates methods-accessors for it - add and remove, and also a delegate field, which is used to subscribe and unsubscribe. The problem, apparently, is that if you use a virtual event, the basic and child classes will have individual (not virtual) delegate-fields that are connected with this event.
In the case of explicit implementation - it is a developer who does that, and takes into account this peculiarity of virtual events behavior in C#. In the example above, I didn't take into account this peculiarity, declaring the delegate property _myCustomEvent as protected in the base and derived classes. Thus, I actually repeated the implementation provided automatically by the compiler for virtual events.
Let's try to achieve the expected behavior of a virtual event, with the help of the second experiment. To do this, let's use a virtual and overridden event with explicit implementation of add and remove accessors, and also a virtual delegate property, related to it. Let's change the text of the program from the first experiment:
class Base
{
public virtual event Action MyEvent;
public virtual event Action MyCustomEvent
{
add { _myCustomEvent += value; }
remove { _myCustomEvent -= value; }
}
public virtual Action _myCustomEvent { get; set; } // <= virtual
public void FooBase()
{
MyEvent?.Invoke();
_myCustomEvent?.Invoke();
}
}
class Child : Base
{
public override event Action MyEvent;
public override event Action MyCustomEvent
{
add { _myCustomEvent += value; }
remove { _myCustomEvent -= value; }
}
public override Action _myCustomEvent { get; set; } // <= override
public void FooChild()
{
MyEvent?.Invoke();
_myCustomEvent?.Invoke();
}
}
static void Main(...)
{
Child child = new Child();
child.MyEvent += () =>
Console.WriteLine("child.MyEvent handler");
child.MyCustomEvent += () =>
Console.WriteLine("child.MyCustomEvent handler");
child.FooChild();
child.FooBase();
}Result of the program execution:
child.MyEvent handler
child.MyCustomEvent handler
child.MyCustomEvent handlerTake note of the fact that there were two executions of the handler for the event child.MyCustomEvent. In debugging mode, it is easy to detect that now, upon the call of _myCustomEvent?.Invoke() in the FooBase() method, the value of the delegate is not null. Thus, we managed to get the expected behavior for virtual events only by using events with explicitly implemented accessors add and remove.
You may say that that's great, of course, but we are talking about some synthetic examples from the theoretical field, so let these virtual and overridden events remain there. I'll give the following comments:
To test the quality of PVS-Studio analyzer's work, we use a pool of test projects. After adding the new rule, V3119, to the analyzer that is devoted to virtual and overridden events, we did a check of the whole pool of projects. Let's see what warnings we got.
This project has been previously checked, and you can find the article here. Now I just give a list of analyzer warnings that are related to virtual and overridden virtual events.
PVS-Studio warning: V3119 Calling overridden event 'Started' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. GlobalOperationNotificationServiceFactory.cs 33
PVS-Studio warning: V3119 Calling overridden event 'Stopped' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. GlobalOperationNotificationServiceFactory.cs 34
private class NoOpService :
AbstractGlobalOperationNotificationService
{
....
public override event EventHandler Started;
public override event
EventHandler<GlobalOperationEventArgs> Stopped;
....
public NoOpService()
{
....
var started = Started; // <=
var stopped = Stopped; // <=
}
....
}In this case, we are most likely dealing with a situation of forced overriding of virtual events. The base class AbstractGlobalOperationNotificationService is abstract, and has declaration of abstract events Started and Stopped:
internal abstract class
AbstractGlobalOperationNotificationService :
IGlobalOperationNotificationService
{
public abstract event EventHandler Started;
public abstract event
EventHandler<GlobalOperationEventArgs> Stopped;
....
}It's not quite clear how the overridden events Started and Stopped will be used further on, because the delegates are just assigned to the local variables started and stopped, and aren't used in the NoOpService in any way. However, this situation is potentially unsafe, and the analyzer warns about this.
The analysis of the project has also been previously described in the article. I'll give here a list of the V3119 analyzer warnings.
PVS-Studio warning: V3119 Calling overridden event 'ParseInformationUpdated' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. CompilableProject.cs 397
....
public override event EventHandler<ParseInformationEventArgs>
ParseInformationUpdated = delegate {};
....
public override void OnParseInformationUpdated (....)
{
....
SD.MainThread.InvokeAsyncAndForget
(delegate { ParseInformationUpdated(null, args); }); // <=
}
....The analyzer detected usage of an overridden virtual event. We'll have a dangerous situation in case of inheritance from the current class, and overriding of the ParseInformationUpdated event in the derived class.
PVS-Studio warning: V3119 Calling overridden event 'ShouldApplyExtensionsInvalidated' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. DefaultExtension.cs 127
....
public override event
EventHandler<DesignItemCollectionEventArgs>
ShouldApplyExtensionsInvalidated;
....
protected void ReapplyExtensions
(ICollection<DesignItem> items)
{
if (ShouldApplyExtensionsInvalidated != null)
{
ShouldApplyExtensionsInvalidated(this, // <=
new DesignItemCollectionEventArgs(items));
}
}
....Again, the analyzer detected usage of an overridden virtual event.
This project was also previously checked by PVS-Studio. You can find the results of the analysis in this article. The new V3119 diagnostics issued 2 warnings.
PVS-Studio warning: V3119 Calling virtual event 'OnAfterComponentAdd' may lead to unpredictable behavior. Consider implementing event accessors explicitly. MyInventoryAggregate.cs 209
PVS-Studio warning: V3119 Calling virtual event 'OnBeforeComponentRemove' may lead to unpredictable behavior. Consider implementing event accessors explicitly. MyInventoryAggregate.cs 218
....
public virtual event
Action<MyInventoryAggregate, MyInventoryBase>
OnAfterComponentAdd;
public virtual event
Action<MyInventoryAggregate, MyInventoryBase>
OnBeforeComponentRemove;
....
public void AfterComponentAdd(....)
{
....
if (OnAfterComponentAdd != null)
{
OnAfterComponentAdd(....); // <=
}
}
....
public void BeforeComponentRemove(....)
{
....
if (OnBeforeComponentRemove != null)
{
OnBeforeComponentRemove(....);
}
}
....We are dealing here with the declaration and usage not of overridden, but of virtual events. In general, the situation is no different from the previous ones.
The RavenDB project is a so called "NoSQL" (or document-oriented) database. Its detailed description is available on the official website. The project is developed using .NET, and the source code is available on GitHub. The analysis of RavenDB by the PVS-Studio analyzer detected three V3119 warnings.
PVS-Studio warning: V3119 Calling overridden event 'AfterDispose' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. DocumentStore.cs 273
PVS-Studio warning: V3119 Calling overridden event 'AfterDispose' may lead to unpredictable behavior. Consider implementing event accessors explicitly or use 'sealed' keyword. ShardedDocumentStore.cs 104
Both of these warnings were issued for similar code fragments. Let's take a look at one such fragment:
public class DocumentStore : DocumentStoreBase
{
....
public override event EventHandler AfterDispose;
....
public override void Dispose()
{
....
var afterDispose = AfterDispose; // <=
if (afterDispose != null)
afterDispose(this, EventArgs.Empty);
}
....
}The event AfterDispose, overridden in the class DocumentStore, is declared as abstract in the base abstract class DocumentStoreBase:
public abstract class DocumentStoreBase : IDocumentStore
{
....
public abstract event EventHandler AfterDispose;
....
}As in the previous examples, the analyzer warns us of the potential danger, should the virtual event AfterDispose be overridden and be used in the classes derived from DocumentStore.
PVS-Studio warning: V3119 Calling virtual event 'Error' may lead to unpredictable behavior. Consider implementing event accessors explicitly. JsonSerializer.cs 1007
....
public virtual event EventHandler<ErrorEventArgs> Error;
....
internal void OnError(....)
{
EventHandler<ErrorEventArgs> error = Error; // <=
if (error != null)
error(....);
}
....Here we have declaration and use of a virtual event. Again, there is a risk of undefined behavior.
I think we can stop here and draw the conclusion that we really shouldn't use implicitly implemented virtual events. Due to the specifics of their implementation in C#, the usage of such events can lead to undefined behavior. In case you have to use overridden virtual events (for example, upon the derivation from an abstract class), this should be done with caution, using explicitly defined accessors add and remove. You can also use the keyword sealed, when declaring a class or an event. And of course, you should use static code analysis tools, like PVS-Studio for example.