Fix semantic classification errors for generics, slices, CEs, and open type#19939
Closed
T-Gro wants to merge 5 commits into
Closed
Fix semantic classification errors for generics, slices, CEs, and open type#19939T-Gro wants to merge 5 commits into
T-Gro wants to merge 5 commits into
Conversation
Contributor
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
Warning No PR link found in some release notes, please consider adding it.
|
T-Gro
pushed a commit
that referenced
this pull request
Jun 12, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
AI-Tooling-Check-Bypassed
T-Gro
commented
Jun 15, 2026
T-Gro
left a comment
T-Gro
left a comment
Member
Author
There was a problem hiding this comment.
AI-generated review (Claude / expert-reviewer agent). Findings posted as inline comments — please apply human judgement before acting.
auduchinok
reviewed
Jun 15, 2026
T-Gro
commented
Jun 15, 2026
T-Gro
left a comment
T-Gro
left a comment
Member
Author
There was a problem hiding this comment.
🤖 AI-generated review (Claude / expert-reviewer agent) — complementary to T-Gro's earlier pass. Findings posted as inline comments; please apply human judgement before acting.
T-Gro
pushed a commit
that referenced
this pull request
Jun 15, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ate decl PR #19813 suppressed a narrow synthesized Invoke/BeginInvoke/EndInvoke classification in a delegate declaration via a one-directional rangeContainsRange check inside the Item.MethodGroup arm. A second wide-range entry whose range covers the whole 'delegate of . -> .' clause still slipped through because the parser-synthesized SynValSig for the delegate's abstract slot is resolved as Item.Value vref (not Item.MethodGroup), with the val's idRange covering the entire `delegate of x: int * y: int -> int` clause (this is the parser's lhs range of the DELEGATE OF topType production used both for the SynIdent and the SynValSig itself). Add a targeted suppression in the Item.Value vref arm (vref.IsMember branch): when the val is a dispatch slot named Invoke/BeginInvoke/EndInvoke whose MemberApparentEntity is an F# delegate tycon, do not emit a Method classification. This precisely matches the parser-synthesized abstract slot only - real call sites like `d.Invoke(42)` resolve as Item.MethodGroup which is handled by the existing arm and is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…od/ctor calls In TcMethodItemThen and TcCtorItemThen, the DelayedTypeApp branches were calling CallNameResolutionSink / CallExprHasTypeSink with the merged 'mExprAndTypeArgs' / 'mExprAndArg' range instead of the identifier range 'mItem'. The classifier then attached one wide Method/DisposableType classification covering the type-argument list (items 3 and 6) and even the constructor argument list (item 4), drowning out the punctuation/lambda that should remain neutral. Use 'mItem' - the same range the type-provider fast path already uses in TcMethodItemThen. The Types CNR in TcTypeItemThen is intentionally left alone (the qualified-name case fixed in #18009 / #19800 relies on it). For items 4 and 6 the wide CNR is registered by upstream paths (ForNewConstructors in TcCtorCall and CallMethodGroupNameResolutionSink in ResolveExprDotLongIdentAndComputeRange) that must keep the wide range for find-references and goto-definition. Add classification-only narrowing in SemanticClassification.fs based on the item's DisplayNameCore length: - Method/Property: narrow to the last `displayName.Length` characters of the CNR range (trailing identifier, e.g. `Start` in `MailboxProcessor<int>.Start`). - CtorGroup: narrow to the leading `displayName.Length` characters (type name comes first in `new T<typeArgs>(...)`). This leaves the CapturedNameResolutions untouched so symbol uses, tooltips and navigation continue to use the wide ranges they always did. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The existing takeCustomBuilder dedup only handled the exactly-2-CNR group (one Item.Value + one Item.CustomBuilder at the same range). Inside a list / array comprehension the captured name resolutions for the builder identifier can include more than two CNRs at the same range, and the fall-through path returned them all, leaving the Item.Value to dominate the classifier output and color the builder identifier as a plain Value. Generalize: at any group of CNRs that share a range, if at least one is Item.CustomBuilder, keep only the CustomBuilder ones. This preserves the existing 2-CNR behavior and additionally fixes the comprehension case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
For 'xs[0..]' (and the set-slice variants) on nominal types, TcIndexingThen synthesizes a DelayedDotLookup for GetSlice/SetSlice/Item. The synthesized identifier carried mWholeExpr (the whole bracketed expression including ']'), which the resolver then turned into a Method-classification CNR that the editor showed on the closing ']'. Syntheticize the identifier range and the DelayedDotLookup range so the CNR is filtered out by the existing IsSynthetic check in the resolutions pipeline. The DelayedApp ranges are left intact so error diagnostics keep their real ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds a regression guard for the case where `open type X` is used purely via an unqualified static-member call (e.g. `Button ...` after `open type Helpers.View`). On HEAD, UnusedOpens already detects this as used; this test pins the behavior so future refactors of ServiceAnalysis.fs cannot silently regress it. The existing enum-only coverage in `Unused opens in non rec module smoke test 1` did not exercise the class-with-static-members path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
Superseded by #19960 (rebased). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #19905
Type.Method<int>()/new T<args>(...)/T<args>.M(...)no longer classify<…>as Function/Type; span narrows to the identifier.list[0..]trailing]no longer classified as Function.[ for .. do builder { } ]classified as ComputationExpression, not Value.open type Tno longer always reported as unused.