Repository navigation
perf(semantic): conform to a named type by the element it names - #1349
devin-ai-integration[bot] wants to merge 3 commits into
Conversation
Co-Authored-By: jason.han <hanhuijun@gmail.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
…route-walks' into perf/conform-by-symbol
…stered element Co-Authored-By: jason.han <hanhuijun@gmail.com>
| if symbols.FQNIs(sym, parent, leaf) { | ||
| return true | ||
| } | ||
| for _, sup := range m.AllSupertypes(sym) { | ||
| if m.fqnOf(sup) == typeFQN { | ||
| if symbols.FQNIs(sup, parent, leaf) { |
There was a problem hiding this comment.
🔴 Imported type aliases lose conformance
When a type reaches Links::BinaryLink through a public import, conformsByName rejects its original symbol. FQNIs sees the declaration's owner chain, not the name registered by the import. Binary connector checks then return false.
Learn more
The index can register the same symbol under more than one qualified name. A wildcard public import registers it under the importing namespace, and a declared short name registers it under another name in the same namespace. symbolByFQN retrieves the element under either key; FQNIs only compares its original declaration chain. Both the candidate and its supertypes can therefore lose a conformance match that the index recognizes. IsBinaryConnector consumes this answer for connector classification.
Example: Declare Lib::BinaryLink, then package Links { public import Lib::*; }. Links::BinaryLink resolves to Lib::BinaryLink. Checking the latter against Links::BinaryLink returns false instead of true.
Recommended fix: Retain an index-aware identity lookup for names that resolve to a registered element, including imported and short-name registrations. Compare symbols.SameElement against the candidate and its supertypes; keep the allocation optimization only where it preserves that lookup's semantics.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This matches develop and is not a regression. There, conformsByName compared m.fqnOf(sup) == typeFQN, and fqnOf is Index.GetFQN, which is symbols.FQNOf: the declaration's owner-chain name. Lib::BinaryLink checked against Links::BinaryLink already returned false on develop. FQNIs(sym, parent, leaf) is defined as FQNOf(sym) == parent::leaf (pinned by TestFQNIsMatchesFQNOf), so it gives the same answer. Only this PR's first revision (55b542a's parent), which compared against the element symbolByFQN returns, would have accepted the alias. Whether a type that reaches the library name only through an import should conform is a semantic change, not a performance one, so it is out of scope here. I'm leaving the thread open for the maintainer.
| if symbols.FQNIs(sym, parent, leaf) { | ||
| return true | ||
| } | ||
| for _, sup := range m.AllSupertypes(sym) { | ||
| if m.fqnOf(sup) == typeFQN { | ||
| if symbols.FQNIs(sup, parent, leaf) { |
There was a problem hiding this comment.
🔴 Distinct namesakes falsely conform to indexed types
When an indexed type has an unindexed namesake, conformsByName accepts both by owner-chain name. symbolByFQN previously identified the indexed element, so the distinct namesake did not conform. This can misclassify types after reindexing.
Learn more
A symbol's owner-chain name does not prove that it is the element currently registered under that name. Workspace reindexing can leave callers holding a symbol from another scope tree. When the index has exactly one element under the target name, the previous lookup compared element identity and rejected a different declaration with the same spelling. FQNIs cannot distinguish those declarations, so the conformance answer changes.
Example: An older document's Links::BinaryLink symbol remains in a query while the active index registers a replacement declaration at Links::BinaryLink. The old symbol now conforms to the new declaration by name, despite representing a different element.
Recommended fix: Resolve the registered target before using the owner-chain fast path. When a unique target exists, compare symbols.SameElement for the candidate and each supertype; only use name-based comparison in the cases where the existing contract permits it.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This matches develop too. There, conformsByName never consulted symbolByFQN. It compared fqnOf(sup) (= symbols.FQNOf, the owner-chain name) to typeFQN, so a stale or unindexed namesake with the same owner chain already conformed. FQNIs is that same comparison without building the string. The identity check described here was only in this PR's first revision, and the previous review found that it misjudges declarations that moved across index generations. That's why I went back to the name comparison. This change keeps develop's contract exactly: the same answers with fewer allocations. I'm leaving the thread open for the maintainer in case identity semantics are wanted as a separate change.
What and why
The 0.11.0 performance record (measured against
v0.10.1) finds the connector-denseImportValuesbenchmarks 29–37% more allocations per load (flat/parts=100/attrs=3135.5k → 175.2k,nested/parts=500/attrs=3864.9k → 1 189.0k),Analyze/synthetic+10% (4.39M → 4.83M) andWorkspaceEdit/synthetic/reindex+diagnostics+9%. Bisected over the pull-request merges to #1298 (constancy conformance on the derived variability):restrictionViolationsnow asksFeatureIsVariablefor every redefinition, which derivesUsage::mayTimeVarythrough fourconformsByNamechecks (Occurrences::Occurrence,Links::SelfLink,Occurrences::HappensLink,Actions::Action), andconformsByNamebuilt the qualified name of the candidate and of each of its supertypes withIndex.GetFQNon every check — about 73k of the 175k allocations of oneImportValuesload (pprof:restrictionViolations → FeatureIsVariable → UsageMayTimeVary → conformsByName → FQNOf).conformsByNamenow splits the asked name once and compares the candidate's and each memoized supertype's owner chain against it segment by segment withsymbols.FQNIs(from #1348, on which this branch is stacked), so the comparison is the same name comparison as before and no qualified name is built. Without an index it compares local names, asfqnOfdid.Specification basis
None: the answers are unchanged; only how the name comparison is made.
How it was verified
TestConformsByNameDoesNotBuildNames(internal/semantic/semantics):Leaf :> Mid :> Baseconforms toT::Base,T::Mid,T::Leafand not toT::Other, the unregisteredT::Absentor the root nameBase, and a repeated check allocates nothing. Ondevelopbefore this change the repeated checks allocate 1–3 objects each and the test fails.2cfcb09df(the parent of the fix(semantic): constancy conformance reads a SysML usage's derived variability #1298 merge) in a scratch worktree |developb60a18c20| this branch (first version, same allocation profile), allocs/op:ImportValues/flat/parts=100/attrs=3135.5k | 175.2k | 135.5k (B/op 11.20 | 12.03 | 11.20 MiB);ImportValues/nested/parts=500/attrs=3864.9k | 1 189.0k | 864.9k;Analyze/synthetic4.394M | 4.834M | 4.339M;WorkspaceEdit/synthetic/reindex+diagnostics4.838M | 5.278M | 4.783M;FeaturesOf5.416M in all three (its move predates2cfcb09dfand is attributed separately). sec/op within the busy machine's noise in every column.go test ./internal/semantic/... ./internal/check/passes ./internal/workspace/model ./tests/resolve ./tests/perfpass,go build ./...,gofmt -l .empty,python3 scripts/changelog.py check.Checklist
make testandmake lintpass locally (the packages above; the full suite runs in CI)changes/unreleased/<slug>.<section>.md, not as an edit toCHANGELOG.mdmake docs-countsrun if a gate count moved (none moved)F4,K5) in the body, docs, or changelogLink to Devin session: https://nasa-jpl-demo.devinenterprise.com/sessions/cf22d7a746814d20b2f0eaf11682fef9
Open in Devin Desktop: https://nasa-jpl-demo.devinenterprise.com/desktop/session/cf22d7a746814d20b2f0eaf11682fef9?variant=devin
Requested by: @HuiJun