refactor: Phase 8 & 9 - Field Types and Return-Type Binding#494
Conversation
|
@oleg-deezus is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
|
This could be really interesting. I'm doing a major refactor in #488 before I can look into this PR. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 4668 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Thank you for waiting! I'm done with the work, please update your branch and let me know when your PR is ready to be reviewd |
|
Could you please also test this for multiple languages? 🙏 |
…ntation # Conflicts: # gitnexus/package-lock.json
|
I found several significant issues in my code besides the language support, so I marked the PR as a draft. I’ll try to publish the fixes as soon as possible. Sorry for the inconvenience. |
|
Dont worry about it now, I got it sorted locally and will publish it shortly . |
…lt binding
Add Swift field-type resolution and call-result binding integration tests
with fixtures, plus merge-conflict fixes for the FieldExtractor code.
**Swift integration tests:**
- `swift-field-types/` fixture (Models.swift + App.swift) — tests
HAS_PROPERTY edges, field-chain CALLS resolution (user.address.save()
→ Address#save), and ACCESSES edges for field reads.
- `swift-call-result-binding/` fixture — tests call-result binding
(let user = getUser(); user.save() → User#save).
- 2 new describe blocks in swift.test.ts with skipIf(!swiftAvailable).
**Swift arity fix:**
- extractMethodSignature fallback counts direct `parameter` children
when no wrapper list node exists (Swift's tree-sitter grammar places
parameters as direct children of function_declaration). Without this,
all Swift functions had parameterCount: 0 and the arity filter rejected
valid call targets.
**FieldExtractor merge-conflict fixes:**
- field-extractor.ts: update import from removed ./utils.js to
./utils/ast-helpers.js; use typeEnv.fileScope() instead of .get('').
- field-extractors/typescript.ts: same import fix.
- field-types.ts: alias TypeEnvironment as TypeEnv (renamed on main).
- field-extraction.test.ts: mock TypeEnvironment interface properly.
|
I increased the scope of this PR and I'm making your implementation of |
… 14 languages, wired into pipeline Implements field extractors for all supported languages and integrates them into the ingestion pipeline as the single source of truth for Property node metadata. **Generic field extractor factory** — `field-extractors/generic.ts` defines a `createFieldExtractor(config)` factory that generates FieldExtractor instances from a per-language `FieldExtractionConfig`. Each config specifies AST node types, name/type/visibility extraction functions, and static/readonly detection — typically 20-40 lines per language vs 300+ for a hand-written extractor. **Per-language configs** — `field-extractors/configs/` has 11 config files covering 13 languages (TS/JS share, Java/Kotlin share). TypeScript keeps its hand-written extractor for richer handling. **LanguageProvider integration** — New optional `fieldExtractor` property on LanguageProviderConfig, set via defineLanguage() in each language file. Follows the same strategy pattern as typeConfig, exportChecker, and labelOverride. Removed the separate FieldExtractorRegistry class and field-extractors/index.ts — extractors are accessed via getProvider(lang).fieldExtractor. **Pipeline wiring** — Both parse-worker.ts (worker pool) and parsing-processor.ts (sequential fallback) now call the FieldExtractor during Property node creation. Results are cached per class node. Property nodes are enriched with: declaredType, visibility, isStatic, isReadonly. **extractPropertyDeclaredType removed** — The 100-line multi-strategy function in type-extractors/shared.ts is replaced by the FieldExtractor. All 14 languages register an extractor, eliminating the need for a generic fallback. The Python config's extractType was fixed to handle annotation-without-value patterns (address: Address). **Integration tests** — Each language's resolver test file gains pipeline-based assertions verifying visibility/isStatic/isReadonly on Property nodes via getNodesByLabelFull. Tests run through runPipelineFromRepo with real fixtures — no direct extractor calls.
…ntation # Conflicts: # gitnexus/src/core/ingestion/languages/csharp.ts # gitnexus/src/core/ingestion/languages/dart.ts # gitnexus/src/core/ingestion/languages/kotlin.ts # gitnexus/src/core/ingestion/languages/python.ts # gitnexus/src/core/ingestion/languages/ruby.ts # gitnexus/src/core/ingestion/languages/rust.ts # gitnexus/src/core/ingestion/languages/typescript.ts
…n, unskip Dart ACCESSES test The type-env's findEnclosingScopeKey had the same Dart sibling problem as findEnclosingFunction — it walked parents but never found function_signature because the call lives inside function_body (a sibling). Instead of hardcoding a function_body check, thread the provider's enclosingFunctionFinder hook through BuildTypeEnvOptions → lookupInEnv → findEnclosingScopeKey. All three buildTypeEnv call sites (call-processor, parsing-processor, parse-worker) now pass the hook. This enables the type-env to resolve scoped parameter bindings for Dart (e.g., `user: User` in processUser), which lets the chain-resolution tier (Step 1c) walk `user.address` and emit ACCESSES edges. Dart integration test unskipped — 10/10 passing including ACCESSES. Reverted CHANGELOG.md to origin/main.
Code Review: PR #494 — Phase 8 & 9: Field Types and Return-Type BindingReviewers: Architecture Strategist, TypeScript Quality Reviewer, Pattern Recognition Specialist Architecture Verdict: APPROVEThe field-extractor architecture is well-designed:
FindingsCRITICAL (blocks merge):
HIGH (should fix):
MEDIUM:
LOW:
Missing Test Coverage
Risk Assessment
RecommendationAPPROVE with 2 required changes:
All other findings are improvements that can be addressed in follow-up. |
CRITICAL:
- parse-worker.ts: classNode: any → SyntaxNode on getFieldInfo
and findEnclosingClassNode; removed redundant as number casts
- parsing-processor.ts: classNode: any → SyntaxNode on seqGetFieldInfo
HIGH:
- ruby.ts: attr_accessor now extracts ALL symbol arguments via
extractNames hook in generic factory (was firstNamedChild only)
- typescript.ts: added JSDoc explaining why hand-written extractor
coexists with config-based typescript-javascript.ts
MEDIUM:
- field-types.ts: FieldVisibility union type replaces string
('public'|'private'|'protected'|'internal'|'package'|'fileprivate'|'open')
Propagated through field-extractor.ts, generic.ts, all 7 config files
- typescript.ts: extractFullType collapsed from 12 branches to 3 lines
- generic.ts: added extractNames? optional hook + buildField refactor
LOW:
- ruby.ts: extractVisibility(node) → extractVisibility(_node)
- python.ts: fixed misleading isStatic comment
TypeScript compiles cleanly.
CRITICAL:
- parse-worker.ts: classNode: any → SyntaxNode on getFieldInfo
and findEnclosingClassNode; removed redundant as number casts
- parsing-processor.ts: classNode: any → SyntaxNode on seqGetFieldInfo
HIGH:
- ruby.ts: attr_accessor now extracts ALL symbol arguments via
extractNames hook in generic factory (was firstNamedChild only)
- typescript.ts: added JSDoc explaining why hand-written extractor
coexists with config-based typescript-javascript.ts
MEDIUM:
- field-types.ts: FieldVisibility union type replaces string
('public'|'private'|'protected'|'internal'|'package'|'fileprivate'|'open')
Propagated through field-extractor.ts, generic.ts, all 7 config files
- typescript.ts: extractFullType collapsed from 12 branches to 3 lines
- generic.ts: added extractNames? optional hook + buildField refactor
LOW:
- ruby.ts: extractVisibility(node) → extractVisibility(_node)
- python.ts: fixed misleading isStatic comment
TypeScript compiles cleanly.
Generic factory (4 tests): - createFieldExtractor with TypeScript config validates factory itself - Body discovery for interfaces, static/readonly modifiers - Non-type node rejection Python (4 tests): - Annotated class field extraction - Underscore-based visibility: _name=protected, __name=private Go (5 tests): - isTypeDeclaration on type_declaration nodes - Config functions: uppercase=public, lowercase=package visibility - extractType, isStatic, isReadonly C++ (5 tests): - public/private/protected access specifier backward-sibling walk - Default visibility: class=private, struct=public - static/const modifier detection Ruby (6 tests): - attr_accessor multi-symbol: :name, :email, :age → 3 fields - attr_reader=readonly, attr_writer=non-readonly - Multiple attr_* calls in one class Total: 46 tests passing
|
@claude Let's run a final review if this PR is production ready and ready to be merged |
|
Claude finished @magyargergo's task in 2m 34s —— View job Final Production-Readiness ReviewRequested by: @magyargergo Todo
Verdict: ✅ APPROVED — Ready to mergeAll 10 blocking items from the previous review have been resolved in commits Review Findings StatusCRITICAL — Both resolved ✅
HIGH — All resolved ✅
MEDIUM — 2 of 3 resolved
LOW — All resolved ✅
New Test CoverageThe
All previously flagged untested paths are now covered. CI Status
The coverage regression is minor and expected. The new code is net-positive for the test suite. Architecture SummaryThe implementation is well-structured:
This PR is production-ready and safe to merge. |
|
@oleg-deezus Would you want to do something similar for |
|
@magyargergo I'd like to but not sure that I have enough capacity now. I'll monitor this task state and take it to the work if it will be still undone. |
|
I'm planning to refactor method extractors and variable extractors for now. But before, i'm working on unifying the cli and web modules. |
* main: (114 commits) feat(csharp): C# MethodExtractor config (abhigyanpatwari#582) docs: add gitnexus-shared build step before gitnexus-web (abhigyanpatwari#585) chore: add enterprise offering section to README, ignore local_docs/ (abhigyanpatwari#579) fix(eval): exclude litellm 1.82.7 and 1.82.8 due to compatibility issues (abhigyanpatwari#580) feat(java,kotlin): MethodExtractor abstraction with per-language configs (abhigyanpatwari#576) feat: added skip-agents-md cli flag (abhigyanpatwari#517) feat(wiki): Azure OpenAI support for wiki command (abhigyanpatwari#562) refactor: reduce explicit any types (abhigyanpatwari#566) feat(java): method references, worker overload disambiguation, interface dispatch (abhigyanpatwari#540) feat: configure eslint with unused import removal (abhigyanpatwari#564) feat: configure prettier with pre-commit hook (abhigyanpatwari#563) feat: unify web and cli ingestion pipeline (abhigyanpatwari#536) fix/opencode mcp gitnexus timeout (abhigyanpatwari#363) chore: bump version to 1.4.10, update CHANGELOG fix: resolve tree-sitter peer dependency conflicts (abhigyanpatwari#538) chore: bump version to 1.4.9, add CHANGELOG.md refactor: Phase 8 & 9 — Field Types and Return-Type Binding (abhigyanpatwari#494) feat: add COBOL language support with regex extraction pipeline (abhigyanpatwari#498) fix: close remaining Dart language support gaps (abhigyanpatwari#524) refactor: split global BUILT_IN_NAMES into per-language provider fields (abhigyanpatwari#523) ... # Conflicts: # gitnexus/src/core/wiki/llm-client.ts
…patwari#494) * feat(phase8): add field type data structures and extractor interface * feat(phase8): implement TypeScript field extractor * feat-phase9-add-call-result-binding * test-phase8-add-field-extraction-unit-tests * docs: update documentation for Phase 8 and Phase 9 * feat(swift): Phase 8/9 integration tests for field-type and call-result binding Add Swift field-type resolution and call-result binding integration tests with fixtures, plus merge-conflict fixes for the FieldExtractor code. **Swift integration tests:** - `swift-field-types/` fixture (Models.swift + App.swift) — tests HAS_PROPERTY edges, field-chain CALLS resolution (user.address.save() → Address#save), and ACCESSES edges for field reads. - `swift-call-result-binding/` fixture — tests call-result binding (let user = getUser(); user.save() → User#save). - 2 new describe blocks in swift.test.ts with skipIf(!swiftAvailable). **Swift arity fix:** - extractMethodSignature fallback counts direct `parameter` children when no wrapper list node exists (Swift's tree-sitter grammar places parameters as direct children of function_declaration). Without this, all Swift functions had parameterCount: 0 and the arity filter rejected valid call targets. **FieldExtractor merge-conflict fixes:** - field-extractor.ts: update import from removed ./utils.js to ./utils/ast-helpers.js; use typeEnv.fileScope() instead of .get(''). - field-extractors/typescript.ts: same import fix. - field-types.ts: alias TypeEnvironment as TypeEnv (renamed on main). - field-extraction.test.ts: mock TypeEnvironment interface properly. * feat(field-extractors): generic table-driven field extractors for all 14 languages, wired into pipeline Implements field extractors for all supported languages and integrates them into the ingestion pipeline as the single source of truth for Property node metadata. **Generic field extractor factory** — `field-extractors/generic.ts` defines a `createFieldExtractor(config)` factory that generates FieldExtractor instances from a per-language `FieldExtractionConfig`. Each config specifies AST node types, name/type/visibility extraction functions, and static/readonly detection — typically 20-40 lines per language vs 300+ for a hand-written extractor. **Per-language configs** — `field-extractors/configs/` has 11 config files covering 13 languages (TS/JS share, Java/Kotlin share). TypeScript keeps its hand-written extractor for richer handling. **LanguageProvider integration** — New optional `fieldExtractor` property on LanguageProviderConfig, set via defineLanguage() in each language file. Follows the same strategy pattern as typeConfig, exportChecker, and labelOverride. Removed the separate FieldExtractorRegistry class and field-extractors/index.ts — extractors are accessed via getProvider(lang).fieldExtractor. **Pipeline wiring** — Both parse-worker.ts (worker pool) and parsing-processor.ts (sequential fallback) now call the FieldExtractor during Property node creation. Results are cached per class node. Property nodes are enriched with: declaredType, visibility, isStatic, isReadonly. **extractPropertyDeclaredType removed** — The 100-line multi-strategy function in type-extractors/shared.ts is replaced by the FieldExtractor. All 14 languages register an extractor, eliminating the need for a generic fallback. The Python config's extractType was fixed to handle annotation-without-value patterns (address: Address). **Integration tests** — Each language's resolver test file gains pipeline-based assertions verifying visibility/isStatic/isReadonly on Property nodes via getNodesByLabelFull. Tests run through runPipelineFromRepo with real fixtures — no direct extractor calls. * fix(type-env): thread enclosingFunctionFinder through scope resolution, unskip Dart ACCESSES test The type-env's findEnclosingScopeKey had the same Dart sibling problem as findEnclosingFunction — it walked parents but never found function_signature because the call lives inside function_body (a sibling). Instead of hardcoding a function_body check, thread the provider's enclosingFunctionFinder hook through BuildTypeEnvOptions → lookupInEnv → findEnclosingScopeKey. All three buildTypeEnv call sites (call-processor, parsing-processor, parse-worker) now pass the hook. This enables the type-env to resolve scoped parameter bindings for Dart (e.g., `user: User` in processUser), which lets the chain-resolution tier (Step 1c) walk `user.address` and emit ACCESSES edges. Dart integration test unskipped — 10/10 passing including ACCESSES. Reverted CHANGELOG.md to origin/main. * fix: resolve all PR abhigyanpatwari#494 review findings (10 items) CRITICAL: - parse-worker.ts: classNode: any → SyntaxNode on getFieldInfo and findEnclosingClassNode; removed redundant as number casts - parsing-processor.ts: classNode: any → SyntaxNode on seqGetFieldInfo HIGH: - ruby.ts: attr_accessor now extracts ALL symbol arguments via extractNames hook in generic factory (was firstNamedChild only) - typescript.ts: added JSDoc explaining why hand-written extractor coexists with config-based typescript-javascript.ts MEDIUM: - field-types.ts: FieldVisibility union type replaces string ('public'|'private'|'protected'|'internal'|'package'|'fileprivate'|'open') Propagated through field-extractor.ts, generic.ts, all 7 config files - typescript.ts: extractFullType collapsed from 12 branches to 3 lines - generic.ts: added extractNames? optional hook + buildField refactor LOW: - ruby.ts: extractVisibility(node) → extractVisibility(_node) - python.ts: fixed misleading isStatic comment TypeScript compiles cleanly. * test: add 24 field extraction tests for generic factory + 5 languages Generic factory (4 tests): - createFieldExtractor with TypeScript config validates factory itself - Body discovery for interfaces, static/readonly modifiers - Non-type node rejection Python (4 tests): - Annotated class field extraction - Underscore-based visibility: _name=protected, __name=private Go (5 tests): - isTypeDeclaration on type_declaration nodes - Config functions: uppercase=public, lowercase=package visibility - extractType, isStatic, isReadonly C++ (5 tests): - public/private/protected access specifier backward-sibling walk - Default visibility: class=private, struct=public - static/const modifier detection Ruby (6 tests): - attr_accessor multi-symbol: :name, :email, :age → 3 fields - attr_reader=readonly, attr_writer=non-readonly - Multiple attr_* calls in one class Total: 46 tests passing * chore: remove plan doc from PR --------- Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
GitNexus Phase 8 & 9 Implementation
Summary
This PR implements Phase 8 (Field and Property Type Resolution) and Phase 9 (Return-Type-Aware Variable Binding) to achieve production-grade static analysis capabilities.
Phase 8: Field and Property Type Resolution
New Files
src/core/ingestion/field-types.ts- Type definitions for field metadatasrc/core/ingestion/field-extractor.ts- Language-agnostic field extractor interfacesrc/core/ingestion/field-extractors/typescript.ts- TypeScript implementationsrc/core/ingestion/field-extractors/index.ts- Module exportstest/unit/field-extraction.test.ts- 22 unit testsFeatures
?:syntax)Phase 9: Return-Type-Aware Variable Binding
Modified Files
src/core/ingestion/type-extractors/types.ts- Extended PendingAssignment unionsrc/core/ingestion/type-env.ts- FQN-aware return type lookupFeatures
Test Results
Documentation Updated
Commits
5b36d04feat(phase8): add field type data structures and extractor interface296c6b2feat(phase8): implement TypeScript field extractor6dcb00afeat(phase9): add call-result bindinga22ac0ctest(phase8): add field extraction unit testsb18a526docs: update documentation for Phase 8 and Phase 9