fix(native): add missing factory-method type-map heuristic - #2533
Merged
Conversation
TS's handleCallExprTypeMap seeds a factory-method heuristic: for any
const x = Foo.create() call (member-expression callee, capitalized
identifier object, not a builtin global), x is typed as Foo at confidence
0.7 — a common static-factory pattern. The Rust mirror,
handle_var_declarator_type_map, only implemented the narrower
Object.create({...}) branch with no general equivalent, so the same source
file could produce a receiver-type edge on one engine but not the other.
Adds the same heuristic to the Rust side, gated identically (member_expression
callee, uppercase-starting identifier object, not in JS_BUILTIN_GLOBALS) and
using push_scoped_type_map_entry for #2235-consistent scoping. No explicit
exclusion of Object.create is needed — "Object" is itself in
JS_BUILTIN_GLOBALS, and the new branch is mutually exclusive with the
existing identifier-callee return-type-propagation branch by construction
(a call's function field is one node, never both kinds).
docs check acknowledged
Contributor
Greptile SummaryThe PR restores native/WASM parity for JavaScript factory-method type inference and fixes the previously reported Unicode edge cases.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "fix(native): match JS's toLowerCase-diff..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile's review of the #2396 fix correctly flagged that the new factory heuristic's capitalization check examined the full Unicode scalar (chars().next().is_uppercase()), while TS's objName[0] !== objName[0].toLowerCase() operates on the first UTF-16 code unit. For an astral-plane leading character (code point > U+FFFF), JS string indexing yields a lone surrogate that never case-folds, so TS's check is always false there — Rust's full-scalar check would incorrectly recognize such identifiers as uppercase, diverging from WASM for this parity-sensitive heuristic. Adds starts_with_uppercase_like_js, replicating the UTF-16-code-unit semantics precisely (treating any surrogate code unit as non-uppercase, matching JS), and uses it in place of the naive scalar check. docs check acknowledged
Contributor
Author
|
@greptileai please re-review — fixed the UTF-16-code-unit vs full-Unicode-scalar mismatch you flagged. Added starts_with_uppercase_like_js to replicate JS's objName[0] !== objName[0].toLowerCase() semantics exactly, including the lone-surrogate case for astral-plane leading characters, plus regression tests for both a BMP non-ASCII uppercase letter and an astral uppercase letter. |
…2396) Greptile's second review round on #2396 flagged that char::is_uppercase() and "does lowercasing change this character" — what JS's objName[0] !== objName[0].toLowerCase() actually implements — disagree for Unicode titlecase letters (category Lt, e.g. 'Dž'): is_uppercase() is false for them, but they lowercase to a different character, so TS's heuristic fires while Rust's didn't. starts_with_uppercase_like_js now compares the lowercased code point against the original directly (c.to_lowercase().ne(iter::once(c))) instead of checking is_uppercase(), matching JS's actual condition rather than an approximation of it. docs check acknowledged
Contributor
Author
|
@greptileai please re-review — fixed the titlecase-letter mismatch you flagged. starts_with_uppercase_like_js now compares against the lowercased form directly (matching JS's actual objName[0] !== objName[0].toLowerCase() condition) instead of using char::is_uppercase(), which excludes Unicode category Lt (titlecase) letters. Added a regression test using 'Dž' (U+01C5). |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
Closes #2396
Root cause
TS's
handleCallExprTypeMap(src/extractors/javascript.ts) seeds a factory-method heuristic: forconst x = Foo.create()(any capitalized-object member-expression call, not justObject.create),xis typed asFooat confidence 0.7 — a common static-factory pattern (const conn = Database.create()).The Rust mirror,
handle_var_declarator_type_map(crates/codegraph-core/src/extractors/javascript.rs), only implemented the narrowerObject.create({...})branch (seed_object_create_entries) with no general equivalent — a real dual-engine parity gap, not just a missing test. The same source file could produce a receiver-type edge on WASM but not on native, or vice versa, for this pattern.Fix
Adds the same heuristic to the Rust side, gated identically to TS:
call_expressionwhosefunctionis amember_expression, whoseobjectis anidentifierstarting with an uppercase letter and not inJS_BUILTIN_GLOBALS, at confidence 0.7 — usingpush_scoped_type_map_entry(from #2235) for consistent scoping with the rest of the function.No explicit exclusion of
Object.createwas needed:"Object"is itself inJS_BUILTIN_GLOBALS, and the new branch only fires on amember_expressioncallee, which is mutually exclusive with the existing identifier-callee return-type-propagation branch right above it (a call'sfunctionfield is one node, never both kinds) — so no priority-ordering logic was needed between them either.Test plan
crates/codegraph-core/src/extractors/javascript.rsmirroring the existing TS tests (seeds at 0.7, ignores lowercase receivers, ignoresObject.create/other builtins). Verified the core test fails without the fix (temporarily reverted the source, confirmed failure, restored).tests/engines/parity.test.ts(native vs WASM), since this is specifically a native/WASM divergence — verified these fail against a native addon built from the pre-fix source (rebuilt+reinstalled the addon twice to prove it), confirming this is a genuine dual-engine bug fix, not just new coverage on one side.cargo test: 1037 passed.cargo clippy --all-targets -- -D warningsandcargo fmt --checkclean.npm test: 5325 passed.npm run lintclean.codegraph diff-impact --staged -T: no function-level TS changes detected (Rust source + Rust/TS test files only).