Skip to content
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.

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

Process longId for SynTypeDefnKind.Augmentation. #15897

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Aug 29, 2023

In the code's embrace, types intertwine,
Augmenting forms, a dance so fine.
Processing identifiers, a coder's art,
New dimensions unfold, a work of heart.

A problem with --test:GraphBasedChecking was found by @TheAngryByrd in https://github.com/TheAngryByrd/IcedTasks/tree/graph-based-playground-1

A link between files was not detected for type extensions. Thus this leads to a consistent compiler error.

The fix of this PR was in the untyped AST processing we do to detect links between the identifiers found in a file and the project Trie. This wasn't overly complicated to fix and I'd like to write down what my methodology was.

First up, I cloned the branch and scrapped the compiler arguments for the project that was failing.
I used telplin for this, but I could have used the scrape.fsx file. In both cases, a build of the project happens and the binlog is processed to extract the compiler arguments of CoreCompile. (Yes, a design-time build would be the more clever thing to do, but let's not get carried away).

Having the compiler arguments or response file (*.rsp) can be used to debug the actual project in

[<TestCaseSource(nameof localProjects)>]
[<Explicit("This only runs with the explicitly mentioned projects above")>]
let ``Test graph-based type-checking`` (projectArgumentsFilePath: string) =
testCompilerFromArgs Method.Graph projectArgumentsFilePath

Running that sufficed, as it was clear that the problem still existed on the main branch.

The compiler error was pretty clear that a type was not known during the checking of a type extension.
So, I extracted a sample from the real project into a scenario:

scenario
            "Identifier in type augmentation can link files"
            [
                sourceFile
                    "PoolingValueTasks.fs"
                    """
namespace IcedTasks
module PoolingValueTasks =
    type PoolingValueTaskBuilderBase() =
        class
        end
"""
                    Set.empty
                sourceFile
                    "ColdTask.fs"
                    """
namespace IcedTasks
module ColdTasks =
    module AsyncExtensions =
        type PoolingValueTasks.PoolingValueTaskBuilderBase with
            member this.Source (a:int) = a
"""
                    (set [| 0 |])
            ]

Mimicking the problem, this was a lot easier to debug of course to see what was happening in FileContentMapping.fs.

Both tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/DependencyResolutionTests.fs and /tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/CompilationTests.fs were failing for the newly added scenario.

It soon became obvious that PoolingValueTasks.PoolingValueTaskBuilderBase should be processed as well because the combined link IceTasks.PoolingValueTasks.PoolingValueTaskBuilderBase does link a module in the Trie and thus introduces the link between the files.

Afterwards, I added another test for signature files. There the AST is surprisingly different for type extensions compared to implementation files. I'm not sure if that is by design or a bit of a gap.

Anyway, this is ready for review.

@nojaf nojaf marked this pull request as ready for review August 30, 2023 07:55
@nojaf nojaf requested a review from a team as a code owner August 30, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants