search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.99k stars 4.03k forks source link

VB EE after EnC: Renaming local in iterator/async method #2737

Open tmat opened 9 years ago

tmat commented 9 years ago

In below console app, set breakpoints as indicated, F5, rename "q" to "Q" and change WriteLine(1) to WriteLine(2), F5 again. The output is "2" so the change has been applied. However the EE doesn't display "Q" or "q" in locals. Evaluation of "Q" expression also fails to bind the local.

Module Module1
    Iterator Function F() As IEnumerable(Of Integer)
        Dim q = 1                                 ' breakpoint 1
        Console.WriteLine(1)
        Yield 2
        Console.WriteLine(q)               ' breakpoint 2
    End Function

    Sub Main()
        F().ToArray()
    End Sub
End Module
tmat commented 9 years ago

@amcasey Another rename issue.

amcasey commented 9 years ago

I'm seeing the old name (q) stay in the Locals window. The Watch window accepts either Q or q.

amcasey commented 9 years ago

Oops, I had EnC disabled. @tmat shouldn't I be warned if I try to continue after editing the program?

amcasey commented 9 years ago

It turns out the file was in Miscellaneous Files (even when opened from the Solution Explorer). I created a new project and now I can repro the original issue.

amcasey commented 9 years ago

inScopeHoistedLocals correctly has an uppercase "Q". However, it appears (superficially, at least) that NamedType ConsoleApplication200.Module1.VB$StateMachine_0_F has a field with a lowercase "q".

amcasey commented 9 years ago

If that isn't a problem in its own right, we can just update the hash set to use a case-insensitive comparer.

amcasey commented 9 years ago

I renamed "q" to "r" instead and it looks like EnC adds a field rather than renaming the old one. I'm guessing there's a conflict between two fields that differ only in case.

amcasey commented 9 years ago

When only the case is changed, EnC reuses the field but the new CDI is emitted with the new name. The EE expects the two to match exactly and they don't. We can

  1. Stop reusing the field.
  2. Allow case-insensitive matching.
  3. Solve the general rename problem.
amcasey commented 9 years ago

Also note that the EE isn't blowing up. Only the q/Q disappears from the Locals and Watch windows. Other variables and expressions are unaffected.

amcasey commented 9 years ago

Per @tmat, this doesn't meet the Update 1 bar.