Fix 7456, 3704, 12019 (debug scopes, self arg, incorrect information display on shadowing) (#12018)

* fix Fix 7456, 3704, 12019 (debug scopes, self arg, incorrect information display on shadowing) #12018

* update baselines

* fix regression in debug codegen

* update test

* update baselines

* fix debug scopes for module initialization code

* fix cases of nested shadowing

* fix shadowing for some cases

* add missing file

* fix build

* update baselines

Co-authored-by: Don Syme <[email protected]>

Author: dsyme

src/fsharp/IlxGen.fs

@@ -2146,7 +2146,6 @@ module Codebuf =
                 mkScopeNode cenv importScope localSigs (s1, e1, cl.DebugMappings, children))
         trees
 
-
     // Emit the SEH tree
     let rec emitExceptionHandlerTree (codebuf: CodeBuffer) (Node (x, childSEH)) =
         List.iter (emitExceptionHandlerTree codebuf) childSEH // internal first
@@ -2183,7 +2182,10 @@ module Codebuf =
 
         // Build the locals information, ready to emit
         let localsTree = makeLocalsTree cenv importScope localSigs pc2pos code.Labels code.Locals
-        localsTree
+
+        // Adjust the scopes for shadowing
+        let unshadowed = List.collect (unshadowScopes >> Array.toList) localsTree
+        unshadowed
 
     let EmitMethodCode cenv importScope localSigs env nm code =
         use codebuf = CodeBuffer.Create nm

src/fsharp/absil/ilwrite.fs

@@ -293,20 +293,6 @@ let scopeSorter (scope1: PdbMethodScope) (scope2: PdbMethodScope) =
     elif (scope1.EndOffset - scope1.StartOffset) < (scope2.EndOffset - scope2.StartOffset) then 1
     else 0
 
-let collectScopes scope =
-    let list = List<PdbMethodScope>()
-    let rec toList scope parent =
-        let nested =
-            match parent with
-            | Some p -> scope.StartOffset <> p.StartOffset || scope.EndOffset <> p.EndOffset
-            | None -> true
-
-        if nested then list.Add scope
-        scope.Children |> Seq.iter(fun s -> toList s (if nested then Some scope else parent))
-
-    toList scope None
-    list.ToArray() |> Array.sortWith<PdbMethodScope> scopeSorter
-
 type PortablePdbGenerator (embedAllSource: bool, embedSourceList: string list, sourceLink: string, checksumAlgorithm, showTimes, info: PdbData, pathMap: PathMap) =
 
     let docs =
@@ -527,30 +513,50 @@ type PortablePdbGenerator (embedAllSource: bool, embedSourceList: string list, s
         importScopesTable.Add(imports, result)
         result
 
-    let writeMethodScopes methToken scope =
-        for s in collectScopes scope do
+    let flattenScopes rootScope = 
+        let list = List<PdbMethodScope>()
+        let rec flattenScopes scope parent =
+
+            list.Add scope
+            for nestedScope in scope.Children do
+                let isNested =
+                    match parent with
+                    | Some p -> nestedScope.StartOffset >= p.StartOffset && nestedScope.EndOffset <= p.EndOffset
+                    | None -> true
+
+                flattenScopes nestedScope (if isNested then Some scope else parent)
+
+        flattenScopes rootScope None
+
+        list.ToArray() 
+        |> Array.sortWith<PdbMethodScope> scopeSorter
+
+    let writeMethodScopes methToken rootScope =
+
+        let flattenedScopes = flattenScopes rootScope
 
-            // Get or create the import scope for this method
-            let importScopeHandle =
+        // Get or create the import scope for this method
+        let importScopeHandle =
 #if EMIT_IMPORT_SCOPES
-                match s.Imports with 
-                | None -> Unchecked.defaultof<_>
-                | Some imports -> getImportScopeIndex imports
+            match s.Imports with 
+            | None -> Unchecked.defaultof<_>
+            | Some imports -> getImportScopeIndex imports
 #else
-                getImportScopeIndex |> ignore // make sure this code counts as used
-                Unchecked.defaultof<_>
+            getImportScopeIndex |> ignore // make sure this code counts as used
+            Unchecked.defaultof<_>
 #endif
 
+        for scope in flattenedScopes do
             let lastRowNumber = MetadataTokens.GetRowNumber(LocalVariableHandle.op_Implicit lastLocalVariableHandle)
             let nextHandle = MetadataTokens.LocalVariableHandle(lastRowNumber + 1)
 
             metadata.AddLocalScope(MetadataTokens.MethodDefinitionHandle(methToken),
                 importScopeHandle,
                 nextHandle,
                 Unchecked.defaultof<LocalConstantHandle>,
-                s.StartOffset, s.EndOffset - s.StartOffset ) |>ignore
+                scope.StartOffset, scope.EndOffset - scope.StartOffset ) |>ignore
 
