Skip to content

Invalid ghost dependency #15130

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Apr 24, 2023
Merged

Invalid ghost dependency #15130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed1c500
Keep track of connected files in namespace.
nojaf Apr 21, 2023
dae60b0
Don't resolve ghost dependency when it is already covered by a real one.
nojaf Apr 21, 2023
183e6c8
Don't process the first file as it cannot have any dependencies.
nojaf Apr 21, 2023
5fc3092
Merge branch 'main' into invalid-ghost-dependency
nojaf Apr 24, 2023
2947b61
Address feedback from review.
nojaf Apr 24, 2023
d9b00cd
Improve comment.
nojaf Apr 24, 2023
fabc72f
Merge branch 'main' into invalid-ghost-dependency
nojaf Apr 24, 2023
ffff7d1
Trigger CI
nojaf Apr 24, 2023
09c8876
Unsure if this PR will break the flow, It seems stuck, but I'll doubl…
nojaf Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 85 additions & 93 deletions src/Compiler/Driver/GraphChecking/DependencyResolution.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<FileIndex> =
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)

/// <summary>
/// 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.
Expand All @@ -136,16 +120,16 @@ let filesInTrie (node: TrieNode) : Set<FileIndex> =
/// - 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.</para>
/// <para>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.</para>
/// 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.</para>
/// </remarks>
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.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 =
Expand All @@ -156,25 +140,23 @@ 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 (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.
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<FileIndex> =
// We know that implementation files backed by signatures cannot be depended upon.
Expand All @@ -190,61 +172,71 @@ 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 =
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)
Expand Down
35 changes: 20 additions & 15 deletions src/Compiler/Driver/GraphChecking/TrieMapping.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +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 = currentFiles), TrieNodeInfo.Namespace (filesThatExposeTypes = otherFiles) ->
for otherFile in otherFiles do
currentFiles.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
Expand Down Expand Up @@ -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, filesDefiningNamespaceWithoutTypes =
if hasTypesOrAutoOpenNestedModules then
HashSet.singleton idx, HashSet.empty ()
else
HashSet.empty (), HashSet.singleton idx

TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes)
else
TrieNodeInfo.Module(name, idx)

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

visit
(fun node ->
let files =
let filesThatExposeTypes, filesDefiningNamespaceWithoutTypes =
match tail with
| [ _ ] ->
// In case you have:
Expand All @@ -179,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)
let current =
TrieNodeInfo.Namespace(name, filesThatExposeTypes, filesDefiningNamespaceWithoutTypes)

mkSingletonDict name { Current = current; Children = node } |> continuation)
tail
Expand Down
2 changes: 1 addition & 1 deletion src/Compiler/Driver/GraphChecking/Types.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type internal FileInProject =
type internal TrieNodeInfo =
| Root of files: HashSet<FileIndex>
| Module of name: Identifier * file: FileIndex
| Namespace of name: Identifier * filesThatExposeTypes: HashSet<FileIndex>
| Namespace of name: Identifier * filesThatExposeTypes: HashSet<FileIndex> * filesDefiningNamespaceWithoutTypes: HashSet<FileIndex>

member x.Files: Set<FileIndex> =
match x with
Expand Down
7 changes: 6 additions & 1 deletion src/Compiler/Driver/GraphChecking/Types.fsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ type internal FileInProject =
type internal TrieNodeInfo =
| Root of files: HashSet<FileIndex>
| Module of name: Identifier * file: FileIndex
| Namespace of name: Identifier * filesThatExposeTypes: HashSet<FileIndex>
| Namespace of
name: Identifier *
/// Files that expose types that are part of this namespace.
filesThatExposeTypes: HashSet<FileIndex> *
/// Files that use this namespace but don't contain any types.
filesDefiningNamespaceWithoutTypes: HashSet<FileIndex>

member Files: Set<FileIndex>

Expand Down
28 changes: 25 additions & 3 deletions tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/QueryTrieTests.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,7 +636,7 @@ let private fantomasCoreTrie: TrieNode =
[|
"System",
{
Current = TrieNodeInfo.Namespace("System", emptyHS ())
Current = TrieNodeInfo.Namespace("System", emptyHS (), emptyHS ())
Children =
dictionary
[|
Expand All @@ -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
[|
Expand Down
Loading