C# Compiler should give warning but doesn't?

Question

Someone on my team tried fixing a 'variable not used' warning in an empty catch clause.

try { ... } catch (Exception ex) { }

-> gives a warning about ex not being used. So far, so good.

The fix was something like this:

try { ... } catch (Exception ex) { string s = ex.Message; }

Seeing this, I thought "Just great, so now the compiler will complain about s not being used."

But it doesn't! There are no warnings on that piece of code and I can't figure out why. Any ideas?

PS. I know catch-all clauses that mute exceptions are a bad thing, but that's a different topic. I also know the initial warning is better removed by doing something like this, that's not the point either.

try { ... } catch (Exception) { }

or

try { ... } catch { }

Answer 1

I think the person that answers this will need some insight into how the compiler works. However, something like FxCop would probably catch this.

Answer 2

The variable s is used... to hold a reference to ex.Message. If you had just string s; you would get the warning.

Answer 3

Properties are just methods, and there's nothing stopping anyone from putting some code that does something in the ex.Message property. Therefore, while you may not be doing anything with s, calling ex.Message COULD potentially have value....

Answer 4

Static analysis is somewhat limited in what it can accomplish today. (Although as Eric pointed out not because it doesn't know in this case.)

The new Code Contracts in .NET 4 enhances static checking considerably and one day I'm sure you'll get more help with obvious bugs like this.

If you've tried Code Contracts you'll know however that doing an exhaustive static analysis of your code is not easy - it can thrash for minutes after each compile. Will static analysis ever be able to find every problem like this at compile time? Probably not: see http://en.wikipedia.org/wiki/Halting_problem.

Answer 5

It's not really the job of a compiler to work out every single instance and corner case when a variable may or may not be used. Some are easy to spot, some are more problematic. Erring on the side of caution is the sensible thing to do (especially when warnings can be set to be treated as errors - imagine if software didn't compile just because the compiler thought you weren't using something you were). Microsoft Compiler team specifically say:

"...our guidance for customers who are interested in discovering unused elements in their code is to use FxCop. It can discover unused fields and much more interesting data about your code."

-Ed Maurer, Development Lead, Managed Compiler Platform

Answer 6

In this case the compiler detects that s is written but not read, and deliberately suppresses the warning.

The reason is because C# is a garbage-collected language, believe it or not.

How do you figure that?

Well, consider the following.

You have a program which calls a method DoIt() that returns a string. You do not have the source code for DoIt(), but you wish to examine in the debugger what its return value is.

Now in your particular case you are using DoIt() for its side effects, not its return value. So you say

DoIt(); // discard the return value

Now you're debugging your program and you go to look at the return value of DoIt() and it's not there because by the time the debugger breaks after the call to DoIt(), the garbage collector might have already cleaned up the unused string.

And in fact the managed debugger has no facility for "look at the thing returned by the previous method call". The unmanaged C++ debugger has that feature because it can look at the EAX register where the discarded return value is still sitting, but you have no guarantee in managed code that the returned value is still alive if it was discarded.

Now, one might argue that this is a useful feature and that the debugger team should add a feature whereby returned values are kept alive if there's a debugger breakpoint immediately following a method execution. That would be a nice feature, but I'm the wrong person to ask for it; go ask the debugger team.

What is the poor C# developer to do? Make a local variable, store the result in the local variable, and then examine the local in the debugger. The debugger does ensure that locals are not garbage collected aggressively.

So you do that and then the compiler gives you a warning that you've got a local that is only written to and never read because the thing doing the reading is not part of the program, it's the developer sitting there watching the debugger. That is a very irritating user experience! Therefore we detect the situation where a non-constant value is being assigned to a local variable or field that is never read, and suppress that warning. If you change up your code so that instead it says string s = "hello"; then you'll start getting the warning because the compiler reasons, well, this can't possibly be someone working around the limitations of the debugger because the value is right there where it can be read by the developer already without the debugger.

That explains that one. There are numerous other cases where we suppress warnings about variables that are never read from; a detailed exegisis of all the compiler's policies for when we report warnings and when we do not would take me quite some time to write up, so I think I will leave it at that.

Answer 7

Resharper would catch that