-            for localVariable in s.Locals do
+            for localVariable in scope.Locals do
                 lastLocalVariableHandle <- metadata.AddLocalVariable(LocalVariableAttributes.None, localVariable.Index, metadata.GetOrAddString(localVariable.Name))
 
     let emitMethod minfo =
@@ -957,7 +963,8 @@ let logDebugInfo (outfile: string) (info: PdbData) =
     fprintfn sw "ENTRYPOINT\r\n  %b\r\n" info.EntryPoint.IsSome
     fprintfn sw "DOCUMENTS"
     for i, doc in Seq.zip [0 .. info.Documents.Length-1] info.Documents do
-      fprintfn sw " [%d] %s" i doc.File
+      // File names elided because they are ephemeral during testing
+      fprintfn sw " [%d] <elided-for-testing>"  i // doc.File
       fprintfn sw "     Type: %A" doc.DocumentType
       fprintfn sw "     Language: %A" doc.Language
       fprintfn sw "     Vendor: %A" doc.Vendor
@@ -987,3 +994,62 @@ let logDebugInfo (outfile: string) (info: PdbData) =
       | None -> ()
       | Some rootscope -> writeScope "" rootscope
       fprintfn sw ""
+
+let rec allNamesOfScope acc (scope: PdbMethodScope) =
+    let acc = (acc, scope.Locals) ||> Array.fold (fun z l -> Set.add l.Name z)
+    let acc = (acc, scope.Children) ||> allNamesOfScopes
+    acc
+and allNamesOfScopes acc (scopes: PdbMethodScope[]) =
+    (acc, scopes) ||> Array.fold allNamesOfScope
+
+let rec pushShadowedLocals (localsToPush: PdbLocalVar[]) (scope: PdbMethodScope) =
+    // Check if child scopes are properly nested
+    if scope.Children |> Array.forall (fun child ->
+            child.StartOffset >= scope.StartOffset && child.EndOffset <= scope.EndOffset) then
+
+        let children = scope.Children |> Array.sortWith scopeSorter
+
+        // Find all the names defined in this scope
+        let scopeNames = set [| for n in scope.Locals -> n.Name |]
+
+        // Rename if necessary as we push
+        let rename, unprocessed = localsToPush |> Array.partition (fun l -> scopeNames.Contains l.Name)
+        let renamed = [| for l in rename -> { l with Name = l.Name + " (shadowed)" } |]
+
+        let localsToPush2 = [| yield! renamed; yield! unprocessed; yield! scope.Locals |]
+        let newChildren, splits = children |> Array.map (pushShadowedLocals localsToPush2) |> Array.unzip
+        
+        // Check if a rename in any of the children forces a split
+        if splits |> Array.exists id then
+            let results =
+                [| 
+                    // First fill in the gaps between the children with an adjusted version of this scope.
+                    let gaps = 
+                        [| yield (scope.StartOffset, scope.StartOffset) 
+                           for newChild in children do   
+                                yield (newChild.StartOffset, newChild.EndOffset)
+                           yield (scope.EndOffset, scope.EndOffset)  |]
+
+                    for ((_,a),(b,_)) in Array.pairwise gaps do 
+                        if a < b then
+                            yield { scope with Locals=localsToPush2; Children = [| |]; StartOffset = a; EndOffset = b}
+                       
+                    yield! Array.concat newChildren
+                |]
+            let results2 = results |> Array.sortWith scopeSorter
+            results2, true
+        else 
+            let splitsParent = renamed.Length > 0
+            [| { scope with Locals=localsToPush2 } |], splitsParent
+    else
+        [| scope |], false
+
+// Check to see if a scope has a local with the same name as any of its children
+// 
+// If so, do not emit 'scope' itself. Instead, 
+//  1. Emit a copy of 'scope' in each true gap, with all locals
+//  2. Adjust each child scope to also contain the locals from 'scope', 
+//     adding the text " (shadowed)" to the names of those with name conflicts.
+let unshadowScopes rootScope =
+   let result, _ = pushShadowedLocals [| |] rootScope
+   result

src/fsharp/absil/ilwritepdb.fs

@@ -119,10 +119,21 @@ type HashAlgorithm =
     | Sha256
 
 val generatePortablePdb : embedAllSource: bool -> embedSourceList: string list -> sourceLink: string -> checksumAlgorithm: HashAlgorithm -> showTimes: bool -> info: PdbData -> pathMap:PathMap -> int64 * BlobContentId * MemoryStream * string * byte[]
+
 val compressPortablePdbStream : uncompressedLength:int64 -> contentId:BlobContentId -> stream:MemoryStream -> int64 * BlobContentId * MemoryStream
