Skip to content

fix(ts): resolve inferred imported constructor receivers - #1999

Open
mikemikimike wants to merge 2 commits into
DeusData:mainfrom
mikemikimike:fix/ts-inferred-receiver-calls-1974
Open

fix(ts): resolve inferred imported constructor receivers#1999
mikemikimike wants to merge 2 commits into
DeusData:mainfrom
mikemikimike:fix/ts-inferred-receiver-calls-1974

Conversation

@mikemikimike

@mikemikimike mikemikimike commented Sep 1, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes #1974.

When TypeScript infers a local variable from new Foo(), resolve the constructor through the existing lexical/import type lookup before falling back to the current module. This preserves the imported class qualified name, so calls such as store.findById() produce the expected CALLS edge without changing explicitly annotated locals.

The same lookup also preserves the bare qualified names registered for TypeScript standard-library classes. Inferred new Map<string, string>() locals can therefore dispatch m.get(id) to the registered Map.get method instead of falling back to weak name-only resolution.

Validation

  • ASAN_OPTIONS=detect_leaks=0 make -f Makefile.cbm test — passed.
  • ASAN_OPTIONS=detect_leaks=0 make -f Makefile.cbm test-focused TEST_SUITES=ts_lsp — passed (303 tests, including the inferred-stdlib receiver regression).
  • cppcheck — passed.
  • git diff --check — passed.
  • scripts/lint.sh --ci — blocked by pre-existing clang-format violations in src/mcp/mcp.c, src/pipeline/pipeline_incremental.c, and src/cli/cli.c; no violation was reported for the changed implementation file.
  • make -f Makefile.cbm security — static, binary-string, and UI audits passed; the existing robustness suite reported 26/32 due to input timeouts, and install/network checks are environment-limited in WSL.

Checklist

  • Every commit is signed off (git commit -s).
  • Tests pass locally (with the repository's known sanitizer leak baseline disabled).
  • Lint passes (blocked by unrelated existing format drift described above).
  • New behavior is covered by reproduce-first regression tests for imported and standard-library constructors.

AI assistance

This change was prepared with AI assistance. The implementation, regression fixtures, and local validation should be reviewed by maintainers.

Signed-off-by: mikemikimike <13286568797@163.com>
@DeusData

DeusData commented Sep 1, 2026

Copy link
Copy Markdown
Owner

This is a good fix, and it does more than your title claims — in the right direction. I would like a test for the part you did not mention.

The mechanism is the right one

Routing a bare constructor name through type_of_identifier instead of assuming module_qn + "." + cname fixes the class of defect rather than the reported instance. The old line asserted "a bare class name is module-local", which is simply false whenever the class is imported, and no amount of special-casing import shapes would have covered it.

The guard is what makes it safe, and it is worth saying why, because it is the part a reviewer would otherwise have to derive:

if (bound && bound->kind == CBM_TYPE_NAMED && bound->data.named.qualified_name)

type_of_identifier can return a function signature (f->signature), cbm_type_unknown() for a scope binding with no known type, or a namespace fallback. Each of those is not CBM_TYPE_NAMED-with-a-QN, so each falls through your else if (ctx->module_qn) to exactly the previous behaviour. The change is therefore strictly additive: it only ever fires where it has a concrete qualified name to offer, and never degrades a case that used to work. That is the right shape for a resolver change in this repo.

The part you did not claim

type_of_identifier's fourth step is the stdlib bare-name lookup, and the TS stdlib registers 28 types with QN equal to the bare name (ts_lsp.c:4018 and neighbours). Thirteen of them are constructible:

Map · Set · Date · Error · Promise · RegExp · Array · Object · Event · Response · Number · String · Boolean

Before this change, const m = new Map() inferred type <module>.Map — a fabricated QN matching nothing — so m.get(k) could not dispatch to the registered Map.get and fell through to the weak name-only strategies. After it, new Map() infers Map and member dispatch resolves properly.

That is a broader improvement than the imported-class case, and it is likely to move TypeScript CALLS edge counts noticeably on any real codebase, because inferred new Map()/new Set()/new Date() locals are everywhere. It is also entirely untested here.

Please add one more fixture in the same shape as the one you wrote — an inferred const m = new Map<string, string>() followed by m.get(id), asserting the stdlib dispatch — and mention the stdlib effect in the description. A silent broad change to edge resolution is the thing most likely to be mis-attributed later when someone bisects an edge-count delta.

One shape it can still get wrong

Scope lookup precedes imports in type_of_identifier, so a local or parameter shadowing the class name with a different NAMED type wins over the import. The previous code was also wrong there (it produced <module>.Name), so this is not a regression and I am not asking you to handle it — just noting it so it is on the record.

Your lint note

blocked by pre-existing clang-format violations in src/mcp/mcp.c, src/pipeline/pipeline_incremental.c, and src/cli/cli.c

That is a known false positive, not drift you caused. Our lint-ci requires the Homebrew LLVM clang-format; a distro or standalone clang-format-20 reports whole-file differences on exactly those large files. main is clean under the pinned build, and CI here will confirm it. Nothing for you to do.

Thanks also for the explicit AI-assistance note and for the reproduce-first fixture — both make this quicker to review honestly.

Add the stdlib test and the description line and I am happy with this.

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

Signed-off-by: mikemikimike <13286568797@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeScript: method calls on a receiver typed by inference (const x = new Foo()) produce no CALLS edge

2 participants