Roslyn: Questionable IDE0052 for private members that keep a side-effect producing reference (timer) alive

[Tragetaschen]

Version Used:
16.0.0 Preview 2.0

Steps to Reproduce:

class C
{
    private readonly Timer timer // IDE0052
            = new Timer(_ => Console.WriteLine("still running"), null, 0, 10_000);
}

That is, the live-time of the timer is bound to the live-time of the instance of C. Without the member, the garbage collector will remove the timer eventually before removing the instance of C.

I typically have a couple of singletons in my code base that simulate actual hardware while developing. These are put in the DI container and remain active for the entire application runtime. I could implement IDisposable, but this seems like overkill just to silence the warning given that the end of the program also ends the timer…

[jinujoseph]

@mavasani
Does this need design review ?

[mavasani]

@jinujoseph Yes - we can either treat fields typed as Timer specially so they are never flagged as dead code OR decide this is a rare case that recommendation would be for users to add suppression.

[andre-ss6]

I think that at least System.Threading.Timer specifically can receive special treatment. In most cases, as long as it was assigned anything, it very probably means it's being used.

Having said that, I think that IDisposable is a good ideia anyway in those cases. I know your class is a singleton and thus the Timer would be finalized along with the rest of the program, but there is a chance that a future developer could write code that instantiates this class more than once. Having IDisposable makes it less likely that they will be surprised with a Timer running after their instance variable left its scope - and, as a bonus, you don't get that message anymore :p

[jinujoseph]

reported at DC

[miloush]

Just run into this with System.Windows.Threading.DispatcherTimer. I don't think ignoring it fora specific type is going to resolve the use case of 'I want to keep a reference to this alive' which I wouldn't think is that rare.

[mavasani]

I don't think ignoring it fora specific type is going to resolve the use case of 'I want to keep a reference to this alive' which I wouldn't think is that rare.

@miloush Thats a specific case where an in source suppression with an appropriate justification comment serves as good documentation for why you want a long living object.

[mavasani]

Also reported at https://developercommunity.visualstudio.com/content/problem/802370/ide-is-recommending-me-to-remove-timers-because-it.html.

I am going to create a PR to special case the Timer type for now.

[miloush]

@masavani and by Timer you mean System.Threading.Timer or System.Timers.Timer or System.Windows.Forms.Timer? Why these and not DispatcherTimer?

[mavasani]

@miloush Good point. I guess I should move it back to Needs Design Review so we get a broader design approval on whatever is chosen.

[mavasani]

Design meeting review:

• There are multiple types of timers – do we exclude all, just System.Threading.Timer or ask user to change the code or update our code fix to put the right code in.
• Jason: Should we trigger this on any unused field that is of IDisposable type?
• Sam: Change equivalence ID for code fix for IDisposable fields, and change the message for disposable fields so it is clear to user that code needs to be restructured as recommended in https://github.com/dotnet/roslyn/issues/32956#issuecomment-493174433

Conclusion: Sam's proposal sounds fine.

[miloush]

@mavasani Thanks for the update. Can you please clarify what the proposed resolution is? I am reading it as the warning will stay for everything but the warning text will be different depending on whether the field type is IDisposable or not.

[mavasani]

Yes, that is correct - the text will be changed so you don't explicitly remove the field but restructure your code as suggested by @andre-ss6. We will also ensure that doing a FixAll in document/project /solution will not remove such members, unless the FixAll was also triggered for a disposable field.

[miloush]

I don't actually see how a field type being IDisposable has anything to do with the intention of ensuring the object stays alive for the lifetime of the containing instance. DispatcherTimer for example does not implement IDisposable. Your quick fix will still remove those and break the code.

I am also not finding the argument of @andre-ss6 and @Tragetaschen very convincing, as it is addressing a singleton scenario, which (purely guessing) I would expect to be much less common than just keeping a timer in e.g. a window, of which I want to have separate instances for each window.

Keeping objects alive does not seem to be the purpose of IDisposable and suggesting developers to resolve this warning by implementing potentially empty IDisposable seems to be supporting its misuse.

As a future developer using existing code, it seems more reasonable to depend on the knowledge of how lifetime of objects works rather than freaking out that fake IDisposable types are not handled according to the usual patterns for disposable types.

[BrainSlugs83]

@mavasani -- it looks like you marked my Visual Studio bug report as a duplicate of this issue and closed it.

Edit: and I reread the original issue, and I think you're right. Apologies for the previous confusion.

[mavasani]

@BrainSlugs83 Your report seems to identical to the repro in the original description of this issue, which also states a false IDE0052 suggestion in VS. How is your issue different?

[BrainSlugs83]

Apologies. I misread the original post. You are correct. -- Sorry for the confusion. -- If you ever run into me in person (I'm in Redmond), I owe you a beer. 😉

I will add my $0.02, that I am of the opinion that Timer deserves special treatment (if my vote counts). 😉

Honestly, I would go so far as to say, any class which could have side effects (so, non-structs, non-primitives) should get the same special treatment, as you can't be sure that they don't have side effects, similar to that of Timer (or even completely unique side effects). -- It's just baked into the language...

Edit: obviously, the warning is still valid for value types and primitives though. 🙂