[PoC] feat: PoC for supertype matcher #1784
Conversation
| tree-sitter-typescript = { package = "tree-sitter-typescript-codemod", version = "0.25.0-codemod.1"} |
There was a problem hiding this comment.
I just manually rebuilt tree-sitter-typescript with the latest version of the ts cli.
| @@ -33,7 +33,7 @@ ignore = { version = "0.4.22" } | |||
| regex = { version = "1.10.4" } | |||
| serde = { version = "1.0.200", features = ["derive"] } | |||
| serde_yaml = "0.9.33" | |||
| tree-sitter = { version = "0.24.5", package = "tree-sitter-facade-sg" } | |||
| tree-sitter = { version = "0.25.0-codemod.2", package = "tree-sitter-facade-sg-codemod" } | |||
There was a problem hiding this comment.
Just a copy of the facade package with the new methods node_kind_is_supertype and supertypes exposed.
WalkthroughThis pull request updates several tree-sitter dependency versions and package names in multiple Cargo.toml files. It replaces function calls with constant references for obtaining the TypeScript language in both configuration and core modules. Additionally, new matcher functionality for handling supertypes is introduced, including modifications to rule structures, a new Rule variant, and the implementation of a SupertypeMatcher with corresponding tests. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Rule Engine
participant SM as SupertypeMatcher
participant L as Language
participant N as Node
R->>SM: match_node_with_env(Node, env)
SM->>L: Retrieve supertype & subtype info
L-->>SM: Return language constants and node kind details
SM->>N: Compare node kind against subtype set
SM-->>R: Return matched node (or null)
Possibly related PRs
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1784 +/- ##
==========================================
- Coverage 87.38% 87.27% -0.12%
==========================================
Files 96 97 +1
Lines 15521 15620 +99
==========================================
+ Hits 13563 13632 +69
- Misses 1958 1988 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/core/src/matcher/supertype.rs (1)
89-141: Consider adding edge case tests.While the current tests cover the basic functionality, consider adding tests for:
- Invalid supertype names
- Empty subtypes list
- Multiple levels of inheritance
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 107-109: crates/core/src/matcher/supertype.rs#L107-L109
Added lines #L107 - L109 were not covered by tests
[warning] 125-127: crates/core/src/matcher/supertype.rs#L125-L127
Added lines #L125 - L127 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)crates/config/Cargo.toml(1 hunks)crates/config/src/lib.rs(1 hunks)crates/config/src/rule/mod.rs(10 hunks)crates/core/Cargo.toml(1 hunks)crates/core/src/language.rs(1 hunks)crates/core/src/matcher.rs(2 hunks)crates/core/src/matcher/kind.rs(2 hunks)crates/core/src/matcher/supertype.rs(1 hunks)crates/dynamic/Cargo.toml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
crates/config/src/rule/mod.rs
[warning] 272-272: crates/config/src/rule/mod.rs#L272
Added line #L272 was not covered by tests
[warning] 293-293: crates/config/src/rule/mod.rs#L293
Added line #L293 was not covered by tests
[warning] 320-320: crates/config/src/rule/mod.rs#L320
Added line #L320 was not covered by tests
[warning] 343-343: crates/config/src/rule/mod.rs#L343
Added line #L343 was not covered by tests
[warning] 508-511: crates/config/src/rule/mod.rs#L508-L511
Added lines #L508 - L511 were not covered by tests
crates/core/src/matcher/kind.rs
[warning] 37-37: crates/core/src/matcher/kind.rs#L37
Added line #L37 was not covered by tests
crates/core/src/matcher/supertype.rs
[warning] 36-37: crates/core/src/matcher/supertype.rs#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 42-47: crates/core/src/matcher/supertype.rs#L42-L47
Added lines #L42 - L47 were not covered by tests
[warning] 49-49: crates/core/src/matcher/supertype.rs#L49
Added line #L49 was not covered by tests
[warning] 51-51: crates/core/src/matcher/supertype.rs#L51
Added line #L51 was not covered by tests
[warning] 66-68: crates/core/src/matcher/supertype.rs#L66-L68
Added lines #L66 - L68 were not covered by tests
[warning] 107-109: crates/core/src/matcher/supertype.rs#L107-L109
Added lines #L107 - L109 were not covered by tests
[warning] 125-127: crates/core/src/matcher/supertype.rs#L125-L127
Added lines #L125 - L127 were not covered by tests
🪛 GitHub Actions: coverage
crates/core/src/matcher/kind.rs
[warning] 115-115: unused variable: b. If this is intentional, prefix it with an underscore: _b
[warning] 116-116: unused variable: c. If this is intentional, prefix it with an underscore: _c
🔇 Additional comments (15)
crates/core/src/language.rs (1)
88-90: LGTM! Consistent with the TypeScript language reference update.The change from function call to constant reference aligns with similar updates across the codebase.
crates/core/src/matcher/supertype.rs (2)
15-19: LGTM! Clear error handling.The error type is well-defined and uses thiserror for deriving the Error trait.
71-87: LGTM! Clean and efficient matcher implementation.The implementation correctly uses BitSet for tracking subtypes and provides efficient matching.
crates/core/src/matcher/kind.rs (1)
33-38: LGTM! Proper supertype handling.The implementation correctly resets the kind ID when a supertype is detected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: crates/core/src/matcher/kind.rs#L37
Added line #L37 was not covered by testscrates/config/src/lib.rs (1)
58-58: LGTM! Consistent TypeScript language reference update.The change from function call to constant reference aligns with similar updates across the codebase.
crates/core/src/matcher.rs (2)
11-11: LGTM!The new module declaration follows the existing pattern and is placed appropriately with other matcher modules.
25-25: LGTM!The new exports follow the existing pattern of exporting matcher types and their associated error types.
crates/config/src/rule/mod.rs (4)
65-67: LGTM!The new
supertypefield is well-documented and follows the existing pattern of optional fields.
142-142: LGTM!The
supertypefield inAtomicRulealigns with the serializable version.
221-221: LGTM!The new
Supertypevariant in theRuleenum follows the existing pattern of atomic matchers.
272-272: Add test coverage for supertype matcher integration.Several methods handling the supertype matcher lack test coverage:
defined_varsfor supertypeverify_utilfor supertypematch_node_with_envfor supertypepotential_kindsfor supertypeConsider adding test cases that verify the integration of supertype matching with the existing matcher framework.
Also applies to: 293-293, 320-320, 343-343
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 272-272: crates/config/src/rule/mod.rs#L272
Added line #L272 was not covered by testscrates/dynamic/Cargo.toml (1)
20-20: LGTM!The update to tree-sitter version 0.25.1 is necessary for supertype support.
crates/core/Cargo.toml (1)
26-26: LGTM!The update to the custom codemod version of tree-sitter-typescript aligns with the manual rebuild mentioned in the previous review.
crates/config/Cargo.toml (1)
29-29: Update tree-sitter-typescript Dependency Specification
The dependency fortree-sitter-typescripthas been updated from a simple version string to a more structured declaration:tree-sitter-typescript = { package = "tree-sitter-typescript-codemod", version = "0.25.0-codemod.1" }This change standardizes the dependency across the project and aligns with the codemod strategy. Please verify that this new specification is compatible with all parts of the codebase that utilize this dependency.
Cargo.toml (1)
36-36: Update tree-sitter Dependency to Codemod Version
Thetree-sitterdependency has been updated to:tree-sitter = { version = "0.25.0-codemod.2", package = "tree-sitter-facade-sg-codemod" }This update reflects the shift to using the codemod version of the Tree-sitter façade. Please ensure that all modules relying on Tree-sitter are tested for compatibility with the latest API changes introduced by this codemod version.
| #[derive(Clone)] | ||
| pub struct SupertypeMatcher<L: Language> { | ||
| supertype: KindId, | ||
| subtypes: BitSet, | ||
| lang: PhantomData<L>, | ||
| } | ||
|
|
||
| impl<L: Language> SupertypeMatcher<L> { | ||
| pub fn new(supertype_kind: &str, lang: L) -> Self { | ||
| let mut supertype_kind = lang | ||
| .get_ts_language() | ||
| .id_for_node_kind(supertype_kind, /*named*/ true); | ||
| if !lang | ||
| .get_ts_language() | ||
| .node_kind_is_supertype(supertype_kind) | ||
| { | ||
| supertype_kind = 0; | ||
| } | ||
| Self::from_id(supertype_kind, lang) | ||
| } | ||
|
|
||
| pub fn try_new(supertype_kind: &str, lang: L) -> Result<Self, SupertypeMatcherError> { | ||
| let s = Self::new(supertype_kind, lang); | ||
| if s.is_invalid() { | ||
| Err(SupertypeMatcherError::InvalidKindName( | ||
| supertype_kind.into(), | ||
| )) | ||
| } else { | ||
| Ok(s) | ||
| } | ||
| } | ||
|
|
||
| pub fn from_id(supertype: KindId, lang: L) -> Self { | ||
| let mut subtypes = BitSet::new(); | ||
| for kind in lang.get_ts_language().subtypes_for_supertype(supertype) { | ||
| subtypes.insert(kind as usize); | ||
| } | ||
| Self { | ||
| supertype, | ||
| subtypes, | ||
| lang: PhantomData, | ||
| } | ||
| } | ||
|
|
||
| /// Whether the kind matcher contains undefined tree-sitter kind. | ||
| pub fn is_invalid(&self) -> bool { | ||
| self.supertype == TS_BUILTIN_SYM_END | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Error Handling Test Coverage is Missing
Our investigation shows that none of the error paths in SupertypeMatcher are exercised by any tests. Specifically:
- The error branch in the
new()function (lines 36–37) that validates the supertype. - The error handling in
try_new()(lines 42–51) triggeringSupertypeMatcherError::InvalidKindName. - The
is_invalid()method (lines 66–68) isn’t being validated via tests.
Please add tests to cover these scenarios.
🔗 Analysis chain
Add test coverage for error handling paths.
The implementation looks good, but some error handling paths are not covered by tests:
- Lines 36-37: Supertype validation
- Lines 42-51: Error handling in
try_new - Lines 66-68:
is_invalidmethod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for error handling paths
# Test: Check for test cases that cover error handling
rg -A 5 'test.*invalid|test.*error' crates/core/src/matcher/supertype.rs
Length of output: 74
Script:
#!/bin/bash
# Description: Verify test coverage for SupertypeMatcher error handling by searching across the repository for tests
# that invoke error paths or reference related error messages.
echo "Searching for test invocations of try_new() in Rust files:"
rg -n "SupertypeMatcher::try_new" -t rust
echo -e "\nSearching for tests referencing error variants (e.g., InvalidKindName) in Rust files:"
rg -n "InvalidKindName" -t rust
echo -e "\nSearching for general error tests in modules related to matcher in Rust files:"
rg -n "SupertypeMatcherError" -t rust
Length of output: 1299
Script:
#!/bin/bash
# Description: Search for tests in the repository that cover error handling paths of SupertypeMatcher,
# particularly tests that call try_new() with invalid parameters to trigger the error paths.
echo "Searching for test cases in the tests/ directory that reference try_new:"
rg -n "try_new" tests/
echo -e "\nSearching for test cases in the tests/ directory that reference InvalidKindName:"
rg -n "InvalidKindName" tests/
Length of output: 479
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-37: crates/core/src/matcher/supertype.rs#L36-L37
Added lines #L36 - L37 were not covered by tests
[warning] 42-47: crates/core/src/matcher/supertype.rs#L42-L47
Added lines #L42 - L47 were not covered by tests
[warning] 49-49: crates/core/src/matcher/supertype.rs#L49
Added line #L49 was not covered by tests
[warning] 51-51: crates/core/src/matcher/supertype.rs#L51
Added line #L51 was not covered by tests
[warning] 66-68: crates/core/src/matcher/supertype.rs#L66-L68
Added lines #L66 - L68 were not covered by tests
| if let Some(supertype) = atomic.supertype { | ||
| rules.push(R::Supertype(SupertypeMatcher::new( | ||
| &supertype, | ||
| env.lang.clone(), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Supertype deserialization not adequately covered in tests
The grep results show tests in other areas (e.g. in matcher and pattern tests), but there isn’t any dedicated test exercising the configuration deserialization branch where a supertype is parsed (the code at crates/config/src/rule/mod.rs, lines 507–512). We need to add tests that ensure:
- Successful deserialization of supertype rules from valid inputs.
- Correct error handling when invalid supertypes are provided.
- Proper integration of supertype rules with other kinds of rules in the overall config.
🔗 Analysis chain
Add test coverage for supertype deserialization.
The supertype deserialization logic lacks test coverage. Consider adding tests to verify:
- Successful deserialization of supertype rules
- Error handling for invalid supertypes
- Integration with other rule types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for supertype-related code
# Look for existing tests related to supertype
rg -l "test.*supertype" .
# Look for test patterns that could be adapted for supertype
rg "test_pattern|test_deserialize_rule" .
Length of output: 1851
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 508-511: crates/config/src/rule/mod.rs#L508-L511
Added lines #L508 - L511 were not covered by tests
d512c17 to
312c47f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/config/src/rule/mod.rs (1)
507-512:⚠️ Potential issueError handling in supertype deserialization needs improvement.
The code uses
new()which silently fails, instead oftry_new()which properly handles errors.Apply this diff to improve error handling:
if let Some(supertype) = atomic.supertype { - rules.push(R::Supertype(SupertypeMatcher::new( + rules.push(R::Supertype(SupertypeMatcher::try_new( &supertype, env.lang.clone(), - ))); + )?)); }
🧹 Nitpick comments (2)
crates/core/src/matcher/supertype.rs (1)
15-19: Error handling needs expansion.The error handling is minimal with only one error variant. Consider adding more specific error cases for:
- Invalid supertype (not a valid tree-sitter kind)
- Non-supertype (valid kind but not a supertype)
#[derive(Debug, Error)] pub enum SupertypeMatcherError { #[error("Kind `{0}` is invalid.")] InvalidKindName(String), + #[error("Kind `{0}` is not a valid tree-sitter kind.")] + InvalidTreeSitterKind(String), + #[error("Kind `{0}` is not a supertype.")] + NotASupertype(String), }crates/config/src/rule/mod.rs (1)
65-67: Documentation needs improvement.The supertype field documentation is minimal. Consider adding:
- Example usage
- Explanation of supertype concept
- Link to related documentation
- /// The supertype name of the node to match. + /// The supertype name of the node to match. Supertypes represent abstract node types + /// that encompass multiple concrete node types. For example, 'declaration' is a supertype + /// that matches class_declaration, function_declaration, etc. + /// + /// Example: + /// ```yaml + /// supertype: declaration + /// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)crates/config/Cargo.toml(1 hunks)crates/config/src/lib.rs(1 hunks)crates/config/src/rule/mod.rs(10 hunks)crates/core/Cargo.toml(1 hunks)crates/core/src/language.rs(1 hunks)crates/core/src/matcher.rs(2 hunks)crates/core/src/matcher/kind.rs(1 hunks)crates/core/src/matcher/supertype.rs(1 hunks)crates/dynamic/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/dynamic/Cargo.toml
- crates/config/src/lib.rs
- crates/core/src/language.rs
- crates/config/Cargo.toml
- Cargo.toml
- crates/core/Cargo.toml
- crates/core/src/matcher/kind.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (2)
crates/core/src/matcher/supertype.rs (1)
89-141: Test coverage needs improvement.Current tests only cover the happy path. Missing test cases for:
- Invalid kind names
- Non-supertype kinds
- Edge cases in potential_kinds()
Run this script to analyze current test coverage:
crates/core/src/matcher.rs (1)
11-11: LGTM! Clean integration of the supertype module.The changes properly integrate the new supertype functionality while maintaining the existing structure.
Also applies to: 25-25
| pub fn new(supertype_kind: &str, lang: L) -> Self { | ||
| let mut supertype_kind = lang | ||
| .get_ts_language() | ||
| .id_for_node_kind(supertype_kind, /*named*/ true); | ||
| if !lang | ||
| .get_ts_language() | ||
| .node_kind_is_supertype(supertype_kind) | ||
| { | ||
| supertype_kind = 0; | ||
| } | ||
| Self::from_id(supertype_kind, lang) | ||
| } |
There was a problem hiding this comment.
Silent failure in new() method needs attention.
The method silently sets supertype_kind to 0 when validation fails. This could lead to subtle bugs as callers won't know about the failure.
Consider using try_new() instead, which properly handles errors:
- pub fn new(supertype_kind: &str, lang: L) -> Self {
- let mut supertype_kind = lang
- .get_ts_language()
- .id_for_node_kind(supertype_kind, /*named*/ true);
- if !lang
- .get_ts_language()
- .node_kind_is_supertype(supertype_kind)
- {
- supertype_kind = 0;
- }
- Self::from_id(supertype_kind, lang)
- }
+ pub fn new(supertype_kind: &str, lang: L) -> Result<Self, SupertypeMatcherError> {
+ Self::try_new(supertype_kind, lang)
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn new(supertype_kind: &str, lang: L) -> Self { | |
| let mut supertype_kind = lang | |
| .get_ts_language() | |
| .id_for_node_kind(supertype_kind, /*named*/ true); | |
| if !lang | |
| .get_ts_language() | |
| .node_kind_is_supertype(supertype_kind) | |
| { | |
| supertype_kind = 0; | |
| } | |
| Self::from_id(supertype_kind, lang) | |
| } | |
| pub fn new(supertype_kind: &str, lang: L) -> Result<Self, SupertypeMatcherError> { | |
| Self::try_new(supertype_kind, lang) | |
| } |
|
I'm not sure if supertype should be included as an API.
utils:
declaration:
any:
- kind: class_declaration
- kind: function_declaration
- kind: generator_function_declaration
- kind: lexical_declaration
- kind: variable_declarationwhich is equivalent to {
"type": "_declaration",
"named": true,
"subtypes": [
{ "type": "class_declaration", "named": true },
{ "type": "function_declaration", "named": true },
{ "type": "generator_function_declaration", "named": true },
{ "type": "lexical_declaration", "named": true },
{ "type": "variable_declaration", "named": true }
]
}
|
Fair points, in that case, I think the least we can and should do is to disallow using supertypes in the 
|
this is also a valid solution. Alternatively, we can support |
Sure! If that sounds good to you I can send a PR to tree-sitter-wasm and update this one accordingly. |
Yes please! |
|
Closing in favor of #1875 |
Tree-sitter recently added APIs for getting supertype info. Currently, if you want to match all expressions, you need to use an
anyrule for each of the expression types.With supertypes, you can simply do:
which will match all expressions.
Also, currently, ast-grep accepts super types as kinds, but it never matches any nodes. Meaning that,
doesn't produce an invalid_kind error, yet it doesn't match anything.
🚫 NOTE
tree-sitter-cliSummary by CodeRabbit
New Features
Chores
tree-sitterandtree-sitter-typescript, to bolster overall stability and performance.