Hardening: eOpenedTypeOperators NameMultiMap + CheckEntityDefn dedup#1
Open
T-Gro wants to merge 14 commits intogusty:feature-operators-extensionsfrom
Open
Conversation
Before: eOpenedTypeOperators was a linear MethInfo list scanned on every SRTP trait-resolution call in SelectExtensionMethInfosForTrait: the loop filtered by string equality on minfo.LogicalName, costing O(N) per lookup where N is the total number of operator methods brought into scope via 'open type'. After: store as NameMultiMap<MethInfo> (NameMap<MethInfo list>), aggregating by LogicalName. Lookup becomes a single NameMultiMap.find, O(log N) + O(bucket) where bucket size is the number of homograph operators. Write path folds operatorMethods into the multi-map. This mirrors the pattern already used for eTyconsByDemangledNameAndArity and similar fields in NameResolutionEnv. Pure refactor: no change in semantics, no observable behavior change, no public API or signature surface impact. Measured on the small ext-operator SRTP stress: TC wall-clock within noise (expected — N is small there). Benefit grows linearly with the number of 'open type' declarations in scope, which is the FSharpPlus- shaped usage pattern this feature targets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…peToNameEnv AddStaticContentOfTypeToNameEnv was calling IntrinsicMethInfosOfType twice with identical arguments: once for method-group collection and again for the operator-method filter used to populate eOpenedTypeOperators. Bind the result once and reuse. Pure refactor, no behavior change. Called per 'open type' (and per type opened via 'open' / 'AutoOpen'), so repeated in compilation units with many such declarations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expert review noted that folding operatorMethods forward into NameMultiMap reversed within-call source order inside each bucket (List.fold + prepend), which could subtly affect overload-resolution diagnostic candidate ordering. Switch to List.foldBack so within-call order matches the prior list-prepend implementation exactly. Also extract the indexing step as AddMethInfoByLogicalName, mirroring the existing AddRecdField helper one NameMap sub-table builder away. Adversarial review flagged the inline fold as a missed reuse opportunity against the AddRecdField pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…refactor
Test: OpenTypeOperatorHomographOrder.fs opens a holder type with three
via SRTP. Pins down the >=2-entries-per-bucket path of the new
NameMultiMap<MethInfo> encoding and the foldBack ordering fix.
Also adds a Changed entry to the 11.0.100 compiler-service release notes
covering the three in-commit refactors on this branch.
Verified:
- SurfaceAreaTest passes (no FCS API drift)
- ExtensionConstraintsTests + IWSAMsAndSRTPs + full Conformance.Types suite
all green (679 tests, 674 passing, 5 skipped)
- fantomas reports no formatting drift on NameResolution.{fs,fsi}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…Defn The ad-hoc grouping of MethInfos by LogicalName in PostInferenceChecks duplicated the pattern already captured by NameMultiMap.initBy. Unifies with the eOpenedTypeOperators indexing convention added in commit 5c44db6d3 (AddMethInfoByLogicalName / NameMultiMap). - hashOfImmediateMeths and hashOfAllVirtualMethsInParent now built via NameMultiMap.initBy. - hashOfImmediateProps left alone — its incremental build with order-dependent self-exclusion is genuinely stateful. - getHash helper retained for props only. No semantic change; targeted tests pass; bench-harness perf neutral. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Small, machine-independent slices from PLAN.md, no perf claims. WS3.1 — Document the order invariant at both ends: - NameResolution.fs:1283 (List.foldBack site) explains WHY the fold direction matters and points to the consumer + the regression test. - NameResolution.fs:1705 (SelectExtensionMethInfosForTrait) mirrors the invariant from the consumer side. WS4.1 — Expand homograph test matrix from 1 to 4 scenarios: - OpenTypeOperatorHomographMultipleHolders.fs — operators spread across two separate 'open type' holders must all accumulate into the same bucket (cross-call bucket accumulation). - OpenTypeOperatorNestedModule.fs — 'open type' in nested module scopes the operator correctly; sibling scope unaffected. - OpenTypeOperatorShadowing.fs — local 'let inline (op)' shadows the extension operator from that point on. WS4.3 — Release notes rewrite: drop overclaimed perf framing; honestly describe as 'internal refactor'. Also mention PostInferenceChecks dedup brought in by the follow-up commit. WS5.2 — Rename hashOfImmediateMeths -> immediateMethsByLogicalName and hashOfAllVirtualMethsInParent -> parentVirtualMethsByLogicalName in CheckEntityDefn. They are NameMultiMaps now, not hashes. Verified: ExtensionConstraintsTests (22 passed), *SameName* (6), *Duplicate* (29), *AbstractMember* (18), all green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- 4 new ComponentTests covering SRTP dispatch, param-type overloads, cross-assembly usage, and CompiledName-renamed operators. - docs/name-resolution-operators.md explaining eOpenedTypeOperators, the order invariant, and consumer paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Broadens regression coverage for duplicate-member diagnostics emitted by CheckEntityDefn in PostInferenceChecks.fs. Covers: - Erased-generic-args duplicate method (FS0442) - Property-vs-method name clash (FS0434) - Derived-hides-abstract warning (FS0864) - Indexer-vs-non-indexer property clash (FS0436) - Getter/setter type mismatch (FS3172) Prerequisite for further dedup work in CheckEntityDefn (props dict). (Re-land of lost commit 9146ec08e dropped by a parallel subagent reset.) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The companion NameMultiMap-based indexes (immediateMethsByLogicalName, parentVirtualMethsByLogicalName) are symmetric full-build + identity-exclusion at each point. hashOfImmediateProps, by contrast, is deliberately scan-so-far so that each duplicate PropInfo pair is reported exactly once. Future readers may be tempted to unify the two patterns — document the constraint at the site. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document that the open-type extension-operator refactor is structurally motivated, not perf-motivated: on the FSharpPlus-like stress harness the delta (1.148s pre / 1.206s post, median of 5) is within noise. Avoids any implicit perf claim; leaves the door open to corpus-scale benchmarking (PLAN.md §0.4). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…trim notes S1 (soften claim): OpenTypeOperatorHomographOrder.fs disambiguates overloads by parameter type, so it does not actually pin within-call source-order tie-break. Rewrite both the NameResolution.fs comment and the docs section to reflect that the invariant is defensive (matches prior list-prepend semantics; cross-open ordering remains observable), not empirically pinned. S2 (diagnostic-text fix): In the prior hand-rolled Dictionary build for parent virtual methods, newer entries were prepended — so List.tryFind (checkForDup EraseAll) at PostInferenceChecks.fs:2553 returned the *last* matching parent virtual in source order. NameMultiMap preserves source order, so the same tryFind would return the *first*. When a base class has >=2 homograph virtuals matching a hiding derived member, the tcNewMemberHidesAbstractMember warning prints a different signature. Switch to List.tryFindBack to preserve the prior last-match selection exactly. N3 (release notes): trim the 11.0.100.md entry from implementation detail to one line matching the file's convention; link to the reference doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A. anti-duplication + reuse agents flagged: two calls to
NameMultiMap.initBy (fun (m: MethInfo) -> m.LogicalName) in
PostInferenceChecks.CheckEntityDefn. Extract a local
methInfosByLogicalName combinator; both sites now read as one concept.
B. clutter + too-big agents flagged: the NameResolution.fs insertion-site
comment (12 lines hedging on order-invariant) and its consumer-side
twin (9 lines) restated the same point twice. Collapse each to 4-5
lines pointing at the canonical explanation in
docs/name-resolution-operators.md.
C. clutter agent flagged: the hashOfImmediateProps preamble was a
prescriptive barrier comment (7 lines arguing against future refactors).
Trim to a 2-line statement of the scan-so-far property, referencing
the contrasting immediateMethsByLogicalName.
Intentionally not adopted:
- Parametrizing the 7 [<Fact>] open-type scenario tests into one
[<Theory>]: the current Facts encode scenario description in the test
name, which appears verbatim in test output. A Theory with InlineData
filename would report 'Theory("OpenTypeOperatorShadowing.fs")' and
lose that.
- Inlining AddMethInfoByLogicalName at its single call site: adversarial
verdicts split (anti-dup vote keep, too-big vote inline). The helper
names the intent of the fold and costs one line.
- Trimming the Measurements / Known-gotchas sections of
docs/name-resolution-operators.md: these are the honest perf record
and the user-visible language gotchas, not implementation chatter.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
T-Gro
changed the title
Internal notes that don't belong in the shipped diff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not a PR into main. Belongs on #19602 if at all. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
gusty
force-pushed
the
feature-operators-extensions
branch
from
67063e0 to
8ae1999
Compare
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.
12 commits on top of
8fef93ddafor PR dotnet#19602.Perf + hardening for
open typeoperator SRTP path. Orthogonal to gusty's func/tuple extension work.Key change:
eOpenedTypeOperators: MethInfo list→NameMultiMap<MethInfo>. Lookup inSelectExtensionMethInfosForTraitdrops from O(N) scan to keyed.Plus
CheckEntityDefnhand-rolledDictionary→NameMultiMap, surfacing an ordering bug fixed viatryFindBackintcNewMemberHidesAbstractMember.