fix(language): some references are not resolved; suppress validation on previous errors #273
Conversation
…on previous errors
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the boolean validation flag with a structured validation object supporting staged stop flags; moves enforcement into ZModelDocumentBuilder’s constructor; refactors linking to use ReferenceInfo and to traverse references via AstUtils.streamReferences; bumps VSCode extension version. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job PR Code ReviewReviewing PR: fix(language): some references are not resolved; suppress validation on previous errors Todo List:
Review SummaryThis PR makes targeted improvements to ZenStack's language processing pipeline, addressing both reference resolution and validation behavior. The changes are well-focused and improve the robustness of the system. Code Quality & Best Practices ✅Strengths:
Minor improvement opportunity:
Bug Fixes & Issues Addressed ✅The PR addresses two key issues:
Performance Considerations ✅Improvements:
Impact: Positive performance impact, especially for documents with errors. Security Concerns ✅Assessment: No security concerns identified. The changes:
Technical Analysis
Test Coverage Implications
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues where some references were not being resolved in the ZModel language implementation and adds validation suppression when errors occur in earlier processing stages.
- Replaced custom reference resolution logic with Langium's built-in
AstUtils.streamReferences()utility - Modified document builder to suppress validation when lexing, parsing, or linking errors are present
- Updated document loading configuration to explicitly specify validation stop conditions
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/language/src/zmodel-linker.ts | Simplified reference resolution by using Langium's utilities instead of manual iteration |
| packages/language/src/zmodel-document-builder.ts | Added constructor logic to configure validation suppression on previous stage errors |
| packages/language/src/index.ts | Updated document loading to use explicit validation configuration instead of boolean flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/language/src/zmodel-linker.ts (1)
496-501: Bug: extraScopes are ignored during default linkingresolveDefault now calls doLink directly, bypassing linkReference and losing the extraScopes resolution path. This breaks scoped resolutions like enum-field disambiguation in resolveDataField and transitive references in resolveAttributeArg.
Apply this diff so resolveDefault uses linkReference per reference and honors extraScopes:
- AstUtils.streamReferences(node).forEach((ref) => this.doLink(ref, document)); + AstUtils.streamReferences(node).forEach(({ container, property }) => { + this.linkReference(container, property, document, extraScopes); + });
🧹 Nitpick comments (1)
packages/language/src/zmodel-document-builder.ts (1)
15-19: Handle undefined validation configIf updateBuildOptions.validation is undefined, current logic leaves it unchanged. Prefer defaulting to stopFlags to ensure staged early‑exit works without explicit config.
- if (validationOptions === true) { + if (validationOptions === true || validationOptions === undefined) { validationOptions = stopFlags; } else if (typeof validationOptions === 'object') { validationOptions = { ...validationOptions, ...stopFlags }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/src/index.ts(1 hunks)packages/language/src/zmodel-document-builder.ts(1 hunks)packages/language/src/zmodel-linker.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/zmodel-linker.tspackages/language/src/zmodel-document-builder.tspackages/language/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (2)
packages/language/src/zmodel-document-builder.ts (1)
21-24: LGTM: centralizing validation behavior in the builderConstructor-based override of updateBuildOptions is clean and aligns with the staged validation approach.
Please confirm we don’t rely elsewhere on mutating BuildOptions at call sites beyond index.ts, to avoid divergent behavior.
packages/language/src/index.ts (1)
73-77: LGTM: switch to structured validation flagsPassing explicit stopAfter* flags to build is consistent with the new staged validation approach. Slightly redundant with the builder’s constructor override, but harmless.
Confirm DocumentBuilder here is the custom ZModelDocumentBuilder so the constructor override is in effect.
|
Claude finished @ymc9's task —— View job PR Code ReviewReviewing PR: fix(language): some references are not resolved; suppress validation on previous errors Todo List:
Review SummaryThis PR makes targeted improvements to ZenStack's language processing pipeline, addressing both reference resolution and validation behavior. The changes are well-focused and improve the robustness of the system. Code Quality & Best Practices ✅Strengths:
Minor improvement opportunity:
Bug Fixes & Issues Addressed ✅The PR addresses two key issues:
Performance Considerations ✅Improvements:
Impact: Positive performance impact, especially for documents with errors. Security Concerns ✅Assessment: No security concerns identified. The changes:
Technical Analysis
Test Coverage Implications
|
|
fixes #259 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/language/src/zmodel-linker.ts (2)
109-126: Use the target’s document when creating node descriptionsCreating the description with the referencing document can misassociate URIs for cross-document targets. Prefer the target’s document (if available) to keep “go to definition” and indexing consistent.
If available in your Langium version, consider:
- targetDoc = AstUtils.getDocument(target) ?? document
- reference._nodeDescription = this.descriptions.createDescription(target, target.name, targetDoc)
Please confirm whether AstUtils.getDocument (or an equivalent) is available in your version; otherwise, relying solely on setting reference._ref may be sufficient here.
395-402: TransitiveFieldReference: ensure default linking doesn’t overwrite custom-scope resolutionThese custom-scope resolutions are correct, but subsequent generic linking of the same ReferenceExpr can override them. The early-return guard in linkReference and the filtered linking in resolveDefault (see suggestions) prevent that.
Please confirm that, with the guard in place, item.target won’t be re-linked by later passes (e.g., when the ReferenceExpr node itself is visited).
Also applies to: 408-415
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/ide/vscode/package.json(1 hunks)packages/language/src/zmodel-linker.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ide/vscode/package.json
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/language/src/zmodel-linker.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/language/src/zmodel-linker.ts (3)
13-13: Importing ReferenceInfo is correct for the new ReferenceInfo-based linkingLooks good.
101-101: Verify DefaultLinker.doLink signature (2-arg vs 1-arg)Some Langium versions expose doLink(info: ReferenceInfo) while newer ones accept (info, document). Ensure the two-argument call matches your version to avoid TS2554.
If your Langium only supports a single arg, use:
- this.doLink(refInfo, document); + this.doLink(refInfo);
270-270: Good change: link InvocationExpr.function via ReferenceInfoSwitching to ReferenceInfo here aligns with streamReferences-based linking and centralizes behavior.
|
Claude finished @ymc9's task —— View job PR Code ReviewReviewing PR: fix(language): some references are not resolved; suppress validation on previous errors Todo List:
Review SummaryThis PR makes targeted improvements to ZenStack's language processing pipeline, addressing both reference resolution and validation behavior. The changes are well-focused and improve the robustness of the system. Code Quality & Best Practices ✅Strengths:
Minor improvement opportunity:
Bug Fixes & Issues Addressed ✅The PR addresses two key issues:
Performance Considerations ✅Improvements:
Impact: Positive performance impact, especially for documents with errors. Security Concerns ✅Assessment: No security concerns identified. The changes:
Critical Issues to Address
Suggested fix: AstUtils.streamReferences(node).forEach((ref) => {
this.linkReference(ref, document, extraScopes);
});
private linkReference(refInfo: ReferenceInfo, document: LangiumDocument, extraScopes: ScopeProvider[]) {
const defaultRef = refInfo.reference as DefaultReference;
if (defaultRef._ref) {
// already linked
return;
}
// ... rest of method
}Technical Analysis
Test Coverage Implications
|
Summary by CodeRabbit
Bug Fixes
Refactor
Chores