Skip to content

fix(java-enum): resolve enum methods from enum_body_declarations - #984

Open
sahil-mangla wants to merge 1 commit into
DeusData:mainfrom
sahil-mangla:fix/java-enum-extraction
Open

fix(java-enum): resolve enum methods from enum_body_declarations#984
sahil-mangla wants to merge 1 commit into
DeusData:mainfrom
sahil-mangla:fix/java-enum-extraction

Conversation

@sahil-mangla

Copy link
Copy Markdown
Contributor

What does this PR do?

Resolves extraction and calling-edge issues for Java enums that define methods (e.g., isWeekend or label in cp_enum_method_java).

In tree-sitter-java, an enum's methods are declared within an enum_body_declarations block, which is a nested child of enum_body rather than a direct child of the enum declaration itself. Previously:

  • find_class_body resolved only the raw enum_body node.
  • extract_class_methods iterated over this body, encountering constants and separator tokens, but missing the nested enum_body_declarations block. As a result, method definitions were ignored and calling edges failed to resolve.

This change:

  1. Updates find_class_body to correctly scan and inspect the enum_body for nested enum_body_declarations nodes and return them.
  2. Adds enum_body and enum_body_declarations to the list of recognized body containers in push_class_body_children so the AST traverser pushes all method declarations under the correct enum QN namespace.
  3. Complies with the 100-character column format limit in extract_defs.c.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@sahil-mangla
sahil-mangla requested a review from DeusData as a code owner July 9, 2026 17:41
@DeusData DeusData added this to the 0.9.1-rc milestone Jul 9, 2026
@DeusData DeusData added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Jul 9, 2026
@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Thanks. I have triaged this as a 0.9.1-rc Java extraction bug fix. Review should verify that enum_body_declarations handling fixes enum methods without changing normal class/interface method extraction or introducing duplicate method definitions.

@DeusData

DeusData commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Thanks for splitting this out of #893 so quickly — exactly the atomic shape we asked for, and the enum_body_declarations analysis matches tree-sitter-java's grammar. One thing missing before merge: a genuine reproduce-first guard. I ran the existing cp_enum_method_java convergence probe on unmodified main and it already PASSES — its assertions (Enum node exists + ≥1 CALLS edge anywhere) are too loose to detect the bug you're fixing, so nothing in CI currently turns red without your change. Please add an extraction-level test that is RED on main and GREEN with this fix — e.g. in tests/test_extraction.c, extract the Day enum fixture and assert isWeekend/label exist as Method definitions (and/or tighten the probe to assert the enum's methods specifically). With that guard in place this merges immediately.

@DeusData

Copy link
Copy Markdown
Owner

Thanks for this, @sahil-mangla — the Java enum-method extraction fix is genuinely good work, and the java_enum_method test is a proper reproduce-first guard (it fails on main without the enum_body_declarations unwrap). I'd happily merge that part quickly.

One blocker before it can land: the PR also carries an undocumented change to the call-resolution coresrc/pipeline/registry.c (+127/−22), restricting resolve_same_module/resolve_name_lookup on receivers (commits e930bd2 + 078de1c). That's a precision-tightening change to CALLS-edge resolution with all-language blast radius, and it's not mentioned in the PR description. Per our atomic-PR policy it needs to be its own PR so its recall-vs-precision impact can be evaluated on the full corpus independently of the enum fix.

Could you split this into two PRs?

  1. The Java enum fixextract_defs.c (enum_body/enum_body_declarations unwrap + enum-name fallback) + tests/test_extraction.c. This is clean and I'll fast-track it.
  2. The registry resolution-tighteningregistry.c + tests/test_registry.c, with a note on what false positive it targets (looks recursion-related) so we can measure edge impact before merging.

Really appreciate the contribution — just want each change reviewable on its own merits.

@sahil-mangla
sahil-mangla force-pushed the fix/java-enum-extraction branch from 41f4196 to 3d6a29c Compare July 16, 2026 10:20
Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
@sahil-mangla
sahil-mangla force-pushed the fix/java-enum-extraction branch from 3d6a29c to 4c01d66 Compare July 16, 2026 10:24
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData as per requested the PRs are now strictly atomic! with registry fix being its own isolated PR at #1128

@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData I feel this PR is ready to be merged as for others i need to fix some issues before i open them up for review respectively being #1128 and #893

@DeusData

