C# Compiler warnings


I wrote the code snippet tool using Everett (VS7.1).  At one point, I struggled for a bit with a typo I had made.  Here’s a simplified version of the code I had:



if (someMethodThatReturnsABool());
    return;


What got me was the semicolon at the end of the first line.  When I tried it out in today’s Whidbey drop, I was happy to see we’re now generating a warning for that case.  This is one of many scenarios the compiler can avoid wasting the developer’s time spent on frustrating little mistakes.  Another example is when a property returns itself causing an infinite loop (I don’t think we’ve added a warning for this one yet, but it’s definitely on our radar). 


In many cases though, even when the warning would be helpful in most cases, we end up not adding a warning for a scenario in order to not generate much noise.  If we went ahead and added more warnings liberally, developers could potentially start ignoring warnings or turning them off completely, which is not where we want to end up. 


With that in mind, are there any scenarios you have run across that you feel would benefit by getting a warning added to the compiler to cover them?  Feel free to send them my way if you have any. 

Comments (13)

  1. Dan Crevier says:

    I wish this had actually been made illegal in the language (requiring {} instead). C# cleaned up so many little things that caused common errors in C/C++, like being able to fall through in case statements by leaving out the break.

  2. RichB says:

    Static analysis for checked exceptions

  3. David Levine says:

    Generate a list of non-public methods that are never called and can be eliminated. It already warns us about non-public fields that are never used – do the same for methods.

    Also, is there a means (e.g. #pragma warn) to disable specific warnings? If not, there ought to be.

  4. Give us a verbose enum so we could select a level of warnings.

    Another approach would be to have a file declaring explicitly warnings we want to see and warnings we want to ignore. This file could be in XML and could even be handled in a high level way by the IDE.

    Given an output file of warnings, the IDE ou another type of view would let us toggle each type of warning.

  5. Talbott Crowell says:

    I suggest using curly braces for "if" statements. Two extra characters to type, but this type of bug is dodged.

  6. Give us plenty of warnings and the ability to turn them on and off.

    I’m already ignoring the warnings because the compiler is insisting on warning me when I write a class that implements an interface that contains an event but the class never fires that event.

  7. Gus Perez says:

    David, I know warning on unused non-public methods has been talked about. I’ll add your vote to the pot. Also, in Whidbey we’ve added a #pragma warning directive. I wrote about it here:

    http://blogs.msdn.com/gusperez/articles/85722.aspx

    Talbott, this could still happen if you always use braces on your if statements. The only thing that would help you notice something is wrong is how the VS editor would incorrectly format the braces, but as far as the compiler goes, it would accept the braces as a new scope.

    Alfred, are you aware of the various warning levels? You could use /warn:n where n is a level between 0-4. 4 being the lowest importance. I’m not sure we have a list though of what falls into what. It wouldn’t be hard for us to come up with one though.

    Rich, this has been talked about a lot. Still haven’t figured out a great way to do it, but it’s definitely still a live issue for us.

    Dejan, thanks for the feedback. If there are any other warnings you run into that you end up ignoring, that would be feedback we want to hear as well.

  8. Gus Perez says:

    Note to self, when simplifying a code scenario test it before posting. I just tried the code I mentioned in the entry and Everett also warned in this situation. So when my "simplification" wasn’t correct and now I don’t remember what exactly my case was. I’ll try to figure it out soon though.

    Regardless, the question I ask still remains. Thanks!

  9. Dave L says:

    Here’s a more esoteric warning request…given the construct…

    try

    {

    // statements that can throw

    }

    catch(SomeException ex)

    {

    if ( somecondition )

    {

    // handle the error

    }

    }

    There should be a warning that the catch clause can fall through without any action taken. In other words, the default behavior of this is to swallow and ignore the exception if none of the if conditions are satisfied.

    Constructs that would not generate a warning are those cases that either have an unconditional else clause or code following the last if clause.

  10. Doug McClean says:

    One thing I think we could definitely benefit from a warning for is something like this:

    class Dog

    {

    private string name;

    public Dog(string name) {

    name = name; // assigns parameter to itself, which is useless. note missing this. qualifier.

    }

    // rest of class

    }

    This error is fairly easy to debug, but common and obviously unintended.

  11. Patrick says:

    I don’t know whether this deserves a warning, but setting the "value" in a Property setter is probably a crazy thing to do:

    set

    {

    value = true;

    this.mostRecentEvaluation = value;

    }

  12. Gus Perez says:

    Doug, the latest drops now warn in any case where you’re assigning the value of a variable to itself. So that covers yours.

    Dave/Patrick, thanks, we’ll consider these.

  13. James says:

    A [Used] attribute would be nice to have, which informs the compiler that a variable which seems to have no purpose really is important. (Such as in the case of reflection, or a public Event that is never called that is needed for an interface.)