Skip to content

fix(514): Support NestJS DI - #2402

Open
antespi wants to merge 5 commits into
DeusData:mainfrom
antespi:514-nestjs-di
Open

antespi wants to merge 5 commits into
DeusData:mainfrom
antespi:514-nestjs-di

Conversation

@antespi

@antespi antespi commented Sep 28, 2026 •

Copy link
Copy Markdown

What does this PR do?

TypeScript constructor parameter properties were never registered as class fields, so calls through NestJS-style injected services did not resolve and trace_path reported zero
callers.

@Controller("cats")
export class CatController {
  constructor(private readonly catService: CatService) {}

  @Get()
  findAll() {
    return this.catService.findAll(); // no CALLS edge before this fix
  }
}

This is the minimal repro from the #514 thread; the maintainer comment there identified the cause.

Root cause

ast_sweep_shapes() in internal/cbm/lsp/ts_lsp.c collected class fields only from public_field_definition members. Parameters declared in the constructor with an accessibility,
readonly or override modifier are fields in TypeScript, but were skipped, so this.catService had no type and the member call could not be resolved.

Fix

In the class-body sweep, a constructor method_definition now has its required_parameter / optional_parameter children inspected. A parameter carrying accessibility_modifier,
readonly or override_modifier is registered as a field with its annotated type. Plain parameters without a modifier are not fields and stay excluded.

ast_sweep_shapes() runs in the per-file pass and in both cross-file passes (cbm_run_ts_lsp_cross, cbm_run_ts_lsp_cross_with_registry), so this one change covers same-file and
cross-file injection.

Tests

  • tests/test_ts_lsp.c
    • tslsp_class_ctor_param_property: same-file this.catService.findAll() resolves to CatService.findAll.
    • tslsp_class_ctor_plain_param_not_field: a constructor parameter without a modifier is not treated as a field.
    • tslsp_crossfile_ctor_param_property: the service is imported from another module (cross-file path).
    • tslsp_class_constructor_param_property: this existing test only checked that nothing crashed. It now asserts that b.tool.fire() resolves.
  • tests/repro/repro_issue514.c
    • repro_issue514_ctor_param_property_callers: end-to-end. It indexes a two-file NestJS fixture and runs trace_path inbound on findAll. Without the fix: callers_total: 0. With
      the fix: callers_total: 1 (CatController.list).

Results (Linux x86_64, ASan + UBSan test build):

  • ts_lsp suite: 307 passed, 0 failed. The three new tests failed before the fix.
  • Full suite: 8203 passed, 15 failed, 9 skipped. All 15 failures are test_cli.c install/uninstall tests. They fail locally because the build directory is group-writable
    (install_dir_group_or_world_writable (mode 0775)); the suites touched by this PR are unaffected.
  • clang-format 20.1.8: clean on internal/cbm/lsp/ts_lsp.c. cppcheck and clang-tidy were not run locally.

Scope

This covers constructor parameter properties only. The other patterns described in #514 are not addressed:

  • token injection (@Inject(TOKEN))
  • ModuleRef.get(...) runtime resolution
  • dispatch through fields typed as an interface or base class
  • reporting unresolved receivers as a coverage gap

Refs #514


Written with Claude Code (AI-assisted). I reviewed the change and certify it via the DCO sign-off.

🤖 Generated with Claude Code

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test (reproduce-first for bug fixes)

@github-actions

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.

@DeusData

DeusData commented Oct 4, 2026

Copy link
Copy Markdown
Owner

Thank you so much, @antespi, for this PR, the clear root-cause write-up and the end-to-end NestJS repro. It is a really useful piece of work.

One thing changed underneath it, and we are sorry for the overlap. Another fix for #514, #2523, landed on main today (merge 369cf76). It registers TypeScript constructor parameter properties as class fields and also resolves fields initialized with new, so it covers what your first commit (3564a22) does.

Your five later commits add things main still does not have:

  • dotted file names;
  • index.ts barrel imports;
  • TS interface method signatures as methods;
  • USAGE edges only to import-resolved classes.

Would you be up for rebasing #2402 onto current main without the first commit? Your three ts_lsp tests and the repro_issue514 end-to-end case are welcome to stay as extra coverage if they pass on main.

Two small things for the rebase:

  • graph-ui/tsconfig.tsbuildinfo looks like a build artifact that slipped in; please drop it.
  • Another open PR also bumps the semantic index version. Whichever lands second only needs to take the next number, so expect a one-line conflict there.

Thanks again for pushing NestJS support forward!

@antespi
antespi force-pushed the 514-nestjs-di branch 2 times, most recently from e9c93db to 8fed357 Compare October 5, 2026 11:15
Signed-off-by: Antonio Espinosa <antespi@gmail.com>
Signed-off-by: Antonio Espinosa <antespi@gmail.com>
Signed-off-by: Antonio Espinosa <antespi@gmail.com>
Signed-off-by: Antonio Espinosa <antespi@gmail.com>
Signed-off-by: Antonio Espinosa <antespi@gmail.com>

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @antespi for the work on #514 and for the careful tests! Since the constructor-property part landed separately in #2523, could you split the rest into focused PRs so each can be measured and reviewed on its own? (a) the dotted basename fix for tsconfig-alias imports; (b) barrel re-export following in the import map; (c) USAGE edges for import-resolved classes; (d) interface method signatures as Method nodes. (d) changes which node a call through an interface-typed field targets, which is an open design question on our side, so please keep it separate; we'll come back on it. Please also drop the first commit (now in main via #2523) and the graph-ui/tsconfig.tsbuildinfo change. Thanks again, this is a real improvement for NestJS projects.

@DeusData

Copy link
Copy Markdown
Owner

Thank you again for the NestJS work. The focused split described on October 17 still applies, with the already-landed constructor-property change omitted. Interface-method target identity remains our separate direction question; sorry for the delay on that decision, and I’ll follow up soon.

@chidev

chidev commented Nov 2, 2026

Copy link
Copy Markdown

Hi @antespi, thank you for the barrel work here. It helped us a lot.

Following the October 4 note about splitting this PR, I opened #2900. It contains your two commits (index.ts barrels and dotted file names) with your authorship and sign-off, plus one commit of mine for renamed re-exports and export * chains. If you would rather land your commits yourself, tell me and I will close #2900.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants