Add reference tracking and circular reference detection #8
Conversation
WalkthroughThe changes focus on enhancing the type validation mechanism in the Yorkie schema validation system. The Changes
Sequence DiagramsequenceDiagram
participant Validator
participant TypeCollectorListener
participant SymbolTable
participant ReferenceMap
Validator->>TypeCollectorListener: Parse Schema
TypeCollectorListener->>SymbolTable: Store Symbol Metadata
TypeCollectorListener->>ReferenceMap: Track Type References
TypeCollectorListener->>Validator: Validate Schema
Validator->>ReferenceMap: Check Type Definitions
Validator->>SymbolTable: Verify Symbol Integrity
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/validator.ts(3 hunks)test/schema.test.ts(2 hunks)
🔇 Additional comments (3)
src/validator.ts (1)
158-175:⚠️ Potential issuePotential infinite loop in circular reference detection
The while loop for detecting circular references could lead to an infinite loop if
currentis never updated toundefinedor'Document'. Ensure that all possible paths eventually break the loop.Verify that all type references will eventually reach a base case. Consider adding a maximum iteration limit or additional checks if necessary.
To confirm that the loop terminates correctly, you can add logging or use debugging tools during development.
test/schema.test.ts (2)
Line range hint
248-256: Updated test to check for circular referencesThe test previously named
'should detect type cycle'has been updated to'should detect circular references'. This improves clarity. Verify that the test accurately detects circular references using the new validation logic.Run the test to ensure it passes:
✅ Verification successful
Test properly validates circular type references
The test case correctly creates a circular reference scenario (Hello ↔ World) that matches the validation logic's cycle detection algorithm. The rename from 'type cycle' to 'circular references' improves clarity and aligns with the error messages produced by the validator.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
# Run the specific test case npm test -- -t 'should detect circular references'Length of output: 152
Script:
#!/bin/bash # Find the complete test implementation ast-grep --pattern 'it($_, () => { const schema = ` type Hello = { field1: string; field2: World; }; type World = { field1: string;$$$ });' # Search for validation logic related to circular references rg -l "circular.*reference" --type tsLength of output: 257
Script:
#!/bin/bash # Get the complete test case with context rg -B 2 -A 10 "should detect circular references" test/schema.test.ts # Check the validation logic rg -B 2 -A 10 "circular.*reference" src/validator.tsLength of output: 779
Line range hint
239-246: Activated test for unused type definitionsThe test
'should restrict unused type definition'is now active. Ensure that the validation logic correctly identifies and reports unused type definitions.Run the test to confirm it behaves as expected:
✅ Verification successful
Validation for unused type definitions is properly implemented
The test case correctly verifies that the schema validator detects and reports unused type definitions. The validator implementation confirms this by checking if all defined types are reachable from the root 'Document' type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
# Run the specific test case npm test -- -t 'should restrict unused type definition'Length of output: 162
Script:
#!/bin/bash # Find validate function implementation ast-grep --pattern 'function validate($_) { $$$ }' # Find validate related files rg -l "validate.*schema" --type ts # Look for similar test patterns in the test file cat test/schema.test.tsLength of output: 6431
Script:
#!/bin/bash # Find validator implementation cat src/validator.ts # Search for unused type validation logic rg -A 5 "unused.*type" --type tsLength of output: 5792
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/validator.ts (1)
36-37:⚠️ Potential issueAdd null check for parent property
The nullable
parentproperty could lead to runtime errors when accessed with non-null assertion inenterTypeReference.Consider one of these solutions:
- Initialize parent appropriately
- Add null check before usage
- Implement
exitTypeAliasDeclarationto reset parent
🧹 Nitpick comments (4)
src/validator.ts (4)
41-47: Consider adding validation for duplicate type declarationsThe
symbolTable.set()call will silently overwrite any existing type declaration with the same name.Add duplicate type declaration check:
enterTypeAliasDeclaration(ctx: TypeAliasDeclarationContext) { const typeName = ctx.Identifier().text; const { line, charPositionInLine } = ctx.Identifier().symbol; + if (this.symbolTable.has(typeName)) { + this.errors.push({ + message: `Duplicate type declaration '${typeName}'`, + line, + column: charPositionInLine, + }); + } this.symbolTable.set(typeName, { name: typeName, line: line, column: charPositionInLine, }); this.parent = typeName; }
145-145: Address TODO comment about performance optimizationThe current implementation has O(V + E) complexity where V is the number of types and E is the number of references.
Consider these optimizations:
- Cache the results of cycle detection
- Use topological sorting for a single-pass solution
- Implement parallel validation for independent type hierarchies
Would you like me to provide a detailed implementation for any of these approaches?
156-175: Enhance circular reference error message with cycle pathThe current error message only shows the start and end of the cycle. Including the complete path would make debugging easier.
if (visited.has(current)) { + const cycle = Array.from(visited).join(' -> '); listener.errors.push({ - message: `Circular reference detected: ${current} -> ${symbol.name}`, + message: `Circular reference detected: ${cycle} -> ${current}`, line: symbol.line, column: symbol.column, }); break; }
177-184: Improve error message for types not in documentThe error message could be more specific about the requirement for types to extend from 'Document'.
if (!current || current !== 'Document') { listener.errors.push({ - message: `Type '${symbol.name}' is not in the document.`, + message: `Type '${symbol.name}' must extend from 'Document' either directly or through inheritance.`, line: symbol.line, column: symbol.column, }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/validator.ts(3 hunks)
🔇 Additional comments (2)
src/validator.ts (2)
19-23: RenameTypeSymbolto avoid conflict with built-inSymboltypeThe custom
TypeSymboltype might cause confusion with JavaScript's built-inSymboltype.Apply this diff:
-type TypeSymbol = { +type SchemaTypeSymbol = { name: string; line: number; column: number; };
108-111: LGTM! Improved error range calculationThe changes correctly calculate the error range for multi-character tokens, improving error reporting accuracy.
What this PR does / why we need it:
Add reference tracking and circular reference detection
Which issue(s) this PR fixes:
Addresses yorkie-team/yorkie#971
Summary by CodeRabbit
New Features
Tests
Bug Fixes