Skip to content

Detect duplicate sibling modules in recursive module groups#19913

Open
T-Gro wants to merge 4 commits into
mainfrom
fix/issue-6694
Open

Detect duplicate sibling modules in recursive module groups#19913
T-Gro wants to merge 4 commits into
mainfrom
fix/issue-6694

Conversation

@T-Gro

@T-Gro T-Gro commented Jun 8, 2026

Copy link
Copy Markdown
Member

Fixes #6694

namespace rec Ns
module A = let x = 1
module A = let y = 2
// before: no error until IL emit → cryptic FS2014 "duplicate entry in type index table"
// after:  FS0037 "Duplicate definition of type or module 'A'" at type-check time

Copilot and others added 2 commits June 8, 2026 17:31
test(#6694): RED tests for recursive module sibling collision
1fac128 
Add 7 tests in ErrorMessages/ModuleTests.fs locking the desired behavior:
duplicate sibling modules in a 'module rec' / 'namespace rec' must produce
FS0037 at type-check time, not FS2014 from IL emit.

Filtered run (--filter-class ErrorMessages.Modules):
  total: 20, failed: 3, succeeded: 17

RED (positive, fail today):
  - Recursive module with duplicate sibling modules emits FS0037
  - Recursive namespace with duplicate sibling modules emits FS0037
  - Recursive module nested siblings with duplicate name emit FS0037

GREEN (negative regression guards, pass today):
  - Recursive module nested cousins with same name compile
  - Non-recursive module with duplicate sibling modules still emits FS0037
  - Single file with two namespace fragments and different modules compiles
  - Recursive module siblings differing only in case compile

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fix(#6694): detect duplicate sibling modules in recursive scopes at p…
4db2911 
…ublish time

In `module rec` / `namespace rec` groups all siblings share the same envInitial during Phase1A, so the existing CheckForDuplicateModule call in TcTyconDefnCore_Phase1A_BuildInitialModule cannot see them. Move the check to the Phase1AB publish point where envAbove's accumulator already contains earlier-published siblings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro requested a review from a team as a code owner June 8, 2026 17:56
docs(#6694): release notes entry for recursive-module duplicate diagn…
9e3b1a8 
…ostic

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev

✅ Found changes and release notes in following paths:

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

@T-Gro T-Gro force-pushed the fix/issue-6694 branch from c86168c to 9e3b1a8 Compare June 8, 2026 17:57
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 8, 2026
@T-Gro T-Gro requested a review from abonie June 12, 2026 10:08
T-Gro
T-Gro commented Jun 12, 2026
View reviewed changes

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: LGTM – clean, well-targeted fix.

Summary

The fix correctly places a CheckForDuplicateModule call in Phase1AB's publish loop where envAbove's mutable accumulator already reflects previously-published siblings. Since MutRecShapes.computeEnvs uses List.map (sequential, left-to-right) and PublishModuleDefn mutates the accumulator, the second duplicate will see the first when it checks. This mirrors the analogous CheckForDuplicateConcreteType pattern for tycons a few lines below.

Details

Aspect Assessment
Correctness Relies on sequential List.map + mutable accumulator - same pattern as tycon duplicate check at line 4115
Non-recursive regression Test confirms non-recursive case still works (different code path)
False positives Cousins (same name, different parent) pass; case-differing siblings pass
Error UX Reuses existing FS0037 Duplicate definition of type or module - consistent
Test coverage 7 tests: 3 positive (must-fail) + 4 negative (must-pass) regression guards
Release notes Clear, links issue and PR

No concerns. Nice fix!

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Jun 12, 2026
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 17, 2026
@T-Gro T-Gro enabled auto-merge (squash) June 18, 2026 14:18
auduchinok
auduchinok reviewed Jun 18, 2026
View reviewed changes
Comment thread tests/FSharp.Compiler.ComponentTests/ErrorMessages/ModuleTests.fs
Comment on lines +201 to +202
module Foobar = let x = 1
module Foobar = let y = 2

@auduchinok auduchinok Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test showing whether the second module is analyzed properly and y is reported to the sink?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abonie abonie abonie approved these changes

+1 more reviewer

@auduchinok auduchinok auduchinok left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed

Projects

F# Compiler and Tooling
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve error message when module names clash in recursive module

3 participants

@T-Gro @auduchinok @abonie