Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to handle error on undefined type reference #7

Merged
merged 1 commit into from
Oct 19, 2024

Conversation

blurfx
Copy link
Member

@blurfx blurfx commented Oct 19, 2024

This PR does:

  • Raises an error if the undeclared type is referenced.
  • Changed the validate function to behave like toDiagnostics andtoDiagnostics is removed.
    • There should be no difference in how the two functions are validated, so one function is enough for now.

Summary by CodeRabbit

  • New Features

    • Introduced a new way to define array types using the keyword 'Array' in the schema.
    • Added a TypeCollectorListener class for enhanced type validation and error handling.
  • Bug Fixes

    • Improved error handling in the validation process to correctly capture and report type-related errors.
  • Tests

    • Updated test cases to validate the new error handling structure and ensure accurate schema validation.

Copy link

coderabbitai bot commented Oct 19, 2024

Walkthrough

This pull request introduces significant modifications to the YorkieSchema grammar, enhancing its type definition capabilities by adding a new arrayType rule. It includes updates to token definitions, renumbering existing tokens, and restructuring the parsing logic across several files. The validate function in the validator module has been altered to return an array of errors instead of a boolean, necessitating corresponding changes in the test cases. Overall, these changes improve the flexibility and robustness of the schema validation process.

Changes

File Change Summary
antlr/YorkieSchema.g4 Added arrayType rule allowing for Array typeArguments.
antlr/YorkieSchema.interp Updated token literal names with new entries; modified ATN transitions and rule organization.
antlr/YorkieSchema.tokens Renumbered tokens to accommodate new entries, including Array.
antlr/YorkieSchemaLexer.interp Added token T__30 and updated token literal names; modified ATN structure.
antlr/YorkieSchemaLexer.tokens Renumbered tokens, added T__30, and removed hyphen-prefixed tokens.
antlr/YorkieSchemaLexer.ts Added token T__30, updated indices for existing tokens, and modified rule arrays.
antlr/YorkieSchemaParser.ts Updated token constants and parsing logic; modified method signatures for context classes.
src/main.ts Changed import from toDiagnostics to validate and updated return statement in yorkieLinter.
src/validator.ts Introduced TypeCollectorListener class; modified validate function to return errors array.
test/schema.test.ts Updated assertions to check for errors array length; modified tests for unsupported types.

Possibly related PRs

  • Reorganize the grammar to fit TypeScript #4: The changes in this PR involve reorganizing the grammar to fit TypeScript, which is directly related to the modifications made in the YorkieSchema grammar, including the addition of the arrayType rule and other structural adjustments.

🐰 In the land where schemas play,
A new arrayType joins the fray.
With tokens renumbered, all in line,
The validate function now shines divine.
Errors collected, no more a chore,
In Yorkie's realm, we leap and explore! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (2)
antlr/YorkieSchemaLexer.tokens (1)

Line range hint 1-69: Summary: Significant restructuring of lexer tokens - proceed with caution.

The changes to YorkieSchemaLexer.tokens represent a substantial restructuring of the grammar's token definitions. Key points:

  1. Addition of a new token (T__30).
  2. Consistent renumbering of existing tokens.
  3. Significant changes to token string representations, including reordering and the addition of new tokens like 'Array'.

While these changes appear intentional and consistent, they may have far-reaching effects on the entire parsing system and any dependent tools. It's crucial to:

  1. Thoroughly test all components of the system that interact with these token definitions.
  2. Update any documentation that references specific token numbers or names.
  3. Communicate these changes clearly to any users or developers of the grammar.

Consider maintaining a changelog for grammar modifications, especially for breaking changes like these. This can help users of the grammar adapt their systems more easily.

antlr/YorkieSchemaLexer.ts (1)

Line range hint 1-270: Summary: Lexer updated to support new 'extends' keyword.

The changes in this file consistently implement the addition of a new token (T__30) and the 'extends' keyword. These modifications suggest an enhancement to the grammar, possibly introducing inheritance or extension capabilities. The updates to token indices, rule names, literal names, and symbolic names are all consistent with this change.

To ensure the correctness of these changes:

  1. Verify that the corresponding changes have been made in the parser and grammar files.
  2. Regenerate the lexer using ANTLR and compare it with this file to confirm the accuracy of all changes, especially the _serializedATN.
  3. Update any relevant documentation to reflect the new language feature (if 'extends' indeed introduces inheritance or extension capabilities).

Consider adding a comment at the top of the file explaining the new 'extends' keyword and its implications for the language's capabilities. This will help future maintainers understand the purpose of these changes quickly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between bb0831d and 3e428e3.

📒 Files selected for processing (10)
  • antlr/YorkieSchema.g4 (1 hunks)
  • antlr/YorkieSchema.interp (3 hunks)
  • antlr/YorkieSchema.tokens (2 hunks)
  • antlr/YorkieSchemaLexer.interp (4 hunks)
  • antlr/YorkieSchemaLexer.tokens (2 hunks)
  • antlr/YorkieSchemaLexer.ts (4 hunks)
  • antlr/YorkieSchemaParser.ts (23 hunks)
  • src/main.ts (1 hunks)
  • src/validator.ts (4 hunks)
  • test/schema.test.ts (18 hunks)
🧰 Additional context used
🔇 Additional comments (35)
antlr/YorkieSchema.tokens (3)

31-38: Token numbering updated correctly.

The token numbers for Identifier, StringLiteral, NumberLiteral, BooleanLiteral, SingleLineComment, MultiLineComment, and WS have been consistently incremented by 1. This change aligns with the addition of new tokens and maintains the integrity of the token numbering system.


48-55: New 'Array' token added and subsequent tokens renumbered.

The addition of the 'Array' token (line 48) enhances the grammar's capability to handle array types. The subsequent tokens ('number', 'string', 'boolean', 'any', 'void', 'null', 'undefined') have been correctly renumbered to accommodate this new addition. This change aligns with the PR objectives of improving type handling.


56-69: Tokens renumbered consistently, new 'extends' token added.

The token numbers for various symbols and Yorkie-specific types have been correctly incremented. The addition of the 'extends' token (line 69) suggests an enhancement in handling type inheritance or extension.

Could you please clarify the purpose of the new 'extends' token? This addition wasn't explicitly mentioned in the PR objectives or summary. To verify its usage, we can run the following script:

✅ Verification successful

Tokens renumbered consistently, new 'extends' token usage verified.

The addition of the extends token is used within the typeParameter rule to support type inheritance, aligning with standard grammar practices. No issues were found with its implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of 'extends' keyword in the grammar files
rg --type-add 'grammar:*.g4' --type grammar -i '\bextends\b' -C 3

Length of output: 298

antlr/YorkieSchemaLexer.tokens (3)

32-38: LGTM: Consistent renumbering of tokens.

The renumbering of tokens from Identifier to WS is consistent with the addition of the new T__30 token. Each token has been incremented by 1, maintaining the correct order and avoiding conflicts.


31-31: LGTM: New token T__30 added.

The addition of T__30 is consistent with the existing token numbering pattern. This suggests an expansion of the grammar, possibly to support new language features.

To ensure this change is properly integrated, please verify the usage of T__30 in the corresponding grammar file:


48-69: Significant changes to token string representations - ensure comprehensive testing.

The changes to token string representations, including the addition of 'Array'=10 and the reordering of subsequent tokens, suggest a substantial restructuring of the grammar. While these changes appear to be intentional and consistent, they may have far-reaching effects:

  1. The prominence of 'Array' as the first token might indicate a new emphasis on array types in the language.
  2. The reordering and reassignment of tokens could impact existing parsers or tools that rely on these token definitions.

To ensure these changes don't introduce regressions:

  1. Update and run all existing parser tests.
  2. Verify that all tools and systems depending on this grammar are updated and tested with the new token assignments.
  3. Run the following script to check for any inconsistencies in token usage:
