9

After a couple showstoppers delayed migration to the .NET 4.6 runtime I was finally comfortable with moving to the C#6/VB14 compilers until I came across a critical issue with iterator functions in VB.NET throwing away local variables.

The following code example will throw a null reference exception on the commented line when compiled in release mode (optimized) in Visual Studio 2015 / msbuild.

Module Module1

    Sub Main()
        For Each o As Integer In GetAllStuff()
            Console.WriteLine(o.ToString())
        Next

        Console.ReadKey()

    End Sub

    Private Iterator Function GetAllStuff() As IEnumerable(Of Integer)
        Dim map As Dictionary(Of String, String) = New Dictionary(Of String, String)
        Dim tasks As New List(Of Integer)
        tasks.Add(1)

        For Each task As Integer In tasks
            Yield task
        Next

        'The value of map becomes null here under the new VB14 compiler in Release on .NET 4.6'
        For Each s As String In map.Values
            Yield 100
        Next
    End Function

End Module

So, that's pretty frightening.

Notably the C# equivalent of this code executes without issue. More importantly, this works (and has worked) under previous versions of the VB compiler. Comparing the MSIL between the state machine created by the two different compilers, the new compiler appears to be using .locals for local variable storage nearly exclusively while the old compiler was using mutable fields on the state machine for saving off local values.

Am I missing something? I was not able to find any documentation of a breaking change with iterators in VB (nor can I imagine that would be the case), but also have not found anyone else having hit this issue.

This particular example can be worked around by moving the construction of map to after the first foreach loop, however my concern is that I do not have any sense of the true flavor of this issue. I'm not interested in modifying code to "just make it work." Where else in our extensive codebase might I run into an issue a same vein? I have submitted the issue on Connect but that often feels like a black hole.

UPDATE

Someone just reported the same issue with the async state machine on the Roslyn GitHub page: https://github.com/dotnet/roslyn/issues/9001

Hopefully this starts to get a little attention.

roken
  • 3,946
  • 1
  • 19
  • 32
  • I've run your code in VS2015, compiled against .NET Framework 4.5.2 and this produces a `NullReferenceException` too. So it's would appear its not isolated to .NET 4.6 runtime. – Ɖiamond ǤeezeƦ Feb 23 '16 at 12:13
  • If I compile using VS2013 against 4.5.2 or 4.6, I don't get the `NullReferenceException`. So, like you say this is a compiler issue, rather than a framework issue. – Ɖiamond ǤeezeƦ Feb 23 '16 at 12:24
  • @Threadcreator: Your code works for me perfectly. Using Windows 10 Pro (x64), Visual Studio 2015 Express for Windows Desktop. Targetframework: 4.5 – SiriSch Feb 23 '16 at 13:28
  • 4
    Yes, very nasty Roslyn bug. There are thousands, this one puts the FUD into VB.NET code correctness rather heavily. About the same bug as [this one](http://stackoverflow.com/questions/34421966/idisposable-dispose-not-called-in-release-mode-for-async-method). DevDiv is not doing business like they used to, they all like the new Agile approach but testing is not agile enough. We get to do the job for them. VS2013 was affected first, VS2015 is a big bag 'o bugs. Update 2 is in CTP right now, it probably fixes this bug. Not sure, I don't want to test it :) – Hans Passant Feb 23 '16 at 14:29

1 Answers1

1

First of all, thanks for bringing some attention to the issue I raised with the Roslyn team.

I have pulled the latest Roslyn source from https://github.com/dotnet/roslyn (master branch), and added an extra unit test to the BasicCompilerEmitTest project which looks like this:

Imports Microsoft.CodeAnalysis.VisualBasic.UnitTests

Public Class KirillsTests
  Inherits BasicTestBase

  <Fact>
  Public Sub IteratorVariableCaptureTest()
    Dim source =
<compilation name="Iterators">
  <file name="a.vb">
Imports System
Imports System.Collections.Generic

Module Module1

    Sub Main()
        For Each o As Integer In GetAllStuff()
            Console.WriteLine(o.ToString())
        Next

        Console.WriteLine("done")
    End Sub

    Private Iterator Function GetAllStuff() As IEnumerable(Of Integer)
        Dim map As Dictionary(Of String, String) = New Dictionary(Of String, String)
        Dim tasks As New List(Of Integer)
        tasks.Add(1)

        For Each task As Integer In tasks
            Yield task
        Next

        'The value of map becomes null here under the new VB14 compiler in Release on .NET 4.6'
        For Each s As String In map.Values
            Yield 100
        Next
    End Function

End Module
  </file>
</compilation>

    Dim expectedOutput = <![CDATA[1
done]]>

    Dim compilation = CompilationUtils.CreateCompilationWithReferences(source, references:=LatestVbReferences, options:=TestOptions.DebugExe)
    CompileAndVerify(compilation, expectedOutput:=expectedOutput)
    CompileAndVerify(compilation.WithOptions(TestOptions.ReleaseExe), expectedOutput:=expectedOutput)
  End Sub

End Class

This may look like a convoluted mess due to XElement and XCData usage, but this is the format used by other Roslyn unit tests.

I only made one alteration to the code you posted in your question - that is replacing Console.ReadKey() with Console.WriteLine("done") so that I could track successful completion (as CompileAndVerify simply ignores exceptions).

The above test passes. There is no NullReferenceException on map.Values access, and the output is:

1
done

... as expected. It would, therefore, appear that your bug has been fixed - although whether or not the fix will be shipped with Visual Studio 2015 Update 2, I cannot tell.

The async variable capture issue was fixed by pull request #7693, but DataFlowPass.SetSlotUnassigned has been rewritten since (split into 2 methods and modified) so I cannot confirm whether the iterator problem you discovered was fixed by that specific pull request, or by some other code change.

Kirill Shlenskiy
  • 9,367
  • 27
  • 39
  • 2
    This is indeed addressed with the Update 2 CTP. I blew a phone-a-friend with Microsoft support to get more details into the specific fix and will update again with whatever information I'm provided with. Sadly, 2015/4.6 has felt like one long CTP. Platform stability used to be such a great selling point for .NET... – roken Feb 24 '16 at 03:30
  • 1
    @roken, good to know that the fix is getting shipped soon. Roslyn is still in its early days and you're right, it does have a bit of an experimental feel to it. It's an exciting time in many ways, but not always for good reasons as this question demostrates. – Kirill Shlenskiy Feb 24 '16 at 04:32