feat(native): port Objective-C extractor to Rust#1106
Conversation
Mirrors `src/extractors/objc.ts` in `crates/codegraph-core/src/extractors/objc.rs`. Adds the `tree-sitter-objc` dependency, wires `LanguageKind::ObjC` (`.m`) in the Rust `parser_registry` and `file_collector`, adds `.m` to `NATIVE_SUPPORTED_EXTENSIONS` on the JS side, and registers `OBJC_AST_TYPES` / `OBJC_AST_CONFIG` so the native and WASM engines extract identical `ast_nodes` for Objective-C source. Handles class interfaces / implementations (with `: Superclass`), protocols, instance and class method declarations/definitions with multi-part selectors assembled from leading identifiers and `method_parameter` children, C-level function declarations/definitions, `#import`/`@import` imports, and message expression call extraction.
|
Claude finished @carlos-alm's task —— View job PR Review: feat(native): port Objective-C extractor to RustTodo List:
Phase 1: Root Cause Analysis ✅ Problem Being Solved: Adding Objective-C language support to complete the dual-engine architecture. The native Rust engine lacks Objective-C extraction capabilities that exist in the WASM engine, creating a parser gap that violates the "both engines must produce identical results" principle. Why This Approach: The PR follows the established pattern for adding language support - adds tree-sitter grammar dependency, implements native Rust extractor mirroring existing WASM extractor ( Risk Assessment: Low risk - purely additive change following established patterns, comprehensive test coverage, no breaking changes to existing functionality. Phase 2: Configuration Change Detection ✅ CLEAN - No high-risk configuration changes detected:
Phase 3: Test Integrity ✅ HEALTHY - Test changes are appropriate:
Phase 4: Code Quality Assessment ✅ EXCELLENT - High-quality implementation: Architecture Consistency:
Implementation Quality:
Code Structure:
No Issues Detected: Security, performance, or maintainability concerns. Root Cause AnalysisProblem Being Solved: Engine parity violation - native Rust engine missing Objective-C support that exists in WASM engine Backlog Compliance
Critical ConcernsNone identified. This is a textbook example of proper language support addition. Final Recommendation
Excellent work - this PR exemplifies how language support should be added to codegraph. |
Codegraph Impact Analysis55 functions changed → 30 callers affected across 3 files
|
Greptile SummaryThis PR ports the Objective-C symbol extractor to Rust and simultaneously brings the JS extractor into full parity with tree-sitter-objc v3 grammar, fixing several silent extraction failures that were surfaced in a prior review round.
Confidence Score: 5/5Safe to merge — the Rust extractor is fully tested, all v3 grammar mismatches in the JS extractor are fixed, and both engines now produce matching output for classes, methods, protocols, imports, and calls. Every silent extraction failure identified in prior review rounds has been corrected and covered by regression tests on both the Rust and JS sides. No new correctness issues were found in this pass. No files require special attention. Important Files Changed
Reviews (17): Last reviewed commit: "fix: resolve merge conflicts with main (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "implementation_definition" => { | ||
| // Wraps a `method_definition` inside `class_implementation`. | ||
| if let Some(method) = find_child(&child, "method_definition") { | ||
| if let Some(sel) = build_selector(&method, source) { | ||
| members.push(child_def(sel, "method", start_line(&method))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
implementation_definition inside collect_class_members
collect_class_members is only ever called from handle_class_interface, and class_interface nodes never contain implementation_definition children — those live exclusively inside class_implementation. This branch can never execute and the comment ("Wraps a method_definition inside class_implementation") even acknowledges the mismatch. If the intent was to also populate children for class implementations, collect_class_members would need to be called from handle_class_implementation as well.
There was a problem hiding this comment.
Fixed in de05222 — removed the unreachable implementation_definition branch from collect_class_members. Confirmed: collect_class_members is only invoked by handle_class_interface, so a class_implementation wrapper would never reach it. The Rust test suite still passes (9 ObjC tests).
| "js", "jsx", "mjs", "cjs", "ts", "tsx", "d.ts", "py", "pyi", "go", "rs", "java", "cs", "rb", | ||
| "rake", "gemspec", "php", "phtml", "tf", "hcl", "c", "h", "cpp", "cc", "cxx", "hpp", "kt", | ||
| "kts", "swift", "scala", "sh", "bash", "ex", "exs", "lua", "dart", "zig", "hs", "ml", "mli", | ||
| "m", |
There was a problem hiding this comment.
.m extension collision with MATLAB/Octave
The .m extension is also the canonical extension for MATLAB and GNU Octave source files. Any MATLAB code in a repository will now be silently routed to the ObjC parser and produce garbled or empty symbol output without any error signal. If supporting MATLAB is a future goal, disambiguation will require heuristics (e.g., checking for ObjC-specific tokens like @interface vs MATLAB keywords). Worth at least documenting the known false-positive risk in a comment.
There was a problem hiding this comment.
Documented in de05222 — added a "Known extension collisions" note to SUPPORTED_EXTENSIONS in crates/codegraph-core/src/file_collector.rs calling out the .m overlap with MATLAB/Octave, the resulting silent garbled-symbol output for MATLAB files, and the disambiguation heuristics that would be needed to support MATLAB later. Disambiguation itself is out of scope for this PR — .m is unambiguously routed to the ObjC parser today.
| fn find_objc_parent_class(node: &Node, source: &[u8]) -> Option<String> { | ||
| let mut current = node.parent(); | ||
| while let Some(parent) = current { | ||
| match parent.kind() { | ||
| "class_interface" | ||
| | "class_implementation" | ||
| | "protocol_declaration" => { | ||
| let name_node = find_objc_decl_name(&parent)?; | ||
| let base = node_text(&name_node, source).to_string(); | ||
| // Categories: include `(Cat)` so methods are grouped per category. | ||
| if let Some(cat) = parent.child_by_field_name("category") { | ||
| return Some(format!("{}({})", base, node_text(&cat, source))); | ||
| } | ||
| return Some(base); | ||
| } | ||
| _ => {} | ||
| } | ||
| current = parent.parent(); | ||
| } | ||
| None |
There was a problem hiding this comment.
find_objc_parent_class includes the category suffix in the parent name (e.g. Foo(Cat).catMethod), but the JS findObjCParentClass returns only the bare class name (Foo.catMethod). The Rust behaviour is technically more precise — the JS version is arguably a bug — but the two engines will produce different definitions entries for category methods, which breaks the goal of identical symbol output stated in the PR description. Worth a deliberate decision: fix the JS side to match Rust, or document the intentional divergence.
There was a problem hiding this comment.
Aligned by updating the JS extractor in de05222 — handleClassInterface, handleClassImplementation, and findObjCParentClass now include the (Category) suffix when the grammar emits class_interface/class_implementation with a category field. Per the CLAUDE.md guidance, when engines diverge we fix the less-accurate side rather than document the gap; the qualified parent is the correct behaviour because two categories on the same class can declare same-named methods that need to stay distinguished in the symbol output.
| let module_node = node.child_by_field_name("path") | ||
| .or_else(|| find_child(node, "identifier")); |
There was a problem hiding this comment.
handle_at_import uses wrong field name "path"
The JS extractor looks up childForFieldName('module') for @import declarations, consistent with the grammar's module field on import_declaration / module_import. The Rust side tries child_by_field_name("path"), which is the field name used for #import (preprocessor includes). Since both fall back to the first identifier child, extraction still works — but the path fallback means the named-field fast-path will always miss, and the field name signals an incorrect assumption about the grammar structure.
| let module_node = node.child_by_field_name("path") | |
| .or_else(|| find_child(node, "identifier")); | |
| let module_node = node.child_by_field_name("module") | |
| .or_else(|| find_child(node, "identifier")); |
There was a problem hiding this comment.
Fixed in de05222 — switched handle_at_import to look up child_by_field_name("module") first, matching the JS extractor and the actual module_import grammar field. The identifier fallback is preserved so older grammar revisions still parse.
- Use `module` field (not `path`) for `@import` in Rust to mirror the JS extractor and match the tree-sitter-objc `module_import` grammar field. - Drop the unreachable `implementation_definition` branch from `collect_class_members` — it is only invoked from `handle_class_interface` and `class_interface` nodes do not contain `implementation_definition` children. - Qualify category methods with `(Category)` in the JS extractor so its output matches Rust for `@interface Foo (Cat)` / `@implementation Foo (Cat)` when the grammar emits `class_interface`/`class_implementation` rather than dedicated `category_interface` nodes. Two categories can declare same-named methods, so the qualified parent disambiguates the symbols. - Document the `.m` extension collision with MATLAB/Octave in the file collector since `.m` files are unconditionally routed to the ObjC parser.
| ]); | ||
| expect(totals['unsupported-by-native']).toBe(10); |
There was a problem hiding this comment.
'src/k.m'
The diff removes 'src/k.m' from the input array (because .m is now natively supported and would no longer fall into the unsupported-by-native bucket), but the expected count was not updated. The array now contains 9 paths yet the assertion is toBe(10), so this test fails at runtime.
| ]); | |
| expect(totals['unsupported-by-native']).toBe(10); | |
| expect(totals['unsupported-by-native']).toBe(9); |
There was a problem hiding this comment.
Resolved by post-review merges with main (cfcd5a2 "fix: correct expected count after merge dropped two entries"). The current assertion is toBe(7) after subsequent merges with main removed two additional entries from the unsupported-by-native bucket. Verified locally — tests/parsers/native-drop-classification.test.ts is green.
The native-drop-classification test asserts the count of WASM-only languages. The Clojure PR (#1097) removed src/c.clj and decremented the count from 11 to 10. This PR removes src/k.m (now natively supported via ObjC). After the merge both entries are removed, so the assertion needs to be 9, not 10.
| } | ||
|
|
||
| if parts.is_empty() { | ||
| return None; | ||
| } | ||
| if has_params { | ||
| Some(format!("{}:", parts.join(":"))) | ||
| } else { | ||
| Some(parts.join(":")) | ||
| } | ||
| } | ||
|
|
||
| /// Build a message-expression selector by collecting all `identifier` | ||
| /// children annotated with the `method` field. | ||
| fn build_message_selector(message_node: &Node, source: &[u8]) -> String { | ||
| let mut parts: Vec<String> = Vec::new(); | ||
| let mut has_colon = false; | ||
| for i in 0..message_node.child_count() { | ||
| if let Some(child) = message_node.child(i) { | ||
| if let Some(field) = message_node.field_name_for_child(i as u32) { | ||
| if field == "method" { | ||
| parts.push(node_text(&child, source).to_string()); |
There was a problem hiding this comment.
The Rust comment on line 392 states that tree-sitter-objc lacks a function field on call_expression, so the named-field lookup always misses and the identifier-child fallback is always used. The JS handleCCallExpr in src/extractors/objc.ts also calls childForFieldName('function') but returns immediately when it is null — no fallback, no call recorded. In practice this means C-style calls like printf(...) or CGContextFillRect(...) appear in the native graph but are absent from the JS one. The stated goal of identical output between both engines is not met for this node type.
There was a problem hiding this comment.
Fixed in 7294941 — JS handleCCallExpr now falls back to the first identifier/field_expression child when the function field is missing, matching handle_c_call_expr in the Rust extractor. C-style calls like printf(...) are no longer silently dropped on the JS path. Added a regression test (extracts C-style function calls without a \function` field) in tests/parsers/objc.test.tsthat assertsprintfappears insymbols.calls`. Per CLAUDE.md ("both engines must produce identical results"), fixing the less-accurate side rather than documenting the gap.
Three engine-parity gaps Greptile flagged on the WASM side that broke the "identical output" goal: - @import statements: tree-sitter-objc v3 emits `module_import` not `import_declaration`, so the JS dispatch arm never matched and every `@import Foundation;` was silently dropped. Accept both node types. - C-style calls (printf, CGContextFillRect, …): the grammar lacks a `function` field on `call_expression`, so the named-field lookup always misses. Rust falls back to the first identifier child; JS did not, so every C call was dropped. Add the same fallback. - Message expressions: the grammar tags each keyword identifier with the `method` field rather than exposing a `selector` field, so the JS selector lookup misfired for multi-keyword selectors. Assemble the selector from `method` children with `:` joining, matching `build_message_selector` in the Rust extractor. Also expose `fieldNameForChild` on the `TreeSitterNode` type and add three JS extractor tests covering the new parity behaviour.
|
Addressed the remaining Greptile feedback in 7294941:
Per CLAUDE.md ("both engines must produce identical results"), the parity gaps were fixed on the less-accurate side (JS) rather than documented. |
The v3 tree-sitter-objc grammar emits flat 'identifier' + 'method_parameter' children directly under method nodes (no 'keyword_selector' wrapper) and nests property names under 'struct_declaration > struct_declarator > [pointer_declarator >] identifier' rather than exposing a 'name' field. The JS extractor was still following the old grammar shape, which silently dropped multi-keyword method definitions, never populated parameter children, and omitted all '@Property' members from class children. - buildSelector: assemble selector from flat 'identifier'+'method_parameter' children directly under the method node, matching build_selector in crates/codegraph-core/src/extractors/objc.rs. - extractMethodParams: iterate 'method_parameter' children directly and use the last 'identifier' child as the parameter name (mirrors extract_method_params in the Rust extractor). - collectClassMembers: extract '@Property' names via a deep identifier walk under 'struct_declaration > struct_declarator' (mirrors extract_property_name in the Rust extractor). Added three regression tests covering keyword method definitions with parameters, and pointer + non-pointer '@Property' member names.
|
Addressed the three remaining JS extractor issues flagged in Greptile's review summary (29ad42a):
Verified the v3 grammar shape by dumping a live tree-sitter parse before fixing; the previous JS code paths matched zero nodes (
All 10 ObjC tests pass; full parser suite (467 tests) is green. Per CLAUDE.md ("both engines must produce identical results"), parity gaps were fixed on the less-accurate side (JS) rather than documented. |
…edged) Resolved merge with origin/main (44a3505 R extractor). Combined both ObjC and R additions to: - crates/codegraph-core/src/file_collector.rs SUPPORTED_EXTENSIONS (keep both 'm' and 'r'/'R') - crates/codegraph-core/src/parser_registry.rs LanguageKind::all() (keep ObjC and R in declaration order) - Bumped EXPECTED_LEN from 30 to 31 to match the new variant count. - tests/parsers/native-drop-classification.test.ts: decrement the unsupported-by-native count from 6 to 5 since the merge correctly removed 'src/k.m' from the input array (ObjC is now natively supported) but main's side of the conflict left the old count. This is a merge resolution — no new features, no doc changes needed (ObjC language support docs were already added in an earlier commit on this branch; R docs landed on main with #1102).
…ption (#1106) The JS extractor was looking up 'protocol_qualifiers' which doesn't exist in tree-sitter-objc v3 (the grammar wraps adopted protocols in 'parameterized_arguments' instead). As a result every '@interface Foo : NSObject <Bar, Baz>' silently dropped its 'implements' relations on the JS side, while the Rust extractor correctly extracted them. Mirror handle_class_interface in crates/codegraph-core/src/extractors/objc.rs (parameterized_arguments + the type_name > type_identifier nesting) and add a regression test.
|
Addressed the remaining Greptile P1 from the review summary in dc1c8de:
Also resolved a CONFLICTING merge with main (Groovy + R + Erlang + Solidity landed since the last review pass) in 66296d4 — merge resolution is a separate commit. Per CLAUDE.md ("both engines must produce identical results"), the JS-side fix is the correct response rather than documenting the parity gap. |
1 participant
Summary
tree-sitter-objcdependency and a native Objective-C extractor incrates/codegraph-core/src/extractors/objc.rs..mwithLanguageKind::ObjCand the Rustfile_collector, adds Objective-C toNATIVE_SUPPORTED_EXTENSIONSon the JS side, and wiresOBJC_AST_TYPES/OBJC_AST_CONFIGon both the native and JS sides so the two engines extract identicalast_nodesfor Objective-C source.extractObjCSymbols:class_interface/class_implementation(with: Superclass) asclass,@protocolasinterface, instance and class method declarations / definitions with multi-part selectors assembled from leading identifiers andmethod_parameterchildren, C-level function declarations / definitions,#import/@importimports, and message-expression call extraction.Closes #1071
Test plan
cargo build --release -p codegraph-core(clean build)cargo test -p codegraph-core --lib— 193/193npx tree-sitter build --wasmregeneratestree-sitter-objc.wasmnpx vitest run tests/parsers/objc.test.ts— 5/5npx vitest run tests/parsers/native-drop-classification.test.ts— 13/13