✅ Verification successful

Token changes verified successfully.

All tokens in antlr/YorkieSchemaLexer.tokens are consistently used within antlr/YorkieSchema.g4, and no inconsistencies were found. The reordering and reassignment of tokens do not appear to introduce any issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for inconsistencies in token usage across the codebase.

# Test: Compare token usage in .g4 files with the .tokens file. Expect: No mismatches.
tokens=$(cat antlr/YorkieSchemaLexer.tokens | grep "'" | cut -d"'" -f2)
for token in $tokens; do
    echo "Checking usage of token: $token"
    rg --type-add 'antlr:*.g4' --type antlr "$token" antlr/
done

Length of output: 15952


Script:

#!/bin/bash
# Description: Check for inconsistencies in token usage across the codebase with proper escaping.

# Function to escape special regex characters
escape_regex() {
    echo "$1" | sed 's/[][(){}?|&<>.,]/\\&/g'
}

# Extract tokens from the .tokens file
tokens=$(grep "'" antlr/YorkieSchemaLexer.tokens | cut -d"'" -f2)

# Iterate over each token and search its usage in .g4 files
for token in $tokens; do
    # Escape token if it contains special regex characters
    escaped_token=$(escape_regex "$token")
    echo "Checking usage of token: $token"
    rg --type-add 'antlr:*.g4' --type antlr "$escaped_token" antlr/
done

Length of output: 17180


Script:

#!/bin/bash
# Description: Check for inconsistencies in token usage across the codebase using fixed-string search.

# Extract tokens from the .tokens file
tokens=$(grep "'" antlr/YorkieSchemaLexer.tokens | cut -d"'" -f2)

# Iterate over each token and search its usage in .g4 files using fixed-string search
for token in $tokens; do
    echo "Checking usage of token: $token"
    rg -F --type-add 'antlr:*.g4' --type antlr "$token" antlr/
done

Length of output: 14745

src/main.ts (2)

8-8: LGTM: Linter function updated to use new validate function.

The change correctly implements the new validation approach, using validate(code).errors instead of toDiagnostics(code).

To ensure the structure of error objects returned by validate is compatible with the existing mapping logic, please run the following script:

#!/bin/bash
# Description: Check the structure of error objects returned by 'validate'

# Test: Search for the 'validate' function definition. Expect: Function returning an object with an 'errors' property.
ast-grep --lang typescript --pattern 'function validate($_) {
  $$$
  return { errors: $_ };
  $$$
}'

4-4: LGTM: Import statement updated correctly.

The change from toDiagnostics to validate aligns with the PR objectives of consolidating validation into a single function.

To ensure this change doesn't affect other parts of the codebase, please run the following script:

test/schema.test.ts (12)

17-17: LGTM: Updated assertion aligns with new validate function behavior.

The change from a boolean check to verifying the length of the errors array is correct and consistent with the PR's objective.


26-26: LGTM: Consistent update to assertion.

The assertion has been correctly updated to check the length of the errors array, maintaining consistency with the new validate function behavior.


36-36: LGTM: Consistent assertion update.

The assertion has been correctly modified to check the errors array length, maintaining consistency across the test suite.


45-45: LGTM: Appropriate update for invalid syntax test.

The assertion has been correctly updated to check for the presence of errors in the invalid syntax scenario. Using toBeGreaterThan(0) is a good choice as it allows for multiple errors to be reported if necessary.


54-54: LGTM: Consistent update for invalid syntax test.

The assertion has been appropriately modified to check for the presence of errors in the invalid syntax scenario, maintaining consistency with previous changes.


66-66: LGTM: Appropriate update for valid schema test.

The assertion has been correctly modified to check for the absence of errors in a valid schema scenario, aligning with the new validate function behavior.


98-98: LGTM: Consistent update for schema with comments test.

The assertion has been appropriately modified to check for the absence of errors in a valid schema with comments, maintaining consistency with the new validate function behavior.


