Skip to content

Commit ab42a32

Browse files
nojafnojaf
authored
Invalid ghost dependency (#15130)
1 parent 0544c14 commit ab42a32

File tree

6 files changed

+176
-117
lines changed

6 files changed

+176
-117
lines changed

‎src/Compiler/Driver/GraphChecking/DependencyResolution.fs‎

Lines changed: 85 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -108,22 +108,6 @@ let rec processStateEntry (queryTrie: QueryTrie) (state: FileContentQueryState)
108108
FoundDependencies = foundDependencies
109109
}
110110

111-
/// Return all files contained in the trie.
112-
let filesInTrie (node: TrieNode) : Set<FileIndex> =
113-
let rec collect (node: TrieNode) (continuation: FileIndex list -> FileIndex list) : FileIndex list =
114-
let continuations: ((FileIndex list -> FileIndex list) -> FileIndex list) list =
115-
[
116-
for node in node.Children.Values do
117-
yield collect node
118-
]
119-
120-
let finalContinuation indexes =
121-
continuation [ yield! node.Files; yield! List.concat indexes ]
122-
123-
Continuation.sequence continuations finalContinuation
124-
125-
Set.ofList (collect node id)
126-
127111
/// <summary>
128112
/// For a given file's content, collect all missing ("ghost") file dependencies that the core resolution algorithm didn't return,
129113
/// but are required to satisfy the type-checker.
@@ -136,16 +120,16 @@ let filesInTrie (node: TrieNode) : Set<FileIndex> =
136120
/// - the namespace does not contain any children that can be referenced implicitly (eg. by type inference),
137121
/// then the main resolution algorithm does not create a link to any file defining the namespace.</para>
138122
/// <para>However, to satisfy the type-checker, the namespace must be resolved.
139-
/// This function returns a list of extra dependencies that makes sure that any such namespaces can be resolved (if it exists).
140-
/// For each unused open namespace we return one or more file links that define it.</para>
123+
/// This function returns an array with a potential extra dependencies that makes sure that any such namespaces can be resolved (if they exists).
124+
/// For each unused namespace `open` we return at most one file that defines that namespace.</para>
141125
/// </remarks>
142126
let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie: QueryTrie) (result: FileContentQueryState) =
143-
// Go over all open namespaces, and assert all those links eventually went anywhere
127+
// For each opened namespace, if none of already resolved dependencies define it, return the top-most file that defines it.
144128
Set.toArray result.OpenedNamespaces
145-
|> Array.collect (fun path ->
129+
|> Array.choose (fun path ->
146130
match queryTrie path with
147131
| QueryTrieNodeResult.NodeExposesData _
148-
| QueryTrieNodeResult.NodeDoesNotExist -> Array.empty
132+
| QueryTrieNodeResult.NodeDoesNotExist -> None
149133
| QueryTrieNodeResult.NodeDoesNotExposeData ->
150134
// At this point we are following up if an open namespace really lead nowhere.
151135
let node =
@@ -156,25 +140,23 @@ let collectGhostDependencies (fileIndex: FileIndex) (trie: TrieNode) (queryTrie:
156140

157141
find trie path
158142

159-
let filesDefiningNamespace =
160-
filesInTrie node |> Set.filter (fun idx -> idx < fileIndex)
161-
162-
let dependenciesDefiningNamespace =
163-
Set.intersect result.FoundDependencies filesDefiningNamespace
164-
165-
[|
166-
if Set.isEmpty dependenciesDefiningNamespace then
167-
// There is no existing dependency defining the namespace,
168-
// so we need to add one.
169-
if Set.isEmpty filesDefiningNamespace then
170-
// No file defines inferrable symbols for this namespace, but the namespace might exist.
171-
// Because we don't track what files define a namespace without any relevant content,
172-
// the only way to ensure the namespace is in scope is to add a link to every preceding file.
173-
yield! [| 0 .. (fileIndex - 1) |]
174-
else
175-
// At least one file defines the namespace - add a dependency to the first (top) one.
176-
yield Seq.head filesDefiningNamespace
177-
|])
143+
match node.Current with
144+
// Both Root and module would expose data, so we can ignore them.
145+
| Root _
146+
| Module _ -> None
147+
| Namespace (filesDefiningNamespaceWithoutTypes = filesDefiningNamespaceWithoutTypes) ->
148+
if filesDefiningNamespaceWithoutTypes.Overlaps(result.FoundDependencies) then
149+
// The ghost dependency is already covered by a real dependency.
150+
None
151+
else
152+
// We are only interested in any file that contained the namespace when they came before the current file.
153+
// If the namespace is defined in a file after the current file then there is no way the current file can reference it.
154+
// Which means that namespace would come from a different assembly.
155+
filesDefiningNamespaceWithoutTypes
156+
|> Seq.sort
157+
|> Seq.tryFind (fun connectedFileIdx ->
158+
// We pick the lowest file index from the namespace to satisfy the type-checker for the open statement.
159+
connectedFileIdx < fileIndex))
178160

179161
let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInProject array) : Graph<FileIndex> =
180162
// We know that implementation files backed by signatures cannot be depended upon.
@@ -190,61 +172,71 @@ let mkGraph (compilingFSharpCore: bool) (filePairs: FilePairMap) (files: FileInP
190172
let trie = TrieMapping.mkTrie trieInput
191173
let queryTrie: QueryTrie = queryTrieMemoized trie
192174

193-
let fileContents = files |> Array.Parallel.map FileContentMapping.mkFileContent
175+
let fileContents =
176+
files
177+
|> Array.Parallel.map (fun file ->
178+
if file.Idx = 0 then
179+
List.empty
180+
else
181+
FileContentMapping.mkFileContent file)
194182

195183
let findDependencies (file: FileInProject) : FileIndex array =
196-
let fileContent = fileContents[file.Idx]
197-
198-
let knownFiles = [ 0 .. (file.Idx - 1) ] |> set
199-
// File depends on all files above it that define accessible symbols at the root level (global namespace).
200-
let filesFromRoot = trie.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx)
201-
// Start by listing root-level dependencies.
202-
let initialDepsResult =
203-
(FileContentQueryState.Create file.Idx knownFiles filesFromRoot), fileContent
204-
// Sequentially process all relevant entries of the file and keep updating the state and set of dependencies.
205-
let depsResult =
206-
initialDepsResult
207-
// Seq is faster than List in this case.
208-
||> Seq.fold (processStateEntry queryTrie)
209-
210-
// Add missing links for cases where an unused open namespace did not create a link.
211-
let ghostDependencies = collectGhostDependencies file.Idx trie queryTrie depsResult
212-
213-
// Add a link from implementation files to their signature files.
214-
let signatureDependency =
215-
match filePairs.TryGetSignatureIndex file.Idx with
216-
| None -> Array.empty
217-
| Some sigIdx -> Array.singleton sigIdx
218-
219-
// Files in FSharp.Core have an implicit dependency on `prim-types-prelude.fsi` - add it.
220-
let fsharpCoreImplicitDependencies =
221-
let filename = "prim-types-prelude.fsi"
222-
223-
let implicitDepIdx =
224-
files
225-
|> Array.tryFindIndex (fun f -> FileSystemUtils.fileNameOfPath f.FileName = filename)
226-
227-
[|
228-
if compilingFSharpCore then
229-
match implicitDepIdx with
230-
| Some idx ->
231-
if file.Idx > idx then
232-
yield idx
233-
| None ->
234-
exn $"Expected to find file '{filename}' during compilation of FSharp.Core, but it was not found."
235-
|> raise
236-
|]
237-
238-
let allDependencies =
239-
[|
240-
yield! depsResult.FoundDependencies
241-
yield! ghostDependencies
242-
yield! signatureDependency
243-
yield! fsharpCoreImplicitDependencies
244-
|]
245-
|> Array.distinct
246-
247-
allDependencies
184+
if file.Idx = 0 then
185+
// First file cannot have any dependencies.
186+
Array.empty
187+
else
188+
let fileContent = fileContents[file.Idx]
189+
190+
let knownFiles = [ 0 .. (file.Idx - 1) ] |> set
191+
// File depends on all files above it that define accessible symbols at the root level (global namespace).
192+
let filesFromRoot = trie.Files |> Set.filter (fun rootIdx -> rootIdx < file.Idx)
193+
// Start by listing root-level dependencies.
194+
let initialDepsResult =
195+
(FileContentQueryState.Create file.Idx knownFiles filesFromRoot), fileContent
196+
// Sequentially process all relevant entries of the file and keep updating the state and set of dependencies.
197+
let depsResult =
198+
initialDepsResult
199+
// Seq is faster than List in this case.
200+
||> Seq.fold (processStateEntry queryTrie)
201+
202+
// Add missing links for cases where an unused open namespace did not create a link.
203+
let ghostDependencies = collectGhostDependencies file.Idx trie queryTrie depsResult
204+
205+
// Add a link from implementation files to their signature files.
206+
let signatureDependency =
207+
match filePairs.TryGetSignatureIndex file.Idx with
208+
| None -> Array.empty
209+
| Some sigIdx -> Array.singleton sigIdx
210+
211+
// Files in FSharp.Core have an implicit dependency on `prim-types-prelude.fsi` - add it.
212+
let fsharpCoreImplicitDependencies =
213+
let filename = "prim-types-prelude.fsi"
214+
215+
let implicitDepIdx =
216+
files
217+
|> Array.tryFindIndex (fun f -> FileSystemUtils.fileNameOfPath f.FileName = filename)
218+
219+
[|
220+
if compilingFSharpCore then
221+
match implicitDepIdx with
222+
| Some idx ->
223+
if file.Idx > idx then
224+
yield idx
225+
| None ->
226+
exn $"Expected to find file '{filename}' during compilation of FSharp.Core, but it was not found."
227+
|> raise
228+
|]
229+
230+
let allDependencies =
231+
[|
232+
yield! depsResult.FoundDependencies
233+
yield! ghostDependencies
234+
yield! signatureDependency
235+
yield! fsharpCoreImplicitDependencies
236+
|]
237+
|> Array.distinct
238+
239+
allDependencies
248240

