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>

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants