Feedback request on new warning

We’ve been internally discussing a new warning that we’ve added to VB 2005 and how we should treat it for upgrade, and I’d like to get some feedback from those people who have been using the beta or the community previews. A common error that we see is using a reference variable before it’s been assigned a value. For example:

Dim x As Collection
x.Add(10)

This code will throw an exception because I forgot to create a new collection using New. In VB 2005, this code will produce the following warning:

Variable 'x' is used before it has been assigned a value. A null reference exception could result at runtime.

We determine this warning by doing flow of control analysis on the function and determining that you’re using the variable before you gave it a value. This is all well and good, but the warning isn’t perfect. Just because a variable hasn’t been assigned a value doesn’t necessarily mean that there’s going to be a problem. For example:

Dim x As Collection

If Test() Then
x = New Collection()
End If

If x IsNot Nothing Then
x.Add(10)
End If

The test “x IsNot Nothing” will cause a warning that x might be used before it has been assigned a value. This is sort-of true if you look at the code in a simplistic way, but it’s really not a problem, of course, because we’re just testing against Nothing. An ideal solution, and one that we’re considering beyond VB 2005, is to add a more sophisticated level of analysis to the compiler that would allow more fine-grained analysis of null reference problems that would recognize the above situation as safe. But that’s beyond the scope of this release. For now, the level of analysis that we can do is identify places where you used a reference variable without assigning it a value.

Now, to the question: have people found this warning to be annoying on upgrade? New projects don’t seem to have major issues with this warning because you fix the warnings as you go along, by and large. But upgraded projects can be a different story: depending on the coding style used, a project might have a ton of these warnings show up when you first open it in VB 2005. For example, I recently upgraded the VBParser project and spent a little while fixing up 100 or so of these warnings, all of which were what you might call “false positives” – I was using variables before they had been assigned, but always in a safe and courteous manner.

What have people’s experiences been with this warning on upgrade? Was it annoying? Was it helpful? Would it be desirable to not have the warning turned on by default in upgrade? Just to be clear: we’re not planning on changing anything as it stands right now. But now that the beta’s been out in the wild for a while, we thought it might be a good time to validate our assumptions against some cold, hard customer feedback…

