Invalid ghost dependency

[nojaf]

When testing out --test:GraphBasedChecking on demystifyfp/FsToolkit.ErrorHandling we noticed a problem with "ghost dependencies" while the graph is being created.

Many files have open FsToolkit.ErrorHandling (which is to open the main project and nothing from the current test project).

For example Option.fs

module OptionTests

open System
#if FABLE_COMPILER
open Fable.Mocha
#else
open Expecto
#endif
open SampleDomain
open TestData
open TestHelpers
open FsToolkit.ErrorHandling

We found a hit in the Trie because Main.fs.

module FsToolkit.ErrorHandling.Tests

#if FABLE_COMPILER
open Fable.Mocha
#else
open Expecto

[<Tests>] //needed for `dotnet test` to work
#endif
let allTests =
    testList "All Tests" [
        ResultTests.allTests
        ResultCETests.allTests
        ResultOptionTests.allTests
        OptionTests.allTests
        OptionCETests.allTests
        AsyncOptionTests.allTests
        AsyncOptionCETests.allTests
        ListTests.allTests
        SeqTests.allTests
        AsyncResultTests.allTests
        AsyncResultCETests.allTests
        AsyncResultOptionTests.allTests
        AsyncResultOptionCETests.allTests
        ValidationTests.allTests
        ValidationCETests.allTests
        ValueOptionTests.allTests
        ValueOptionCETests.allTests
    ]


[<EntryPoint>]
let main argv =
#if FABLE_COMPILER
    Mocha.runTests allTests
#else
    Tests.runTestsWithCLIArgs [] Array.empty allTests
#endif

Main.fs introduces namespace nodes FsToolkit and ErrorHandling but does not expose any types in these namespaces.

So after processing Option.fs we know we have an open statement that leads to a node in the Trie. But that found node didn't expose any files, so we didn't take any dependencies yet.
The current algorithm tries to find any file index in the Trie to satisfy the open statement leads somewhere in order to have valid code.

The problem with the current approach is that if we don't find any file index, as a last resort we link to all the files that came before it. And in the case of FsToolkit.ErrorHandling.Tests we were doing that almost for every file as the open statement both have a node in the Trie but is also referring to another assembly. And so in this PR, we removed that last resort check and resolve the file index by storing more information in the TrieNodeInfo.Namespace case.

This is the current graph:

flowchart RL
    0["obj\Release\net7.0\.NETCoreApp,Version=v7.0.AssemblyAttributes.fs"]
    1["obj\Release\net7.0\FsToolkit.ErrorHandling.Tests.AssemblyInfo.fs"]
    2["SampleDomain.fs"]
    3["TestData.fs"]
    4["Expect.fs"]
    5["Result.fs"]
    6["ResultCE.fs"]
    7["ResultOption.fs"]
    8["Option.fs"]
    9["OptionCE.fs"]
    10["ValueOption.fs"]
    11["ValueOptionCE.fs"]
    12["AsyncOption.fs"]
    13["AsyncOptionCE.fs"]
    14["List.fs"]
    15["Seq.fs"]
    16["AsyncResult.fs"]
    17["AsyncResultCE.fs"]
    18["AsyncResultOption.fs"]
    19["AsyncResultOptionCE.fs"]
    20["Validation.fs"]
    21["ValidationCE.fs"]
    22["Main.fs"]
    2 --> 0
    2 --> 1
    3 --> 2
    5 --> 2
    5 --> 3
    5 --> 4
    5 --> 0
    5 --> 1
    6 --> 2
    6 --> 3
    6 --> 4
    6 --> 0
    6 --> 1
    6 --> 5
    7 --> 2
    7 --> 3
    7 --> 4
    7 --> 0
    7 --> 1
    7 --> 5
    7 --> 6
    8 --> 2
    8 --> 3
    8 --> 4
    8 --> 0
    8 --> 1
    8 --> 5
    8 --> 6
    8 --> 7
    9 --> 0
    9 --> 1
    9 --> 2
    9 --> 3
    9 --> 4
    9 --> 5
    9 --> 6
    9 --> 7
    9 --> 8
    10 --> 2
    10 --> 3
    10 --> 4
    10 --> 0
    10 --> 1
    10 --> 5
    10 --> 6
    10 --> 7
    10 --> 8
    10 --> 9
    11 --> 2
    11 --> 3
    11 --> 4
    11 --> 0
    11 --> 1
    11 --> 5
    11 --> 6
    11 --> 7
    11 --> 8
    11 --> 9
    11 --> 10
    12 --> 2
    12 --> 3
    12 --> 4
    12 --> 0
    12 --> 1
    12 --> 5
    12 --> 6
    12 --> 7
    12 --> 8
    12 --> 9
    12 --> 10
    12 --> 11
    13 --> 2
    13 --> 3
    13 --> 4
    13 --> 0
    13 --> 1
    13 --> 5
    13 --> 6
    13 --> 7
    13 --> 8
    13 --> 9
    13 --> 10
    13 --> 11
    13 --> 12
    14 --> 2
    14 --> 3
    14 --> 4
    14 --> 0
    14 --> 1
    14 --> 5
    14 --> 6
    14 --> 7
    14 --> 8
    14 --> 9
    14 --> 10
    14 --> 11
    14 --> 12
    14 --> 13
    15 --> 2
    15 --> 3
    15 --> 4
    15 --> 0
    15 --> 1
    15 --> 5
    15 --> 6
    15 --> 7
    15 --> 8
    15 --> 9
    15 --> 10
    15 --> 11
    15 --> 12
    15 --> 13
    15 --> 14
    16 --> 2
    16 --> 3
    16 --> 4
    16 --> 0
    16 --> 1
    16 --> 5
    16 --> 6
    16 --> 7
    16 --> 8
    16 --> 9
    16 --> 10
    16 --> 11
    16 --> 12
    16 --> 13
    16 --> 14
    16 --> 15
    17 --> 2
    17 --> 3
    17 --> 4
    17 --> 0
    17 --> 1
    17 --> 5
    17 --> 6
    17 --> 7
    17 --> 8
    17 --> 9
    17 --> 10
    17 --> 11
    17 --> 12
    17 --> 13
    17 --> 14
    17 --> 15
    17 --> 16
    18 --> 2
    18 --> 4
    18 --> 0
    18 --> 1
    18 --> 3
    18 --> 5
    18 --> 6
    18 --> 7
    18 --> 8
    18 --> 9
    18 --> 10
    18 --> 11
    18 --> 12
    18 --> 13
    18 --> 14
    18 --> 15
    18 --> 16
    18 --> 17
    19 --> 2
    19 --> 3
    19 --> 4
    19 --> 0
    19 --> 1
    19 --> 5
    19 --> 6
    19 --> 7
    19 --> 8
    19 --> 9
    19 --> 10
    19 --> 11
    19 --> 12
    19 --> 13
    19 --> 14
    19 --> 15
    19 --> 16
    19 --> 17
    19 --> 18
    20 --> 2
    20 --> 3
    20 --> 4
    20 --> 0
    20 --> 1
    20 --> 5
    20 --> 6
    20 --> 7
    20 --> 8
    20 --> 9
    20 --> 10
    20 --> 11
    20 --> 12
    20 --> 13
    20 --> 14
    20 --> 15
    20 --> 16
    20 --> 17
    20 --> 18
    20 --> 19
    21 --> 2
    21 --> 3
    21 --> 4
    21 --> 0
    21 --> 1
    21 --> 5
    21 --> 6
    21 --> 7
    21 --> 8
    21 --> 9
    21 --> 10
    21 --> 11
    21 --> 12
    21 --> 13
    21 --> 14
    21 --> 15
    21 --> 16
    21 --> 17
    21 --> 18
    21 --> 19
    21 --> 20
    22 --> 5
    22 --> 6
    22 --> 7
    22 --> 8
    22 --> 9
    22 --> 10
    22 --> 11
    22 --> 12
    22 --> 13
    22 --> 14
    22 --> 15
    22 --> 16
    22 --> 17
    22 --> 18
    22 --> 19
    22 --> 20
    22 --> 21

