127

When using myDelegate -= eventHandler ReSharper (version 6) issues:

Delegate subtraction has unpredictable result

The rational behind this is explained by JetBrains here. The explanation makes sense and, after reading it, I'm doubting all my uses of - on delegates.

How then,

  • can I write a non-auto event without making ReSharper grumpy?
  • or, is there a better and/or "correct" way to implement this?
  • or, can I just ignore ReSharper?

Here is simplified code:

public delegate void MyHandler (object sender);

MyHandler _myEvent;

public event MyHandler MyEvent
{
    add
    {
        _myEvent += value;
        DoSomethingElse();
    }
    remove
    {
        _myEvent -= value; // <-- ReSharper warning here
    }
}
  • Mono gives the same warning. Here is R#'s description of the problem https://confluence.jetbrains.com/display/ReSharper/Delegate+subtraction+has+unpredictable+semantics (which only apply to lists of delegates) – thoredge Oct 19 '15 at 08:13

3 Answers3

139

Don't be afraid! The first part of ReSharper's warning only applies to removing lists of delegates. In your code, you're always removing a single delegate. The second part talks about ordering of delegates after a duplicate delegate was removed. An event doesn't guarantee an order of execution for its subscribers, so it doesn't really affect you either.

Since the above mechanics can lead to unpredictable results, ReSharper issues a warning whenever it encounters a delegate subtraction operator.

ReSharper is issuing this warning because multicast delegate subtraction can have gotchas, it isn't condemning that language feature entirely. Luckily those gotchas are in fringe cases and you are unlikely to encounter them if you're just instrumenting simple events. There is no better way to implement your own add/remove handlers, you just gotta take notice.

I'd suggest downgrading ReSharper's warning level for that message to "Hint" so that you don't get desensitized to their warnings, which are usually useful.

Allon Guralnek
  • 15,813
  • 6
  • 60
  • 93
  • 72
    I think it's bad of R# to call the results "unpredictable". They're very clearly specified. "Not what the user might predict" isn't in any way the same thing as "unpredictable". (It's also inaccurate to say that the .NET framework defines overloads - it's baked into the C# compiler. `Delegate` does *not* overload `+` and `-`.) – Jon Skeet Jun 24 '12 at 18:44
  • 6
    @Jon: I agree. I guess everyone got used to the high bar Microsoft set for itself. The level of polish being as high as it is, with so many things in the .NET world that make you "fall into the pit of success", encountering a language feature that is a mere brisk walk next to the pit where there is a chance you may miss it is considered by some to be jarring and warrants a sign saying `PIT OF SUCCESS IS THAT WAY --->`. – Allon Guralnek Jun 24 '12 at 20:01
  • 5
    @AllonGuralnek: On the other hand, when was the last time you heard of someone actually having a problem due to this? – Jon Skeet Jun 24 '12 at 20:05
  • 6
    @Jon: Heard of a problem with it? I didn't even _know_ about this behavior before this question was posted. – Allon Guralnek Jun 24 '12 at 20:29
  • Well, sounds like a good answer to me. (ReSharper doesn't warn about "normal use" of +=/-= delegates, I guess it can determine those to be the "non-list" kind?) –  Jun 25 '12 at 01:48
  • Check JetBrains wiki: http://confluence.jetbrains.net/display/ReSharper/Delegate+subtraction+has+unpredictable+semantics There is example of tricky cases with delegate subtraction – Evgeny Pasynkov Jul 05 '12 at 18:47
  • 3
    It's curious that R# would warn about delegate subtraction, but not warn about the common implementations of events which have *the exact same issue*. The core problem is that .net uses a single `Delegate.Combine` which "flattens" multicast delegates, so if it's given delegates [X,Y] and Z, it can't tell whether the result should be [X,Y,Z] or [[X,Y],Z] (the latter delegate holding the `[X,Y]` delegate as its `Target`, and that delegate's `Invoke` method as its `Method`). – supercat Feb 01 '13 at 23:02
  • @Allon Guralnek: You can easily add and subtract delegate lists via add/remove operators since value in those methods is of type MyHandler which is a delegate and by that can easily hold delegate lists. – mmmmmmmm Apr 11 '13 at 12:07
  • I think there seldom occurs a problem since most of the time you remove the same thing you add. So if you add a delegate list BC you also remove that same list BC and not CB. And in this case (as I understood the R# description) delegate - works. Nevertheless, I would prefer a perfect solution :-) – mmmmmmmm Apr 11 '13 at 12:08
  • I think blimacs answer should be the accepted one, because it resolves the issue in a cleaner way (as opposed to downgrading ReSharper's warning level). – mike Aug 15 '18 at 11:07
30

You should not directly use delegates to sum or subtract. Instead your field 

MyHandler _myEvent;

Should be instead declared as an event as well. This will solve the problem without risking your solution and still have the benefit of event usage.

event MyHandler _myEvent;

Usage of delegate sum or subtract is dangerous because you can lose events when simply assigning the delegate (as per declaration, the developer will not directly infer this is a Multicast delegate as when it is declared as an event). Just to exemplify, if the property mentioned on this question was not flagged as an event, the code below will case the two first assignments to be LOST, because someone simply assigned to the delegate (which is also valid!).

myObject.MyEvent += Method1; 
myObject.MyEvent += Method2;
myObject.MyEvent = Method3;

When assigning Method3, I completely lost the two initial subscriptions. Event usage will avoid this problem and at same time remove the ReSharper warning.

blimac
  • 319
  • 3
  • 3
  • I never thought of doing it this way, but it does make sense to protect the event delegate usage as long as you keep that underlying event private. This however doesn't work well when there are more specific thread synchronization that must happen in the add/remove handlers such as multiple event subscriptions or sub-subscriptions that need to be tracked as well. However, in any case it removes the warnings. – Jeremy Dec 01 '17 at 13:14
-17

set it to = null instead of using -=

Jonathan Beresford
  • 9
  • 6
  • 5
    the `remove` method of an event should not remove all handlers, but rather the handler that was requested to be removed. – Servy Feb 23 '15 at 17:02
  • if he's only added one then if he only removes one in effect he's removed them all. I'm not saying use it in all cases just for this specific resharper message. – Jonathan Beresford Feb 23 '15 at 22:06
  • 6
    But you *don't* know that the delegate is always removing the only item on the invocation list. This is making the resharper message go away by turning correct working code into incorrect broken code that will, in certain circumstances, coincidentally work, but that will break in unusual and hard to diagnose ways in many cases. – Servy Feb 23 '15 at 22:08
  • I had to upvote this because -17 is too harsh of a penalty for someone taking the time to write a "potential" answer. +1 for not being passive, even if you were incorrect. – John C Feb 06 '20 at 14:54