23 thoughts on “Feedback request on new warning

  1. Scott Shriver

    I agree with Phillip. I like the warning and I think it’s helpful much more often than it gets in the way. If I had a nickel for every time I forgot the New keyword in VS2003 and had to stop and restart the app….

    Reply
  2. Geoff Appleby

    < ![CDATA[Warnings are only that: warnings. They don't stop the compile, they don't stop it running. A few false positives is a small price to pay for the times that it really does show a problem. I can see that it will become annoying in large projects when you go through the warnings and a lot of this sort, but every single one is safe (like your second example), but you need to periodically check all instances again and again in case you brought in a new one somewhere… Leave it there, and on by default, but allow us to switch it off when we get too annoyed with :)]]>

    Reply
  3. Daniel Moth

    < ![CDATA[If the feature worked (like you propose it will do in future versions) then it would be a great feature. Having tried to upgrade a project I also have seen the warning window fill up. In most cases it basically means having to assign null to each object variable declaration (much like in C-based languages, C# included). Superfluous but livable. Now to give you an example:
    Dim s As String ‘ warning!

    s &= "Why?"

    Another one:

    ‘ if only VB had "out" like C# has

    Private Function ReturnsValueByRef(ByRef theMethodAssignsThis As String) As Boolean

    theMethodAssignsThis = "some value to return"

    Return True

    End Function

    Private Sub Calls()

    Dim s As String ‘ warning!

    Me.ReturnsValueByRef(s)

    End Sub

    I filed a bug report back in July… it is still open!

    http://lab.msdn.microsoft.com/productfeedback/viewfeedback.aspx?feedbackid=7a497fd9-3d09-438f-97f3-9d43c86c8db8]]>

    Reply
  4. Corrado Cavalli

    < ![CDATA[While i agree that in some case you might have a 'not real' warning, I've found that in many cases it is really helpful to avoid the NullReferenceException at runtime.
    So my opinion is leave it on by default, warnings don’t stop compile.]]>

    Reply
  5. Dan McKinley

    I’ve found this to be useful. The one time I ran a very large project through the upgrade, my impression was that by and large the issues found were legitimate. I definitely would leave it on.

    Reply
  6. Sergio Pereira

    < ![CDATA[This warning is so useful that should be on by default even if it is not possible to make it smarter right now.
    It is a good indication that there might be some hole in the logic and, even if there isn’t, you’ll be compelled to be more defensive.]]>

    Reply
  7. Karl

    < ![CDATA[Current version of C# treats the above code as an error..thus forcing you to do: dim x as Collection = null;
    but you quickly pick up the right habbit to stpp the compiler from complaining.

    As far as upgrades, it seems like few people use the "Treat compiler warnings as errors" flag. My experience with large upgrades is that such flags get turned off and it takes a while to be able to turn them back on again…which I think hurts the application (think option strict). As has been suggested, I think as long as you can disable that specific warning (as opposed to disabling all warnings as errors) you give developers total flexibility.

    Karl]]>

    Reply
  8. Mike

    < ![CDATA[I'd say keep this off by default--false positives on errors have always annoyed the hell out of me. (For instance, ask ten people about the grammar checking in Word, and I guarantee you that at least eight of the responses will be laced with expletives. Unless you're, like, in a church or something. But I digress...) Not only are they confusing, but they dilute error lists--if there are two legitimate warnings in a list buried among a dozen erroneous null reference warnings, it'll take me longer to find them in the list, and if I lose faith in the validity of a majority of the warnings, I might just start ignoring the warning list completely. As a language-agnostic programmer, I have certain expectations for compilers in all languages. The fewer quirks I have to deal with, the better off I am. If I don’t know a language very well, I don’t want to see "ghost" error messages, and then have to spend an hour digging through the documentation to figure out what the hell is going on (a weekly occurrence during my first year of VB6 programming, but I digress again), particularly when the issue is caused by the fact that the compiler is prone to making mistakes. If you force users to turn it on first, at least they’ll be somewhat aware of its presence, and they’ll know where to look if things start to go wrong. In short, you might be saving a programmer a few minutes by preventing a quick debugging session, but you’d be doing so at the expense of consistency and user trust. Maybe I’m exaggerating a bit, but turning this on by default just seems like a bit of a bad idea.]]>

    Reply
  9. Mike Gale

    < ![CDATA[This is not direct feedback just a couple of observations: 1) Standard OO base types (String, Integer…) have a skimpy design and result in huge amounts of excess code. Like all that DBNull checking. Regular expressions have got it right here. (*, +, , {2,5}…) If CLS types incorporated that (without that Nullable of… syntax) programmers could think a lot faster when freed of this make work. 2) The VS experience still has a lot of them and us about it. Rather like in old style VB the language had big limits. In this case it’s what the compiler tells you. The language developers are the them, programmers are the us. Programmers have the skills and inclination to adjust compiler output. Somebody will substantially give them that ability, some day. Ways to change strings and ultimately enhanced logic seem inevitable. Some using it become much more productive, than they can now be. (This also applies to the wizard / design-surface tools. These are code generators, we need a way to make them work right for us and extend them. Hopefully programmers are using the tools not point and click artists, they can handle it!!)]]>

    Reply
  10. Tim Hall

    < ![CDATA[I havent actaully got around to using 2005 yet. However one thing i would like to see with respect to warnings is some way of acknowledging the warnings on a case by case basis so they can be hidden from the list. The warnings list should also be able to be filtered to show/hide acknowledged warnings. Maybe even modifying the line of code that the warning is about would re-enable the warnings (optional). Id love to use that warning, but on the other hand if it meant a few dozen warnings id be pretty peeved if i had to globally disable it.]]>

    Reply
  11. Simon Geering

    < ![CDATA[In general I've found this warning very useful in new projects. Upgrades are a different story all together, I end up with so many warnings and compile errors ( I compile with option strict set on for all code) that it is confusing to know were to start fixing things. There is defiantly a need for more fine grained control over that window, may be like you get in fxcop As such my view is that Microsoft should take the same approach with application features as with operating system components, always default to the simplest option possible i.e. disabled and have an option to turn features on. That way you have to make a conscious decision to use something and should therefore understand what the consequences of doing so are, the same way as you do choosing to install IIS on windows 2003 server. You could use the settings export features in 2005 to save you having to repeat this process between machines.
    ]]>

    Reply
  12. Pingback: Daniel Moth

  13. Kyle Reed

    < ![CDATA[Yes it is very annoying. I'm converting several projects now where we use this pattern extensively: Dim dv as dataview Try ‘some stuff
    dv = new dataview(tbl)

    Catch ex as exception

    throw ex

    Finally

    if not dv is nothing then

    dv.dispose

    end if

    End Try

    To fix this problem I have to do something very dumb and declare the variable like this: dim dv as dataview = Nothing. ]]>

    Reply
  14. Jamie Fraser

    < ![CDATA[Ive found it quite annoying so far - its certainly useful in some situations, but I have about 100 warnings when using the code below (or some variation of it) Dim ctrl as LogoFileCtrl ….. If not isnothing(ctrl) then ctrl.focus() Ideally it should just include some logic to see if its being compared against Nothing, because at the moment its pretty useless and was quickly turned off! ]]>

    Reply
  15. Christopher Quick

    < ![CDATA[Jamie: Turned off HOW??? Please tell me! Having to do Dim X as String = Nothing precludes doing multiple declarations which is a very nice time and space saver: Dim X, Y, Z as String Even if it checks for things such as If IsNothing(X) then… It still won’t help for things such as Dim X, Y, Z as String Try
    ….

    Catch



    Return X

    End Try

    We need to be able to do a "Ignore" on a warning like we can in any kind of spell checking system. Please!]]>

    Reply
  16. Jamie working on a large VB.NET

    < ![CDATA[It's a little late, but I find this feature so annoying that I thought I'd make a few comments anyway. This reminds me of the story about the boy who cried wolf. I’m sure most know this story, but for the few who may not. The boy falsely cries wolf so many times that the town begins to ignore him. When there is actually a wolf, no one comes to help because they think it’s just another prank. The compiler falsely issues this warning so many times, even in native VB.NET 2005 code, that developers begin to ignore it. The false positives undermine the usefulness and credibility, as does having to explicitly assign Nothing to the var just to address the warning (I’m concerned about the performance implications of this in a server environment). I’ve also seen instances where a null reference warning should have been issued, but it wasn’t and I received the null reference exception at runtime. Seems like this feature wasn’t quite ready for primetime and should have been delayed to next release. If the feature cannot be made more accurate, I think you should, as others have suggested, provide a way to ignore individual violations much like you can tell the code analysis tool to ignore specific issues it finds (where it logs them to the GlobalSuppressions file). This would be a great feature if it worked. I anxiously await the improved version.]]>

    Reply

Leave a Reply