Roslyn-analyzers: Event handler unhook properly

[curia-damiano]

Describe the problem you are trying to solve

The idea of this rule has been born after doing code reviews to code with memory leaks; some of them were caused by missed unhook of event handlers, and with my surprise I've found that there are currently no OOTB rules for this purpose.
So the idea would be to check the biggest issue with event handlers.

Describe suggestions on how to achieve the rule

I would like that the rule, for each class:

• detects event hooks (with +=);
• detects event unhooks (with -=);
• compares the two sets and reports differences between the two sets (hook without unhooks and vice versa).

Additional checks that this rule, or a new one, could do as well:

• detect event hooks with "=" instead of "+=";
• detect event unhooks with "=null" instead of "-=";
• detect even unhooks in the Dispose() or Dispose(bool) methods (as they don't solve the problems, because the event hook will prevent the Dispose method to run).

Additional context

In this case, I think warning/errors are more important than fixes.

I don't have experience in writing custom analyzers, so it would be nice if someone could pick up on them.

[mavasani]

Looks good to me. @sharwell @jmarolf any concerns? Otherwise, I can mark the rule as up-for-grabs.

[jmarolf]

Looks good to me!

[mavasani]

Tagging @Youssef1313 @Evangelink if you are keen on picking this up. This can be implemented as a SymbolStart/SymbolEnd based IOperation analyzer. To start with, we probably want to implement only the simple case of detecting += with missing -= and vice versa.

[KrisVandermotten]

I don't understand what this rule is supposed to detect. The premisse that hooking an event without unhooking it results in a memory leak is simply not correct.

Take the example of a Winforms application, with a Form with a Button on it. The Form has a reference to the Button (through its Controls property), and the Button has a reference to the Form (in its Parent property). Often, the Button's click event is handled by an instance method from the Form. What is the problem? There is no memory leak. When there are no references any more to the Form or the Button (other then their references to eachother and from the delegate), everything will be garbage collected.

[sharwell]

I generally agree with @KrisVandermotten here. While I understand the thought process that leads to a rule proposal like this, I don't believe that adherence to the rule addresses the original problem with enough specificity to overcome the maintenance complexity. I have no objection to the creation of the rule provided it is disabled by default and does not get enabled in projects where I'm a maintainer.

From a broader perspective, I see this similar to the way I see the current IDisposable analyzers. While I agree that it may be possible to have analyzers reduce the number of cases where incorrect use of the types leads to runtime behavior problems, I do not believe the current rule designs are optimal for achieving that outcome. I'd like to see the designs completely rewritten to try and find a superior approach.

[Evangelink]

I don't have the exact scenario in mind but I remember I faced a big issue with events on a WPF custom control that was actually bound to the fact I wasn't unsubscribing correctly causing the control to be shown on the UI when it wasn't supposed to be.

[mavasani]

@KrisVandermotten The rule is supposed to encourage consistent coding patterns, with use of source suppressions as documentation at sites where user is sure that unhooking is not required (similar to the example you shared). The analyzer will need to special case and not flag scenarios (like the Winforms case you mentioned) to remove noise for scenarios we know for sure that unhooking is not required.

@sharwell Lets say I have disagreed with your stance since the inception of the analyzers, and our views are not going to converge :-). Yes, changing API design or semantics can make things easier for users, but such changes normally are very hard to move forward given the semantic breaks associated with it (we have seen this with multiple years of fighting the dispose patterns and not making any progress on changing the dispose pattern/guidelines). The goal of the analyzers for such cases is to help users develop consistent coding patterns that will catch critical bugs. False positives can be reduced over time with iterative improvements for special cases. We implemented disposable analyzers (CA2000 and CA2213) with similar approach and they were the most popular FxCop analyzers in legacy implementation and also currently have an extremely low noise ratio in analyzer implementation because they find critical leaks (customer bugs filed initially on these rules to help improve these rules prove that customers valued these rules instead of turning them off).

I have no objection to the creation of the rule provided it is disabled by default

Again, I disagree - we should apply the same bar as IDE code style suggestions. The rule should be implemented, executed on some real world code repos, and if the default analyzer implementation does not show any real false positives on them, it should be triaged by the team to consider being an IDE suggestion.

[sharwell]

... does not show any real false positives on them ...

This is the heart of the problem. In my mind, the majority of diagnostics reported by this analyzer are false positives because it is exceedingly rare for an event to need to be unhooked for correct application behavior.

[mavasani]

exceedingly rare for an event to need to be unhooked for correct application behavior

Would that still be true if the analyzer is primarily a _performance_ recommendation?

[curia-damiano]

Hi Manish,
Our Microsoft guidelines recommend to unhook event handlers that are hooked manually, full stop.
Even if a memory leak is not guaranteed, it is still possible, and so the reason of my proposal for this rule.
Thank you for your support in pushing it.

[KrisVandermotten]

@mavasani

The analyzer will need to special case and not flag scenarios (like the Winforms case you mentioned) to remove noise for scenarios we know for sure that unhooking is not required.

I respectfully disagree. In my experience, unhooking is not required in the vast majority of cases. And when I say "not required", I mean that with or without unhooking, the same objects will become eligible for garbage collection at the same time, in other words, there is zero benefit to unhooking.

It would be great if we could have an analyzer that finds the rare true problems, scenarios where we know for sure that unhooking is required, instead of one that finds a lot of non-problems and attempts to filter out some of the noise.

To build such an analyzer, we should be able to understand the notion of object ownership in the code (similar indeed to how this understanding would benefit the IDisposable analyzers). Given that .NET has no way to express ownership (e.g. this button is owned by this form), it could turn out to be very difficult to build such an analyzer.

Would that still be true if the analyzer is primarily a _performance_ recommendation?

What else would it be?

[KrisVandermotten]

@curia-damiano

Our Microsoft guidelines recommend to unhook event handlers that are hooked manually, full stop.

That's an interesting phrase. What exactly do you mean when you say "manually"? How would the analyzer be able to tell the difference between an event handler that was hooked manually and one that was hooked in some other way?

Even if a memory leak is not guaranteed, it is still possible, and so the reason of my proposal for this rule.

I many cases where an event is hooked, a memory leak is not possible. In fact, in many cases it can be guaranteed (by understanding the program design) that there will be no memory leak. The problem is that it would be very hard to build an analyzer that has the required level of understanding to determine this.