Take the current index into account in the QueryTrieNodeResult.

[nojaf]

When playing around with GBC in https://github.com/fslaborg/Graphoscope/blob/developer/src/Graphoscope/Graphoscope.fsproj I discovered another edge case.

Consider the following setup:

        scenario
            "Ghost dependency via top-level namespace"
            [
                sourceFile
                    "Graph.fs"
                    """
namespace Graphoscope.Graph
type UndirectedGraph = obj
"""
                    Set.empty
                sourceFile
                    "DiGraph.fs"
                    """
namespace Graphoscope
open Graphoscope
type DiGraph = obj 
"""
                    (set [| 0 |])
            ]

DiGraph.fs should depend on Graph.fs to satisfy the open Graphoscope.
The open statement can safely be removed in the actual code but is valid nonetheless given the file order.

The Trie:

classDiagram

class root

root <|-- ns_Graphoscope
class ns_Graphoscope {
    DiGraph.fs[1]

    Graph.fs(0)
}

ns_Graphoscope <|-- ns_Graph
class ns_Graph {
    Graph.fs[0]

}

Loading

DiGraph.fs adds data to ns_Graphoscope, but Graph.fs does not.
Out of necessity to resolve the open Graphoscope we need to establish that from DiGraph.fs point of view the node (ns_Graphoscope) does not expose data and thus the ghost dependency resolution should kick in.

In practice, the more obvious thing to do is of course to remove the unused open statement, but we will want to be able to type-check the project using GBC.

@safesparrow could you take a look at this one as well, please?

[safesparrow]

This fixes the described issue, so happy for it to be merged.

I found it a bit hard to understand what the actual problem is and how this change fixes it. It might be worth elaborating that on the PR or in the code comments.
As I understand, what happened is:

1. When a file references a namespace (rather than any symbols inside), by default we create a link to all the files that define that namespace and contain any inferable items inside it.
2. We avoid creating links to files that don't list any inferable items as a graph optimisation.
3. In the case that no files list any inferable items for a namespace, that namespace is still relevant for open x lines, but the main mechanism doesn't generate a link - we have a separate mechanism for ensuring that a link is created for these situations when needed, and we call such links "ghost dependencies".
4. The "ghost dependency" link adding is only enabled when we think we need it.
5. The problematic case is when a namespace does contain inferable items defined in some files, but all of those files are listed after the file referencing the namespace.
6. In the above case, old code doesn't add the "ghost dependency", because it thinks that some files "exposing data" for a namespace is enough, but it doesn't consider where in the compilation order those files are.
7. New code handles it by checking that at least one of the files "exposing data" can be "seen"/referenced by the file currently being scanned.

Please correct if that's wrong.

Similar to the other PR, to me this one indicates that the time to update/refactor the design might be soon - but less urgent than merging this.

[nojaf]

Yes, your explanation is correct. We should indeed at some point refactor this a little bit to take the ghost dependency problem better into account. For now, I would merge this as is.

[psfinaki]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).