Copy link
Copy Markdown
Owner

Thanks, the requested split is clear and no further contributor action is needed on #984 right now. We will handle the enum-only change through a separate exact-head review of the current diff, commits, and CI before making any merge decision. The registry-resolution work remains isolated in #1128 as requested.

@DeusData

Copy link
Copy Markdown
Owner

Thank you for splitting this out to be enum-only after the earlier review — that was the right call and it made this a much cleaner thing to reason about. The core of it is correct, and I want to be precise about what I verified before getting to the three things that need changing.

What is right, and verified against the vendored grammar. enum_body_declarations really is the tree-sitter-java container for members declared after the ; in an enum body, and rerouting through the class-body path produces Method defs with the correct label, the correct qualified name under the enum, and the right parent edge. That is the fix shape this project wants: production changed to emit the precise label, rather than the test relaxed to accept the imprecise one. Today those methods surface as mislabeled package-scope Function defs, so this is a real accuracy win. As a bonus, enum fields now get seen by extract_class_fields and extract_class_variables too.

Genuinely nice detail: adding enum_body to push_class_body_children shows you spotted the double-extraction trap — the one our Groovy closure comment documents. Without it you would have ended up with the new Method defs and the old mislabeled Function twins. I traced every path I could find and there are no duplicates as written.

Three things to fix, and one of them matters a lot.

1. Enums that declare methods now lose their constants.

extract_enum_members shares find_class_body. Once that redirect hands back enum_body_declarations, the constants are no longer reachable — in tree-sitter-java the enum_constant nodes are children of enum_body, siblings of enum_body_declarations, not inside it. So for exactly the shape this PR targets — an enum that has methods — Day.MON, Day.TUE and friends silently stop being extracted as Variable defs, and main extracts them today.

The fix that keeps everything working: leave find_class_body returning enum_body, and have extract_class_methods / extract_class_fields / extract_class_variables locally descend into the enum_body_declarations child. extract_enum_members then needs no change at all, because its invariant — "I see enum_body's direct children" — is preserved.

Please also add ASSERT(has_def(r, "Variable", "MON")) to the test, and ideally an assertion on the Day.isWeekend qualified name. Right now nothing in any suite guards enum constants, which is why CI stayed green through this.

This is the same defect as the one in your #1128. The review there found extract_enum_members starved of enum_constant siblings by the same mechanism. The fix shape above resolves both. Whichever of the two lands first, the other will need re-diffing against the new main — worth knowing so it does not surprise you.

2. Nested types inside an enum body are no longer walked.

push_nested_class_nodes pushes only direct class-type children and recurses through field_declaration / template_declaration / declaration wrappers. enum_body_declarations is none of those, so a class or interface declared inside an enum body is now missed entirely. Adding enum_body_declarations to that recursion (or pushing it explicitly) closes it.

3. Please gate the enum_body addition to Java.

enum_body is defined in ten of our vendored grammars — Dart and Scala 3 enums have methods too, and TS/TSX/QML list enum_declaration among their class types. Adding it ungated shifts walker routing for all of them without anyone having assessed the effect. The codebase's own precedent is explicit gating; the Groovy closure entry does exactly this. CBM_LANG_JAVA is the right guard here.

While you are in there: the enum_body_declarations entry in that same list looks dead, since it is never a direct child of a class-type node. And the extract_class_def name-fallback extended from Objective-C to also cover Java is not mentioned in the description — enum_declaration has a name field, so we could not find a parse that needs it, and on a malformed tree it could grab the wrong identifier rather than skipping. Either drop it or say what it is for.

One thing to name rather than fix. Methods on individual enum constants with a class body — the anonymous-subclass idiom — are not extracted either before or after this change. But there is a subtle regression: on main they at least appear as mislabeled package-scope Function defs, and with this change they become entirely absent, because push_nested_class_nodes never pushes enum_constant. That moves them from "wrong label, still findable" to "gone", which is the direction we care about most. Deferring the proper fix is completely fine — it raises a real design question about the qualified name (Day.MON.label versus Day.label) — but please note it in the description as a known gap.

Small non-blocking tidy: the find_class_body tail that builds a null node just to return it reduces to return result;.

None of this is a rejection — the diagnosis is right, the label-precision instinct is right, and the double-extraction awareness is better than most contributions in this area get. Fix the constants regression and gate the routing change and this is a clear merge.

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

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants