0

My library has some methods whose return value should never be discarded. Leaking them is a very popular mistake even for me, the author. So I want the compiler to alert programmer when it does so.

Such value may be either stored or used as an argument for another method. It's not strictly to use the stored value but if it's simply discarded it's 100% error.

Is there any easy to setup way to enforce this for my library users?

var x = instance.Method(); // ok
field = instance.Method(); // ok
instance.OtherMethod(instance.Method()); // ok
MyMethod(instance.Method()); // ok, no need to check inside MyMethod
instance.Method(); // callvirt and pop - error!

I thought about making IL analyzer for post-build event but it feels like so overcomplicated...

Vlad
  • 3,001
  • 1
  • 22
  • 52
  • What goal are you trying to achieve? Is `var x = instance.Method();` any better than `instance.Method();`, in scenarios when variable `x` is never read after the assignment? How about calling the method through reflection, and ignoring the return value? – Sergey Kalinichenko Jan 14 '16 at 09:06
  • @dasblinkenlight 1) the goal is to prevent mistakes which are hard to debug. 2) this is not about security, reflection is ok – Vlad Jan 14 '16 at 09:07
  • 1
    I forced myself once by using an out parameter for such a case. Not pretty, but works. – Stefan Jan 14 '16 at 09:11
  • @Stefan this is a very common case to use it as an argument for another method like `g.Assign(x, s.Dangerous(something))`. So `out` would make the code too ugly. May be a combination of out for storing and finalizer for return values would work but finalizers are for runtime... – Vlad Jan 14 '16 at 09:15
  • You can also return the out value as return value. This allows both types of use. The key is that the out parameter reminds the caller about assignment, mostly because you have an immutable class or struct. – Stefan Jan 14 '16 at 09:21

2 Answers2

1

If you implement Code Analysis / FXCop, the rule CA1806 - Do not ignore method results would cover this case.

See: How to Enable / Disable Code Analysis for Managed Code

Basically, it's as simple as going to the project file, code analysis tab, checking a box and selecting what rules to error / warn on.

enter image description here

Basically tick the checkbox @ 1, and then use 2 to get to a window where you can configure a ruleset file (this can either be one you share between libraries or something more global (if you have a build server, make sure its stored somewhere the build can get to, i.e. with the source not on a local machine).

Here's a ruleset with the rule I mean:

enter image description here

NikolaiDante
  • 18,469
  • 14
  • 77
  • 117
  • 1. your link "Code Analysis" doesn't open. 2. How can I "implement" it for my library? Or do you mean to enable this rule globally? – Vlad Jan 14 '16 at 09:11
  • @Vlad I've checked the links and they're working again, whatever was up with MSDN has resolved itself. – NikolaiDante Jan 14 '16 at 11:42
0

The Nicolai's answer enables ruleset for any types but I needed this check for only my library types (I don't want to force my library users to apply rule set on all their code).

Using out everywhere as suggested in the comments makes the library usage to hard.

Therefore I've chosen another approach.

  1. In finalizer I check whether any method was called (it's enough for me to confirm usage). If not - InvalidOperationException. Object creation StackTrace is optionally recorded and appended to the error message.
  2. User may call SetNotLeaked() to disable the check for particular object and all internal objects recursively.

This is not a compile-time check but it will surely be noticed.

This is not a very elegant solution and it breaks some guidelines but it does what I need, doesn't make user to view through unnecessary warnings (RuleSet solution) and doesn't affect code cleanliness (out).

For tests I had to make a base class where I setup Appdomain.UnhandledException handler in SetUp method and check (after GC.Collect) whether any exception was thrown in TearDown because finalizer is called from another thread and NUnit otherwise shows the test as passed.

Vlad
  • 3,001
  • 1
  • 22
  • 52