125-125: LGTM: Consistent update for Yorkie Types test.

The assertion has been appropriately modified to check for the absence of errors in a valid schema with Yorkie Types, aligning with the new validate function behavior.


135-135: LGTM: Appropriate update for Yorkie types with user-defined attributes test.

The assertion has been correctly modified to check for the absence of errors in a valid schema with Yorkie types and user-defined attributes, maintaining consistency with the new validate function behavior.


109-114: Consider the implications of using any type and undefined field.

While the assertion update is correct, the changes to the schema definition raise some concerns:

  1. The use of any type reduces type safety. Consider using more specific types if possible.
  2. Adding an undefined field might lead to unexpected behavior. Ensure this aligns with the intended schema design.

These changes might impact type checking and runtime behavior. Please verify if these modifications are necessary and align with the project's type safety goals.

To ensure these changes don't introduce unintended consequences, please run the following command:

#!/bin/bash
# Check for any new type-related warnings or errors in the codebase
npm run type-check

Line range hint 145-260: LGTM: Consistent updates to assertions, but skipped tests need attention.

The assertions have been appropriately updated to check the errors array length, maintaining consistency with the new validate function behavior.

However, there are still skipped tests:

  1. Line 239: "should restrict unused type definition"
  2. Line 248: "should detect type cycle"

These skipped tests might indicate incomplete implementation or known issues not addressed in this PR. Could you please provide more information about these skipped tests? Are they planned to be implemented in a future PR, or is there a specific reason they remain skipped?

To get more context on the skipped tests, please run the following command:

#!/bin/bash
# List all skipped tests in the file
grep -n "it.skip" test/schema.test.ts

77-88: LGTM: Consistent updates and important test activation.

The assertion for optional properties has been correctly updated. More importantly, the test for unsupported types has been activated and properly updated. This change is crucial as it aligns with the PR's objective of enhancing error handling for undefined type references.

To ensure the new test is working as expected, please run the following command:

antlr/YorkieSchemaLexer.ts (4)

72-73: LGTM: Rule names updated correctly.

The ruleNames array has been properly updated to include "T__30", maintaining consistency with the new token addition.


Line range hint 79-91: LGTM: Literal and symbolic names updated correctly.

The _LITERAL_NAMES array has been updated to include the new literal 'extends', and the _SYMBOLIC_NAMES array has been adjusted to accommodate the new token. This suggests that 'extends' is now a keyword in the grammar, possibly indicating the introduction of inheritance or extension capabilities.

Let's verify the usage of 'extends' in the grammar file:

#!/bin/bash
# Description: Check for the usage of 'extends' in the grammar file
# Expected: 'extends' should be present as a rule or token in the grammar file

rg --type antlr "'extends'" antlr/YorkieSchema.g4

124-261: Verify ATN serialization through ANTLR regeneration.

The _serializedATN string has been significantly modified, which is expected due to the addition of the new token and the 'extends' keyword. However, manually verifying the correctness of this serialized ATN is impractical and error-prone.

To ensure the correctness of these changes:

  1. Regenerate the lexer using ANTLR with the updated grammar file.
  2. Compare the regenerated lexer file with this one to confirm that the changes match.

You can use the following command to regenerate the lexer (adjust paths as necessary):

#!/bin/bash
# Description: Regenerate the lexer using ANTLR
# Note: This command assumes ANTLR is installed and available in the PATH

antlr4ts -o generated antlr/YorkieSchema.g4
diff antlr/YorkieSchemaLexer.ts generated/YorkieSchemaLexer.ts

If there are no differences in the output, it confirms that the current changes are correct.


49-56: LGTM: New token added and indices updated correctly.

The addition of the new token T__30 and the subsequent shift in token indices for Identifier, StringLiteral, NumberLiteral, BooleanLiteral, SingleLineComment, MultiLineComment, and WS are implemented correctly. This change likely corresponds to a grammar rule modification.

