Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Feb 12, 2026

Summary

Adds two F# projects to the regression test pipeline, and fixes a bug discovered through them in graph-based parallel type checking.

Bug fix: Module+Module merge in TrieMapping

When two files in the same namespace define a module with the same name (a legal F# pattern using CompilationRepresentationFlags.ModuleSuffix), the trie-based dependency graph failed to merge them. The second Module node silently replaced the first, losing its file index. Files in child namespaces that open-ed the parent then got an incomplete dependency set, leading to type-check errors under parallel compilation (graph-based checking).

Problematic pattern from the Nu game engine

// Assets.fs
namespace Nu

[<RequireQualifiedAccess>]
module Assets =
    module Default =
        let PackageName = "Default"
        let BlackName = "Black"

// WorldAssets.fs
namespace Nu

[<RequireQualifiedAccess; CompilationRepresentation(CompilationRepresentationFlags.ModuleSuffix)>]
module Assets =
    module Default =
        let Black = asset<Image> PackageName Assets.Default.BlackName

// WorldConstants.fs
namespace Nu.Constants
open Nu

module Dissolve =
    let Default = { DissolveImage = Assets.Default.Black } // ← could fail: file dependency on WorldAssets.fs was lost

Fix

Added a Module(_,file1), Module(_,file2) match arm in mergeTrieNodes that promotes the pair to a Namespace node, preserving both file indices—matching the existing Module, Namespace handling.

Regression test matrix

Added to azure-pipelines-PR.yml:

  • bryanedds/Prime @ 8d55f4e8e1d76e42f5fb3c9ba69eb79fe695e9fa
  • bryanedds/Nu @ b35cbe02029e0e33d72a4846816cf22714eb3aac

Copilot AI changed the title [WIP] Add two F# projects to regression test matrix Add Prime and Nu to compiler regression test matrix Feb 12, 2026
Copilot AI requested a review from T-Gro February 12, 2026 10:35
When two files define the same module name in the same namespace
(e.g., module Assets in namespace Nu across Core/Assets.fs and
World/WorldAssets.fs), mergeTrieNodes' fallback case kept only the
first file's index, losing the second. This caused missing dependency
edges in the graph-based parallel type checker, leading to FS0039
errors when files were typechecked before their dependencies.

Fix: Add an explicit Module+Module case that promotes to a Namespace
node preserving both file indices in filesThatExposeTypes, following
the existing Module+Namespace merge pattern.

Added three test scenarios covering sub-namespace access patterns
and the duplicate module name case.
- Add scenario for type constructor usage through open parent namespace
- Add scenario for multiple namespace declarations in one file with AutoOpen
- Add descriptive comments explaining test groups and Module+Module fix rationale
- All 48 dependency resolution tests and 96 compilation tests pass
Address TEST-COVERAGE gap: the existing 'Two modules with the same name'
test only had 2 files, so mkTrie (which skips the last file) never
exercised the Module+Module merge branch in mergeTrieNodes.

Add three targeted tests:
- Module+Module merge preserves both file indices (3 files)
- Module+Module merge across three files (4 files, 3 sharing name)
- Module+Module merge preserves children from both sides
- TrieMappingTests: Assert merged node is Namespace with correct
  filesThatExposeTypes (not just Files property)
- DependencyResolutionTests: Add focused test verifying Module+Module
  merge creates dependency edges to both defining files
- Remove standalone Module+Module merge Fact test from
  DependencyResolutionTests.fs (duplicated scenario-based coverage,
  used Contains instead of set equality)
- Extract assertModuleMergePromotesToNamespace helper in TrieMappingTests
- Combine 2-file and 3-file Module+Module merge tests into a single
  parameterized Theory test
@github-actions
Copy link
Contributor

github-actions bot commented Feb 12, 2026

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/10.0.300.md

@T-Gro T-Gro marked this pull request as ready for review February 12, 2026 18:42
@T-Gro T-Gro requested a review from a team as a code owner February 12, 2026 18:42
@T-Gro T-Gro changed the title Add Prime and Nu to compiler regression test matrix Add Prime and Nu to regression test matrix; fix graph resolution Module+Module merge Feb 12, 2026
@T-Gro T-Gro requested a review from abonie February 12, 2026 18:56
@T-Gro T-Gro enabled auto-merge (squash) February 12, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New

Development

Successfully merging this pull request may close these issues.

2 participants