249241
files
250242
|> Array.Parallel.map (fun file -> file.Idx, findDependencies file)

‎src/Compiler/Driver/GraphChecking/TrieMapping.fs‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,18 @@ let doesFileExposeContentToTheRoot (ast: ParsedInput) : bool =
5656
|| kind = SynModuleOrNamespaceKind.GlobalNamespace)
5757

5858
let mergeTrieNodes (defaultChildSize: int) (tries: TrieNode array) =
59+
/// Add the current node as child node to the root node.
60+
/// If the node already exists and is a namespace node, the existing node will be updated with new information via mutation.
5961
let rec mergeTrieNodesAux (root: TrieNode) (KeyValue (k, v)) =
6062
if root.Children.ContainsKey k then
6163
let node = root.Children[k]
6264

6365
match node.Current, v.Current with
64-
| TrieNodeInfo.Namespace (filesThatExposeTypes = currentFiles), TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles) ->
65-
for otherFile in otherFiles do
66-
currentFiles.Add(otherFile) |> ignore
66+
| TrieNodeInfo.Namespace (filesThatExposeTypes = currentFilesThatExposeTypes
67+
filesDefiningNamespaceWithoutTypes = currentFilesWithoutTypes),
68+
TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles; filesDefiningNamespaceWithoutTypes = otherFilesWithoutTypes) ->
69+
currentFilesThatExposeTypes.UnionWith otherFiles
70+
currentFilesWithoutTypes.UnionWith otherFilesWithoutTypes
6771
| _ -> ()
6872

6973
for kv in v.Children do
@@ -142,13 +146,13 @@ let processSynModuleOrNamespace<'Decl>
142146
// The reasoning is that a type could be inferred and a nested auto open module will lift its content one level up.
143147
let current =
144148
if isNamespace then
145-
TrieNodeInfo.Namespace(
146-
name,
147-
(if hasTypesOrAutoOpenNestedModules then
148-
HashSet.singleton idx
149-
else
150-
HashSet.empty ())
151-
)
149+
let filesThatExposeTypes, filesDefiningNamespaceWithoutTypes =
150+
if hasTypesOrAutoOpenNestedModules then
151+
HashSet.singleton idx, HashSet.empty ()
152+
else
153+
HashSet.empty (), HashSet.singleton idx
154+
155+
TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes)
152156
else
153157
TrieNodeInfo.Module(name, idx)
154158

@@ -167,7 +171,7 @@ let processSynModuleOrNamespace<'Decl>
167171

168172
visit
169173
(fun node ->
170-
let files =
174+
let filesThatExposeTypes, filesDefiningNamespaceWithoutTypes =
171175
match tail with
172176
| [ _ ] ->
173177
// In case you have:
@@ -179,12 +183,13 @@ let processSynModuleOrNamespace<'Decl>
179183
let topLevelModuleOrNamespaceHasAutoOpen = isAnyAttributeAutoOpen attributes
180184

181185
if topLevelModuleOrNamespaceHasAutoOpen && not isNamespace then
182-
HashSet.singleton idx
186+
HashSet.singleton idx, HashSet.empty ()
183187
else
184-
HashSet.empty ()
185-
| _ -> HashSet.empty ()
188+
HashSet.empty (), HashSet.singleton idx
189+
| _ -> HashSet.empty (), HashSet.singleton idx
186190

187-
let current = TrieNodeInfo.Namespace(name, files)
191+
let current =
192+
TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes)
188193

