3

Edit: Here is a much more simpler example of this issue (i've deleted my original question):

Dim numbers1 As New List(Of Int32)({1, 2, 3})
Dim numbers2 As New List(Of Int32)({3, 4, 5})
For Each n1 In numbers1
    ' no warning '
    Dim contains = numbers2.Contains(n1)
Next
For Each n1 In numbers1
    ' warning on n1'
    Dim contains = (From num In numbers2 Where num = n1).Any
Next

So i still don't understand why the compiler thinks that i may get unexpected results in the second iteration whereas i'm safe with the first. I don't think that the interesting link of @ee-m provides the reason for this behaviour,(this is not a for-each issue, For n1 As Int32 = 1 To 3 would also result in a compiler warning).

I'm not really convinced that following should be "best-practice":

For Each n1 In numbers1
    Dim number1 = n1
    ' no warning'
    Dim contains = (From num In numbers2 Where num = number1).Any
Next

The local variable number1 is redundant and makes the code less readable as @Meta-Knight already has emphasized. Note: All three ways are safe and give the correct result.

Community
  • 1
  • 1
Tim Schmelter
  • 450,073
  • 74
  • 686
  • 939

2 Answers2

4

Eric Lippert wrote a couple of blog posts on this topic (code examples are in C#, not VB) which goes over some of the "gotchas" that can arise from this sort of code which you may find interesting:

Closing over the loop variable considered harmful

lee-m
  • 2,269
  • 17
  • 29
  • 1
    It is indeed interesting. In C# there is no such warning. In Lippert's words: "a warning which warns about correct behaviour is a very bad warning". – Meta-Knight Aug 29 '11 at 15:50
  • +1 interesting link. Maybe i've misunderstood eric's point but the issue is the same even if i replace the for-each loop with `For n1 As Int32 = 1 To 3`. Shouldn't then the [variable's scope be the body of the loop](http://msdn.microsoft.com/en-us/library/5z06z1kb.aspx#Y2554)? – Tim Schmelter Aug 29 '11 at 17:53
  • @Tim Schmelter: The exact same issue arises with simple for loops. Keep in mind that there are differences between C# and VB. In VB you cannot by any mean refer to the loop variable outside of the loop. But this doesn't change the fact the the same variable gets reused from an iteration to the next. – Meta-Knight Aug 29 '11 at 18:05
3

As the message says, it "may have" undesired effects. In your case the .ToList() makes it safe but that is hard for the compiler to verify.

I would suggest to adopt copying to a local var (Dim exc = excel) as standard 'best practice'

H H
  • 263,252
  • 30
  • 330
  • 514
  • Why is it hard for the compiler to verify? I would understand that the LINQ-Query cannot know that it will be executed immediately because the `ToList` is out of it's context. But the iteration-variable cannot be changed because it is already executed at this point. I would assume that the compiler knows that, so he could "hard-assign" the iteration variable into the query. But i've adopted your suggestion to copy it into a local variable. – Tim Schmelter Aug 29 '11 at 11:29
  • The _Why_ part id for the compiler builders. But how far should they backtrack? Here we can quickly see that AllExcelFiles is a List(Of T) but usually it will be an IEnumerable(Of T). I suppose the warning-algo is limited in scope to the ForEach. – H H Aug 29 '11 at 13:09
  • 1
    The problem with this is that the "Dim exc = excel" line is completely superfluous and makes the code less clear, so I don't know if we can really call this a "best practice". I'm thinking of suppressing this warning message myself, because there are a lot of false-positives like in this case. – Meta-Knight Aug 29 '11 at 14:26
  • @meta- superfluous: yes, less clear: not necessarily. Just pick a good name. It's about thinking ahead, change to a Parallel.ForEach and you're really going to need it. – H H Aug 29 '11 at 14:29
  • It's pretty hard to find a good name for a variable which points to the exact same object as the loop variable. So you either use an abbreviation, which makes the code less clear, or add a superfluous adjective like "currentExcel" or "excel2", which can also be confusing. Do you have any tip about naming such a variable? – Meta-Knight Aug 29 '11 at 14:47
  • @Meta-Knight,@Henk Holterman: in this case i've named the first local variable `Dim idExcel = excel.idReport` since later i'm only using this integer and not the whole object. It's still superfluous and verbose but not really less clear. I'm not sure how complex it would be for the compiler team to fix this, but probably they have other priorities. – Tim Schmelter Aug 29 '11 at 15:00
  • @Henk: sorry for unaccepting for now, i'm hoping that i get additional informations why this should be "best-practise" or needed. – Tim Schmelter Aug 29 '11 at 17:56
  • @Meta-Knight, I have adopted the convention of affixing '_dummy' to the copied var (inspired by the "dummy variable" of integral calculus, which itself is an iterated var of a sort). So 'excel' is copied by 'excel_dummy'. I agree this is still ugly code and wish for a more perfect solution, but I think it's better than, say, 'excel2', which looks quite confusing - is that a copy or a second distinct excel var? – Tekito Feb 27 '12 at 16:55