Extract JavaScript object literal method entities#288
Closed
Iron-Ham wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
inspect review
Triage: 19 entities analyzed | 0 critical, 0 high, 9 medium, 10 low
Verdict: standard_review
Findings (5)
- [low] In
strip_children_content, the function attempts to find child content within a search range but useschild.contentwhich may contain leading/trailing whitespace or formatting differences from the actual source text. If the exact string match fails, it falls back to excluding the entire line range, which could exclude more content than intended. This is a logic error in the content stripping algorithm that could lead to incorrect diff suppression. - [low] In
js_ts_object_initializer_children, the function filters formethod_definitionOR pairs with function values, but then returns ALL matching children with suppression_context set to "object". Later inextract_js_ts_object_function_pair, it only processes nodes whensuppression_context == Some("object")AND the node is apair. This meansmethod_definitionnodes will be added to the worklist with "object" suppression but won't be processed correctly, potentially causing them to be skipped or mishandled. - [low] In
find_name_byte_range, the new code for handlingpairnodes returns the byte range of the key, but this doesn't account for the fact thatjs_ts_object_key_namefilters out empty keys, computed keys (starting with '['), and template strings with interpolation. If a pair has such a key,find_name_byte_rangewill return a byte range butextract_js_ts_object_function_pairwill return None for the name, causing an inconsistency where an entity might be created with an invalid name or the wrong byte range. - [low] In
extract_js_ts_object_function_pair, the function returns a tuple with the function value node, but the caller invisit_nodepushes this value onto the worklist withSome(value.kind().to_string())as suppression context. However, the value is the function node (arrow_function, function_expression, etc.), not "object". This means nested entities inside these functions won't have the correct suppression context. - [low] In
js_ts_object_key_name, the function filters out keys thatcontains("${")to exclude template strings with interpolation. However, it only checks after trimming quotes from template_string nodes. A template string like`key${x}`would have its backticks trimmed first, leavingkey${x}, which would then be filtered. But a regular string"key${x}"(literal text, not interpolation) would also be incorrectly filtered out.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
e949dc1 to
b3e8a8c
Compare
There was a problem hiding this comment.
inspect review
Triage: 19 entities analyzed | 0 critical, 0 high, 9 medium, 10 low
Verdict: standard_review
Findings (4)
- [low] In
strip_children_content, the function attempts to find child content within a search range but doesn't handle the case wherechild.contentmight span multiple lines or contain different whitespace than the parent content. The substring searchcontent[search_start..search_end].find(&child.content)will fail if the child content has been normalized differently (e.g., different line endings, whitespace), causing the exclusion logic to silently fail and include child content that should be stripped. - [low] In
js_ts_object_key_name, the function filters out template strings containing${but uses a simplecontainscheck after trimming backticks. This means a key like"key${x}"(which is a regular string, not a template string) would pass through, but the test expects it to be extracted. However, if the tree-sitter parser marks it as a template_string node type, it would be incorrectly filtered out despite being a valid static key. - [low] In
js_ts_object_initializer_children, the function sets suppression_context toSome("object".to_string())for pairs with function values, but inextract_js_ts_object_function_pair, it checkssuppression_context != Some("object")and returns None if it doesn't match. This means the function will only extract pairs when the suppression_context is exactly "object", but the worklist entry is created with this context. However, if a pair is visited through a different path without this context, it won't be extracted as a method entity. - [low] In
push_js_ts_initializer_children, the function reverses the order of initializer children before pushing them onto the worklist with.rev(). However,js_ts_initializer_childrencan now return multiple children fromjs_ts_object_initializer_children, which are already collected in source order. Reversing them will cause object properties to be visited in reverse order, which may break assumptions about processing order.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
Contributor
Author
|
Superseded by #309. |
rs545837
pushed a commit
that referenced
this pull request
Jun 5, 2026
## Summary - consolidate the open TypeScript/JavaScript parser, extraction, accessor, namespace, abstract entity, and import-resolution fixes - keep the combined graph/scope behavior from the parent PRs in one reviewable branch Supersedes #303, #305, #291, #289, #288, #285. Fixes #264. Fixes #262. Fixes #265. Closes #267. Fixes #266. Fixes #263. ## Test plan - cargo test --manifest-path crates/Cargo.toml -p sem-core typescript - cargo test --manifest-path crates/Cargo.toml -p sem-core js_ts - cargo test --manifest-path crates/Cargo.toml -p sem-core import_resolution - cargo test --manifest-path crates/Cargo.toml -p sem-core scope_resolve_typescript - cargo test --manifest-path crates/Cargo.toml -p sem-cli --test accessor_cli - cargo check --manifest-path crates/Cargo.toml -p sem-core -p sem-cli - cargo test --manifest-path crates/Cargo.toml -p sem-core graph
This file contains hidden or 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
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.
Summary
Fixes #266
Test plan
Review