From ed1c500f75a9518c6613fbb445afc6b965a86206 Mon Sep 17 00:00:00 2001 From: nojaf Date: Fri, 21 Apr 2023 09:32:40 +0200 Subject: [PATCH 1/7] Keep track of connected files in namespace. --- .../GraphChecking/DependencyResolution.fs | 55 +++++-------------- .../Driver/GraphChecking/TrieMapping.fs | 24 ++++---- src/Compiler/Driver/GraphChecking/Types.fs | 2 +- src/Compiler/Driver/GraphChecking/Types.fsi | 6 +- .../TypeChecks/Graph/QueryTrieTests.fs | 28 +++++++++- .../TypeChecks/Graph/Scenarios.fs | 25 +++++++-- 6 files changed, 81 insertions(+), 59 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index 78e312e5fe..14358fb771 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -108,22 +108,6 @@ let rec processStateEntry (queryTrie: QueryTrie) (state: FileContentQueryState) FoundDependencies = foundDependencies } -/// Return all files contained in the trie. -let filesInTrie (node: TrieNode) : Set = - let rec collect (node: TrieNode) (continuation: FileIndex list -> FileIndex list) : FileIndex list = - let continuations: ((FileIndex list -> FileIndex list) -> FileIndex list) list = - [ - for node in node.Children.Values do - yield collect node - ] - - let finalContinuation indexes = - continuation [ yield! node.Files; yield! List.concat indexes ] - - Continuation.sequence continuations finalContinuation - - Set.ofList (collect node id) - /// /// For a given file's content, collect all missing ("ghost") file dependencies that the core resolution algorithm didn't return, /// but are required to satisfy the type-checker. @@ -136,16 +120,16 @@ let filesInTrie (node: TrieNode) : Set = /// - the namespace does not contain any children that can be referenced implicitly (eg. by type inference), /// then the main resolution algorithm does not create a link to any file defining the namespace. /// However, to satisfy the type-checker, the namespace must be resolved. -/// This function returns a list of extra dependencies that makes sure that any such namespaces can be resolved (if it exists). -/// For each unused open namespace we return one or more file links that define it. +/// This function returns an array with a potential extra dependencies that makes sure that any such namespaces can be resolved (if it exists). +/// For each unused open namespace we might return one link that defined it. /// let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: QueryTrie) (result: FileContentQueryState) = - // Go over all open namespaces, and assert all those links eventually went anywhere + // Go over all open namespaces, and assert all those links eventually went anywhere. Set.toArray result.OpenedNamespaces - |> Array.collect (fun path -> + |> Array.choose (fun path -> match queryTrie path with | QueryTrieNodeResult.NodeExposesData _ - | QueryTrieNodeResult.NodeDoesNotExist -> Array.empty + | QueryTrieNodeResult.NodeDoesNotExist -> None | QueryTrieNodeResult.NodeDoesNotExposeData -> // At this point we are following up if an open namespace really lead nowhere. let node = @@ -156,25 +140,16 @@ let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: find trie path - let filesDefiningNamespace = - filesInTrie node |> Set.filter (fun idx -> idx < fileIndex) - - let dependenciesDefiningNamespace = - Set.intersect result.FoundDependencies filesDefiningNamespace - - [| - if Set.isEmpty dependenciesDefiningNamespace then - // There is no existing dependency defining the namespace, - // so we need to add one. - if Set.isEmpty filesDefiningNamespace then - // No file defines inferrable symbols for this namespace, but the namespace might exist. - // Because we don't track what files define a namespace without any relevant content, - // the only way to ensure the namespace is in scope is to add a link to every preceding file. - yield! [| 0 .. (fileIndex - 1) |] - else - // At least one file defines the namespace - add a dependency to the first (top) one. - yield Seq.head filesDefiningNamespace - |]) + match node.Current with + // Both Root and module would expose data, so we can ignore them. + | Root _ + | Module _ -> None + | Namespace (connectedFiles = connectedFiles) -> + // We are only interested in any file that contained the namespace when they came before the current file. + // If the namespace is defined in a file after the current file then there is no way the current file can reference it. + // Which means that namespace would come from a different assembly. + connectedFiles + |> Seq.tryFind (fun connectedFileIdx -> connectedFileIdx < fileIndex)) let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInProject array) : Graph = // We know that implementation files backed by signatures cannot be depended upon. diff --git a/src/Compiler/Driver/GraphChecking/TrieMapping.fs b/src/Compiler/Driver/GraphChecking/TrieMapping.fs index 4ab2e64dc5..f8216ce7c0 100644 --- a/src/Compiler/Driver/GraphChecking/TrieMapping.fs +++ b/src/Compiler/Driver/GraphChecking/TrieMapping.fs @@ -61,9 +61,13 @@ let mergeTrieNodes (defaultChildSize: int) (tries: TrieNode array) = let node = root.Children[k] match node.Current, v.Current with - | TrieNodeInfo.Namespace (filesThatExposeTypes = currentFiles), TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles) -> + | TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes; connectedFiles = currentConnectedFiles), + TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; connectedFiles = otherConnectedFiles) -> for otherFile in otherFiles do - currentFiles.Add(otherFile) |> ignore + currentFilesThatExposeTypes.Add(otherFile) |> ignore + + for otherFile in otherConnectedFiles do + currentConnectedFiles.Add(otherFile) |> ignore | _ -> () for kv in v.Children do @@ -142,13 +146,13 @@ let processSynModuleOrNamespace<'Decl> // The reasoning is that a type could be inferred and a nested auto open module will lift its content one level up. let current = if isNamespace then - TrieNodeInfo.Namespace( - name, - (if hasTypesOrAutoOpenNestedModules then - HashSet.singleton idx - else - HashSet.empty ()) - ) + let filesThatExposeTypes = + if hasTypesOrAutoOpenNestedModules then + HashSet.singleton idx + else + HashSet.empty () + + TrieNodeInfo.Namespace(name, filesThatExposeTypes, HashSet.singleton idx) else TrieNodeInfo.Module(name, idx) @@ -184,7 +188,7 @@ let processSynModuleOrNamespace<'Decl> HashSet.empty () | _ -> HashSet.empty () - let current = TrieNodeInfo.Namespace(name, files) + let current = TrieNodeInfo.Namespace(name, files, HashSet.singleton idx) mkSingletonDict name { Current = current; Children = node } |> continuation) tail diff --git a/src/Compiler/Driver/GraphChecking/Types.fs b/src/Compiler/Driver/GraphChecking/Types.fs index 67a0c56493..019b75e081 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fs +++ b/src/Compiler/Driver/GraphChecking/Types.fs @@ -32,7 +32,7 @@ type internal FileInProject = type internal TrieNodeInfo = | Root of files: HashSet | Module of name: Identifier * file: FileIndex - | Namespace of name: Identifier * filesThatExposeTypes: HashSet + | Namespace of name: Identifier * filesThatExposeTypes: HashSet * connectedFiles: HashSet member x.Files: Set = match x with diff --git a/src/Compiler/Driver/GraphChecking/Types.fsi b/src/Compiler/Driver/GraphChecking/Types.fsi index 852b642bd5..af3db850ba 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fsi +++ b/src/Compiler/Driver/GraphChecking/Types.fsi @@ -30,7 +30,11 @@ type internal FileInProject = type internal TrieNodeInfo = | Root of files: HashSet | Module of name: Identifier * file: FileIndex - | Namespace of name: Identifier * filesThatExposeTypes: HashSet + | Namespace of + name: Identifier * + filesThatExposeTypes: HashSet * + /// Files that use this namespace but don't contribute to it. + connectedFiles: HashSet member Files: Set diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs index 03ccc68b63..4e9fd318ce 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs @@ -636,7 +636,7 @@ let private fantomasCoreTrie: TrieNode = [| "System", { - Current = TrieNodeInfo.Namespace("System", emptyHS ()) + Current = TrieNodeInfo.Namespace("System", emptyHS (), emptyHS ()) Children = dictionary [| @@ -649,13 +649,35 @@ let private fantomasCoreTrie: TrieNode = } "Fantomas", { - Current = TrieNodeInfo.Namespace("Fantomas", emptyHS ()) + Current = + TrieNodeInfo.Namespace( + "Fantomas", + emptyHS (), + HashSet([| + indexOf "ISourceTextExtensions.fs" + indexOf "RangeHelpers.fs" + indexOf "AstExtensions.fs" + indexOf "TriviaTypes.fs" + indexOf "Utils.fs" + indexOf "SourceParser.fs" + |])) Children = dictionary [| "Core", { - Current = TrieNodeInfo.Namespace("Core", emptyHS ()) + Current = + TrieNodeInfo.Namespace( + "Core", + emptyHS (), + HashSet([| + indexOf "ISourceTextExtensions.fs" + indexOf "RangeHelpers.fs" + indexOf "AstExtensions.fs" + indexOf "TriviaTypes.fs" + indexOf "Utils.fs" + indexOf "SourceParser.fs" + |])) Children = dictionary [| diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index ad1a7717a7..2519099b06 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -263,12 +263,12 @@ let c = 0 """ (set [| 0 |]) ] - // This is a very last resort measure to link C to all files that came before it. - // `open X` does exist but there is no file that is actively contributing to the X namespace + // `open X` does exist but there is no file that is actively contributing to the X namespace. // This is a trade-off scenario, if A.fs had a type or nested module we would consider it to contribute to the X namespace. // As it is empty, we don't include the file index in the trie. + // To satisfy the open statement we link it to the lowest file idx that came before it. scenario - "A open statement that leads nowhere should link to every file that came above it." + "A open statement that leads nowhere should link to the first that file that came before it." [ sourceFile "A.fs" @@ -289,7 +289,7 @@ namespace Z open X """ - (set [| 0; 1 |]) + (set [| 0 |]) ] // The nested module in this case adds content to the namespace // Similar if a namespace had a type. @@ -577,4 +577,21 @@ let Foo () : unit = """ (set [| 0 |]) ] + scenario + "Ghost dependency takes file index into account" + [ + sourceFile "X.fs" "module X" Set.empty + // opened namespace 'System.IO' will be found in the Trie. + // However, we should not link A.fs to X.fs (because of the ghost dependency mechanism) + // because B.fs introduces nodes `System` and `IO` and comes after A.fs. + sourceFile + "A.fs" + """ +module A + +open System.IO + """ + Set.empty + sourceFile "B.fs" "namespace System.IO" Set.empty + ] ] From dae60b05012d14652aae9a01b166a076b8ce61ac Mon Sep 17 00:00:00 2001 From: nojaf Date: Fri, 21 Apr 2023 11:48:23 +0200 Subject: [PATCH 2/7] Don't resolve ghost dependency when it is already covered by a real one. --- .../GraphChecking/DependencyResolution.fs | 14 +++++++++----- .../TypeChecks/Graph/Scenarios.fs | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index 14358fb771..bfb629a3be 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -145,11 +145,15 @@ let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: | Root _ | Module _ -> None | Namespace (connectedFiles = connectedFiles) -> - // We are only interested in any file that contained the namespace when they came before the current file. - // If the namespace is defined in a file after the current file then there is no way the current file can reference it. - // Which means that namespace would come from a different assembly. - connectedFiles - |> Seq.tryFind (fun connectedFileIdx -> connectedFileIdx < fileIndex)) + if connectedFiles.Overlaps(result.FoundDependencies) then + // The ghost dependency is already covered by a real dependency. + None + else + // We are only interested in any file that contained the namespace when they came before the current file. + // If the namespace is defined in a file after the current file then there is no way the current file can reference it. + // Which means that namespace would come from a different assembly. + connectedFiles + |> Seq.tryFind (fun connectedFileIdx -> connectedFileIdx < fileIndex)) let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInProject array) : Graph = // We know that implementation files backed by signatures cannot be depended upon. diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index 2519099b06..863cc5ee2c 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -594,4 +594,22 @@ open System.IO Set.empty sourceFile "B.fs" "namespace System.IO" Set.empty ] + scenario + "Ghost dependency that is already linked via module" + [ + sourceFile "X.fs" "module Foo.Bar.X" Set.empty + sourceFile "Y.fs" "module Foo.Bar.Y" Set.empty + // This file is linked to Y.fs due to opening the module `Foo.Bar.Y` + // The link to Y.fs should also satisfy the ghost dependency created after opening `Foo.Bar`. + // There is no need to add an additional link to the lowest index in node `Foo.Bar`. + sourceFile + "Z.fs" + """ +module Z + +open Foo.Bar // ghost dependency +open Foo.Bar.Y // Y.fs +""" + (set [| 1 |]) + ] ] From 183e6c8bd00e9d6631d54956b08f3b63e0d8403e Mon Sep 17 00:00:00 2001 From: nojaf Date: Fri, 21 Apr 2023 12:01:31 +0200 Subject: [PATCH 3/7] Don't process the first file as it cannot have any dependencies. --- .../GraphChecking/DependencyResolution.fs | 108 +++++++++--------- .../GraphChecking/FileContentMapping.fs | 62 +++++----- .../Graph/FileContentMappingTests.fs | 8 +- 3 files changed, 96 insertions(+), 82 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index bfb629a3be..8b0420c92a 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -172,58 +172,62 @@ let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInP let fileContents = files |> Array.Parallel.map FileContentMapping.mkFileContent let findDependencies (file: FileInProject) : FileIndex array = - let fileContent = fileContents[file.Idx] - - let knownFiles = [ 0 .. (file.Idx - 1) ] |> set - // File depends on all files above it that define accessible symbols at the root level (global namespace). - let filesFromRoot = trie.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx) - // Start by listing root-level dependencies. - let initialDepsResult = - (FileContentQueryState.Create file.Idx knownFiles filesFromRoot), fileContent - // Sequentially process all relevant entries of the file and keep updating the state and set of dependencies. - let depsResult = - initialDepsResult - // Seq is faster than List in this case. - ||> Seq.fold (processStateEntry queryTrie) - - // Add missing links for cases where an unused open namespace did not create a link. - let ghostDependencies = collectGhostDependencies file.Idx trie queryTrie depsResult - - // Add a link from implementation files to their signature files. - let signatureDependency = - match filePairs.TryGetSignatureIndex file.Idx with - | None -> Array.empty - | Some sigIdx -> Array.singleton sigIdx - - // Files in FSharp.Core have an implicit dependency on `prim-types-prelude.fsi` - add it. - let fsharpCoreImplicitDependencies = - let filename = "prim-types-prelude.fsi" - - let implicitDepIdx = - files - |> Array.tryFindIndex (fun f -> FileSystemUtils.fileNameOfPath f.FileName = filename) - - [| - if compilingFSharpCore then - match implicitDepIdx with - | Some idx -> - if file.Idx > idx then - yield idx - | None -> - exn $"Expected to find file '{filename}' during compilation of FSharp.Core, but it was not found." - |> raise - |] - - let allDependencies = - [| - yield! depsResult.FoundDependencies - yield! ghostDependencies - yield! signatureDependency - yield! fsharpCoreImplicitDependencies - |] - |> Array.distinct - - allDependencies + if file.Idx = 0 then + // First file cannot have any dependencies. + Array.empty + else + let fileContent = fileContents[file.Idx] + + let knownFiles = [ 0 .. (file.Idx - 1) ] |> set + // File depends on all files above it that define accessible symbols at the root level (global namespace). + let filesFromRoot = trie.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx) + // Start by listing root-level dependencies. + let initialDepsResult = + (FileContentQueryState.Create file.Idx knownFiles filesFromRoot), fileContent + // Sequentially process all relevant entries of the file and keep updating the state and set of dependencies. + let depsResult = + initialDepsResult + // Seq is faster than List in this case. + ||> Seq.fold (processStateEntry queryTrie) + + // Add missing links for cases where an unused open namespace did not create a link. + let ghostDependencies = collectGhostDependencies file.Idx trie queryTrie depsResult + + // Add a link from implementation files to their signature files. + let signatureDependency = + match filePairs.TryGetSignatureIndex file.Idx with + | None -> Array.empty + | Some sigIdx -> Array.singleton sigIdx + + // Files in FSharp.Core have an implicit dependency on `prim-types-prelude.fsi` - add it. + let fsharpCoreImplicitDependencies = + let filename = "prim-types-prelude.fsi" + + let implicitDepIdx = + files + |> Array.tryFindIndex (fun f -> FileSystemUtils.fileNameOfPath f.FileName = filename) + + [| + if compilingFSharpCore then + match implicitDepIdx with + | Some idx -> + if file.Idx > idx then + yield idx + | None -> + exn $"Expected to find file '{filename}' during compilation of FSharp.Core, but it was not found." + |> raise + |] + + let allDependencies = + [| + yield! depsResult.FoundDependencies + yield! ghostDependencies + yield! signatureDependency + yield! fsharpCoreImplicitDependencies + |] + |> Array.distinct + + allDependencies files |> Array.Parallel.map (fun file -> file.Idx, findDependencies file) diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index f20c95fa3d..d9ab4d9374 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -619,32 +619,36 @@ let visitSynMemberSig (ms: SynMemberSig) : FileContentEntry list = | SynMemberSig.NestedType _ -> [] let mkFileContent (f: FileInProject) : FileContentEntry list = - [ - match f.ParsedInput with - | ParsedInput.SigFile (ParsedSigFileInput (contents = contents)) -> - for SynModuleOrNamespaceSig (longId = longId; kind = kind; decls = decls; attribs = attribs) in contents do - yield! List.collect visitSynAttributeList attribs - - match kind with - | SynModuleOrNamespaceKind.GlobalNamespace - | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleSigDecl decls - | SynModuleOrNamespaceKind.DeclaredNamespace -> - let path = longIdentToPath false longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) - | SynModuleOrNamespaceKind.NamedModule -> - let path = longIdentToPath true longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) - | ParsedInput.ImplFile (ParsedImplFileInput (contents = contents)) -> - for SynModuleOrNamespace (longId = longId; attribs = attribs; kind = kind; decls = decls) in contents do - yield! List.collect visitSynAttributeList attribs - - match kind with - | SynModuleOrNamespaceKind.GlobalNamespace - | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleDecl decls - | SynModuleOrNamespaceKind.DeclaredNamespace -> - let path = longIdentToPath false longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) - | SynModuleOrNamespaceKind.NamedModule -> - let path = longIdentToPath true longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) - ] + if f.Idx = 0 then + // We don't need to process the first file as it cannot have any dependencies. + List.empty + else + [ + match f.ParsedInput with + | ParsedInput.SigFile (ParsedSigFileInput (contents = contents)) -> + for SynModuleOrNamespaceSig (longId = longId; kind = kind; decls = decls; attribs = attribs) in contents do + yield! List.collect visitSynAttributeList attribs + + match kind with + | SynModuleOrNamespaceKind.GlobalNamespace + | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleSigDecl decls + | SynModuleOrNamespaceKind.DeclaredNamespace -> + let path = longIdentToPath false longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) + | SynModuleOrNamespaceKind.NamedModule -> + let path = longIdentToPath true longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) + | ParsedInput.ImplFile (ParsedImplFileInput (contents = contents)) -> + for SynModuleOrNamespace (longId = longId; attribs = attribs; kind = kind; decls = decls) in contents do + yield! List.collect visitSynAttributeList attribs + + match kind with + | SynModuleOrNamespaceKind.GlobalNamespace + | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleDecl decls + | SynModuleOrNamespaceKind.DeclaredNamespace -> + let path = longIdentToPath false longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) + | SynModuleOrNamespaceKind.NamedModule -> + let path = longIdentToPath true longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) + ] diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs index 78a364cbae..1093ca94b3 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs @@ -7,7 +7,8 @@ open TestUtils let private getContent isSignature sourceCode = let fileName = if isSignature then "Test.fsi" else "Test.fs" let ast = parseSourceCode ("Test.fs", sourceCode) - FileContentMapping.mkFileContent { Idx = 0; FileName = fileName; ParsedInput = ast } + // We use index 1 in this test because the index 0 will always return an empty list + FileContentMapping.mkFileContent { Idx = 1; FileName = fileName; ParsedInput = ast } let private (|TopLevelNamespace|_|) value e = match e with @@ -121,3 +122,8 @@ module B = C match content with | [ TopLevelNamespace "" [ PrefixedIdentifier "C" ] ] -> Assert.Pass() | content -> Assert.Fail($"Unexpected content: {content}") + +[] +let ``First file index always returns an empty list`` () = + let content = FileContentMapping.mkFileContent { Idx = 0; FileName = "FirstFile.fs"; ParsedInput = Unchecked.defaultof<_> } + Assert.IsTrue content.IsEmpty From 2947b61b6b9e04f8250cc9abefc64302a665192f Mon Sep 17 00:00:00 2001 From: nojaf Date: Mon, 24 Apr 2023 09:29:27 +0200 Subject: [PATCH 4/7] Address feedback from review. --- .../GraphChecking/DependencyResolution.fs | 23 ++++--- .../GraphChecking/FileContentMapping.fs | 62 +++++++++---------- .../Driver/GraphChecking/TrieMapping.fs | 33 +++++----- src/Compiler/Driver/GraphChecking/Types.fs | 2 +- src/Compiler/Driver/GraphChecking/Types.fsi | 5 +- .../Graph/FileContentMappingTests.fs | 8 +-- .../TypeChecks/Graph/Scenarios.fs | 4 +- 7 files changed, 69 insertions(+), 68 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index 8b0420c92a..c7392666f2 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -120,8 +120,8 @@ let rec processStateEntry (queryTrie: QueryTrie) (state: FileContentQueryState) /// - the namespace does not contain any children that can be referenced implicitly (eg. by type inference), /// then the main resolution algorithm does not create a link to any file defining the namespace. /// However, to satisfy the type-checker, the namespace must be resolved. -/// This function returns an array with a potential extra dependencies that makes sure that any such namespaces can be resolved (if it exists). -/// For each unused open namespace we might return one link that defined it. +/// This function returns an array with a potential extra dependencies that makes sure that any such namespaces can be resolved (if they exists). +/// For each unused namespace `open` we return at most one file that defines that namespace. /// let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: QueryTrie) (result: FileContentQueryState) = // Go over all open namespaces, and assert all those links eventually went anywhere. @@ -144,16 +144,19 @@ let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: // Both Root and module would expose data, so we can ignore them. | Root _ | Module _ -> None - | Namespace (connectedFiles = connectedFiles) -> - if connectedFiles.Overlaps(result.FoundDependencies) then + | Namespace (filesDefiningNamespaceWithoutTypes = filesDefiningNamespaceWithoutTypes) -> + if filesDefiningNamespaceWithoutTypes.Overlaps(result.FoundDependencies) then // The ghost dependency is already covered by a real dependency. None else // We are only interested in any file that contained the namespace when they came before the current file. // If the namespace is defined in a file after the current file then there is no way the current file can reference it. // Which means that namespace would come from a different assembly. - connectedFiles - |> Seq.tryFind (fun connectedFileIdx -> connectedFileIdx < fileIndex)) + filesDefiningNamespaceWithoutTypes + |> Seq.sort + |> Seq.tryFind (fun connectedFileIdx -> + // We pick the lowest file index from the namespace to satisfy the type-checker for the open statement. + connectedFileIdx < fileIndex)) let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInProject array) : Graph = // We know that implementation files backed by signatures cannot be depended upon. @@ -169,7 +172,13 @@ let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInP let trie = TrieMapping.mkTrie trieInput let queryTrie: QueryTrie = queryTrieMemoized trie - let fileContents = files |> Array.Parallel.map FileContentMapping.mkFileContent + let fileContents = + files + |> Array.Parallel.map (fun file -> + if file.Idx = 0 then + List.empty + else + FileContentMapping.mkFileContent file) let findDependencies (file: FileInProject) : FileIndex array = if file.Idx = 0 then diff --git a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs index d9ab4d9374..f20c95fa3d 100644 --- a/src/Compiler/Driver/GraphChecking/FileContentMapping.fs +++ b/src/Compiler/Driver/GraphChecking/FileContentMapping.fs @@ -619,36 +619,32 @@ let visitSynMemberSig (ms: SynMemberSig) : FileContentEntry list = | SynMemberSig.NestedType _ -> [] let mkFileContent (f: FileInProject) : FileContentEntry list = - if f.Idx = 0 then - // We don't need to process the first file as it cannot have any dependencies. - List.empty - else - [ - match f.ParsedInput with - | ParsedInput.SigFile (ParsedSigFileInput (contents = contents)) -> - for SynModuleOrNamespaceSig (longId = longId; kind = kind; decls = decls; attribs = attribs) in contents do - yield! List.collect visitSynAttributeList attribs - - match kind with - | SynModuleOrNamespaceKind.GlobalNamespace - | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleSigDecl decls - | SynModuleOrNamespaceKind.DeclaredNamespace -> - let path = longIdentToPath false longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) - | SynModuleOrNamespaceKind.NamedModule -> - let path = longIdentToPath true longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) - | ParsedInput.ImplFile (ParsedImplFileInput (contents = contents)) -> - for SynModuleOrNamespace (longId = longId; attribs = attribs; kind = kind; decls = decls) in contents do - yield! List.collect visitSynAttributeList attribs - - match kind with - | SynModuleOrNamespaceKind.GlobalNamespace - | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleDecl decls - | SynModuleOrNamespaceKind.DeclaredNamespace -> - let path = longIdentToPath false longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) - | SynModuleOrNamespaceKind.NamedModule -> - let path = longIdentToPath true longId - yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) - ] + [ + match f.ParsedInput with + | ParsedInput.SigFile (ParsedSigFileInput (contents = contents)) -> + for SynModuleOrNamespaceSig (longId = longId; kind = kind; decls = decls; attribs = attribs) in contents do + yield! List.collect visitSynAttributeList attribs + + match kind with + | SynModuleOrNamespaceKind.GlobalNamespace + | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleSigDecl decls + | SynModuleOrNamespaceKind.DeclaredNamespace -> + let path = longIdentToPath false longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) + | SynModuleOrNamespaceKind.NamedModule -> + let path = longIdentToPath true longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleSigDecl decls) + | ParsedInput.ImplFile (ParsedImplFileInput (contents = contents)) -> + for SynModuleOrNamespace (longId = longId; attribs = attribs; kind = kind; decls = decls) in contents do + yield! List.collect visitSynAttributeList attribs + + match kind with + | SynModuleOrNamespaceKind.GlobalNamespace + | SynModuleOrNamespaceKind.AnonModule -> yield! List.collect visitSynModuleDecl decls + | SynModuleOrNamespaceKind.DeclaredNamespace -> + let path = longIdentToPath false longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) + | SynModuleOrNamespaceKind.NamedModule -> + let path = longIdentToPath true longId + yield FileContentEntry.TopLevelNamespace(path, List.collect visitSynModuleDecl decls) + ] diff --git a/src/Compiler/Driver/GraphChecking/TrieMapping.fs b/src/Compiler/Driver/GraphChecking/TrieMapping.fs index f8216ce7c0..fbecfab95c 100644 --- a/src/Compiler/Driver/GraphChecking/TrieMapping.fs +++ b/src/Compiler/Driver/GraphChecking/TrieMapping.fs @@ -56,18 +56,18 @@ let doesFileExposeContentToTheRoot (ast: ParsedInput) : bool = || kind = SynModuleOrNamespaceKind.GlobalNamespace) let mergeTrieNodes (defaultChildSize: int) (tries: TrieNode array) = + /// Add the current node as child node to the root node. + /// If the node already exists and is a namespace node, the existing node will be updated with new information via mutation. let rec mergeTrieNodesAux (root: TrieNode) (KeyValue (k, v)) = if root.Children.ContainsKey k then let node = root.Children[k] match node.Current, v.Current with - | TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes; connectedFiles = currentConnectedFiles), - TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; connectedFiles = otherConnectedFiles) -> - for otherFile in otherFiles do - currentFilesThatExposeTypes.Add(otherFile) |> ignore - - for otherFile in otherConnectedFiles do - currentConnectedFiles.Add(otherFile) |> ignore + | TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes + filesDefiningNamespaceWithoutTypes = currentFilesWithoutTypes), + TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; filesDefiningNamespaceWithoutTypes = otherFilesWithoutTypes) -> + currentFilesThatExposeTypes.UnionWith otherFiles + currentFilesWithoutTypes.UnionWith otherFilesWithoutTypes | _ -> () for kv in v.Children do @@ -146,13 +146,13 @@ let processSynModuleOrNamespace<'Decl> // The reasoning is that a type could be inferred and a nested auto open module will lift its content one level up. let current = if isNamespace then - let filesThatExposeTypes = + let filesThatExposeTypes, filesDefiningNamespaceWithoutTypes = if hasTypesOrAutoOpenNestedModules then - HashSet.singleton idx + HashSet.singleton idx, HashSet.empty () else - HashSet.empty () + HashSet.empty (), HashSet.singleton idx - TrieNodeInfo.Namespace(name, filesThatExposeTypes, HashSet.singleton idx) + TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes) else TrieNodeInfo.Module(name, idx) @@ -171,7 +171,7 @@ let processSynModuleOrNamespace<'Decl> visit (fun node -> - let files = + let filesThatExposeTypes, filesDefiningNamespaceWithoutTypes = match tail with | [ _ ] -> // In case you have: @@ -183,12 +183,13 @@ let processSynModuleOrNamespace<'Decl> let topLevelModuleOrNamespaceHasAutoOpen = isAnyAttributeAutoOpen attributes if topLevelModuleOrNamespaceHasAutoOpen && not isNamespace then - HashSet.singleton idx + HashSet.singleton idx, HashSet.empty () else - HashSet.empty () - | _ -> HashSet.empty () + HashSet.empty (), HashSet.singleton idx + | _ -> HashSet.empty (), HashSet.singleton idx - let current = TrieNodeInfo.Namespace(name, files, HashSet.singleton idx) + let current = + TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes) mkSingletonDict name { Current = current; Children = node } |> continuation) tail diff --git a/src/Compiler/Driver/GraphChecking/Types.fs b/src/Compiler/Driver/GraphChecking/Types.fs index 019b75e081..c0e8e0f84b 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fs +++ b/src/Compiler/Driver/GraphChecking/Types.fs @@ -32,7 +32,7 @@ type internal FileInProject = type internal TrieNodeInfo = | Root of files: HashSet | Module of name: Identifier * file: FileIndex - | Namespace of name: Identifier * filesThatExposeTypes: HashSet * connectedFiles: HashSet + | Namespace of name: Identifier * filesThatExposeTypes: HashSet * filesDefiningNamespaceWithoutTypes: HashSet member x.Files: Set = match x with diff --git a/src/Compiler/Driver/GraphChecking/Types.fsi b/src/Compiler/Driver/GraphChecking/Types.fsi index af3db850ba..7d0ba9bbdd 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fsi +++ b/src/Compiler/Driver/GraphChecking/Types.fsi @@ -32,9 +32,10 @@ type internal TrieNodeInfo = | Module of name: Identifier * file: FileIndex | Namespace of name: Identifier * + /// Files that expose types that are part of this namespace. filesThatExposeTypes: HashSet * - /// Files that use this namespace but don't contribute to it. - connectedFiles: HashSet + /// Files that use this namespace but don't contain any types. + filesDefiningNamespaceWithoutTypes: HashSet member Files: Set diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs index 1093ca94b3..78a364cbae 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/FileContentMappingTests.fs @@ -7,8 +7,7 @@ open TestUtils let private getContent isSignature sourceCode = let fileName = if isSignature then "Test.fsi" else "Test.fs" let ast = parseSourceCode ("Test.fs", sourceCode) - // We use index 1 in this test because the index 0 will always return an empty list - FileContentMapping.mkFileContent { Idx = 1; FileName = fileName; ParsedInput = ast } + FileContentMapping.mkFileContent { Idx = 0; FileName = fileName; ParsedInput = ast } let private (|TopLevelNamespace|_|) value e = match e with @@ -122,8 +121,3 @@ module B = C match content with | [ TopLevelNamespace "" [ PrefixedIdentifier "C" ] ] -> Assert.Pass() | content -> Assert.Fail($"Unexpected content: {content}") - -[] -let ``First file index always returns an empty list`` () = - let content = FileContentMapping.mkFileContent { Idx = 0; FileName = "FirstFile.fs"; ParsedInput = Unchecked.defaultof<_> } - Assert.IsTrue content.IsEmpty diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index 863cc5ee2c..1ae135cf44 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -266,9 +266,9 @@ let c = 0 // `open X` does exist but there is no file that is actively contributing to the X namespace. // This is a trade-off scenario, if A.fs had a type or nested module we would consider it to contribute to the X namespace. // As it is empty, we don't include the file index in the trie. - // To satisfy the open statement we link it to the lowest file idx that came before it. + // To satisfy the open statement we link it to the lowest file idx of the found namespace node X in the trie. scenario - "A open statement that leads nowhere should link to the first that file that came before it." + "An open statement that leads to a namespace node without any types, should link to the lowest file idx of that namespace node." [ sourceFile "A.fs" From d9b00cdb534b7b3fcb85dc90a1cab9821ca27505 Mon Sep 17 00:00:00 2001 From: nojaf Date: Mon, 24 Apr 2023 09:36:40 +0200 Subject: [PATCH 5/7] Improve comment. --- src/Compiler/Driver/GraphChecking/DependencyResolution.fs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index c7392666f2..be6d3c64df 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -124,7 +124,7 @@ let rec processStateEntry (queryTrie: QueryTrie) (state: FileContentQueryState) /// For each unused namespace `open` we return at most one file that defines that namespace. /// let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: QueryTrie) (result: FileContentQueryState) = - // Go over all open namespaces, and assert all those links eventually went anywhere. + // For each opened namespace, if none of already resolved dependencies define it, return the top-most file that defines it. Set.toArray result.OpenedNamespaces |> Array.choose (fun path -> match queryTrie path with From ffff7d11311311b2b9d3c5c726fea6b2c51ade74 Mon Sep 17 00:00:00 2001 From: nojaf Date: Mon, 24 Apr 2023 13:54:54 +0200 Subject: [PATCH 6/7] Trigger CI From 09c88768fbaacb769c5aad590c614947c53bd32f Mon Sep 17 00:00:00 2001 From: nojaf Date: Mon, 24 Apr 2023 16:18:03 +0200 Subject: [PATCH 7/7] Unsure if this PR will break the flow, It seems stuck, but I'll double-check and know.