+
 val embedPortablePdbInfo: uncompressedLength: int64 -> contentId: BlobContentId -> stream: MemoryStream -> showTimes: bool -> fpdb: string -> cvChunk: BinaryChunk -> pdbChunk: BinaryChunk -> deterministicPdbChunk: BinaryChunk -> checksumPdbChunk: BinaryChunk -> algorithmName: string -> checksum: byte[] -> embeddedPdb: bool -> deterministic: bool -> idd[]
+
 val writePortablePdbInfo: contentId: BlobContentId -> stream: MemoryStream -> showTimes: bool -> fpdb: string -> pathMap: PathMap -> cvChunk: BinaryChunk -> deterministicPdbChunk: BinaryChunk -> checksumPdbChunk: BinaryChunk -> algorithmName: string -> checksum: byte[] -> embeddedPdb: bool -> deterministic: bool -> idd[]
 
 #if !FX_NO_PDB_WRITER
 val writePdbInfo : showTimes:bool -> f:string -> fpdb:string -> info:PdbData -> cvChunk:BinaryChunk -> idd[]
 #endif
+
+/// Check to see if a scope has a local with the same name as any of its children
+/// 
+/// If so, do not emit 'scope' itself. Instead, 
+///  1. Emit a copy of 'scope' in each true gap, with all locals
+///  2. Adjust each child scope to also contain the locals from 'scope', 
+///     adding the text " (shadowed)" to the names of those with name conflicts.
+val unshadowScopes: PdbMethodScope -> PdbMethodScope[]

src/fsharp/absil/ilwritepdb.fsi

@@ -21,11 +21,13 @@ open TestFramework
 [<Sealed>]
 type ILVerifier (dllFilePath: string) =
 
-    member this.VerifyIL (expectedIL: string list) =
+    member _.VerifyIL (expectedIL: string list) =
         ILChecker.checkIL dllFilePath expectedIL
 
-    //member this.VerifyILWithDebugPoints (expectedIL: string list) =
-    //    ILChecker.checkILWithDebugPoints dllFilePath expectedIL
+[<Sealed>]
+type PdbDebugInfo(debugInfo: string) =
+
+    member _.InfoText = debugInfo
 
 type Worker () =
     inherit MarshalByRefObject()
@@ -622,6 +624,28 @@ type CompilerAssert private () =
             f (ILVerifier outputFilePath)
         )
 
+    static member CompileLibraryAndVerifyDebugInfoWithOptions options (expectedFile: string) (source: string) =
+        let options = [| yield! options; yield"--test:DumpDebugInfo" |]
+        compile false options source (fun (errors, outputFilePath) ->
+            let errors =
+                errors |> Array.filter (fun x -> x.Severity = FSharpDiagnosticSeverity.Error)
+            if errors.Length > 0 then
+                Assert.Fail (sprintf "Compile had errors: %A" errors)
+            let debugInfoFile = outputFilePath + ".debuginfo"
+            if not (File.Exists expectedFile) then 
+                File.Copy(debugInfoFile, expectedFile)
+                failwith $"debug info expected file {expectedFile} didn't exist, now copied over"
+            let debugInfo = File.ReadAllLines(debugInfoFile)
+            let expected = File.ReadAllLines(expectedFile)
+            if debugInfo <> expected then 
+                File.Copy(debugInfoFile, expectedFile, overwrite=true)
+                failwith $"""debug info mismatch
+Expected is in {expectedFile}
+Actual is in {debugInfoFile}
+Updated automatically, please check diffs in your pull request, changes must be scrutinized
+"""
+        )
+
     static member CompileLibraryAndVerifyIL (source: string) (f: ILVerifier -> unit) =
         CompilerAssert.CompileLibraryAndVerifyILWithOptions [||] source f
 

tests/FSharp.Test.Utilities/CompilerAssert.fs

