From 18cad798ea3c0f5af4184e75420b9a24ea47967c Mon Sep 17 00:00:00 2001 From: nojaf Date: Mon, 9 Oct 2023 14:48:16 +0200 Subject: [PATCH 1/7] WIP: incremental Trie --- .../GraphChecking/DependencyResolution.fs | 21 ++- .../Driver/GraphChecking/TrieMapping.fs | 170 ++++++++++-------- .../Driver/GraphChecking/TrieMapping.fsi | 2 +- src/Compiler/Driver/GraphChecking/Types.fs | 20 ++- src/Compiler/Driver/GraphChecking/Types.fsi | 18 +- .../TypeChecks/Graph/QueryTrieTests.fs | 9 - .../TypeChecks/Graph/Scenarios.fs | 1 + .../TypeChecks/Graph/TrieMappingTests.fs | 44 ++--- 8 files changed, 153 insertions(+), 132 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index c3206efbdfb..688138fe97d 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -185,6 +185,7 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph Some f) let trie = TrieMapping.mkTrie trieInput + ignore trie let fileContents = files @@ -201,20 +202,28 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph set + // The Trie we want to use is the one that contains only files before our current index. + // As we skip implementation files (backed by a signature), we cannot just use the current file index to find the right Trie. + let trieForFile = + trie + |> Array.fold (fun acc (idx, t) -> if idx < file.Idx then t else acc) TrieNode.Empty + + ignore trieForFile + // 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) + let filesFromRoot = + trieForFile.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx) // Start by listing root-level dependencies. let initialDepsResult = - (FileContentQueryState.Create file.Idx knownFiles filesFromRoot), fileContent + (FileContentQueryState.Create file.Idx 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 trie) + ||> Seq.fold (processStateEntry trieForFile) // Add missing links for cases where an unused open namespace did not create a link. - let ghostDependencies = collectGhostDependencies file.Idx trie depsResult + let ghostDependencies = collectGhostDependencies file.Idx trieForFile depsResult // Add a link from implementation files to their signature files. let signatureDependency = @@ -237,4 +246,6 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph Array.Parallel.map (fun file -> file.Idx, findDependencies file) |> readOnlyDict + let trie = trie |> Array.last |> snd + graph, trie diff --git a/src/Compiler/Driver/GraphChecking/TrieMapping.fs b/src/Compiler/Driver/GraphChecking/TrieMapping.fs index e34939abac1..32806275279 100644 --- a/src/Compiler/Driver/GraphChecking/TrieMapping.fs +++ b/src/Compiler/Driver/GraphChecking/TrieMapping.fs @@ -1,16 +1,19 @@ module internal FSharp.Compiler.GraphChecking.TrieMapping -open System.Collections.Generic +open System.Collections.Generic +open System.Collections.Immutable open System.Text open FSharp.Compiler.IO open FSharp.Compiler.Syntax +// TODO: Deal with Immutable collections and benchmark whether they are worth it? + [] -module private HashSet = +module private ImmutableHashSet = /// Create a new HashSet<'T> with a single element. - let singleton value = HashSet(Seq.singleton value) + let singleton (value: 'T) = ImmutableHashSet.Create<'T>(Array.singleton value) /// Create new new HashSet<'T> with zero elements. - let empty () = HashSet(Seq.empty) + let empty () = ImmutableHashSet.Empty let autoOpenShapes = set @@ -57,7 +60,8 @@ let doesFileExposeContentToTheRoot (ast: ParsedInput) : bool = (isAnyAttributeAutoOpen attribs && longId.Length < 2) || kind = SynModuleOrNamespaceKind.GlobalNamespace) -let mergeTrieNodes (defaultChildSize: int) (tries: TrieNode array) = +/// Merge all the accumulator Trie nodes into the current Trie node. +let mergeTrieNodes (accumulatorTrie: TrieNode) (currentTrie: TrieNode) : TrieNode = /// 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)) = @@ -67,7 +71,7 @@ let mergeTrieNodes (defaultChildSize: int) (tries: TrieNode array) = match node.Current, v.Current with | TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes filesDefiningNamespaceWithoutTypes = currentFilesWithoutTypes), - TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; filesDefiningNamespaceWithoutTypes = otherFilesWithoutTypes) -> + TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; filesDefiningNamespaceWithoutTypes = otherFilesWithoutTypes) -> currentFilesThatExposeTypes.UnionWith otherFiles currentFilesWithoutTypes.UnionWith otherFilesWithoutTypes // Edge case scenario detected in https://github.com/dotnet/fsharp/issues/15985 @@ -86,38 +90,27 @@ let mergeTrieNodes (defaultChildSize: int) (tries: TrieNode array) = else root.Children.Add(k, v) - match Array.tryExactlyOne tries with - | Some ({ Current = TrieNodeInfo.Root _ } as singleTrie) -> singleTrie - | _ -> - let rootFiles = HashSet.empty () - - let root = - { - Current = TrieNodeInfo.Root rootFiles - Children = Dictionary<_, _>(defaultChildSize) - } - - for trie in tries do - for rootIndex in trie.Files do - rootFiles.Add rootIndex |> ignore - - match trie.Current with - | TrieNodeInfo.Root _ -> () - | current -> System.Diagnostics.Debug.Assert(false, $"The top level node info of a trie should be Root, got {current}") - - for kv in trie.Children do - mergeTrieNodesAux root kv + match accumulatorTrie.Current, currentTrie.Current with + | TrieNodeInfo.Root accFiles, TrieNodeInfo.Root currentFiles -> + + + for rootIndex in currentTrie.Files do + accFiles.Add rootIndex |> ignore - root + for kv in currentTrie.Children do + mergeTrieNodesAux accumulatorTrie kv -let mkDictFromKeyValuePairs (items: KeyValuePair<'TKey, 'TValue> list) = - let dict = Dictionary(Seq.length items) + accumulatorTrie + | _ -> + System.Diagnostics.Debug.Assert( + false, + $"The top level node info of the accumulator and current trie should be Root, got {accumulatorTrie.Current} and {currentTrie.Current}" + ) - for KeyValue (k, v) in items do - if not (dict.ContainsKey(k)) then - dict.Add(k, v) + accumulatorTrie - dict +let mkImmutableDictFromKeyValuePairs (items: KeyValuePair<'TKey, 'TValue> list) = + ImmutableDictionary.CreateRange(items) let mkSingletonDict key value = let dict = Dictionary(1) @@ -167,7 +160,7 @@ let processSynModuleOrNamespace<'Decl> TrieNodeInfo.Module(name, idx) let children = - List.choose (mkTrieForDeclaration idx) decls |> mkDictFromKeyValuePairs + List.choose (mkTrieForDeclaration idx) decls |> mkImmutableDictFromKeyValuePairs mkSingletonDict name @@ -206,7 +199,7 @@ let processSynModuleOrNamespace<'Decl> if kind = SynModuleOrNamespaceKind.AnonModule then // We collect the child nodes from the decls - decls |> List.choose (mkTrieForDeclaration idx) |> mkDictFromKeyValuePairs + decls |> List.choose (mkTrieForDeclaration idx) |> mkImmutableDictFromKeyValuePairs else visit id name @@ -215,52 +208,58 @@ let processSynModuleOrNamespace<'Decl> Children = children } -let rec mkTrieNodeFor (file: FileInProject) : TrieNode = +let rec mkTrieNodeFor (file: FileInProject) : FileIndex * TrieNode = let idx = file.Idx if doesFileExposeContentToTheRoot file.ParsedInput then // If a file exposes content which does not need an open statement to access, we consider the file to be part of the root. + idx, { - Current = Root(HashSet.singleton idx) - Children = Dictionary(0) + Current = Root (ImmutableHashSet.singleton idx) + Children = ImmutableDictionary.Empty } else - match file.ParsedInput with - | ParsedInput.SigFile (ParsedSigFileInput (contents = contents)) -> - contents - |> List.map - (fun (SynModuleOrNamespaceSig (longId = longId - kind = kind - attribs = attribs - decls = decls - accessibility = _accessibility)) -> - let hasTypesOrAutoOpenNestedModules = - decls - |> List.exists (function - | SynModuleSigDecl.Types _ -> true - | SynModuleSigDecl.NestedModule(moduleInfo = SynComponentInfo (attributes = attributes)) -> - isAnyAttributeAutoOpen attributes - | _ -> false) - - processSynModuleOrNamespace mkTrieForSynModuleSigDecl idx longId attribs kind hasTypesOrAutoOpenNestedModules decls) - |> List.toArray - |> mergeTrieNodes contents.Length - | ParsedInput.ImplFile (ParsedImplFileInput (contents = contents)) -> - contents - |> List.map - (fun (SynModuleOrNamespace (longId = longId; attribs = attribs; kind = kind; decls = decls; accessibility = _accessibility)) -> - let hasTypesOrAutoOpenNestedModules = - List.exists - (function - | SynModuleDecl.Types _ -> true - | SynModuleDecl.NestedModule(moduleInfo = SynComponentInfo (attributes = attributes)) -> - isAnyAttributeAutoOpen attributes - | _ -> false) + let trie = + match file.ParsedInput with + | ParsedInput.SigFile (ParsedSigFileInput (contents = contents)) -> + contents + |> List.map + (fun (SynModuleOrNamespaceSig (longId = longId + kind = kind + attribs = attribs + decls = decls + accessibility = _accessibility)) -> + let hasTypesOrAutoOpenNestedModules = decls - - processSynModuleOrNamespace mkTrieForSynModuleDecl idx longId attribs kind hasTypesOrAutoOpenNestedModules decls) - |> List.toArray - |> mergeTrieNodes contents.Length + |> List.exists (function + | SynModuleSigDecl.Types _ -> true + | SynModuleSigDecl.NestedModule(moduleInfo = SynComponentInfo (attributes = attributes)) -> + isAnyAttributeAutoOpen attributes + | _ -> false) + + processSynModuleOrNamespace mkTrieForSynModuleSigDecl idx longId attribs kind hasTypesOrAutoOpenNestedModules decls) + |> List.reduce mergeTrieNodes + | ParsedInput.ImplFile (ParsedImplFileInput (contents = contents)) -> + contents + |> List.map + (fun (SynModuleOrNamespace (longId = longId + attribs = attribs + kind = kind + decls = decls + accessibility = _accessibility)) -> + let hasTypesOrAutoOpenNestedModules = + List.exists + (function + | SynModuleDecl.Types _ -> true + | SynModuleDecl.NestedModule(moduleInfo = SynComponentInfo (attributes = attributes)) -> + isAnyAttributeAutoOpen attributes + | _ -> false) + decls + + processSynModuleOrNamespace mkTrieForSynModuleDecl idx longId attribs kind hasTypesOrAutoOpenNestedModules decls) + |> List.reduce mergeTrieNodes + + idx, trie and mkTrieForSynModuleDecl (fileIndex: FileIndex) (decl: SynModuleDecl) : KeyValuePair option = match decl with @@ -270,7 +269,7 @@ and mkTrieForSynModuleDecl (fileIndex: FileIndex) (decl: SynModuleDecl) : KeyVal let children = decls |> List.choose (mkTrieForSynModuleDecl fileIndex) - |> mkDictFromKeyValuePairs + |> mkImmutableDictFromKeyValuePairs Some( KeyValuePair( @@ -291,7 +290,7 @@ and mkTrieForSynModuleSigDecl (fileIndex: FileIndex) (decl: SynModuleSigDecl) : let children = decls |> List.choose (mkTrieForSynModuleSigDecl fileIndex) - |> mkDictFromKeyValuePairs + |> mkImmutableDictFromKeyValuePairs Some( KeyValuePair( @@ -302,11 +301,24 @@ and mkTrieForSynModuleSigDecl (fileIndex: FileIndex) (decl: SynModuleSigDecl) : } ) ) - | _ -> None -let mkTrie (files: FileInProject array) : TrieNode = - mergeTrieNodes 0 (files |> Array.Parallel.map mkTrieNodeFor) +let mkTrie (files: FileInProject array) : (FileIndex * TrieNode) array = + if files.Length = 1 then + Array.singleton (mkTrieNodeFor files[0]) + else + files + |> Array.take (files.Length - 1) // Do not process the last file, it will never be looked up by anything anyway. + |> Array.Parallel.map mkTrieNodeFor + |> Array.scan + (fun (_, acc) (idx, current) -> + // We first need to create a copy of the Trie as Children are inside a mutable Dictionary. + let accCopy = mergeTrieNodes TrieNode.Empty acc + // Next we add the data of the current file to the copy. + idx, mergeTrieNodes accCopy current) + (System.Int32.MinValue, TrieNode.Empty) + // We can ignore the initial state that was used in the scan + |> Array.skip 1 type MermaidBoxPos = | First diff --git a/src/Compiler/Driver/GraphChecking/TrieMapping.fsi b/src/Compiler/Driver/GraphChecking/TrieMapping.fsi index 00784a5da33..649a2a25e0e 100644 --- a/src/Compiler/Driver/GraphChecking/TrieMapping.fsi +++ b/src/Compiler/Driver/GraphChecking/TrieMapping.fsi @@ -2,6 +2,6 @@ module internal FSharp.Compiler.GraphChecking.TrieMapping /// Process all the files (in parallel) in a project to construct a Root TrieNode. /// When the project has signature files, the implementation counterparts will not be processed. -val mkTrie: files: FileInProject array -> TrieNode +val mkTrie: files: FileInProject array -> (FileIndex * TrieNode) array val serializeToMermaid: path: string -> filesInProject: FileInProject array -> trie: TrieNode -> unit diff --git a/src/Compiler/Driver/GraphChecking/Types.fs b/src/Compiler/Driver/GraphChecking/Types.fs index 04359519b95..18d8fdf6ed6 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fs +++ b/src/Compiler/Driver/GraphChecking/Types.fs @@ -48,6 +48,14 @@ type internal TrieNode = member x.Files = x.Current.Files + static member Empty = + let rootFiles = HashSet(Seq.empty) + + { + Current = TrieNodeInfo.Root rootFiles + Children = Dictionary<_, _>(0) + } + /// A significant construct found in the syntax tree of a file. /// This construct needs to be processed in order to deduce potential links to other files in the project. type internal FileContentEntry = @@ -76,24 +84,21 @@ type internal FileContentQueryState = OpenedNamespaces: Set FoundDependencies: Set CurrentFile: FileIndex - KnownFiles: Set } - static member Create (fileIndex: FileIndex) (knownFiles: Set) (filesAtRoot: Set) = + static member Create (fileIndex: FileIndex) (filesAtRoot: Set) = { OwnNamespace = None OpenedNamespaces = Set.empty FoundDependencies = filesAtRoot CurrentFile = fileIndex - KnownFiles = knownFiles } member x.AddOwnNamespace(ns: LongIdentifier, ?files: Set) = match files with | None -> { x with OwnNamespace = Some ns } | Some files -> - let foundDependencies = - Set.filter x.KnownFiles.Contains files |> Set.union x.FoundDependencies + let foundDependencies = files |> Set.union x.FoundDependencies { x with OwnNamespace = Some ns @@ -101,7 +106,7 @@ type internal FileContentQueryState = } member x.AddDependencies(files: Set) : FileContentQueryState = - let files = Set.filter x.KnownFiles.Contains files |> Set.union x.FoundDependencies + let files = files |> Set.union x.FoundDependencies { x with FoundDependencies = files } member x.AddOpenNamespace(path: LongIdentifier, ?files: Set) = @@ -111,8 +116,7 @@ type internal FileContentQueryState = OpenedNamespaces = Set.add path x.OpenedNamespaces } | Some files -> - let foundDependencies = - Set.filter x.KnownFiles.Contains files |> Set.union x.FoundDependencies + let foundDependencies = files |> Set.union x.FoundDependencies { x with FoundDependencies = foundDependencies diff --git a/src/Compiler/Driver/GraphChecking/Types.fsi b/src/Compiler/Driver/GraphChecking/Types.fsi index 67403d51d98..9a642413d40 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fsi +++ b/src/Compiler/Driver/GraphChecking/Types.fsi @@ -1,6 +1,6 @@ namespace FSharp.Compiler.GraphChecking -open System.Collections.Generic +open System.Collections.Immutable open FSharp.Compiler.Syntax /// The index of a file inside a project. @@ -28,14 +28,14 @@ type internal FileInProject = /// Only when the namespace exposes types that could later be inferred. /// Children of a namespace don't automatically depend on each other for that reason type internal TrieNodeInfo = - | Root of files: HashSet + | Root of files: ImmutableHashSet | Module of name: Identifier * file: FileIndex | Namespace of name: Identifier * /// Files that expose types that are part of this namespace. - filesThatExposeTypes: HashSet * + filesThatExposeTypes: ImmutableHashSet * /// Files that use this namespace but don't contain any types. - filesDefiningNamespaceWithoutTypes: HashSet + filesDefiningNamespaceWithoutTypes: ImmutableHashSet member Files: Set @@ -45,12 +45,14 @@ type internal TrieNode = /// Information about this node. Current: TrieNodeInfo /// Child nodes - Children: Dictionary + Children: ImmutableDictionary } /// Zero or more files that define the LongIdentifier represented by this node. member Files: Set + static member Empty: TrieNode + /// A significant construct found in the syntax tree of a file. /// This construct needs to be processed in order to deduce potential links to other files in the project. type internal FileContentEntry = @@ -76,11 +78,9 @@ type internal FileContentQueryState = { OwnNamespace: LongIdentifier option OpenedNamespaces: Set FoundDependencies: Set - CurrentFile: FileIndex - KnownFiles: Set } + CurrentFile: FileIndex } - static member Create: - fileIndex: FileIndex -> knownFiles: Set -> filesAtRoot: Set -> FileContentQueryState + static member Create: fileIndex: FileIndex -> filesAtRoot: Set -> FileContentQueryState member AddOwnNamespace: ns: LongIdentifier * ?files: Set -> FileContentQueryState member AddDependencies: files: Set -> FileContentQueryState member AddOpenNamespace: path: LongIdentifier * ?files: Set -> FileContentQueryState diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs index b82014b178a..5c1f57fc6ed 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs @@ -795,15 +795,6 @@ let ``ProcessOpenStatement full path match`` () = let state = FileContentQueryState.Create sourceParser.Idx - (set - [| - indexOf "AssemblyInfo.fs" - indexOf "ISourceTextExtensions.fs" - indexOf "RangeHelpers.fs" - indexOf "AstExtensions.fsi" - indexOf "TriviaTypes.fs" - indexOf "Utils.fs" - |]) Set.empty let result = diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index 1211e6b19f3..f971c8dd876 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -773,3 +773,4 @@ type DiGraph = obj (set [| 0 |]) ] ] +] \ No newline at end of file diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs index d8a2e9da6a6..94685ab7464 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs @@ -40,27 +40,28 @@ type C = { CX: int; CY: int } } : FileInProject) let trie = TrieMapping.mkTrie files - - match trie.Current with - | TrieNodeInfo.Root _ -> () - | current -> Assert.Fail($"mkTrie should always return a TrieNodeInfo.Root, got {current}") - - let xNode = trie.Children.["X"] - Assert.AreEqual(1, xNode.Children.Count) - Assert.True(Seq.isEmpty xNode.Files) - - let yNode = xNode.Children["Y"] - Assert.AreEqual(2, yNode.Children.Count) - Assert.AreEqual(set [| 2 |], yNode.Files) - - let aNode = yNode.Children["A"] - Assert.AreEqual(0, aNode.Children.Count) - Assert.AreEqual(set [| 0 |], aNode.Files) - - let bNode = yNode.Children["B"] - Assert.AreEqual(0, bNode.Children.Count) - Assert.AreEqual(set [| 1 |], bNode.Files) - + ignore trie + + // match trie.Current with + // | TrieNodeInfo.Root _ -> () + // | current -> Assert.Fail($"mkTrie should always return a TrieNodeInfo.Root, got {current}") + // + // let xNode = trie.Children.["X"] + // Assert.AreEqual(1, xNode.Children.Count) + // Assert.True(Seq.isEmpty xNode.Files) + // + // let yNode = xNode.Children["Y"] + // Assert.AreEqual(2, yNode.Children.Count) + // Assert.AreEqual(set [| 2 |], yNode.Files) + // + // let aNode = yNode.Children["A"] + // Assert.AreEqual(0, aNode.Children.Count) + // Assert.AreEqual(set [| 0 |], aNode.Files) + // + // let bNode = yNode.Children["B"] + // Assert.AreEqual(0, bNode.Children.Count) + // Assert.AreEqual(set [| 1 |], bNode.Files) +(* [] let ``Toplevel AutoOpen module with prefixed namespace`` () = let trie = @@ -498,3 +499,4 @@ module C = begin end let aNode = trie.Children["A"] Assert.AreEqual(2, aNode.Children.Count) +*) \ No newline at end of file From 41f5b484cd39f476af43fcba0324a967b009e27e Mon Sep 17 00:00:00 2001 From: nojaf Date: Mon, 9 Oct 2023 17:48:42 +0200 Subject: [PATCH 2/7] Use Immutable structures for Trie types. Create incremental Trie instances. --- .../Driver/GraphChecking/TrieMapping.fs | 120 ++++++++--------- src/Compiler/Driver/GraphChecking/Types.fs | 15 ++- .../TypeChecks/Graph/QueryTrieTests.fs | 46 ++++--- .../TypeChecks/Graph/TrieMappingTests.fs | 124 ++++++++++++------ 4 files changed, 168 insertions(+), 137 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/TrieMapping.fs b/src/Compiler/Driver/GraphChecking/TrieMapping.fs index 32806275279..8e836332071 100644 --- a/src/Compiler/Driver/GraphChecking/TrieMapping.fs +++ b/src/Compiler/Driver/GraphChecking/TrieMapping.fs @@ -1,17 +1,17 @@ module internal FSharp.Compiler.GraphChecking.TrieMapping -open System.Collections.Generic +open System.Collections.Generic open System.Collections.Immutable open System.Text open FSharp.Compiler.IO open FSharp.Compiler.Syntax -// TODO: Deal with Immutable collections and benchmark whether they are worth it? - [] module private ImmutableHashSet = /// Create a new HashSet<'T> with a single element. - let singleton (value: 'T) = ImmutableHashSet.Create<'T>(Array.singleton value) + let singleton (value: 'T) = + ImmutableHashSet.Create<'T>(Array.singleton value) + /// Create new new HashSet<'T> with zero elements. let empty () = ImmutableHashSet.Empty @@ -61,61 +61,45 @@ let doesFileExposeContentToTheRoot (ast: ParsedInput) : bool = || kind = SynModuleOrNamespaceKind.GlobalNamespace) /// Merge all the accumulator Trie nodes into the current Trie node. -let mergeTrieNodes (accumulatorTrie: TrieNode) (currentTrie: TrieNode) : TrieNode = - /// 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 - filesDefiningNamespaceWithoutTypes = currentFilesWithoutTypes), - TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; filesDefiningNamespaceWithoutTypes = otherFilesWithoutTypes) -> - currentFilesThatExposeTypes.UnionWith otherFiles - currentFilesWithoutTypes.UnionWith otherFilesWithoutTypes - // Edge case scenario detected in https://github.com/dotnet/fsharp/issues/15985 - | TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes), TrieNodeInfo.Module (_name, file) -> - // Keep the namespace (as it can still have nested children). - currentFilesThatExposeTypes.Add file |> ignore - | TrieNodeInfo.Module (_name, file), TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes) -> - currentFilesThatExposeTypes.Add file |> ignore - // Replace the module in favour of the namespace (which can hold nested children). - root.Children[ k ] <- v - | _ -> () - - for kv in v.Children do - mergeTrieNodesAux node kv - - else - root.Children.Add(k, v) - - match accumulatorTrie.Current, currentTrie.Current with - | TrieNodeInfo.Root accFiles, TrieNodeInfo.Root currentFiles -> - - - for rootIndex in currentTrie.Files do - accFiles.Add rootIndex |> ignore - - for kv in currentTrie.Children do - mergeTrieNodesAux accumulatorTrie kv - - accumulatorTrie - | _ -> - System.Diagnostics.Debug.Assert( - false, - $"The top level node info of the accumulator and current trie should be Root, got {accumulatorTrie.Current} and {currentTrie.Current}" - ) +let rec mergeTrieNodes (accumulatorTrie: TrieNode) (currentTrie: TrieNode) : TrieNode = + let nextNodeInfo: TrieNodeInfo = + match accumulatorTrie.Current, currentTrie.Current with + | TrieNodeInfo.Root accFiles, TrieNodeInfo.Root currentFiles -> TrieNodeInfo.Root(accFiles.Union currentFiles) + | TrieNodeInfo.Namespace (name = name + filesThatExposeTypes = currentFilesThatExposeTypes + filesDefiningNamespaceWithoutTypes = currentFilesWithoutTypes), + TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; filesDefiningNamespaceWithoutTypes = otherFilesWithoutTypes) -> + TrieNodeInfo.Namespace( + name, + currentFilesThatExposeTypes.Union otherFiles, + currentFilesWithoutTypes.Union otherFilesWithoutTypes + ) + // Edge case scenario detected in https://github.com/dotnet/fsharp/issues/15985 + // Keep the namespace (as it can still have nested children). + | TrieNodeInfo.Namespace (name, currentFilesThatExposeTypes, filesDefiningNamespaceWithoutTypes), TrieNodeInfo.Module (_name, file) + // Replace the module in favour of the namespace (which can hold nested children). + | TrieNodeInfo.Module (_name, file), TrieNodeInfo.Namespace (name, currentFilesThatExposeTypes, filesDefiningNamespaceWithoutTypes) -> + TrieNodeInfo.Namespace(name, currentFilesThatExposeTypes.Add file, filesDefiningNamespaceWithoutTypes) + | _ -> accumulatorTrie.Current + + let nextChildren = + (accumulatorTrie.Children, currentTrie.Children) + ||> Seq.fold (fun accChildren (KeyValue (k, v)) -> + if not (accChildren.ContainsKey k) then + accChildren.Add(k, v) + else + let accNode = accChildren[k] + accChildren.SetItem(k, mergeTrieNodes accNode v)) - accumulatorTrie + { + Current = nextNodeInfo + Children = nextChildren + } -let mkImmutableDictFromKeyValuePairs (items: KeyValuePair<'TKey, 'TValue> list) = - ImmutableDictionary.CreateRange(items) +let mkImmutableDictFromKeyValuePairs (items: KeyValuePair<'TKey, 'TValue> list) = ImmutableDictionary.CreateRange(items) let mkSingletonDict key value = - let dict = Dictionary(1) - dict.Add(key, value) - dict + ImmutableDictionary.Empty.Add(key, value) /// Process a top level SynModuleOrNamespace(Sig) let processSynModuleOrNamespace<'Decl> @@ -151,9 +135,9 @@ let processSynModuleOrNamespace<'Decl> if isNamespace then let filesThatExposeTypes, filesDefiningNamespaceWithoutTypes = if hasTypesOrAutoOpenNestedModules then - HashSet.singleton idx, HashSet.empty () + ImmutableHashSet.singleton idx, ImmutableHashSet.empty () else - HashSet.empty (), HashSet.singleton idx + ImmutableHashSet.empty (), ImmutableHashSet.singleton idx TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes) else @@ -186,10 +170,10 @@ let processSynModuleOrNamespace<'Decl> let topLevelModuleOrNamespaceHasAutoOpen = isAnyAttributeAutoOpen attributes if topLevelModuleOrNamespaceHasAutoOpen && not isNamespace then - HashSet.singleton idx, HashSet.empty () + ImmutableHashSet.singleton idx, ImmutableHashSet.empty () else - HashSet.empty (), HashSet.singleton idx - | _ -> HashSet.empty (), HashSet.singleton idx + ImmutableHashSet.empty (), ImmutableHashSet.singleton idx + | _ -> ImmutableHashSet.empty (), ImmutableHashSet.singleton idx let current = TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes) @@ -199,12 +183,14 @@ let processSynModuleOrNamespace<'Decl> if kind = SynModuleOrNamespaceKind.AnonModule then // We collect the child nodes from the decls - decls |> List.choose (mkTrieForDeclaration idx) |> mkImmutableDictFromKeyValuePairs + decls + |> List.choose (mkTrieForDeclaration idx) + |> mkImmutableDictFromKeyValuePairs else visit id name { - Current = Root(HashSet.empty ()) + Current = Root(ImmutableHashSet.empty ()) Children = children } @@ -215,7 +201,7 @@ let rec mkTrieNodeFor (file: FileInProject) : FileIndex * TrieNode = // If a file exposes content which does not need an open statement to access, we consider the file to be part of the root. idx, { - Current = Root (ImmutableHashSet.singleton idx) + Current = Root(ImmutableHashSet.singleton idx) Children = ImmutableDictionary.Empty } else @@ -312,10 +298,8 @@ let mkTrie (files: FileInProject array) : (FileIndex * TrieNode) array = |> Array.Parallel.map mkTrieNodeFor |> Array.scan (fun (_, acc) (idx, current) -> - // We first need to create a copy of the Trie as Children are inside a mutable Dictionary. - let accCopy = mergeTrieNodes TrieNode.Empty acc - // Next we add the data of the current file to the copy. - idx, mergeTrieNodes accCopy current) + let next = mergeTrieNodes acc current + idx, next) (System.Int32.MinValue, TrieNode.Empty) // We can ignore the initial state that was used in the scan |> Array.skip 1 @@ -335,7 +319,7 @@ let serializeToMermaid (path: string) (filesInProject: FileInProject array) (tri | Module (name, _) -> $"mod_{name}" | Namespace (name, _, _) -> $"ns_{name}" - let toBoxList (boxPos: MermaidBoxPos) (files: HashSet) = + let toBoxList (boxPos: MermaidBoxPos) (files: ImmutableHashSet) = let sb = StringBuilder() let orderedIndexes = Seq.sort files diff --git a/src/Compiler/Driver/GraphChecking/Types.fs b/src/Compiler/Driver/GraphChecking/Types.fs index 18d8fdf6ed6..4fbfe37cf2d 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fs +++ b/src/Compiler/Driver/GraphChecking/Types.fs @@ -1,6 +1,6 @@ namespace FSharp.Compiler.GraphChecking -open System.Collections.Generic +open System.Collections.Immutable open FSharp.Compiler.Syntax /// The index of a file inside a project. @@ -30,9 +30,12 @@ type internal FileInProject = /// Only when the namespace exposes types that could later be inferred. /// Children of a namespace don't automatically depend on each other for that reason type internal TrieNodeInfo = - | Root of files: HashSet + | Root of files: ImmutableHashSet | Module of name: Identifier * file: FileIndex - | Namespace of name: Identifier * filesThatExposeTypes: HashSet * filesDefiningNamespaceWithoutTypes: HashSet + | Namespace of + name: Identifier * + filesThatExposeTypes: ImmutableHashSet * + filesDefiningNamespaceWithoutTypes: ImmutableHashSet member x.Files: Set = match x with @@ -43,17 +46,17 @@ type internal TrieNodeInfo = type internal TrieNode = { Current: TrieNodeInfo - Children: Dictionary + Children: ImmutableDictionary } member x.Files = x.Current.Files static member Empty = - let rootFiles = HashSet(Seq.empty) + let rootFiles = ImmutableHashSet.Empty { Current = TrieNodeInfo.Root rootFiles - Children = Dictionary<_, _>(0) + Children = ImmutableDictionary.Empty } /// A significant construct found in the syntax tree of a file. diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs index 5c1f57fc6ed..ee9c92432b2 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs @@ -1,6 +1,7 @@ module TypeChecks.QueryTrieTests open System.Collections.Generic +open System.Collections.Immutable open NUnit.Framework open FSharp.Compiler.GraphChecking open FSharp.Compiler.GraphChecking.DependencyResolution @@ -614,15 +615,12 @@ let private files = |] let dictionary<'key, 'value when 'key: equality> (entries: ('key * 'value) seq) = - let dict = Dictionary(Seq.length entries) + entries + |> Seq.map KeyValuePair + |> ImmutableDictionary.CreateRange - for k, v in entries do - dict.Add(k, v) - - dict - -let private noChildren = Dictionary(0) -let emptyHS () = HashSet(0) +let private noChildren = ImmutableDictionary.Empty +let emptyHS () = ImmutableHashSet.Empty let indexOf name = Array.find (fun (fc: FileContent) -> fc.FileName = name) files |> fun fc -> fc.Idx @@ -653,14 +651,14 @@ let private fantomasCoreTrie: TrieNode = TrieNodeInfo.Namespace( "Fantomas", emptyHS (), - HashSet([| - indexOf "ISourceTextExtensions.fs" - indexOf "RangeHelpers.fs" - indexOf "AstExtensions.fs" - indexOf "TriviaTypes.fs" - indexOf "Utils.fs" - indexOf "SourceParser.fs" - |])) + ImmutableHashSet.CreateRange [| + indexOf "ISourceTextExtensions.fs" + indexOf "RangeHelpers.fs" + indexOf "AstExtensions.fs" + indexOf "TriviaTypes.fs" + indexOf "Utils.fs" + indexOf "SourceParser.fs" + |]) Children = dictionary [| @@ -670,14 +668,14 @@ let private fantomasCoreTrie: TrieNode = TrieNodeInfo.Namespace( "Core", emptyHS (), - HashSet([| - indexOf "ISourceTextExtensions.fs" - indexOf "RangeHelpers.fs" - indexOf "AstExtensions.fs" - indexOf "TriviaTypes.fs" - indexOf "Utils.fs" - indexOf "SourceParser.fs" - |])) + ImmutableHashSet.CreateRange [| + 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/TrieMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs index 94685ab7464..4a3ddf6a7bd 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs @@ -6,6 +6,8 @@ open TestUtils let private noDependencies = Set.empty +let private getLastTrie files = TrieMapping.mkTrie files |> Array.last |> snd + [] let ``Basic trie`` () = let sampleFiles = @@ -27,6 +29,10 @@ let b = [] namespace X.Y type C = { CX: int; CY: int } + """ + "D.fs", + """ +module D """ |] @@ -39,33 +45,32 @@ type C = { CX: int; CY: int } ParsedInput = parseSourceCode (fileName, code) } : FileInProject) - let trie = TrieMapping.mkTrie files - ignore trie - - // match trie.Current with - // | TrieNodeInfo.Root _ -> () - // | current -> Assert.Fail($"mkTrie should always return a TrieNodeInfo.Root, got {current}") - // - // let xNode = trie.Children.["X"] - // Assert.AreEqual(1, xNode.Children.Count) - // Assert.True(Seq.isEmpty xNode.Files) - // - // let yNode = xNode.Children["Y"] - // Assert.AreEqual(2, yNode.Children.Count) - // Assert.AreEqual(set [| 2 |], yNode.Files) - // - // let aNode = yNode.Children["A"] - // Assert.AreEqual(0, aNode.Children.Count) - // Assert.AreEqual(set [| 0 |], aNode.Files) - // - // let bNode = yNode.Children["B"] - // Assert.AreEqual(0, bNode.Children.Count) - // Assert.AreEqual(set [| 1 |], bNode.Files) -(* + let trie = getLastTrie files + + match trie.Current with + | TrieNodeInfo.Root _ -> () + | current -> Assert.Fail($"mkTrie should always return a TrieNodeInfo.Root, got {current}") + + let xNode = trie.Children.["X"] + Assert.AreEqual(1, xNode.Children.Count) + Assert.True(Seq.isEmpty xNode.Files) + + let yNode = xNode.Children["Y"] + Assert.AreEqual(2, yNode.Children.Count) + Assert.AreEqual(set [| 2 |], yNode.Files) + + let aNode = yNode.Children["A"] + Assert.AreEqual(0, aNode.Children.Count) + Assert.AreEqual(set [| 0 |], aNode.Files) + + let bNode = yNode.Children["B"] + Assert.AreEqual(0, bNode.Children.Count) + Assert.AreEqual(set [| 1 |], bNode.Files) + [] let ``Toplevel AutoOpen module with prefixed namespace`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -92,7 +97,7 @@ let a = 0 [] let ``Toplevel AutoOpen module with multi prefixed namespace`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -121,7 +126,7 @@ let a = 0 [] let ``Global namespace should link files to the root node`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -149,6 +154,12 @@ type B = { Y : int } """ ) } + { + Idx = 2 + FileName = "B.fs" + // The last file shouldn't be processed + ParsedInput = Unchecked.defaultof + } |] Assert.AreEqual(set [| 0; 1 |], trie.Files) @@ -156,7 +167,7 @@ type B = { Y : int } [] let ``Module with a single ident and AutoOpen attribute should link files to root`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -186,6 +197,12 @@ type B = { Y : int } """ ) } + { + Idx = 2 + FileName = "B.fs" + // The last file shouldn't be processed + ParsedInput = Unchecked.defaultof + } |] Assert.AreEqual(set [| 0; 1 |], trie.Files) @@ -194,7 +211,7 @@ type B = { Y : int } [] let ``Module with AutoOpen attribute and two ident should expose file at two levels`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -221,7 +238,7 @@ type A = { A : int } [] let ``Module with AutoOpen attribute and three ident should expose file at last two levels`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -250,7 +267,7 @@ type A = { A : int } [] let ``Nested AutoOpen module in namespace will expose the file to the namespace node`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -281,7 +298,7 @@ module Z = [] let ``Two modules with the same name, only the first file exposes the index`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -318,7 +335,7 @@ let _ = () [] let ``Two nested modules with the same name, in named namespace`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -345,7 +362,7 @@ module ``module`` = [] let ``Two nested modules with the same name, in namespace global`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -371,7 +388,7 @@ module ``module`` = [] let ``Two nested modules with the same name, in anonymous module`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 1 @@ -395,7 +412,7 @@ module ``module`` = [] let ``Two nested modules with the same name, in nested module`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -424,7 +441,7 @@ module B = [] let ``Two nested modules with the same name, in nested module in signature file`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -451,7 +468,7 @@ module B = [] let ``Two namespaces with the same name in the same implementation file`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -477,7 +494,7 @@ module C = begin end [] let ``Two namespaces with the same name in the same signature file`` () = let trie = - TrieMapping.mkTrie + getLastTrie [| { Idx = 0 @@ -499,4 +516,33 @@ module C = begin end let aNode = trie.Children["A"] Assert.AreEqual(2, aNode.Children.Count) -*) \ No newline at end of file + +[] +let ``Tries are built up incrementally`` () = + let trie = + TrieMapping.mkTrie + [| + { + Idx = 0 + FileName = "A.fs" + ParsedInput = parseSourceCode ("A.fs", "module A") + } + { + Idx = 1 + FileName = "B.fs" + ParsedInput = parseSourceCode ("B.fs", "module B") + } + { + Idx = 2 + FileName = "C.fs" + ParsedInput = parseSourceCode ("C.fs", "module C") + } + { + Idx = 3 + FileName = "D.fs" + ParsedInput = parseSourceCode ("D.fs", "module D") + } + |] + + for idx, t in trie do + Assert.AreEqual(idx + 1, t.Children.Count) From 6a9fbc4792741ce99eba730d4462ce543c59c555 Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 10 Oct 2023 09:26:07 +0200 Subject: [PATCH 3/7] Format DependencyResolution.fs --- 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 688138fe97d..d4a94af8b43 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -209,7 +209,7 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph Array.fold (fun acc (idx, t) -> if idx < file.Idx then t else acc) TrieNode.Empty ignore trieForFile - + // File depends on all files above it that define accessible symbols at the root level (global namespace). let filesFromRoot = trieForFile.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx) From 6c339a84ad39247db1d512af5c08cf6eb6462f4a Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 10 Oct 2023 09:35:46 +0200 Subject: [PATCH 4/7] Remove whitespace. --- .../TypeChecks/Graph/TrieMappingTests.fs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs index 4a3ddf6a7bd..e5bc3ef0299 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/TrieMappingTests.fs @@ -30,10 +30,7 @@ namespace X.Y type C = { CX: int; CY: int } """ - "D.fs", - """ -module D - """ + "D.fs", "module D" |] let files = @@ -50,19 +47,19 @@ module D match trie.Current with | TrieNodeInfo.Root _ -> () | current -> Assert.Fail($"mkTrie should always return a TrieNodeInfo.Root, got {current}") - + let xNode = trie.Children.["X"] Assert.AreEqual(1, xNode.Children.Count) Assert.True(Seq.isEmpty xNode.Files) - + let yNode = xNode.Children["Y"] Assert.AreEqual(2, yNode.Children.Count) Assert.AreEqual(set [| 2 |], yNode.Files) - + let aNode = yNode.Children["A"] Assert.AreEqual(0, aNode.Children.Count) Assert.AreEqual(set [| 0 |], aNode.Files) - + let bNode = yNode.Children["B"] Assert.AreEqual(0, bNode.Children.Count) Assert.AreEqual(set [| 1 |], bNode.Files) From c52d555d8f54c339d2216a9384dc0a17fedd8cc8 Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 10 Oct 2023 13:23:31 +0200 Subject: [PATCH 5/7] Remove `]` --- .../TypeChecks/Graph/Scenarios.fs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs index f971c8dd876..acf321211d0 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/Scenarios.fs @@ -772,5 +772,4 @@ type DiGraph = obj """ (set [| 0 |]) ] - ] -] \ No newline at end of file + ] \ No newline at end of file From ef3a2a732a8435466f20e038f6cc726093769355 Mon Sep 17 00:00:00 2001 From: nojaf Date: Tue, 10 Oct 2023 14:50:11 +0200 Subject: [PATCH 6/7] Remove ignore calls. --- src/Compiler/Driver/GraphChecking/DependencyResolution.fs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index d4a94af8b43..5ff139ea77f 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -185,7 +185,6 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph Some f) let trie = TrieMapping.mkTrie trieInput - ignore trie let fileContents = files @@ -208,8 +207,6 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph Array.fold (fun acc (idx, t) -> if idx < file.Idx then t else acc) TrieNode.Empty - ignore trieForFile - // File depends on all files above it that define accessible symbols at the root level (global namespace). let filesFromRoot = trieForFile.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx) From d48da09a46cc0b633b2c5433193513cd29b2988a Mon Sep 17 00:00:00 2001 From: nojaf Date: Thu, 12 Oct 2023 17:12:46 +0200 Subject: [PATCH 7/7] Don't take the current file index into account for the FileContentQueryState. --- .../GraphChecking/DependencyResolution.fs | 30 ++++++++----------- .../GraphChecking/DependencyResolution.fsi | 3 +- src/Compiler/Driver/GraphChecking/Types.fs | 4 +-- src/Compiler/Driver/GraphChecking/Types.fsi | 5 ++-- .../TypeChecks/Graph/QueryTrieTests.fs | 13 +++----- 5 files changed, 20 insertions(+), 35 deletions(-) diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs index 5ff139ea77f..243d5fb1772 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fs +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fs @@ -17,33 +17,29 @@ let queryTriePartial (trie: TrieNode) (path: LongIdentifier) : TrieNode option = visit trie path -let mapNodeToQueryResult (currentFileIndex: FileIndex) (node: TrieNode option) : QueryTrieNodeResult = +let mapNodeToQueryResult (node: TrieNode option) : QueryTrieNodeResult = match node with | Some finalNode -> - if - Set.isEmpty finalNode.Files - // If this node exposes files which the current index cannot see, we consider it not to have data at all. - || Set.forall (fun idx -> idx >= currentFileIndex) finalNode.Files - then + if Set.isEmpty finalNode.Files then QueryTrieNodeResult.NodeDoesNotExposeData else QueryTrieNodeResult.NodeExposesData(finalNode.Files) | None -> QueryTrieNodeResult.NodeDoesNotExist /// Find a path in the Trie. -let queryTrie (currentFileIndex: FileIndex) (trie: TrieNode) (path: LongIdentifier) : QueryTrieNodeResult = - queryTriePartial trie path |> mapNodeToQueryResult currentFileIndex +let queryTrie (trie: TrieNode) (path: LongIdentifier) : QueryTrieNodeResult = + queryTriePartial trie path |> mapNodeToQueryResult /// Same as 'queryTrie' but allows passing in a path combined from two parts, avoiding list allocation. -let queryTrieDual (currentFileIndex: FileIndex) (trie: TrieNode) (path1: LongIdentifier) (path2: LongIdentifier) : QueryTrieNodeResult = +let queryTrieDual (trie: TrieNode) (path1: LongIdentifier) (path2: LongIdentifier) : QueryTrieNodeResult = match queryTriePartial trie path1 with | Some intermediateNode -> queryTriePartial intermediateNode path2 | None -> None - |> mapNodeToQueryResult currentFileIndex + |> mapNodeToQueryResult /// Process namespace declaration. let processNamespaceDeclaration (trie: TrieNode) (path: LongIdentifier) (state: FileContentQueryState) : FileContentQueryState = - let queryResult = queryTrie state.CurrentFile trie path + let queryResult = queryTrie trie path match queryResult with | QueryTrieNodeResult.NodeDoesNotExist -> state @@ -53,7 +49,7 @@ let processNamespaceDeclaration (trie: TrieNode) (path: LongIdentifier) (state: /// Process an "open" statement. /// The statement could link to files and/or should be tracked as an open namespace. let processOpenPath (trie: TrieNode) (path: LongIdentifier) (state: FileContentQueryState) : FileContentQueryState = - let queryResult = queryTrie state.CurrentFile trie path + let queryResult = queryTrie trie path match queryResult with | QueryTrieNodeResult.NodeDoesNotExist -> state @@ -103,13 +99,12 @@ let rec processStateEntry (trie: TrieNode) (state: FileContentQueryState) (entry ||> Array.fold (fun state takeParts -> let path = List.take takeParts path // process the name was if it were a FQN - let stateAfterFullIdentifier = - processIdentifier (queryTrieDual state.CurrentFile trie [] path) state + let stateAfterFullIdentifier = processIdentifier (queryTrieDual trie [] path) state // Process the name in combination with the existing open namespaces (stateAfterFullIdentifier, state.OpenNamespaces) ||> Set.fold (fun acc openNS -> - let queryResult = queryTrieDual state.CurrentFile trie openNS path + let queryResult = queryTrieDual trie openNS path processIdentifier queryResult acc)) | FileContentEntry.NestedModule (nestedContent = nestedContent) -> @@ -142,7 +137,7 @@ let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (result: Fi // 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 fileIndex trie path with + match queryTrie trie path with | QueryTrieNodeResult.NodeExposesData _ | QueryTrieNodeResult.NodeDoesNotExist -> None | QueryTrieNodeResult.NodeDoesNotExposeData -> @@ -211,8 +206,7 @@ let mkGraph (filePairs: FilePairMap) (files: FileInProject array) : Graph Set.filter (fun rootIdx -> rootIdx < file.Idx) // Start by listing root-level dependencies. - let initialDepsResult = - (FileContentQueryState.Create file.Idx filesFromRoot), fileContent + let initialDepsResult = (FileContentQueryState.Create filesFromRoot), fileContent // Sequentially process all relevant entries of the file and keep updating the state and set of dependencies. let depsResult = initialDepsResult diff --git a/src/Compiler/Driver/GraphChecking/DependencyResolution.fsi b/src/Compiler/Driver/GraphChecking/DependencyResolution.fsi index a2c52adcea9..8804999117e 100644 --- a/src/Compiler/Driver/GraphChecking/DependencyResolution.fsi +++ b/src/Compiler/Driver/GraphChecking/DependencyResolution.fsi @@ -3,10 +3,9 @@ module internal FSharp.Compiler.GraphChecking.DependencyResolution /// /// Query a TrieNode to find a certain path. -/// The result will take the current file index into account to determine if the result node contains data. /// /// This code is only used directly in unit tests. -val queryTrie: currentFileIndex: FileIndex -> trie: TrieNode -> path: LongIdentifier -> QueryTrieNodeResult +val queryTrie: trie: TrieNode -> path: LongIdentifier -> QueryTrieNodeResult /// Process an open path (found in the ParsedInput) with a given FileContentQueryState. /// This code is only used directly in unit tests. diff --git a/src/Compiler/Driver/GraphChecking/Types.fs b/src/Compiler/Driver/GraphChecking/Types.fs index 4fbfe37cf2d..72370ed41d0 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fs +++ b/src/Compiler/Driver/GraphChecking/Types.fs @@ -86,15 +86,13 @@ type internal FileContentQueryState = OwnNamespace: LongIdentifier option OpenedNamespaces: Set FoundDependencies: Set - CurrentFile: FileIndex } - static member Create (fileIndex: FileIndex) (filesAtRoot: Set) = + static member Create(filesAtRoot: Set) = { OwnNamespace = None OpenedNamespaces = Set.empty FoundDependencies = filesAtRoot - CurrentFile = fileIndex } member x.AddOwnNamespace(ns: LongIdentifier, ?files: Set) = diff --git a/src/Compiler/Driver/GraphChecking/Types.fsi b/src/Compiler/Driver/GraphChecking/Types.fsi index 9a642413d40..468ef65889c 100644 --- a/src/Compiler/Driver/GraphChecking/Types.fsi +++ b/src/Compiler/Driver/GraphChecking/Types.fsi @@ -77,10 +77,9 @@ type internal FileContent = type internal FileContentQueryState = { OwnNamespace: LongIdentifier option OpenedNamespaces: Set - FoundDependencies: Set - CurrentFile: FileIndex } + FoundDependencies: Set } - static member Create: fileIndex: FileIndex -> filesAtRoot: Set -> FileContentQueryState + static member Create: filesAtRoot: Set -> FileContentQueryState member AddOwnNamespace: ns: LongIdentifier * ?files: Set -> FileContentQueryState member AddDependencies: files: Set -> FileContentQueryState member AddOpenNamespace: path: LongIdentifier * ?files: Set -> FileContentQueryState diff --git a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs index ee9c92432b2..e6fd44a3505 100644 --- a/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs +++ b/tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs @@ -760,7 +760,7 @@ let private fantomasCoreTrie: TrieNode = [] let ``Query non existing node in trie`` () = let result = - queryTrie 7 fantomasCoreTrie [ "System"; "System"; "Runtime"; "CompilerServices" ] + queryTrie fantomasCoreTrie [ "System"; "System"; "Runtime"; "CompilerServices" ] match result with | QueryTrieNodeResult.NodeDoesNotExist -> Assert.Pass() @@ -768,7 +768,7 @@ let ``Query non existing node in trie`` () = [] let ``Query node that does not expose data in trie`` () = - let result = queryTrie 7 fantomasCoreTrie [ "Fantomas"; "Core" ] + let result = queryTrie fantomasCoreTrie [ "Fantomas"; "Core" ] match result with | QueryTrieNodeResult.NodeDoesNotExposeData -> Assert.Pass() @@ -777,7 +777,7 @@ let ``Query node that does not expose data in trie`` () = [] let ``Query module node that exposes one file`` () = let result = - queryTrie 7 fantomasCoreTrie [ "Fantomas"; "Core"; "ISourceTextExtensions" ] + queryTrie fantomasCoreTrie [ "Fantomas"; "Core"; "ISourceTextExtensions" ] match result with | QueryTrieNodeResult.NodeExposesData file -> @@ -787,13 +787,8 @@ let ``Query module node that exposes one file`` () = [] let ``ProcessOpenStatement full path match`` () = - let sourceParser = - Array.find (fun (f: FileContent) -> f.FileName = "SourceParser.fs") files - let state = - FileContentQueryState.Create - sourceParser.Idx - Set.empty + FileContentQueryState.Create Set.empty let result = processOpenPath fantomasCoreTrie [ "Fantomas"; "Core"; "AstExtensions" ] state