fix: ts parser #55
Merged
fix: ts parser #55
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates focus on enhancing parser functionalities and build processes across various languages including JavaScript, TypeScript, TSX, and Vue. Notable changes include modifying test directives, adding new test functions, adjusting build commands and dependencies, and refining parser declarations. These adjustments aim to improve the development workflow and ensure a more efficient and accurate parsing and building process for the involved languages. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
…ew grammar which is commented out
ilevyor
commented
Mar 25, 2024
View earlier commitsOn commit 458a4da, Grit tried to heal these tests:
On commit c20a282, Grit tried to heal these tests:
On commit 72c5740, Grit tried to heal these tests:
Tip You can view and edit CI healing settings on the Grit App. |
ilevyor
commented
Mar 25, 2024
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (19)
crates/cli_bin/tests/snapshots/apply__output_jsonl.snapis excluded by:!**/*.snapcrates/core/src/snapshots/marzano_core__test__clone_activites.snapis excluded by:!**/*.snapcrates/wasm-bindings/wasm_parsers/tree-sitter-javascript.wasmis excluded by:!**/*.wasmcrates/wasm-bindings/wasm_parsers/tree-sitter-tsx.wasmis excluded by:!**/*.wasmcrates/wasm-bindings/wasm_parsers/tree-sitter-typescript.wasmis excluded by:!**/*.wasmresources/language-metavariables/tree-sitter-javascript/src/grammar.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-javascript/src/node-types.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/build/Release/obj.target/tree_sitter_javascript_binding/src/parser.ois excluded by:!**/*.oresources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/build/Release/obj.target/tree_sitter_javascript_binding/src/scanner.ois excluded by:!**/*.oresources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/src/grammar.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/src/node-types.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/tree-sitter-javascript.wasmis excluded by:!**/*.wasmresources/language-metavariables/tree-sitter-typescript/tsx/src/grammar.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-typescript/tsx/src/node-types.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-typescript/typescript/src/grammar.jsonis excluded by:!**/*.jsonresources/language-metavariables/tree-sitter-typescript/typescript/src/node-types.jsonis excluded by:!**/*.jsonresources/node-types/javascript-node-types.jsonis excluded by:!**/*.jsonresources/node-types/tsx-node-types.jsonis excluded by:!**/*.jsonresources/node-types/typescript-node-types.jsonis excluded by:!**/*.json
Files selected for processing (15)
- crates/core/src/test.rs (1 hunks)
- crates/language/src/tsx.rs (1 hunks)
- crates/language/test.ts (1 hunks)
- resources/edit_grammars.mjs (1 hunks)
- resources/language-metavariables/tree-sitter-javascript/grammar.js (9 hunks)
- resources/language-metavariables/tree-sitter-typescript/build/Makefile (1 hunks)
- resources/language-metavariables/tree-sitter-typescript/build/tree_sitter_typescript_binding.target.mk (1 hunks)
- resources/language-metavariables/tree-sitter-typescript/common/define-grammar.js (4 hunks)
- resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/build/Makefile (1 hunks)
- resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/build/tree_sitter_javascript_binding.target.mk (1 hunks)
- resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/grammar.js (9 hunks)
- resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/src/tree_sitter/parser.h (4 hunks)
- resources/language-metavariables/tree-sitter-vue/node_modules/tree-sitter-html/build/Makefile (1 hunks)
- resources/metavariable-grammars/javascript-metavariable-grammar.js (9 hunks)
- resources/metavariable-grammars/typescript-metavariable-define-grammar.js (4 hunks)
Files not summarized due to errors (5)
- resources/language-metavariables/tree-sitter-javascript/grammar.js: Error: Message exceeds token limit
- resources/language-metavariables/tree-sitter-typescript/common/define-grammar.js: Error: Message exceeds token limit
- resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/grammar.js: Error: Message exceeds token limit
- resources/metavariable-grammars/javascript-metavariable-grammar.js: Error: Message exceeds token limit
- resources/metavariable-grammars/typescript-metavariable-define-grammar.js: Error: Message exceeds token limit
Additional comments: 39
crates/language/test.ts (2)
- 3-7: The
RouteResponsesinterface defines three properties with the typeobject. Using theobjecttype is not recommended as it is too broad and does not provide specific information about the shape of the object. Consider defining more specific types for these properties to improve type safety and code readability.- 32-41: The implementation of the
getRouteVariantcommand uses template literals to construct the URL, which is generally safe. However, ensure thatrouteVariantis sanitized or validated elsewhere in the code to prevent potential injection vulnerabilities when constructing URLs or making requests based on user input.resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/src/tree_sitter/parser.h (1)
- 166-172: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [169-179]
The
SHIFTmacro is defined to create a shift action for the parser. It's important to ensure that thestate_valuepassed to this macro is always valid and within the expected range. Invalid state values can lead to undefined behavior in the parser. Consider adding validation or documentation to ensure correct usage.resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/build/tree_sitter_javascript_binding.target.mk (1)
- 139-155: The rules for compiling
.cand.ccfiles to object files are correctly defined. However, ensure that thedo_cmdfunction properly handles errors and logs them for easier debugging. It's also important to verify that theFORCE_DO_CMDdependency is correctly used to force recompilation when necessary.resources/language-metavariables/tree-sitter-typescript/build/tree_sitter_typescript_binding.target.mk (1)
- 141-157: Similar to the previous Makefile, the rules for compiling
.cand.ccfiles to object files are well-defined. Ensure consistency in the use ofdo_cmdacross different Makefiles to maintain a uniform build process. Additionally, consider centralizing common build logic to reduce duplication and simplify maintenance.resources/edit_grammars.mjs (1)
- 228-237: Commenting out the execution of
tree-sitter build-wasmcommands fortsxandtypescriptdirectories and the copying of wasm parsers indicates a temporary or strategic adjustment in the build process. Ensure that this change is well-documented and communicated to the team to avoid confusion. Additionally, verify that the build and deployment processes are adjusted accordingly to handle these changes.resources/language-metavariables/tree-sitter-typescript/build/Makefile (1)
- 330-330: The order of included files in the
Makefiletarget definition has been changed. Ensure that this reordering does not affect the build process negatively. It's crucial to verify that dependencies are correctly resolved and that the build process remains stable and predictable.resources/language-metavariables/tree-sitter-vue/node_modules/tree-sitter-html/build/Makefile (1)
- 330-330: The order of dependencies for the
Makefiletarget has been updated. This change can affect the build process by ensuring that dependencies are correctly built in the necessary order. It's important to verify that this new order reflects the actual dependency chain required for the build process to succeed. If the dependencies are not correctly ordered, it could lead to build failures or inconsistencies.resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/build/Makefile (1)
- 330-330: The order of included files for the
Makefiletarget has been updated. As with the previous file, it's crucial to ensure that this new order correctly represents the dependency chain needed for a successful build. Incorrect ordering could lead to build failures or inconsistencies. Please verify that the updated order aligns with the actual build requirements.crates/language/src/tsx.rs (1)
- 516-526: The new test function
parse_filecorrectly initializes a parser, sets the language, and parses a test file. However, it currently only prints the S-expression of the root node. Consider adding assertions to verify the correctness of the parsed output, ensuring the test is more comprehensive and robust.resources/language-metavariables/tree-sitter-typescript/common/define-grammar.js (1)
- 441-1183: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1183]
The modifications to the TypeScript grammar in
define-grammar.jsare comprehensive and well-implemented. The utility functions provided at the end of the file are correctly defined and serve their purpose in simplifying the grammar rule creation process. This file is a solid foundation for extending and customizing the TypeScript grammar for specific parsing needs.resources/metavariable-grammars/typescript-metavariable-define-grammar.js (6)
- 1-2: The use of
requirefor importing the JavaScript grammar is appropriate here, given the context of extending a grammar in a Node.js environment. However, consider verifying if the latest version oftree-sitter-javascriptis being used to ensure compatibility and access to the most recent grammar improvements.- 4-4: The function
defineGrammarcorrectly takes adialectparameter, allowing for dynamic grammar adjustments based on the dialect (TypeScript or TSX). This approach enhances modularity and flexibility in handling different TypeScript dialects.- 7-10: Adding external rules like
_function_signature_automatic_semicolonand__error_recoveryis a good practice for enhancing the grammar's robustness and error recovery capabilities. Ensure these additions are well-documented to aid future maintainability.- 173-192: The addition of an optional type annotation to the
catch_clauseis a valuable enhancement for TypeScript parsing. This change aligns with TypeScript's syntax allowing type annotations in catch clauses. Ensure that this modification is tested thoroughly to confirm its correctness in various scenarios.- 234-243: The conditional inclusion of
type_assertionbased on the dialect is a smart approach to handle the differences between TypeScript and TSX. This kind of dynamic rule adjustment is crucial for accurately parsing different dialects. It's recommended to ensure that all dialect-specific differences are handled similarly for consistency.- 1141-1189: Utility functions like
commaSep1,commaSep,sepBy, andsepBy1are well-defined and contribute to the readability and maintainability of the grammar rules. These abstractions simplify the grammar definitions and make them more concise. It's good practice to continue using such utility functions for complex grammar patterns.resources/language-metavariables/tree-sitter-javascript/grammar.js (6)
- 4-14: The addition of
html_commentto theexternalsarray is a significant change. Ensure that the parser's external scanner is updated accordingly to handle HTML comments within JavaScript code correctly. This change could impact parsing performance and accuracy, so it's crucial to verify through tests that HTML comments are handled as expected in various contexts.- 76-76: Adding
$.lexical_declarationto theprecedencesarray is a critical update. It's essential to ensure that this change does not introduce any conflicts or unexpected behavior in the parsing of lexical declarations. Consider adding specific unit tests to cover cases where lexical declarations might interact with other elements in the grammar to ensure that the precedence is applied correctly.- 118-121: The introduction of the
hash_bangfield in theprogramrule is a notable change. This addition allows for the parsing of hashbang lines (#!) at the beginning of JavaScript files, which is useful for scripts intended to be executed directly on Unix-like systems. Ensure that this change is thoroughly tested, especially for files without a hashbang line, to confirm that it does not affect the parsing of standard JavaScript files.- 129-151: The modifications to the
export_statementrule, including the handling of decorators and default exports, are significant. It's crucial to ensure that these changes align with the latest ECMAScript specifications and do not introduce any regressions in the parsing of export statements. Specifically, verify that decorators are correctly applied to export statements and that default exports are handled as expected in various scenarios.- 198-206: The update to the
import_statementrule, including the optionalattributefield, introduces support for import attributes, which are a relatively new feature in JavaScript. This change enhances the parser's capabilities but requires thorough testing to ensure that import statements with and without attributes are parsed correctly. Additionally, verify that the syntax for import attributes aligns with the current stage of the proposal in the ECMAScript specification.- 1381-1403: The utility functions
commaSep1andcommaSepare well-implemented and provide a concise way to define comma-separated lists in the grammar. The documentation comments are clear and helpful. However, ensure that these functions are used consistently throughout the grammar rules to maintain readability and reduce redundancy.resources/language-metavariables/tree-sitter-typescript/node_modules/tree-sitter-javascript/grammar.js (4)
- 4-14: Consider documenting the purpose and usage of newly introduced external rules, especially the
grit_metavariableand its impact on parsing JavaScript. This will aid in maintainability and clarity for future contributors.- 76-76: Adding
$.lexical_declarationto the precedence rules is a significant change. Ensure this adjustment aligns with the JavaScript language specification, particularly in handling variable declarations within complex expressions.- 118-121: The introduction of a
hash_bangfield in theprogramrule is a useful addition for parsing scripts with shebang lines. Ensure that this change is tested across various environments where JavaScript may be executed, such as Node.js and browser environments.- 1381-1403: The utility functions
commaSep1andcommaSepare well-implemented for creating rules that match comma-separated lists. Consider adding examples in the documentation comments to illustrate their usage within the grammar, enhancing clarity for future contributors.resources/metavariable-grammars/javascript-metavariable-grammar.js (11)
- 4-14: The external rules section has been updated. Ensure that all external rules are correctly defined and referenced throughout the grammar. External rules like
$._automatic_semicolon,$._template_chars, and others are crucial for parsing accuracy.- 16-20: The extras section defines tokens that should be ignored by the parser, such as whitespace and comments. Verify that the inclusion of
$.html_commentand the Unicode characters in the regex pattern align with the intended parsing behavior for JavaScript.- 76-76: The addition of
$.lexical_declarationto the precedence rules is important for correct parsing of variable declarations. Confirm that this change integrates well with the existing precedence rules and does not introduce conflicts.- 118-121: The program rule has been updated to include a
hash_bangfield. This is a significant addition for supporting scripts with a shebang line. Ensure that this change is compatible with the overall grammar and does not affect the parsing of scripts without a shebang line.- 129-157: The export statement grammar has been refined. It's crucial to ensure that all forms of export statements are correctly parsed, including named exports, default exports, and namespace exports. This change could impact module parsing and should be thoroughly tested.
- 198-206: The import statement grammar has been updated. This section is critical for correctly parsing module imports. Verify that the changes support all valid import statement formats, including named imports, default imports, and dynamic imports using
import().- 258-281: The statement grammar encompasses a wide range of JavaScript statements. Given the extensive changes throughout the file, it's essential to verify that all statement types, including control flow statements, declarations, and expression statements, are parsed accurately.
- 795-802: The function declaration grammar has been updated. Ensure that both regular and async function declarations are correctly parsed, including proper handling of async keywords and function names.
- 961-969: The ternary expression grammar has been modified. Ternary expressions are common in JavaScript, and it's crucial to ensure that these changes do not affect the parsing of complex expressions involving nested ternary operators.
- 1063-1087: The string grammar has been updated to include
$.grit_metavariable. This addition is significant for supporting metavariables within strings. Verify that this change allows for the correct parsing of strings containing metavariables without affecting the parsing of regular strings.- 1211-1211: The identifier rule now includes
$.grit_metavariable. This is a crucial update for supporting metavariables as identifiers. Ensure that this change does not conflict with the parsing of standard JavaScript identifiers.crates/core/src/test.rs (1)
- 6006-6006: The modification of the
languagedirective fromjstojs(js_do_not_use)is noted. Ensure that this change is reflected in any related documentation or test guidelines, and verify that it does not adversely affect any dependent tests or tools that might rely on the originaljsdirective.
Comment on lines
+153
to
+171
| // TODO use the below version of the rule as it is up to date with the upstream typescript grammar | ||
| // unfortunatly using it breaks the react class componenets to functional componenets pattern | ||
|
|
||
| // public_field_definition: ($) => seq( | ||
| // repeat(field('decorator', $.decorator)), | ||
| // optional(choice( | ||
| // seq('declare', optional($.accessibility_modifier)), | ||
| // seq($.accessibility_modifier, optional('declare')), | ||
| // )), | ||
| // choice( | ||
| // seq(field('static', optional($.static)), optional($.override_modifier), optional('readonly')), | ||
| // seq(optional('abstract'), optional('readonly')), | ||
| // seq(optional('readonly'), optional('abstract')), | ||
| // ), | ||
| // field('name', $._property_name), | ||
| // optional(choice('?', '!')), | ||
| // field('type', optional($.type_annotation)), | ||
| // optional($._initializer), | ||
| // ), |
There was a problem hiding this comment.
The commented-out version of public_field_definition suggests an attempt to align with the upstream TypeScript grammar. However, it's noted that using this version breaks certain patterns. This is a significant concern that warrants further investigation and possibly a more robust solution that doesn't compromise compatibility with React component patterns.
+ // TODO: Investigate and resolve the issue with React class components when using the updated public_field_definition rule.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Suggested change
| // TODO use the below version of the rule as it is up to date with the upstream typescript grammar | |
| // unfortunatly using it breaks the react class componenets to functional componenets pattern | |
| // public_field_definition: ($) => seq( | |
| // repeat(field('decorator', $.decorator)), | |
| // optional(choice( | |
| // seq('declare', optional($.accessibility_modifier)), | |
| // seq($.accessibility_modifier, optional('declare')), | |
| // )), | |
| // choice( | |
| // seq(field('static', optional($.static)), optional($.override_modifier), optional('readonly')), | |
| // seq(optional('abstract'), optional('readonly')), | |
| // seq(optional('readonly'), optional('abstract')), | |
| // ), | |
| // field('name', $._property_name), | |
| // optional(choice('?', '!')), | |
| // field('type', optional($.type_annotation)), | |
| // optional($._initializer), | |
| // ), | |
| // TODO use the below version of the rule as it is up to date with the upstream typescript grammar | |
| // unfortunatly using it breaks the react class componenets to functional componenets pattern | |
| // TODO: Investigate and resolve the issue with React class components when using the updated public_field_definition rule. | |
| // public_field_definition: ($) => seq( | |
| // repeat(field('decorator', $.decorator)), | |
| // optional(choice( | |
| // seq('declare', optional($.accessibility_modifier)), | |
| // seq($.accessibility_modifier, optional('declare')), | |
| // )), | |
| // choice( | |
| // seq(field('static', optional($.static)), optional($.override_modifier), optional('readonly')), | |
| // seq(optional('abstract'), optional('readonly')), | |
| // seq(optional('readonly'), optional('abstract')), | |
| // ), | |
| // field('name', $._property_name), | |
| // optional(choice('?', '!')), | |
| // field('type', optional($.type_annotation)), | |
| // optional($._initializer), | |
| // ), |
Comment on lines
+129
to
+151
| export_statement: $ => choice( | ||
| seq( | ||
| 'export', | ||
| choice( | ||
| seq('*', $._from_clause), | ||
| seq(field('export', $.namespace_export), $._from_clause), | ||
| seq(field('export', $.export_clause), $._from_clause), | ||
| field('export', $.export_clause), | ||
| ), | ||
| seq( | ||
| repeat(field('decorator', $.decorator)), | ||
| 'export', | ||
| choice( | ||
| field('declaration', $.declaration), | ||
| seq( | ||
| 'default', | ||
| choice( | ||
| field('declaration', $.declaration), | ||
| seq(field('declaration', $.expression), $._semicolon), | ||
| $._semicolon, | ||
| ), | ||
| seq( | ||
| repeat(field('decorator', $.decorator)), | ||
| 'export', | ||
| choice( | ||
| field('declaration', $.declaration), | ||
| seq( | ||
| 'default', | ||
| choice( | ||
| field('declaration', $.declaration), | ||
| seq( | ||
| field('declaration', $.expression), | ||
| $._semicolon, |
There was a problem hiding this comment.
The modifications to export_statement to include decorators and handle default exports more explicitly are welcome improvements. It's crucial to add comprehensive tests covering these scenarios to ensure compatibility with ECMAScript modules.
Would you like me to help generate tests covering the new export statement scenarios?
seren5240
approved these changes
Mar 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the typescript and javascript parsers to parity with most recent commits on those repos
fixes: #27
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation