Skip to content

Add FS3889 diagnostic for namespace/type collision#19802

Open
T-Gro wants to merge 14 commits into
mainfrom
fix/issue-17827
Open

Add FS3889 diagnostic for namespace/type collision#19802
T-Gro wants to merge 14 commits into
mainfrom
fix/issue-17827

Conversation

@T-Gro

@T-Gro T-Gro commented May 25, 2026

Copy link
Copy Markdown
Member

Summary

When a namespace name collides with a type name across files in the same assembly, the compiler previously emitted FS0247 stating the partner was a module. However the partner is actually a type, so this PR introduces a new FS3888 diagnostic for that case while keeping FS0247 for namespace/module collisions.

Changes

  • New diagnostic FS3888 (tastNamespaceAndTypeWithSameNameInAssembly): The namespace '%s' clashes with the type '%s'.
  • TypedTreeOps.Remapping.fs: Updated match logic to detect namespace/type vs namespace/module collisions and emit the appropriate diagnostic.
  • XLF translations: Updated all localization files with the new string.
  • Tests: Added two component tests in NamespaceTests.fs:
    • Verifies FS3888 is emitted for namespace/type collision.
    • Verifies FS0247 is still emitted for namespace/module collision.

T-Gro and others added 2 commits May 25, 2026 11:07
When a namespace name collides with a type name across files in the same
assembly, the compiler previously emitted FS0247 stating the partner was a
module. The partner is actually a type, so emit the new FS3888 diagnostic in
that case while keeping FS0247 for namespace/module collisions.

Fixes #17827

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

github-actions Bot commented May 25, 2026

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 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 Summary

The core fix in TypedTreeOps.Remapping.fs is correct and the tests are good. However, there are two issues that need to be addressed before merging.


🚫 Remove .executor-pid from the PR

The file .executor-pid (containing a PID number) is committed in the second commit. This is clearly a build/process artifact and should not be part of the repository. Please remove it and consider adding it to .gitignore.


🚫 Unrelated change in ServiceParsedInputOps.fs

The second commit (""Apply remaining changes"") adds a tryFindLastHashRLineInScript function and modifies FindNearestPointToInsertOpenDeclaration to ensure open statements are placed after #r directives in .fsx scripts. This change is completely unrelated to the FS3888 namespace/type collision diagnostic.

The PR description inaccurately states: ""ServiceParsedInputOps.fs: Updated logic to detect namespace/type vs namespace/module collisions and emit the appropriate diagnostic."" — the actual change has nothing to do with diagnostic detection.

Please either:

  • Remove this change from this PR and submit it as a separate PR with its own tests, or
  • At minimum, correct the PR description and add test coverage for the new open-insertion behavior.

✅ Core diagnostic logic is correct

The match pattern refactoring in TypedTreeOps.Remapping.fs correctly separates:

  • namespace vs module → FS0247 (tastNamespaceAndModuleWithSameNameInAssembly)
  • namespace vs type → FS3888 (tastNamespaceAndTypeWithSameNameInAssembly)

The pattern ordering handles all cases properly: (true, _, _, true) and (_, true, true, _) correctly catch the namespace+module cases first, leaving (true, _, _, _) and (_, true, _, _) to cover namespace+type (since namespace+namespace is already matched above).

✅ Tests look good

Both new tests in NamespaceTests.fs properly verify FS3888 for namespace/type collision and that FS0247 is still emitted for namespace/module collision.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label May 25, 2026
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label May 25, 2026
@T-Gro T-Gro requested a review from abonie June 3, 2026 10:16
Copilot and others added 3 commits June 3, 2026 12:45
- Run fantomas on ServiceParsedInputOps.fs to fix formatting check
- Add release notes entry for FS3888 diagnostic

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label Jun 3, 2026
@T-Gro T-Gro enabled auto-merge (squash) June 4, 2026 12:38
@T-Gro

T-Gro commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@copilot : Remove the unrelated change in src/Compiler/Service/ServiceParsedInputOps.fs

Co-authored-by: T-Gro <46543583+T-Gro@users.noreply.github.com>
auto-merge was automatically disabled June 4, 2026 12:45

Head branch was pushed to by a user without write access

Copilot AI commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

@copilot : Remove the unrelated change in src/Compiler/Service/ServiceParsedInputOps.fs

Done. Reverted src/Compiler/Service/ServiceParsedInputOps.fs to its original state and removed the associated release note entry. See commit Remove unrelated ServiceParsedInputOps.fs change and its release note.

Copilot finished work on behalf of T-Gro June 4, 2026 12:59
Comment thread docs/release-notes/.Language/preview.md Outdated
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 9, 2026
Copilot and others added 3 commits June 12, 2026 12:51
The diagnostic is not gated by language version and the release note
is already covered in .FSharp.Compiler.Service/11.0.100.md.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
After merging main, FS3888 was taken by PR #19880 (implAttributeMissingFromSignature).
The namespace/type collision diagnostic was renumbered to FS3889 in FSComp.txt,
but the test and release notes still referenced the old number.

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

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

/azp run fsharp-ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

MSBuild 18.9.0-preview-26312-01 transitively depends on System.Formats.Nrbf,
System.Resources.Extensions, and System.Text.Json which reference
System.ValueTuple 4.0.5.0. This conflicts with the net472 framework facade
(4.0.2.0). Since net472 EXE projects have AutoGenerateBindingRedirects, the
conflict is resolved at runtime. Suppress the build-time MSB3277 warning that
gets promoted to error by CI's /warnaserror flag.

Applied at root level (FSharpBuild.Directory.Build.props) to cover fsc, fsi,
vsintegration, and test projects that all reference MSBuild packages.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro force-pushed the fix/issue-17827 branch from e259fe1 to 1dc2d80 Compare June 12, 2026 12:51
The MSBuildWarningsAsMessages addition was introduced to suppress
System.ValueTuple version conflicts for net472, but it actually caused
MSB3277 errors in CI (MockOfficial, WindowsNoRealsig_testDesktop).
Main branch passes without this suppression.

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

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

/azp run fsharp-ci

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@T-Gro T-Gro changed the title Add FS3888 diagnostic for namespace/type collision Add FS3889 diagnostic for namespace/type collision Jun 12, 2026
@T-Gro

T-Gro commented Jun 18, 2026

Copy link
Copy Markdown
Member Author

@abonie :

I am sorry, you have to approve again:
"Waiting on 1 reapproval from someone other than the last pusher. Review from abonie is stale because it was submitted before the most recent code changes."

@T-Gro T-Gro requested a review from abonie June 18, 2026 14:14
@T-Gro T-Gro enabled auto-merge (squash) June 18, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

3 participants