Should we produce warnings for unused/uninitialized internal fields?

Should we produce warnings for unused/uninitialized internal fields?

Rate This
  • Comments 39

You may have noticed as you've been developing C# applications that if you have an internal field of an internal type which you never use, we give you a warning about it. In fact, there are a number of warnings that we give for such fields, depending upon whether the field was never read or written, initialized but not read, or read but not initialized (and therefore always set to its default value.)

Now, obviously we ought not to give similar warnings for public fields on public types. The thing doing the reading or writing is highly likely to be an external assembly which may not even have been written yet, so it would be silly to give a warning. (And similarly, we suppress this warning on internals if we are building a module that is going to be linked to another module; the reads and writes might be in the other module.  We also suppress it if the internal members of the assembly are visible to an external assembly.)

The C# 2.0 compiler was a bit weak though. If you had an unused public field on an internal class, the compiler would say, well, that's a public field, suppress the warning. But a "public" field on an internal class is not actually visible to anything in the outside world (again, modulo the parenthetical above) so really we could produce the warnings here.

There were a bunch of cases like that. A few months ago I tightened up the C# 3.0 compiler so that we now detect and warn for all the possible cases where we can actually deduce that the field is not used/not initialized.

Naturally this means that people started getting warnings on code which had previously compiled without warnings, so other teams at Microsoft that test pre-release builds of the compilers started to notice. This change has raised a few questions amongst users on other teams, which might motivate changing this behaviour again. It is particularly hard on users who compile with "all warnings are errors" turned on.

Two scenarios in particular have been brought to my attention.

First, more and more classes are now being written specifically to be used with private reflection and/or lightweight codegen. That is, something outside of the C# language is going to be manipulating the fields of a particular class, and it is by design to have the field unread/unwritten by the static portion of the program. It's irksome to have a warning about something which is by design.

Second, more and more classes are now being written where a field is initialized but never read, but the field is initialized to a finalizable object, and the finalizer effectively “reads” the field and does work on it.

There are a number of possible responses to these scenarios:

1) Roll back the behaviour to that of C# 2.0. That doesn’t actually solve any of these problems; we still potentially give warnings which the user doesn’t want.  But we give warnings in fewer scenarios.  We can also make the plausible statement “this is what the compiler has always done, you can live with it”. 

The downside of course is that we would be eliminating warnings which the user may want. The whole point of this exercise in the first place was to produce useful warnings.

2) Do nothing. Tell the people who are raising these objections to live with it.  They can turn off the warning programmatically if they want. We strike a blow for language purity and ignore the real-world objections of people who are using features external to the language like reflection.

3) Give the "unread" warning only if the field is of a type known to have no finalizer. (Or, weaker, suppress the "unread" warning only if the field is of a type known to have a finalizer.)

4) Eliminate all the warnings altogether.  Declare a new design principle: warnings should be only for patterns which are highly likely to be  dangerous mistakes.  The warnings in question are just helpful reminders that you might have dead code to eliminate.  FXCOP produces this warning already, so if people want to see it, they can use FXCOP or the static analysis tool of their choice.

5) Create some new attributes that let people document how a field is intended to be used. Be smart about looking at the attribute, and if we see attributes which say "this struct is intended for use in p/invoke", or "this field is to be accessed via private reflection", and so on, then suppress the warnings.

6) Do something else that Eric hasn't thought of.

Anyone have thoughts or opinions on this?

Thanks!

  • The first thing I thought of before even seeing your list of ideas was an attribute based approach.

  • This is a pertinent issue to my organization. We use a custom OR-mapping framework that uses reflection as described in your first scenario. We get many instances of the warning for unused/uninitialized fields and unfortunately sometimes our developers end up with the bad habit of ignoring compiler warnings - ANY warnings. It would be excellent if something like Option #5 could be implemented.

  • I would vote for #5 iff (if and only if) this attribute can solve some other compiler problems, like inlining or type inference optimization - not just for a warning. An attribute like [External] to flag that the field is used in externall (or unsafe) code could be used. The problem is System.Reflection(.Emit) that can really mess things up but I don't think that everyone else should pay the price just because I like to play around with Reflection.Emit. Using attributes/keywords to make optimizations possible (or rather switch off optimizations) is already incorporated with the volatile-keyword so using attributes/keywords to solve Reflection.Emit or unsafe code problems wouldn't really change the feeling of the language - but it would have the huge downsize that it would break a lot of c#2.0-code... .

    On a similar note: I get warnings about unused fields in private nested structs if I only reach these fields indirectly in unsafe code using pointer-manipulations and pointer-casts to get to them (which is possible and "safe" if I have a fixed layout on the struct). E.g.:

    Struct1* s1Ptr = &s;

    Struct2* s2Ptr = (Struct2*)s1Ptr;

    In this case I would get a warning about the fields in Struct2 not being used, even if they might be used What I am trying to say is that #3 is an OK solution since you are giving warnings in to many cases already anyway and I think the developers realize that you can't think of all the strange cases. If you don't like to add even more possibly incorrect warnings I would vote for roberthahn's solution (i.e. one more warning level).

  • I vote #2. There was never a guarantee that the GC wouldn't collect unused fields after their last use, was there? (It does, after all, reserve the right -- and exercises that right! -- to collect an object even while one of its own methods is running.) If it's that important that the reference stay alive in beyond its last use, the class can do a GC.KeepAlive() on it in the destructor.

  • Welcome to the XXVIII Community Convergence. In these posts I try to wrap up events that have occurred

  • I vote #5.  We use reflection a lot, and it violates standard information hiding, so it is vitally important developers reading the code can identify these fields.  An attribute would solve both problems, and conveniently issue a warning when the attribute is forgotten.

  • Basically what's needed in my opinion is greater flexibility and control in the hands of the developer, while at the same time keeping the tedium to a minimum.

    But another way is to look at the settings for warning levels as being slightly too crude. Assuming all warnings have a severity level they also have a separate type of impact on your code. CS0649 etc isn't necessarily a warning - but it can only be determined on a case by case basis.

    #5 Works very well in that it would play nicely with documentation - signalling that this property will be modified in ways beyond statical analysis etc. It's also the only proposed solution with a case by case flexibility.

  • I definitely like #5.  It combines the requirement that someone properly plan / explicitly define those properties... something that should be minor if they are going to be using something as hardcore (and often not warranted in use) as refelection, takes no runtime clock cycles, and still properly punishes the lazy [omitted] on my dev team that like to copy / paste entire classes and only use part of them.

  • #5!

    IIRC, in C++ there also was something linke UnusedParameter which was there to supress the warning.

Page 3 of 3 (39 items) 123