Loading

And this is the graph after this PR:

flowchart RL
    0["obj\Release\net7.0\.NETCoreApp,Version=v7.0.AssemblyAttributes.fs"]
    1["obj\Release\net7.0\FsToolkit.ErrorHandling.Tests.AssemblyInfo.fs"]
    2["SampleDomain.fs"]
    3["TestData.fs"]
    4["Expect.fs"]
    5["Result.fs"]
    6["ResultCE.fs"]
    7["ResultOption.fs"]
    8["Option.fs"]
    9["OptionCE.fs"]
    10["ValueOption.fs"]
    11["ValueOptionCE.fs"]
    12["AsyncOption.fs"]
    13["AsyncOptionCE.fs"]
    14["List.fs"]
    15["Seq.fs"]
    16["AsyncResult.fs"]
    17["AsyncResultCE.fs"]
    18["AsyncResultOption.fs"]
    19["AsyncResultOptionCE.fs"]
    20["Validation.fs"]
    21["ValidationCE.fs"]
    22["Main.fs"]
    3 --> 2
    5 --> 2
    5 --> 3
    5 --> 4
    6 --> 2
    6 --> 3
    6 --> 4
    7 --> 2
    7 --> 3
    7 --> 4
    8 --> 2
    8 --> 3
    8 --> 4
    10 --> 2
    10 --> 3
    10 --> 4
    11 --> 2
    11 --> 3
    11 --> 4
    12 --> 2
    12 --> 3
    12 --> 4
    13 --> 2
    13 --> 3
    13 --> 4
    14 --> 2
    14 --> 3
    14 --> 4
    15 --> 2
    15 --> 3
    15 --> 4
    16 --> 2
    16 --> 3
    16 --> 4
    17 --> 2
    17 --> 3
    17 --> 4
    18 --> 2
    18 --> 4
    19 --> 2
    19 --> 3
    19 --> 4
    20 --> 2
    20 --> 3
    20 --> 4
    21 --> 2
    21 --> 3
    21 --> 4
    22 --> 5
    22 --> 6
    22 --> 7
    22 --> 8
    22 --> 9
    22 --> 10
    22 --> 11
    22 --> 12
    22 --> 13
    22 --> 14
    22 --> 15
    22 --> 16
    22 --> 17
    22 --> 18
    22 --> 19
    22 --> 20
    22 --> 21

Loading

We try to be smarter about resolving any opened namespace that didn't introduce any dependency.
Also, note that Main.fs comes Option.fs in the file order. We take this into account as well when processing the remaining open statements.

As a bonus, I've added a check to skip the first file from being processed. It can never depend on anything so we should do anything there.

Thanks @TheAngryByrd for testing out this new feature flag!

[nojaf]

@safesparrow could you please take a look at this PR? Thanks!

[safesparrow]

Changes look ok, with minor questions, and a few nitpicks.

I'll note that it took me a few reads of the description and looking at the code for a while to understand what the actual problem was.

Could we update the description to summarise:

• The problematic sample.
• What the problem is. Does it break anything, or just slow down compilation.
• Why is the dependency "invalid" - is it just unnecessary?

[nojaf]

Thanks for the review @safesparrow.
I've updated the PR description to better illustrate the problem.