Extract TypeScript ambient function signatures#291
Closed
Iron-Ham wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
inspect review
Triage: 12 entities analyzed | 0 critical, 0 high, 4 medium, 8 low
Verdict: standard_review
Findings (3)
- [low] In
should_skip_ts_overload_signature, the function only checks if afunction_signaturehas a matchingfunction_declarationimplementation, but it doesn't handle the case where the implementation itself is also afunction_signature(ambient function without body). This meansdeclare function ambientFn(x: number): number;will be incorrectly skipped if there's anotherfunction_signaturewith the same name in the same scope. - [low] The
collect_ts_function_implementation_namesfunction only iterates over direct named children of the scope, but it doesn't recursively search for function declarations. This means nested function implementations (e.g., inside blocks or other statements) won't be detected, causing their overload signatures to not be skipped when they should be. - [low] The
collect_ts_function_implementation_namesfunction only collects direct named children of the scope, but in the testtest_typescript_nested_overload_signatures_do_not_duplicate_implementation, the overload signatures and implementation are inside a namespace body. The function usesscope.named_children(&mut cursor)which only gets immediate children, so it won't find function declarations nested inside namespace bodies, class bodies, or other block structures. This means overload signatures inside namespaces won't be properly deduplicated.
Reviewed by inspect | Entity-level triage found 0 high-risk changes
bd34d98 to
b297ea4
Compare
There was a problem hiding this comment.
inspect review
Triage: 13 entities analyzed | 0 critical, 0 high, 4 medium, 9 low
Verdict: standard_review
Findings (2)
- [low] In
should_skip_ts_overload_signature, the function collects implementation names lazily per scope, but the worklist processes nodes in reverse order (LIFO viapop()). This means function_signature nodes may be processed BEFORE their corresponding function_declaration implementations, causing the implementation name lookup to miss and incorrectly skip ambient signatures that should be kept. - [low] In
ts_function_declaration_name, the function only checks ifdeclaration.kind() == "function_declaration"but doesn't handle the case where the declaration itself might be wrapped in other node types. The logic assumeschild_by_field_name("declaration")always returns a function_declaration directly, but export_statement declarations can be other types, potentially causing silent failures.
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.
Fixes #265
Summary
function_signaturenodes so ambientdeclare functiondeclarations appear as function entitiesTest plan
cargo test -p sem-corecargo test -p sem-cli/tmp/sem-issue-265-*validatingsem entitiesandsem difffor ambient declarations, overload de-duplication, nested namespace overloads, mixed export overloads, and declaration-file signature changes