To ensure consistency, let's verify that this new token is used in the parser:

antlr/YorkieSchema.interp (3)

Line range hint 12-19: Addition of new token literals for primitive types

The inclusion of new token literals such as 'Array', 'number', 'string', 'boolean', 'any', 'void', 'null', and 'undefined' is appropriate. This update enhances the grammar to support these primitive types, which is necessary for improved type handling in the schema.


Line range hint 74-81: Addition of new token symbolic names

The addition of symbolic names like Identifier, StringLiteral, NumberLiteral, BooleanLiteral, SingleLineComment, MultiLineComment, and WS enhances the parser's ability to recognize and process different token types effectively. This aligns with the updates made to the grammar.


Line range hint 109-199: Updated ATN transitions reflect grammar enhancements

The extensive modifications in the ATN transitions indicate significant updates to the grammar to accommodate the new tokens and rules. This is expected given the additions of new primitive types and token definitions. Ensure that all generated parser and lexer files are up-to-date to maintain consistency across the codebase.

antlr/YorkieSchemaLexer.interp (5)

12-12: Addition of 'Array' to Token Literal Names

The inclusion of 'Array' in the token literal names extends the lexer's capability to recognize array type declarations. This change aligns with the introduction of the arrayType rule and is necessary for proper parsing of array types.


Line range hint 74-81: New Symbolic Names Added

The addition of the symbolic names Identifier, StringLiteral, NumberLiteral, BooleanLiteral, SingleLineComment, MultiLineComment, and WS enhances the lexer's ability to tokenize identifiers, literals, comments, and whitespace effectively. This improvement supports a more comprehensive parsing process.


131-131: Significant Updates to the ATN Structure

The changes to the ATN (Abstract Syntax Notation) reflect substantial modifications in the lexer's parsing logic to accommodate new tokens and rules. While these updates are expected due to the introduced enhancements, it's crucial to ensure they don't introduce regressions or unintended parsing behaviors.

Consider regenerating the lexer and parser and executing the full test suite to validate that the changes function as intended.


Line range hint 74-81: Ensure Consistency in Token Definitions and Usage

With the addition of new tokens and symbolic names, it's essential to maintain consistency across token definitions, symbolic names, and rule names to prevent any parsing inconsistencies or conflicts.

Run the following script to verify the consistency of token assignments:

#!/bin/bash
# Description: Verify that the token definitions, symbolic names, and rule names are consistent.

# Extract token literal names, symbolic names, and rule names from the lexer interpretation file.
grep -E 'token literal names:|token symbolic names:|rule names:' antlr/YorkieSchemaLexer.interp -A 50

# Ensure that each token literal name and symbolic name has a corresponding rule name.

If inconsistencies are found, adjust the token definitions or rule names accordingly to maintain synchronization.

Also applies to: 114-114


114-114: Introduction of T__30 in Rule Names

The addition of T__30 indicates a new token has been defined in the lexer. Ensure that this token corresponds correctly to the new 'Array' literal added to the token literal names and is appropriately handled in the parser.

To confirm the definition and usage of T__30, run the following script:

antlr/YorkieSchemaParser.ts (3)

460-512: Refactored Control Flow in arrayType Method Enhances Readability

The modification of the arrayType method to use a switch statement improves clarity and aligns the parsing logic more closely with the grammar definitions. This refactoring enhances maintainability and reduces the potential for errors associated with complex loop conditions.


1105-1107: Verify Token Offset Calculations in literal Method

In the literal method, the token range check uses an offset calculation:

if (!(((((_la - 33)) & ~0x1F) === 0 && ((1 << (_la - 33)) & ...) !== 0))) {
  this._errHandler.recoverInline(this);
}

Please verify that the offset and bitmask calculations correctly correspond to the intended token range. Incorrect calculations can lead to improper token recognition or parsing errors.


61-68: Ensure Token Constants Are Updated Consistently

The addition of public static readonly T__30 = 31; and the renumbering of subsequent token constants require thorough verification. Please ensure that all references to these token constants throughout the codebase are updated to match the new values. This will prevent potential parsing errors due to mismatched token identifiers.

To assist with this verification, you can run the following script:

✅ Verification successful

Token Constants Are Consistently Updated

All references to the updated token constants in YorkieSchemaParser.ts and YorkieSchemaLexer.ts have been verified and are consistent with the new values. No mismatches or hardcoded old values were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all references to updated token constants are consistent.

# Expected: No occurrences of old token constants or hardcoded old values.

# Search for old token constant assignments.
rg --type typescript 'public static readonly (Identifier|StringLiteral|NumberLiteral|BooleanLiteral|SingleLineComment|MultiLineComment|WS)\s*=\s*(31|32|33|34|35|36|37);'

# Search for usages of old token constants.
rg --type typescript 'YorkieSchemaParser\.(Identifier|StringLiteral|NumberLiteral|BooleanLiteral|SingleLineComment|MultiLineComment|WS)\b'

# Search for hardcoded old token values.
rg --type typescript '\b(31|32|33|34|35|36|37)\b' --context 2

Length of output: 481


Script:

#!/bin/bash
# Description: Verify that all references to updated token constants are consistent by searching TypeScript files explicitly.

# Expected: No occurrences of old token constants or hardcoded old values.

# Search for old token constant assignments.
rg -g '*.ts' 'public static readonly (Identifier|StringLiteral|NumberLiteral|BooleanLiteral|SingleLineComment|MultiLineComment|WS)\s*=\s*(31|32|33|34|35|36|37);'

# Search for usages of old token constants.
rg -g '*.ts' 'YorkieSchemaParser\.(Identifier|StringLiteral|NumberLiteral|BooleanLiteral|SingleLineComment|MultiLineComment|WS)\b'

# Search for hardcoded old token values.
rg -g '*.ts' '\b(31|32|33|34|35|36|37)\b' --context 2

Length of output: 7388

@@ -35,6 +35,7 @@ intersectionType

arrayType
: primaryType ('[' ']')*
| 'Array' typeArguments
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Action Required: Update Parser, Validator, and Documentation

The addition of | 'Array' typeArguments to the arrayType rule is a positive enhancement. However, the following updates are missing to fully integrate this change:

  1. Parser and Validator Updates: Ensure that both parser and validator components are modified to handle the new 'Array' syntax correctly.
  2. Documentation: Update the relevant documentation to reflect the new array type definition, providing clarity for users.

Please address these areas to ensure seamless integration and functionality.

🔗 Analysis chain

Approved: Enhanced array type definition

The addition of | 'Array' typeArguments to the arrayType rule is a valuable enhancement. It allows for more flexible and precise array type definitions, similar to TypeScript's syntax for generic arrays.

To ensure this change is fully integrated and doesn't introduce any issues:

  1. Verify that the parser and validator have been updated to handle this new syntax correctly.
  2. Check for any existing uses of 'Array' as an identifier in current schemas, as this change could potentially break them.
  3. Update documentation to reflect this new way of defining array types.

Let's verify the impact of this change:

This script will help us identify potential conflicts and ensure that necessary updates have been made across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential conflicts with the new 'Array' keyword

# Test 1: Search for uses of 'Array' as an identifier
echo "Searching for uses of 'Array' as an identifier:"
rg --type-add 'schema:*.{yorkie,ys}' --type schema -i '\bArray\b' -g '!antlr/'

# Test 2: Check if parser and validator files have been updated
echo "Checking for updates in parser and validator files:"
git diff origin/main -- '**/parser*.{ts,js}' '**/validator*.{ts,js}'

# Test 3: Check for documentation updates
echo "Checking for documentation updates:"
git diff origin/main -- '**/*.md'

Length of output: 641

