Repository navigation
fix(parser): resolve generic inheritance targets - #939
merlincat11 wants to merge 2 commits into
Conversation
code-review-graph reviewOverall risk: 0.40 (MEDIUM) — 20 changed function(s)/class(es), 0 affected flow(s), 9 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~234,374 fewer tokens (~98%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
INHERITS edges stored the base type as raw source text, so a generic base
became the target "GenericBase<string>", which never matched the class node
"GenericBase". The relationship was invisible, and inheritors_of reported
zero results with a confidence string asserting no such edge existed.
Target the base type's declaration name instead, derived from the syntax
tree with the type-argument and comment subtrees skipped. It is deliberately
not derived by scanning for angle brackets: those characters also appear in
trivia the grammar ignores, so "class C : I</* < */ string> {}" -- legal C#
-- reads as unbalanced to a character counter and would keep the constructed
spelling, leaving the bug in place. _get_base_type_nodes is split out of
_get_bases so the spelling and the target come from one traversal.
C# allows several closed constructions of one generic interface
("class C : I<int>, I<string> {}"). They erase to a single target and share
the class declaration line, so one edge per base would collide on
upsert_edge's identity of (kind, source, target, file_path, line), which
excludes extra, and the last would overwrite the rest. Bases are grouped by
target and emitted as one edge listing every construction in
extra.constructed_types, always as a list.
The target is a lookup name, not a language-level declaration identity: it
drops generic arity and carries no namespace. That matches how non-generic
bases already resolve, since class nodes are keyed by bare name. Retaining
the constructed spelling keeps the evidence a namespace-aware resolver would
need; see tirth8205#940 for that resolver and tirth8205#941 for existing graphs, which pick up
correct targets on reparse or a full build.
Malformed type text is left unchanged, so an error-tolerant parse of a
half-edited file never rewrites a target into a bogus name.
Fixes tirth8205#935
6507904 to
433ab4c
Compare
…e targets Changing how a Java/C# INHERITS target is spelled leaves existing graphs stale: nothing reparses an unchanged file, so their GenericBase<string> targets survive the upgrade and tirth8205#935 persists until a manual rebuild. Use the mechanism already in place for exactly this, CPP_IDENTITY_VERSION. An INHERITS_IDENTITY_VERSION marks the spelling a graph was built with, and incremental_update rebuilds once when it is missing or stale and the graph holds Java or C# nodes. The rebuild runs the real parser, so unlike a SQL data migration there is no second implementation of the transformation to keep in step. cpp_errors becomes failed_languages, since only its emptiness was ever used and the same guard is needed per language: a file that failed to parse keeps its old edges, so the version must not be marked current for that language. C++ behaviour is unchanged.
|
Parking this as a draft rather than merging. The review findings hold up, and the last round showed the approach itself is the problem, not the details. What I verifiedAll three findings reproduce, and all three are introduced by this branch. Each was built with the same source under Spring resolution picks the wrong implementation. This rewrites a CALLS edge, so it propagates into impact radius, flows and review context. C# arity. With Distinct from #940, which is namespace ambiguity already present on Identity-version rebuild loop. One Java file that always fails to parse makes every incremental update a full rebuild, permanently. Filed as #944, since the same loop is reachable on Why parking rather than patchingThe root cause is that this branch makes Fixing both regressions inside the current design means changing Spring resolution and the arity handling as well, which grows this PR into three subsystems. The better move is the separation asked for repeatedly in review: keep the raw spelling, derive a canonical declaration reference that carries arity, and have resolution consume that — rather than reshaping stored data so a bare-name fallback happens to match. Captured in #943 with the reproductions above as constraints and #935 as the motivating test case. State of the branchThe parser work here is still useful input to #943: base names are derived from the Tree-sitter nodes rather than by scanning angle brackets, which is what a character counter gets wrong on Not closing, so the branch and discussion stay available for #943. |
|
Changes required: replacing a constructed inheritance target with its bare name loses arity and permits incorrect binding. Build |
|
This no longer merges into Worth knowing before you resolve:
git fetch origin
git merge origin/staging
# resolve, then
uv run pytest tests/ -q
uv run ruff check code_review_graph/
uv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optional
git pushI have not reviewed the change itself yet. That comes once it merges and the checks run against the merged state, since staging has moved a long way and the result is what matters. |
This branch has not been deployed
3 participants
Summary
Resolve Java and C# generic inheritance against the base type's declaration name instead
of the constructed spelling, so
class Child : GenericBase<string>links toGenericBase. Every type argument is retained on the edge inextra.constructed_types.Fixes #935. Closes #941.
How the target is derived
The declaration name comes from the syntax tree, skipping the type-argument and comment
subtrees. It is deliberately not derived by scanning for
<and>: those charactersalso occur in trivia the grammar ignores, so
class C : I</* < */ string> {}— legal C#— reads as unbalanced to a character counter and would keep the constructed spelling as
its target, leaving the original bug in place.
_get_base_type_nodesis split out of_get_basesso the source spelling and the targetcome from a single traversal.
Repeated generic bases
C# permits one class to implement several closed constructions of the same generic
interface (
class C : I<int>, I<string> {}). These erase to one target and share theclass declaration line, so
upsert_edge's identity —(kind, source, target, file_path, line), which excludesextra— would treat one-edge-per-base as the same edge and keeponly the last. Bases are grouped by target and emitted as one edge recording every
construction.
Existing graphs
Changing a persisted spelling leaves old graphs stale, since nothing reparses an
unchanged file. This uses the mechanism already in the project for that situation,
CPP_IDENTITY_VERSIONinincremental.py: anINHERITS_IDENTITY_VERSIONrecords thespelling a graph was built with, and
incremental_updaterebuilds once when it ismissing or stale and the graph holds Java or C# nodes.
The rebuild runs the real parser, so there is no second implementation of the
normalization to keep in step. An earlier revision of this PR carried a v10 SQL migration
that rewrote rows from stored text; it was dropped because it duplicated the parser's
logic — including comment handling — and both review findings against it landed on that
seam.
cpp_errorsbecomesfailed_languages, since only its emptiness was ever used and thesame guard is needed per language: a file that failed to parse keeps its old edges, so
the version must not be marked current for that language. C++ behaviour is unchanged.
Verified on a graph built with pre-PR code, then updated with no file changes: the
rebuild triggers,
inheritors_of GenericBasereturnsFromGeneric, and a second updatedoes not rebuild again.
What the target is (and is not)
The target is a lookup name, not a language-level declaration identity. It drops
generic arity (
G<T>andG<T, U>both yieldG) and carries no namespace or package.inheritors_ofalready resolves non-generic bases the same way: class nodes are keyed bybare name, so
G<T>andG<T, U>collapse to one node regardless of this PR, and thebare-name fallback already unions same-named classes. This brings generic bases to parity
with that path rather than introducing a new resolution model. The constructed spelling is
preserved losslessly in
extra.constructed_types, so a namespace-aware resolver later hasthe evidence it needs — tracked in #940, deliberately not attempted here since it
reproduces on
mainfor non-generic bases.Safety
INHERITStargets containing type arguments changeTesting
uv run pytest --tb=short -q --cov=code_review_graph --cov-report=term-missing --cov-fail-under=65(2993 passed, 9 skipped, 2 xpassed; 85.07% coverage)uv run ruff check code_review_graph/ tests/test_multilang.pyuv run mypy code_review_graph/ --ignore-missing-imports --no-strict-optionalRegression tests cover nested generic arguments (Java and C#), angle brackets inside
trivia, repeated closed constructions, unbalanced type text, and the upgrade rebuild.