@@ -0,0 +1,66 @@
+ENTRYPOINT
+  false
+
+DOCUMENTS
+ [0] <elided-for-testing>
+     Type: None
+     Language: None
+     Vendor: None
+
+METHODS
+ f2
+     Params: []
+     Range: Some "[0,5:9] - [0,5:11]"
+     Points:
+      - Doc: 0 Offset:0 [5:5]-[5-15]
+      - Doc: 0 Offset:2 [6:5]-[6-14]
+      - Doc: 0 Offset:3 [16707566:0]-[16707566-0]
+      - Doc: 0 Offset:6 [7:9]-[7-21]
+      - Doc: 0 Offset:16 [8:9]-[8-18]
+      - Doc: 0 Offset:17 [16707566:0]-[16707566-0]
+      - Doc: 0 Offset:20 [9:12]-[9-24]
+      - Doc: 0 Offset:26 [10:12]-[10-22]
+      - Doc: 0 Offset:28 [11:12]-[11-14]
+      - Doc: 0 Offset:30 [13:12]-[13-24]
+      - Doc: 0 Offset:37 [14:12]-[14-22]
+      - Doc: 0 Offset:40 [15:12]-[15-14]
+      - Doc: 0 Offset:43 [17:9]-[17-21]
+      - Doc: 0 Offset:54 [18:9]-[18-18]
+      - Doc: 0 Offset:55 [16707566:0]-[16707566-0]
+      - Doc: 0 Offset:58 [19:12]-[19-24]
+      - Doc: 0 Offset:65 [20:12]-[20-22]
+      - Doc: 0 Offset:68 [21:12]-[21-14]
+      - Doc: 0 Offset:71 [23:12]-[23-24]
+      - Doc: 0 Offset:78 [24:12]-[24-22]
+      - Doc: 0 Offset:81 [25:12]-[25-14]
+     Scopes:
+      - [0-84]
+        - [1-15]
+          Locals: ["0: v1"]
+        - [15-25]
+          Locals: ["0: v1"; "1: v2"]
+        - [25-27]
+          Locals: ["0: v1 (shadowed)"; "1: v2"; "2: v1"]
+        - [27-30]
+          Locals: ["1: v2 (shadowed)"; "0: v1 (shadowed)"; "2: v1"; "3: v2"]
+        - [30-35]
+          Locals: ["0: v1"; "1: v2"]
+        - [35-38]
+          Locals: ["0: v1 (shadowed)"; "1: v2"; "4: v1"]
+        - [38-43]
+          Locals: ["1: v2 (shadowed)"; "0: v1 (shadowed)"; "4: v1"; "5: v2"]
+        - [43-52]
+          Locals: ["0: v1"]
+        - [52-63]
+          Locals: ["0: v1"; "6: v2"]
+        - [63-66]
+          Locals: ["0: v1 (shadowed)"; "6: v2"; "7: v1"]
+        - [66-71]
+          Locals: ["6: v2 (shadowed)"; "0: v1 (shadowed)"; "7: v1"; "8: v2"]
+        - [71-76]
+          Locals: ["0: v1"; "6: v2"]
+        - [76-79]
+          Locals: ["0: v1 (shadowed)"; "6: v2"; "9: v1"]
+        - [79-84]
+          Locals: ["6: v2 (shadowed)"; "0: v1 (shadowed)"; "9: v1"; "10: v2"]
+

tests/fsharp/Compiler/CodeGen/EmittedIL/ComplexShadowingFunction.debuginfo.expected

@@ -0,0 +1,71 @@
+// Copyright (c) Microsoft Corporation.  All Rights Reserved.  See License.txt in the project root for license information.
+
+namespace FSharp.Compiler.UnitTests.CodeGen.EmittedIL
+
+open FSharp.Test
+open NUnit.Framework
+
+[<TestFixture>]
+module DebugScopes =
+
+    [<Test>]
+    let SimpleFunction() =
+        CompilerAssert.CompileLibraryAndVerifyDebugInfoWithOptions 
+            [|"--debug:portable"; "--optimize-"; "--optimize-"|]
+            (__SOURCE_DIRECTORY__ + "/SimpleFunction.debuginfo.expected")
+            """
+module Test
+let f x = 
+    let y = 1
+    2
+            """
+
+    [<Test>]
+    let SimpleShadowingFunction() =
+        CompilerAssert.CompileLibraryAndVerifyDebugInfoWithOptions 
+            [|"--debug:portable"; "--optimize-"; "--optimize-"|]
+            (__SOURCE_DIRECTORY__ + "/SimpleShadowingFunction.debuginfo.expected")
+            """
+module Test
+let f x = 
+    let y = 1
+    let y = y+1
+    let y = y+1
+    2
+            """
+
+    [<Test>]
+    let ComplexShadowingFunction() =
+        CompilerAssert.CompileLibraryAndVerifyDebugInfoWithOptions 
+            [|"--debug:portable"; "--optimize-"; "--optimize-"|]
+            (__SOURCE_DIRECTORY__ + "/ComplexShadowingFunction.debuginfo.expected")
+            """
+module Test
+
+let f2 (a, b) =
+    let v1 = 1
+    if a then
+        let v2 = 1.4
+        if b then
+           let v1 = "3"
+           let v2 = 5
+           v1
+        else
+           let v1 = "3"
+           let v2 = 5
+           v1
+    else
+        let v2 = 1.4
+        if b then
+           let v1 = "3"
+           let v2 = 5
+           v1
+        else
+           let v1 = "3"
+           let v2 = 5
+           v1
+
+
+
+            """
+