fix(r): setMethod emits call edge, not duplicate definition#1125
Conversation
In idiomatic S4 code, `setGeneric("greet", ...)` followed by
`setMethod("greet", "Person", ...)` produced two `function` definition
nodes for the same name. Split the handler: `setGeneric` still emits a
function definition (the generic), while `setMethod` emits a call edge
to the generic — modelling the implementation registration without
duplicating the symbol. The method body's calls are still picked up by
the recursive walk of the anonymous function argument.
While auditing the WASM extractor, found three sibling handlers that
silently produced no output because they didn't unwrap the `argument`
node that tree-sitter-r places around each positional string literal:
setClass, setGeneric, and source(). The native (Rust) extractor handled
this correctly via `first_argument_value`. Introduced a parity helper
`firstStringArgument` mirroring the Rust logic and routed all three
handlers through it. Closes #1109.
|
Claude finished @carlos-alm's task —— View job Code Review CompleteReviewing PR #1125: "fix(r): setMethod emits call edge, not duplicate definition" Review Checklist:
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR addresses a legitimate technical issue where R S4 idiomatic code ( Why This Approach: The fix correctly models the semantic difference:
This is architecturally sound and fixes the root cause rather than masking symptoms. Phase 2: Configuration Change Detection ✅No concerning configuration changes detected: No biome rules disabled, no test thresholds lowered, no build configuration modified, no TypeScript strict flags disabled. This is pure code logic improvement. Phase 3: Test Integrity ✅Tests demonstrate correctness, not weakness:
The test plan shows comprehensive validation:
Phase 4: Code Quality Review ✅Excellent implementation quality: Rust extractor (
TypeScript extractor (
No significant issues detected:
Minor observation: The Phase 5: Backlog Compliance Assessment ✅Root Cause AnalysisProblem Being Solved: Fixes duplicate function definition nodes for R S4 method dispatch patterns, which broke resolution in both engines Backlog Compliance
Critical ConcernsNone identified. This is a solid bug fix with excellent test coverage. Final Recommendation
|
Greptile SummaryThis PR fixes a graph correctness bug where idiomatic S4 R code (
Confidence Score: 5/5Safe to merge — the change is a targeted correctness fix with no destructive side effects on existing symbol extraction. Both extractors (Rust and TypeScript) are updated in lock-step and converge on identical semantics. The firstStringArgument helper correctly handles both bare string nodes and the argument-wrapped form that tree-sitter-r uses for positional args. The refactored handleSetMethod emits a call edge rather than a duplicate definition, which is the right model. Seven new tests directly cover the fixed behaviors and Rust/WASM parity, and the existing 2000+ test suite shows no regressions. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["handleCall(node)"] --> B{funcName?}
B -->|setGeneric| C["handleSetGeneric(node)"]
B -->|setMethod| D["handleSetMethod(node)"]
B -->|setClass / setRefClass| E["handleSetClass(node)"]
B -->|source| F["handleSourceCall(node)"]
B -->|library / require| G["handleLibraryCall(node)"]
B -->|other| H["emit call edge\n{ name: funcName }"]
C --> I["firstStringArgument(node)"]
D --> I
E --> I
F --> I
I --> J{arg.type?}
J -->|'string'| K["stripQuotes → return name"]
J -->|'argument'| L["childForFieldName('value')"]
L -->|string node| K
L -->|null| M["fallback: iterate children\nfind first 'string'"]
M --> K
J -->|other| N["return null"]
C --> O["ctx.definitions.push\n{ kind: 'function' }"]
D --> P["ctx.calls.push\n{ name: genericName }"]
E --> Q["ctx.definitions.push\n{ kind: 'class' }"]
F --> R["ctx.imports.push\n{ names: 'source' }"]
style D fill:#d4edda,stroke:#28a745
style P fill:#d4edda,stroke:#28a745
style I fill:#fff3cd,stroke:#ffc107
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/r-setmethod..." | Re-trigger Greptile |
Codegraph Impact Analysis12 functions changed → 10 callers affected across 2 files
|
1 participant
Summary
setGeneric("greet", ...)+setMethod("greet", "Person", ...)) produced twofunctiondefinition nodes with the same name in both engines, breaking resolution. Split the handler sosetGenericemits a function definition (the generic) andsetMethodemits a call edge to the generic, modelling the implementation registration without duplicating the symbol.setClass,setGeneric,source()) that silently produced no output because they did not unwrap theargumentnode that tree-sitter-r places around each positional string literal. The native (Rust) extractor handled this correctly viafirst_argument_value. Introduced a parity helperfirstStringArgumentmirroring the Rust logic and routed all three handlers through it.Test plan
cargo test r_lang::— 19/19 pass (3 new:set_method_does_not_duplicate_generic_definition,set_method_emits_call_to_generic,set_method_body_calls_are_still_captured)npx vitest run tests/parsers/r.test.ts— 11/11 pass (4 new parity guards:setClass,setGeneric,setMethodbody captures,source()import)npx vitest run tests/parsers tests/integration tests/graph tests/unit— 2233/2235 pass (the 2 failures are pre-existing flaky tests intests/unit/snapshot.test.tsunrelated to this change; verified by running them onorigin/mainwhere they also fail)npm run lint)