Comment on lines +127 to +136
const semanticErrors: Array<Diagnostic> = listener.errors.map((error) => {
return {
severity: 'error',
message: error.message,
range: {
start: { column: error.column, line: error.line },
end: { column: error.column + 1, line: error.line },
},
};
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure error ranges cover the full length of identifiers

Currently, the error range in diagnostics only covers a single character (error.column + 1), which may not encompass multi-character identifiers. For more accurate error highlighting, adjust the range to include the full length of the undefined identifier.

Update the code to include the length of the identifier when collecting errors and adjust the error range accordingly:

  1. Modify enterTypeReference to capture the identifier's length:
 enterTypeReference(ctx: TypeReferenceContext) {
   const typeName = ctx.Identifier().text;
   const { line, charPositionInLine, startIndex, stopIndex } = ctx.Identifier().symbol;
+  const length = stopIndex - startIndex + 1;
   this.typeReferences.push({
     typeName: typeName,
     line: line,
     column: charPositionInLine,
+    length: length,
   });
 }
  1. Include length when pushing errors in the validation logic:
 listener.typeReferences.forEach((ref) => {
   if (!listener.symbolTable.has(ref.typeName)) {
     listener.errors.push({
       message: `Type '${ref.typeName}' is not defined.`,
       line: ref.line,
       column: ref.column,
+      length: ref.length,
     });
   }
 });
  1. Adjust the error range in the diagnostic mapping:
 const semanticErrors: Array<Diagnostic> = listener.errors.map((error) => {
   return {
     severity: 'error',
     message: error.message,
     range: {
       start: { column: error.column, line: error.line },
-      end: { column: error.column + 1, line: error.line },
+      end: { column: error.column + error.length, line: error.line },
     },
   };
 });

By implementing these changes, the diagnostics will accurately highlight the entire undefined identifier, improving the developer experience during debugging.

Committable suggestion was skipped due to low confidence.

const diagnostics: Array<Diagnostic> = [];
const tree = parser.document();
const listener = new TypeCollectorListener();
ParseTreeWalker.DEFAULT.walk(listener as any, tree);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid casting 'listener' to 'any' in ParseTreeWalker.DEFAULT.walk

Casting listener to any bypasses TypeScript's type checking, which can hide potential issues. To maintain type safety, ensure that TypeCollectorListener correctly implements all necessary interfaces.

Apply this diff to fix the issue:

+import { ParseTreeListener } from 'antlr4ts/tree/ParseTreeListener';

-export class TypeCollectorListener implements YorkieSchemaListener {
+export class TypeCollectorListener implements YorkieSchemaListener, ParseTreeListener {
  // class implementation
}

-ParseTreeWalker.DEFAULT.walk(listener as any, tree);
+ParseTreeWalker.DEFAULT.walk(listener, tree);

This change ensures that listener conforms to the expected type without casting, preserving type safety and catching potential errors at compile time.

Committable suggestion was skipped due to low confidence.

Comment on lines +1516 to +1520
public primaryType(): PrimaryTypeContext | undefined {
return this.tryGetRuleContext(0, PrimaryTypeContext);
}
public typeArguments(): TypeArgumentsContext | undefined {
return this.tryGetRuleContext(0, TypeArgumentsContext);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle Optional Return Types in ArrayTypeContext Methods

The methods primaryType() and typeArguments() in ArrayTypeContext now return optional types (PrimaryTypeContext | undefined and TypeArgumentsContext | undefined, respectively) using tryGetRuleContext. Ensure that any code invoking these methods properly handles the possibility of undefined to prevent runtime errors.

Consider implementing null checks or using optional chaining when accessing the return values of these methods. For example:

const primaryType = arrayTypeContext.primaryType();
if (primaryType) {
  // Safe to use primaryType
}

@hackerwins hackerwins self-requested a review October 19, 2024 14:57
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

@hackerwins hackerwins merged commit 5627a9f into yorkie-team:main Oct 19, 2024
2 checks passed
@blurfx blurfx deleted the fix/type-refer branch October 19, 2024 15:00
@hackerwins
Copy link
Member

screen-capture.webm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants