Why are unused using directives not a warning?

Why are unused using directives not a warning?

Rate This
  • Comments 56

As I’ve discussed before, we try to reserve warnings for only those situations where we can say with almost certainty that the code is broken, misleading or useless. One reason for trying to ensure that warnings are not “false positives” is that we don’t ever want to encourage someone to take working, correct code and break it so as to remove the warning. Another is that since many people compile with “warnings are errors” turned on, we do not want to introduce a whole lot of unnecessary build breakages should the warning turn out to be unwarranted.

An unused using directive is almost certainly not “broken”, and it’s probably not misleading. It certainly is useless though. Why not produce a warning for this, so that you can remove the useless directive from your program?

Suppose we make an unused using into a warnings. Suppose you do have “warnings are errors” turned on. And suppose you create a new console project in Visual Studio.  We generate you this code:

using System;
using System.Text;
using System.Linq;

class Program
{
  static void Main(string[] arguments)
  {
  }
}

… which doesn’t compile. Which is better, generating code that doesn't compile, or omitting the using directives and making you type "using System;" the first time you try to use Console.WriteLine?

Either way is a really bad user experience. So, no such feature.

  • Oh Sweet Jesus, why would anyone even want such a "feature"?

    It's a _compiler_, it's job is to turn code into executable and tell you why if it couldn't.

    Warnings themselves are a feature!

    If you want something to complain about things like "unused using statement", then you should be using a code style analyzer.  Like maybe StyleCop? Or FxCop? Both very good products!

    You may have orange juice as well as coffee every morning, and you might have the worlds best coffeemaker, but that's a terrible reason to try and get your coffeemaker to make orange juice for you as well.

    I mean really, the right solution for the problem.  Right?

    The C# compiler is a fantastic tool, the idea shouldn't be to add "features" to it till it sucks like everything else.  Where do these crazy ideas come from?

    Thank-you Eric, keep shooting down the crazy ideas! Keep the C# compiler beautiful!

  • @Jon:

    >  Defaulting to creating event handlers such as "button1_Click"

    I understand the frustration with all your other points, but what's the problem in this case? Are you unhappy about "button1" (but then you've already listed the naming of new controls as a separate point)? Or is it the "ControlName_EventName" convention - but if the latter, what's wrong with it?

  • An issue to be aware of with treating unused usings as a warning/error is that sometimes the using isn't needed in the generated code but it is needed to avoid other warnings.  The classic example is doc comments.  If you reference something in a <see> tag then you'll get a compiler warning if the item can't be found.  The using statements in the code determine whether that item is found or not.  The code itself will compile without the using statement.  It is tedious and wasteful to have to use full type names in comments so that really isn't an option.

  • @Pavel: It's the "ControlName_EventName" convention, which is made worse by violating normal naming conventions (as the control name is usually in camel case).

    My problem with that convention is it's non-descriptive - it describes the caller, rather than what it will do. If I have a save button and I click it, I want to hook that up to a SaveDocument method, not a button1_Click method - then:

    1) When I'm browsing the member list I can tell exactly what that method's going to do

    2) When I'm looking through the event subscriptions I know what will happen when I click on the save button

    3) If I want to hook the same method up to multiple events, I can do so with no extra work and within the same convention, because I'm describing what the action should do, rather than what's causing it.

    Where else do we name methods based on the circumstances under which they're called, rather than what they're meant to do?

  • Java has an annotation (the equivelent of attributes in .NET) for suppressing warnings. I'd love to see this in .NET:

    [SupressWarning]

    some line of code that causes a warning

    Just an idea.

  • @Tim: There's already #pragma warning disable/restore for this purpose.

  • Maybe you could introduce another level of compiler messages called "polite reminders" for cases like that.

  • @Jon:

    I think that it's still perfectly valid, because, well, you really shouldn't have "SaveDocument" method in your form. You'd probably have one in your model, and the event handler in the form would be just glue to call that method in the model.  However, the event handler naming in that case is correct - it's precisely just "code that gets called when Save button is clicked".

    I mean, a method called "SaveDocument" with a signature of "(object sender, EventArgs e)"? Am I alone in thinking that it would be a rather weird sight?

    > Where else do we name methods based on the circumstances under which they're called, rather than what they're meant to do?

    Nowhere, but event handlers are special in that the caller does not know _anything_ about the method he's calling in their case. He's just told to call "that method" (via a delegate), and that's it.

  • > I mean, a method called "SaveDocument" with a signature of "(object sender, EventArgs e)"? Am I alone in thinking that it would be a rather weird sight?

    Indeed, and that's why it should be called "OnSaveDocument" to fit in with naming conventions typically associated with event handling. The "OnSaveDocument" method would then call the "SaveDocument" method of the model.

  • @Jon Skeet

    > Where else do we name methods based on the circumstances under which they're called, rather than what they're meant to do?

    You don't have to look far from the "ControlName_EventName" convention. The whole official .NET event pattern is rife with iffy names, from origination to handlers. You're supposed to raise event Foo with

    protected virtual void OnFoo(FooEventArgs e) {...}

    I've worked with plenty of people who can't remember whether "On" is the canonical prefix for methods that raise events or for handlers, and frankly, I don't blame them.  "On" is merely informative enough to remind you of eventing without actually clarifying which end of the event pipe it's supposed to be.

    I prefer to name the originators RaiseFoo().  It's my way of stickin' it to the man.  (No offense, Eric.)

  • > Indeed, and that's why it should be called "OnSaveDocument" to fit in with naming conventions typically associated with event handling.

    "OnSaveDocument" (as noted by Mark above) is reserved for protected virtual methods raising the event, and shouldn't be used by handlers (so long as we're sticking to the standard coding style, for better or worse).

    Also, even "OnSaveDocument", ignoring the convention, would sound to me like a handler for event "SaveDocument" on the _model_ (because such an event shouldn't be seen in the view/form).

  • > I've worked with plenty of people who can't remember whether "On" is the canonical prefix for methods that raise events or for handlers

    It's both, actually (which doesn't exactly help the confusion, but still at least there's a reason for "On" there) - in derived class, you're supposed to override "On..." if you want to handle the given event on your own object. The fact that "On..." in some base class up the chain also raises the actual event for outside clients is orthogonal to that.

    Now, personally, I never really understood the need for this two-stage pattern in the first place. It's not like derived classes can't attach event handlers to parent's events... I suspect it was put in place for the sake of optimization (as overriding a vtable slot is effectively "free", while delegates have to be allocated, and dispatch is a little bit slower, too), but I doubt it actually buys us much if anything today. The overhead from a typical L2O query alone is probably an order of magnitude worse.

  • Going to have to disagree with Jon and some of the others here.

    I do think that "Form1" is a crummy default - "MainForm" would probably be a much better one.

    But...

    1) Copying file XYZ to XYZ - Copy.  The alternative is apparently to ask for a better name.  Great, but what if you're trying to copy 5 files?  Do you get 5 dialogs asking for new names?  1 dialog with a 5-row table asking for 5 new names?  This would be infuriating, especially since it's exactly the same amount of work to rename afterward.  Forcing me to type new names before allowing me to make the copies would be a classic example of "stopping the proceedings with idiocy."

    2) What's the alternative to default names like textBox1?  Is it supposed to pop up a dialog box asking me for a better name every time I create a control?  Dear lord, that would kill my workflow!  I always end up giving my controls better names, but most if not all of the time, the first thing I'm concerned about is layout, and any impediment to this process is wholly unwelcome.

    3) Why wouldn't I want to bind saveButton (or btnSave, take your pick) to saveButton_Click?  The text and position of a UI control should already indicate clearly what it does - you shouldn't need to look at the bindings for it.  If you can't understand it, neither can the user.

    Furthermore, not every event is a simple "EventHandler".  What if you want to fire the same event on, say, a button click and a key press (search anyone?)  In one event handler you just call "Search()" directly, in another you have to actually check the event arguments.  The "map control events directly to methods" convention doesn't scale well with different types of controls and events.  In a great deal of event-driven programming, the circumstances under which an event is raised really *do* matter!

    4) Default assembly references - well, the reason for that was already outlined, it takes bloody ages for that dialog box to come up.  If this is indeed fixed in VS2010 then I'll be very happy, but for now, I'm glad that I don't have to add those references manually, especially since I do use those assemblies fairly often.  IMO, this could be solved by letting us choose which assemblies to add by default as opposed to always using a minimal set (I know, configuration is complication, but we are programmers after all...)

    Visual Studio *is* annoying in many ways, but making users jump through unnecessary hoops in the name of cleaner code is not a step forward!

  • If I have "XML Documentation File" turned on and "Warnings as Errors" turned on as well, the generated code won't compile. Sometimes people with "Warnings as Errors" turned on are willing to do a little work to get template code compiling.

  • "Now, personally, I never really understood the need for this two-stage pattern in the first place. It's not like derived classes can't attach event handlers to parent's events..."

    It seems to me that a big reason for the "two-stage pattern" is so that derived classes can take control over the event-raising process, even inhibiting the raising of events altogether if need be, and at the very least injecting their own behavior at a specific point relative to the event raising, rather than being at the mercy of the order of subscription (being first is relatively easy, as long as the constructor is correctly written, but being last is a lot harder, and being both first and last is even harder still!)

    That said, I do find the "On…" nomenclature wrong.  The word "On" implies that the method is called when the event _happens_, i.e. in response to the event.  I agree that "Raise…" is a much better convention, and it's what I use in my own code.

    As far as Jon's other rants, it's a mixed bag for me.  I agree that "Form1" is a silly default name; at the very least, use the name of the solution and project.  Maybe even provide an optional field in the new-project wizard that if filled in allows me to override that default.

    Also, the default references are really annoying.  I almost never need half of the ones automatically inserted for me into a project.  I almost wonder if the main reason so many are included is that for so long, it's been so painful to add a new reference, because of how long it takes the Add Reference… dialog to come up the first time.  In any case, a much more user-friendly way of managing references, and not cluttering up my project settings with extraneous ones would be very welcome.

    But I also agree with Aaron that requiring me to provide names for EVERY new Designer-added element would really slow me down.  Yes, I do eventually go back and rename things as needed, but I generally do that only for those elements that I actually need to access.  There are lots of structural things that go into a form that don't need any real name, or even a class field for that matter.  (In fact, what would really improve my workflow would be if some clever UI could be designed where a class field is not generated for real until I actually try to use it :) ).

    As for the naming of event handlers, I'm ambivalent.  On the one hand, I recognize Jon's frustration with both the capitalization convention used, as well as the apparent uselessness of the name.  I share his general preference for descriptive, self-documenting code.  On the other hand, it's unusual when an event handler – at least, one for a GUI-related event – I write is actually where the main work is done.  Quite often, I've got some helper method that the event handler actually calls off to.  This is especially true for any non-trivial event handling.  So, the event handler itself describes what event it's designed to respond to, while the helper method's name is descriptive of the actual action being taken in response to the event.

    The one place where the default event handler name becomes really inappropriate is when a single event handler is designed to deal with events from multiple controls.  For example, check-state changes for a radio group.  In that case, the event handler name needs generalization even if it's going to be describing merely the handling of the event.  But these situations come up infrequently enough, and are so much harder to generalize a solution for, that I'm not bothered that the IDE isn't helping me out with those.

    (Though, frankly…for that one specific example, it sure would have been nice if the Forms API had provided for something that represents the entire radio button group, rather than forcing me to always wrap the group in my own abstraction.  I mean, after all, the Forms code itself does need to do the abstraction anyway to properly handle the checking/unchecking of the radio buttons that share a group; why not expose that to the client code?)

    And finally, I cannot submit this comment without including my own pet peeve regarding compiler errors and warnings.  That is, false-positive uninitialized variable errors.  Consider:

     {

       int num;

       bool fValid;

       try

       {

         num = Convert.ToInt32("50");

         fValid = true;

       }

       catch (InvalidCastException)

       {

         fValid = false;

       }

       if (fValid)

       {

         Console.WriteLine("Value of number is " + num); // CS0165 here

       }

     }

    The compiler doesn't like that the I haven't initialized "num" in the case where "fValid" is false.

    Now, I readily grant that the difficulty of the analysis I want the compiler to do as compared to the difficulty of the analysis the compiler actually does is much greater.  It's not that I don't understand the challenge.  But at the same time, as a programmer who can look at the code and know that I never use "num" before it's initialized, I get frustrated.

    This wouldn't be so bad, except that the fix is to initialize the variable to some random default at some point (either in the declaration or in the other code path).  And as I alluded to in the previous topic regarding initialization of value types, default initialization can actually be just as bad a bug as not initializing at all, because default initialization can result in silent errors, where the code doesn't fail immediately, but goes on to produce results that are wrong in subtle, hard-to-detect ways.

    I'd sure love to see the tools get better about this stuff, so that I never have to initialize something I don't really use, and so that I'm always warned when I use something that's not initialized explicitly (i.e. not just because some non-spec default initialization happened to occur).  The bugs related to getting initialization wrong in these ways that are currently not detected can be hard to track down, due to their subtlety.

Page 2 of 4 (56 items) 1234