189194
mkSingletonDict name { Current = current; Children = node } |> continuation)
190195
tail

‎src/Compiler/Driver/GraphChecking/Types.fs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ type internal FileInProject =
3232
type internal TrieNodeInfo =
3333
| Root of files: HashSet<FileIndex>
3434
| Module of name: Identifier * file: FileIndex
35-
| Namespace of name: Identifier * filesThatExposeTypes: HashSet<FileIndex>
35+
| Namespace of name: Identifier * filesThatExposeTypes: HashSet<FileIndex> * filesDefiningNamespaceWithoutTypes: HashSet<FileIndex>
3636

3737
member x.Files: Set<FileIndex> =
3838
match x with

‎src/Compiler/Driver/GraphChecking/Types.fsi‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,12 @@ type internal FileInProject =
3030
type internal TrieNodeInfo =
3131
| Root of files: HashSet<FileIndex>
3232
| Module of name: Identifier * file: FileIndex
33-
| Namespace of name: Identifier * filesThatExposeTypes: HashSet<FileIndex>
33+
| Namespace of
34+
name: Identifier *
35+
/// Files that expose types that are part of this namespace.
36+
filesThatExposeTypes: HashSet<FileIndex> *
37+
/// Files that use this namespace but don't contain any types.
38+
filesDefiningNamespaceWithoutTypes: HashSet<FileIndex>
3439

3540
member Files: Set<FileIndex>
3641

‎tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs‎

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -636,7 +636,7 @@ let private fantomasCoreTrie: TrieNode =
636636
[|
637637
"System",
638638
{
639-
Current = TrieNodeInfo.Namespace("System", emptyHS ())
639+
Current = TrieNodeInfo.Namespace("System", emptyHS (), emptyHS ())
640640
Children =
641641
dictionary
642642
[|
@@ -649,13 +649,35 @@ let private fantomasCoreTrie: TrieNode =
649649
}
650650
"Fantomas",
651651
{
652-
Current = TrieNodeInfo.Namespace("Fantomas", emptyHS ())
652+
Current =
653+
TrieNodeInfo.Namespace(
654+
"Fantomas",
655+
emptyHS (),
656+
HashSet([|
657+
indexOf "ISourceTextExtensions.fs"
658+
indexOf "RangeHelpers.fs"
659+
indexOf "AstExtensions.fs"
660+
indexOf "TriviaTypes.fs"
661+
indexOf "Utils.fs"
662+
indexOf "SourceParser.fs"
663+
|]))
653664
Children =
654665
dictionary
655666
[|
656667
"Core",
657668
{
658-
Current = TrieNodeInfo.Namespace("Core", emptyHS ())
669+
Current =
670+
TrieNodeInfo.Namespace(
671+
"Core",
672+
emptyHS (),
673+
HashSet([|
674+
indexOf "ISourceTextExtensions.fs"
675+
indexOf "RangeHelpers.fs"
676+
indexOf "AstExtensions.fs"
677+
indexOf "TriviaTypes.fs"
678+
indexOf "Utils.fs"
679+
indexOf "SourceParser.fs"
680+
|]))
659681
Children =
660682
dictionary
661683
[|

0 commit comments

Comments
 (0)