Skip to content

Report FS0039 only once for undefined type in inherit clause#19862

Open
T-Gro wants to merge 12 commits into
mainfrom
fix/issue-16432
Open

Report FS0039 only once for undefined type in inherit clause#19862
T-Gro wants to merge 12 commits into
mainfrom
fix/issue-16432

Conversation

@T-Gro

@T-Gro T-Gro commented May 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #16432

inherit T() with undefined T emitted FS0039 three times — one per
type-checking pass over the inherit clause. Now emitted once.

Copilot and others added 5 commits May 29, 2026 13:15
[RED] Add failing tests for issue #16432 - single FS0039 on inherit o…
12ae67e 
…f unknown type

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fix(checker): dedup FS0039 within a single inherit clause (#16432)
57ffff0 
Multiple typecheck passes (EstablishTypeDefinitionCores FirstPass and SecondPass, plus Phase2AInherit's TcType/TcNewExpr) each emit the same FS0039 for an undefined base type in an 'inherit'/'interface inherit' clause, surfacing three identical diagnostics. Introduce a DedupInheritDiagnosticsLogger overlay scoped via UseTransformedDiagnosticsLogger to TcMutRecDefinitions. The dedup key is the formatted message of UndefinedName so duplicate FS0039s for the same identifier collapse to one while unrelated diagnostics, including FS0039 for distinct identifiers, pass through unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[GREEN] Dedup FS0039 across inherit-clause type-checking passes
c2735ab 
Adds a cenv-scoped `(range, idText)` dedup of `UndefinedName` diagnostics applied around Phase 1F inherit type checking and Phase 2A `Phase2AInherit` processing in CheckDeclarations.fs. Resolves the triple-reporting in #16432 without changing the surface API.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
chore(#16432): release notes for inherit FS0039 dedup
e34c612 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[REFACTOR] Format and address expert review for #16432
88a176e 
- Switch reportedUndefinedNames to ConcurrentDictionary<struct (range * string), unit> for consistency with the sibling argInfoCache field and to be defensive against any future parallel cenv use.

- Forward CheckForRealErrorsIgnoringWarnings to the wrapped logger so the InheritDedupDiagnosticsLogger is a faithful pass-through for everything other than the intended UndefinedName dedup.

- Document the rationale for unwrapping only WrappedError in isDuplicateUndefinedName.

Expert reviewer item 1 (merging suggestions from later UndefinedName into the first) intentionally not applied: in the inherit-clause case the colliding diagnostics carry identical suggestion sets, and merging would couple this site to NameResolution.AddResults internals.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro requested a review from a team as a code owner May 29, 2026 14:01
@github-actions

github-actions Bot commented May 29, 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

docs: add PR link to release notes for #16432
966314a 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
auduchinok
auduchinok reviewed May 29, 2026
View reviewed changes
Comment thread src/Compiler/Checking/CheckDeclarations.fs Outdated
@github-actions github-actions Bot added the AI-Tooling-Check-Scanned-Clean Tooling check: diff analyzed, no interesting infrastructure files label May 29, 2026
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 2, 2026
@T-Gro T-Gro requested a review from abonie June 3, 2026 10:14
@auduchinok auduchinok mentioned this pull request Jun 3, 2026
Merged
T-Gro and others added 5 commits June 5, 2026 12:21
@T-Gro
[RESEARCH] Document 3 call sites for inherit FS0039 (#16432)
d34b815 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
Root-cause fix for inherit FS0039 triple-report (#16432)
4968066 
Replace the InheritDedupDiagnosticsLogger sink-wrapper (PR #19862) with a
per-cenv 'inheritResolutionFailed' marker. Phase 1D records (tycon.Stamp,
synBaseTy.Range) when name resolution fails with UndefinedName. Phase 1F
and Phase 2A consult the marker and skip re-resolution entirely, so the
diagnostic is emitted once and the redundant work is eliminated. Non-
UndefinedName failures (constraint, type-provider, etc.) continue to flow
through normal recovery in every pass.

Addresses review comment 3324874944 on PR #19862.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
Address verifier feedback for inherit FS0039 root-cause fix (#16432)
e2bd7bf 
- Extend isUndefinedNameFailure to also unwrap the constraint-solver wrappers
  (ErrorFromAddingTypeEquation, ErrorFromAddingConstraint, ErrorFromApplyingDefault)
  so the marker is set when UndefinedName surfaces from the CheckCxs second pass
  via a generic inherit-clause type-argument failure.
- Remove a misplaced /// doc comment from 'type cenv = TcFileState'.
- Collapse three duplicated InheritsDeclarations regression Fact methods into a
  single Theory + InlineData driving an assertSingleFS39 helper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro
Merge remote-tracking branch 'origin/main' into fix/issue-16432
8dfccdb
@T-Gro
Clean up FS0039 inherit dedup: drop bloat, strict tests, active pattern
73e5702 
- Delete .tools/ralph/research-notes.md (agent scratchpad, not for repo).
- Replace isUndefinedNameFailure helper + XML doc with active pattern
  (|UndefinedNameError|_|) co-located with the catch.
- Extract tryResolveInheritType local in EstablishTypeDefinitionCores so
  the call site reads as a single mapFold.
- Drop narrative block comments at both Phase2A and Phase 1D/F sites; the
  function name and pattern carry the intent.
- Trim 5-line XML doc on inheritResolutionFailed to a one-liner.
- Rewrite tests with strict withDiagnostics asserting exact code, range,
  and message for the full diagnostic set (no per-code filtering).
@T-Gro T-Gro changed the title Fix #16432: Report FS0039 only once for undefined type in inherit clause Report FS0039 only once for undefined type in inherit clause Jun 9, 2026
@T-Gro T-Gro requested a review from auduchinok June 9, 2026 14:06
@T-Gro T-Gro enabled auto-merge (squash) June 9, 2026 14:06
auduchinok
auduchinok suggested changes Jun 9, 2026
View reviewed changes

@auduchinok auduchinok left a comment

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.

The active pattern for undefined name errors captures unrelated errors which is wrong. The whole approach seems to still do the same analysis multiple times and these changes just try to hide this problem.

Comment thread src/Compiler/Checking/CheckBasics.fs

argInfoCache: ConcurrentDictionary<string * range, ArgReprInfo>

/// Inherit clauses whose type already failed UndefinedName; skip re-resolution to avoid duplicate FS0039.

@auduchinok auduchinok Jun 9, 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.

The comment duplicates the signature.

Comment thread src/Compiler/Checking/CheckDeclarations.fs
| UndefinedName _ -> Some ()
| WrappedError(inner, _)
| ErrorFromAddingTypeEquation(error = inner)
| ErrorFromAddingConstraint(error = inner)

@auduchinok auduchinok Jun 9, 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.

This is definitely not a undefined name error.

Comment thread src/Compiler/Checking/CheckDeclarations.fs
Comment on lines +1379 to +1381
if cenv.inheritResolutionFailed.ContainsKey(struct (tcref.Stamp, synBaseTy.Range)) then
mkUnit g m, tpenv
else

@auduchinok auduchinok Jun 9, 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.

I'm not sure I'm following the idea. If there was an error previously then we skip the analysis and if there wasn't we still do it the second time? Why do we need to resolve the same identifiers more than once at all?

@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abonie abonie Awaiting requested review from abonie

1 more reviewer

@auduchinok auduchinok auduchinok requested changes

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

F# Compiler and Tooling
Status: In Progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unresolved name error in 'inherit' member reported multiple times

3 participants

@T-Gro @auduchinok