feat(KindMatcher): Support super types #1875
Conversation
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)KindMatcher Matching ProcesssequenceDiagram
participant Client
participant KindMatcher
participant BitSet
Client->>KindMatcher: new(node_kind)
alt node_kind is supertype
KindMatcher->>BitSet: Populate with all subtypes
else
KindMatcher->>BitSet: Insert node_kind id
end
Client->>KindMatcher: match_node_with_env(node)
KindMatcher-->>Client: Return match result (true/false)
Language Loading ProcesssequenceDiagram
participant Client
participant LanguageLoader
participant Language
Client->>LanguageLoader: load_ts_language()
LanguageLoader->>Language: abi_version()
Language-->>LanguageLoader: Return ABI version
LanguageLoader->>LanguageLoader: Validate version compatibility
alt Version valid
LanguageLoader-->>Client: Return language instance
else
LanguageLoader-->>Client: Return error
end
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/core/src/matcher/kind.rs (1)
33-49: Consider validating empty subtypes.
The logic correctly populates subtypes for a supertype. However, if subtypes_for_supertype returns an empty set, you might want to handle that case (e.g., treat it as invalid).
📜 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 (4)
Cargo.toml(1 hunks)crates/core/src/matcher/kind.rs(4 hunks)crates/dynamic/src/lib.rs(1 hunks)crates/napi/src/doc.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (11)
Cargo.toml (1)
36-36: Confirm upgrade to tree-sitter 0.25.3.
Please verify that this version aligns with upstream changes and does not introduce unexpected behavior.Would you like me to check for any known compatibility issues or release notes for tree-sitter 0.25.3?
crates/napi/src/doc.rs (1)
59-59: Ensure parse_utf16_le suits all targeted platforms.
Switching from parse_utf16 to parse_utf16_le is correct for little-endian environments, but may break big-endian support. Confirm that big-endian architectures are not required or handle them separately.Would you like me to run a script to confirm no big-endian references exist in the codebase?
crates/dynamic/src/lib.rs (1)
129-129: Validate usage of abi_version().
Switching from lang.version() to lang.abi_version() is potentially more accurate for ABI compatibility, but please confirm your range check remains valid.I can help verify that MIN_COMPATIBLE_LANGUAGE_VERSION..=LANGUAGE_VERSION is appropriate for ABI version checking with tree-sitter 0.25.3.
crates/core/src/matcher/kind.rs (8)
27-27: Use of BitSet for subtypes
Replacing a single kind with a BitSet is a solid design choice for handling multiple subtypes.
50-51: Constructor usage is straightforward.
Storing the BitSet in Self is clearly organized.
66-67: Initialize subtypes for from_id.
Creating a new BitSet and inserting the provided kind looks good.
69-69: Returning Self with subtypes is clear.
No issues here; straightforward instantiation.
76-76: is_invalid check.
Using TS_BUILTIN_SYM_END (0) to detect an invalid kind is a suitable solution for unknown node kinds.
107-107: Check membership in subtypes.
This direct membership check is efficient and clear.
115-115: Expose potential kinds as a cloned BitSet.
Returning a clone of subtypes is a clean approach for further usage.
156-169: Supertype matching test.
Tests confirm that a supertype correctly matches relevant subtypes. Nicely done.
0b60e9f to
d171492
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/core/src/matcher/kind.rs (1)
155-167: Good test coverage for the new supertype matching functionality.The test correctly verifies that a matcher created with a supertype ("declaration") successfully matches a node of a subtype (class declaration).
Consider adding a few more test cases covering different supertype-subtype relationships to ensure comprehensive coverage of the feature.
#[test] fn test_supertype_match() { let supertype_kind = "declaration"; let cand = pattern_node("class A { a = 123 }"); let cand = cand.root(); let pattern = KindMatcher::new(supertype_kind, Tsx); assert!( pattern.find_node(cand.clone()).is_some(), "goal: {}, candidate: {}", supertype_kind, cand.to_sexp(), ); } + #[test] + fn test_expression_supertype_match() { + let supertype_kind = "expression"; + let cand = pattern_node("const a = 1 + 2"); + let cand = cand.root(); + let pattern = KindMatcher::new(supertype_kind, Tsx); + assert!( + pattern.find_node(cand.clone()).is_some(), + "goal: {}, candidate: {}", + supertype_kind, + cand.to_sexp(), + ); + }
📜 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 (4)
Cargo.toml(1 hunks)crates/core/src/matcher/kind.rs(4 hunks)crates/dynamic/src/lib.rs(1 hunks)crates/napi/src/doc.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/dynamic/src/lib.rs
- crates/napi/src/doc.rs
- Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: coverage
🔇 Additional comments (7)
crates/core/src/matcher/kind.rs (7)
27-27: Excellent change to support supertype matching.Replacing the single
kind: KindIdfield with asubtypes: BitSetis a great approach to support matching multiple kinds at once, which is essential for implementing supertype support.
32-53: LGTM - The implementation correctly handles both supertype and regular kind matching.The new implementation elegantly handles both cases:
- For supertypes: retrieves all subtypes and populates the BitSet
- For regular kinds: simply adds the single kind to the BitSet
This approach maintains backward compatibility while adding the new supertype functionality.
64-71: Good adaptation offrom_idto work with the new BitSet approach.The method correctly initializes a BitSet with a single kind, ensuring compatibility with existing code that uses this factory method.
73-76: Validation logic correctly adapted to work with BitSet.The check for invalid kinds appropriately verifies if the special
TS_BUILTIN_SYM_ENDvalue is present in the set.
100-111: Matching logic properly updated to work with multiple subtypes.The implementation now checks if the node's kind is contained within any of the subtypes in the BitSet, rather than doing a direct comparison to a single kind.
113-115: Proper implementation ofpotential_kinds.The method correctly returns a clone of the entire BitSet containing all potential kinds that this matcher could match.
170-178:❓ Verification inconclusive
Existing test needs to be updated.
The existing test for
potential_kindsexpects exactly one potential kind, but this assumption may no longer be valid when matching a supertype that has multiple subtypes.
🏁 Script executed:
#!/bin/bash # Check if the test for potential_kinds passes with the supertype implementation cargo test -p ast-grep-core --test matcher::kind::test::test_kind_potential_kinds -- --exact # Check how many kinds are returned for a supertype cat << 'EOF' > /tmp/check_supertype_kinds.rs fn main() { use tree_sitter::Language; // This is just a placeholder for demonstration, the actual check // would depend on how to access Tree-sitter language definitions let lang = tree_sitter_typescript::language_tsx(); let supertype_id = lang.id_for_node_kind("declaration", true); let subtypes = lang.subtypes_for_supertype(supertype_id); println!("Number of subtypes for 'declaration': {}", subtypes.len()); } EOF echo "Note: This script is demonstrative. The actual implementation would need to be adapted to your project's structure."Length of output: 440
Attention: Update the Expected Number of Potential Kinds
The current test in
crates/core/src/matcher/kind.rs(lines 170–178) expects exactly one potential kind. However, when matching a supertype that can have multiple subtypes, this assumption is no longer valid. Please update the test so that it verifies the returned potential kinds align with the new matching logic. Consider one of the following approaches:
- Adjust the assertion: Instead of expecting a fixed length of one, validate that the list of potential kinds accurately reflects the supertype's multiple subtypes.
- Parameterize the test: Make the expected count configurable or determined based on the actual language definition for the supertype.
- Double-check language definitions: Ensure that the underlying Tree-sitter definitions (or any language-specific mappings) are correctly incorporated into the matching logic.
|
the coverage test is failing, would you like to fix it? |
The test is using tree-sitter-typescript and tree-sitter-typescript crate needs to be updated with the latest CLI, until then, this PR's test won't pass. |
sure, should we change this PR to draft now? |
|
Yeah, or we can just ignore the test and merge. Whatever you prefer. Just for context, it makes our job at Codemod easier if we merge sooner, so we can use this feature without having to maintain a fork and we can just swap parsers when needed (possible because of the napi dynamic lib support).
|
There was a problem hiding this comment.
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
crates/napi/src/doc.rs:59
- [nitpick] Ensure that switching from 'parse_utf16' to 'parse_utf16_le' is intentional and consistent with the encoding expectations throughout the application.
parser.parse_utf16_le(self.inner.as_slice(), tree)
crates/dynamic/src/lib.rs:129
- [nitpick] Consider renaming the variable from 'version' to 'abi_version' for greater clarity on its meaning.
let version = lang.abi_version();
crates/core/src/matcher/kind.rs:37
- If 'subtypes_for_supertype' returns an empty iterator for a supertype, the matcher may end up with an empty BitSet leading to unexpected matching behavior; consider handling or documenting this case explicitly.
if lang.get_ts_language().node_kind_is_supertype(kind_id) {
As mentioned in #1784,
ast-grepcurrently accepts supertypes like "declaration" and "expression" as valid types but doesn't actually match anything.Ideally, it should either return an
InvalidKindNameerror or match all subtypes instead of doing nothing.Tree-sitter recently introduced support for exposing supertype data. While the tree-sitter crate already includes the necessary API changes, the parser dependencies haven't been updated in the last four months. As a result, this PR won't have any effect until the tree-sitter team updates those crates using the new
tree-sitter-cli. We can disable the test and merge, or just wait until thetree-sitter-typescriptcrate is updated.To make the test pass, you can replace the tree-sitter-typescript dependency in
core/Cargo.tomlwith the following:Summary by CodeRabbit
tree-sitterlibrary dependency to version0.25.3, enhancing underlying performance and stability.