Skip to content

fix(scala): recognize trait as a class-like node, guard same-name kind collisions - #2247

Open
Yyunozor wants to merge 1 commit into
Graphify-Labs:v8from
Yyunozor:fix/scala-trait-classlike
Open

fix(scala): recognize trait as a class-like node, guard same-name kind collisions#2247
Yyunozor wants to merge 1 commit into
Graphify-Labs:v8from
Yyunozor:fix/scala-trait-classlike

Conversation

@Yyunozor

Copy link
Copy Markdown
Contributor

Bug

Scala trait declarations are invisible to structural extraction: no node
for the trait, and its extends/with heritage is never evaluated.

Root cause: _SCALA_CONFIG.class_types (graphify/extract.py) only listed
class_definition/object_definition. tree-sitter-scala parses trait Foo
as its own kind, trait_definition, so it never entered walk()'s
class-node branch: no node was created, and members fell to the generic
recurse fallback, which resets parent_class_nid to None (misattributed
to file scope). trait_definition reuses the template_body shape already
in body_fallback_child_types, so the existing heritage/field-reference
code picks it up unchanged once dispatched — confirmed against the
grammar's own parse tree and a live graphify update . run, not just
source review.

Recognizing this third class-like kind exposes a real second gap:
class_nid has no notion of node kind, so two independently real,
differently-typed definitions sharing a name in one file — trait Foo +
companion object Foo — collide onto one id, merging both definitions'
members onto whichever is registered first. Verified this isn't
hypothetical: it already happens today, before this PR, for the more
common case class Point + companion object Point
(class_definition/object_definition alone trigger it). This PR adds a
Scala-scoped guard — track which kind first claims an id, salt a later
differently-kinded claim's id — so recognizing traits doesn't add to an
existing problem.

Related work

#1792 (Synvoya, open since 2026-07-11) proposes the same class_types
addition alone. #2042 (sub4biz, open since 2026-07-19) documents the
same root cause plus adjacent gaps, and flags #1792's diff alone would
newly introduce this id collision. I traced the root cause and collision
independently while on #2052, then found both — this PR folds in the guard
#2042 raised as an open question, and shows it covers a pre-existing bug
neither attributes to.

Testing

New tests/test_scala_trait_classlike.py (11 tests, own fixtures): node
creation, heritage onto real (non-stub) nodes, member ownership, negative
space (collisions, both orders), a warm-cache rerun, and an affected
consumer path. Full suite + Ruff clean on Python 3.10/3.12, identical
pre-existing failures both sides, delta exactly the 11 new tests.

Known limitation

A same-named external reference still resolves to whichever definition
kept the plain id — the same ambiguity any homonymous top-level Scala pair
already has, not something this narrow fix resolves. A project-wide
kind-aware id scheme is a natural follow-up, left out to keep this diff
minimal. #2042's given/extension/enum/function_declaration gaps are
also untouched here.

…d collisions

trait_definition was absent from _SCALA_CONFIG.class_types, so a trait never
entered walk()'s class-node branch: no node for the trait itself, its
extends_clause heritage never evaluated (no inherits/mixes_in edges), and
every member underneath it fell to the generic recurse fallback, which
resets parent_class_nid to None (fields/methods misattributed to file
scope). trait_definition reuses the same template_body shape already
declared in body_fallback_child_types, so the existing heritage/field-
reference handling picks it up unchanged once dispatched.

Recognizing a third class-like Scala node kind exposes that node ids
(stem + namespace + name, with no notion of kind) let two independently
real, differently-typed definitions sharing a name in the same file collide
onto one id — trait Foo + object Foo, or, verified independently, the
already-possible (pre-existing, unrelated to this change) case class Point +
companion object Point. Without a guard the second definition's members
silently merge onto the first's node. Scala-scoped fix: track which node
type first claimed each id and salt a later, differently-typed claim's id
instead of reusing it.

@safishamsi safishamsi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Yyunozor. The trait-as-class-like recognition is exactly right (one-line config addition; traits reuse template_body, existing class/object handling untouched), and the tests are thorough.

The blocker is the same-name kind-collision guard. Salting the second same-named definition's id (class_nid = _make_id(class_nid, t)) is fine within a file, but it regresses cross-file resolution on Scala's most common idiom (a class/trait plus its companion object). _rewire_unique_stub_nodes only collapses a sourceless stub onto a real definition when the label is unique; with the salted id there are now two real candidates for the label, so the rewire bails and every cross-file reference to a class+companion (or trait+companion) pair lands on a dangling sourceless stub instead of the real node. Reproduced with a two-file corpus (case class Point + object Point, referenced from another file): on this PR the reference target is a ('Point','') stub, where v8 resolves to the real node. For a tool whose core value is affected, that's a functional regression, and the PR's tests only pin the same-file case.

Requested change: keep the trait recognition; fix the rewire side so a kind-split companion pair still resolves — when all same-label real candidates come from one source file and one holds the plain (unsalted) id, rewire the stub to that plain-id holder instead of bailing on non-uniqueness. Alternatively, split the trait recognition (which I'd merge as-is) from the collision guard into separate PRs. Happy to re-review once the cross-file rewire is covered (a two-file class+companion reference test would pin it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants