Skip to content

fix(python): resolve ClassName() calls to __init__ - #2424

Merged
DeusData merged 5 commits into
DeusData:mainfrom
mvanhorn:fix/1642-python-constructor-calls-init
Nov 3, 2026
Merged

DeusData merged 5 commits into
DeusData:mainfrom
mvanhorn:fix/1642-python-constructor-calls-init

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

What does this PR do?

Refs #1642 (the constructor half, per your note on 2026-09-25; dunders and callbacks passed by reference are left for the separate work you described).

Embedder(model="y") resolved to the Embedder Class node, so a user __init__ never got a caller from a construction call, and the dead-code view listed live constructors as unreferenced.

With this change, when the called name is the class itself, the Python LSP resolves the call to the __init__ found on the class or, depth-first, on its declared bases. That is the same lookup super().__init__() already uses, so not full C3 MRO. The new strategy lsp_constructor_init joins the textual call site through its reason, the way lsp_import_alias does, and is added to cbm_pipeline_invocation_reason_join_strategy. A class with no user __init__, or whose only __init__ is a builtins.* one, keeps today's lsp_constructor edge to the class. A call through an instance (f = Foo(); f()) is left alone, since it dispatches to __call__.

One behavior change to confirm: for a class that defines __init__, the Class node no longer receives the instantiation CALLS edge. The edge now lands on __init__, which matches how Java and C++ constructor calls resolve here. Three tests pinned the old contract, and I updated them: es_usage_crossfile_python, lrp_python_usage_instantiation, and the kPyConstructor repro (now a class without __init__, plus a sibling repro for lsp_constructor_init). Keeping both edges would need a second emission path in the pipeline, because two different targets at one call site read as ambiguous and fall back to the class. I kept that out of this PR. If you want both edges, I'm happy to follow up.

Tests: 7 new py_lsp cases (explicit, inherited, no __init__, callable instance, builtins guard, cross-file import, cross-file base) and the issue's repro as an end-to-end edge test in edge_structural. With py_lsp.c and lsp_resolve.h reverted to main, 8 of these fail (scripts/test.sh --suites py_lsp,edge_structural,lsp_resolution_probe: 200 passed, 8 failed). With the fix, all 208 pass. The full scripts/test.sh run passes on macOS arm64 (ASan + UBSan), and so does make -f Makefile.cbm lint-ci.

AI was used for assistance. I reviewed the change and ran the commands above myself.

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 added bug Something isn't working parsing/quality Graph extraction bugs, false positives, missing edges labels Oct 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

Thanks for keeping the constructor work separate from dunders/callbacks and for documenting the inherited-init limit. The proposed move of the CALLS target from Class to init is an explicit contract choice for maintainer review; I have recorded it without asking for a second-edge implementation now.

@DeusData

Copy link
Copy Markdown
Owner

Thanks again for the constructor-call proposal and the stated inherited-init limit. Choosing the Class-versus-init CALLS contract remains our decision; no alternate implementation is requested while that is pending. Sorry for the delay—I’ll follow up soon.

@mvanhorn

Copy link
Copy Markdown
Contributor Author

A note on the one red CI job on 904ecca: test-unix (macos-15-intel) failed only on subprocess_run_exit_nonzero in tests/test_subprocess.c. The exit 7 shell child came back as outcome 5 (CBM_PROC_SPAWN_FAILED), so /bin/sh never started on that runner. This PR only touches the Python LSP resolver and its tests (py_lsp.c, lsp_resolve.h, and the LSP/edge test files), not src/foundation/subprocess.c. The same suite passed on macos-14 and on every Linux and Windows shard of this run, so I read it as a runner flake rather than something this change caused. Happy to rebase on main if a fresh run would help.

A Python construction call resolved to the Class node, so a user
__init__ never received a caller from ClassName(...) and the dead-code
view reported live constructors as unreferenced.

When the called name is the class itself, resolve it to the __init__
found on the class or, depth-first, its declared bases (the lookup
super().__init__() already uses), with a new lsp_constructor_init
strategy that joins the textual call site through its reason. Classes
with no user __init__ keep the Class target (lsp_constructor), as does
a builtins.* __init__. Calls through an instance (f = Foo(); f()) are
unchanged, since they dispatch to __call__.

This changes the documented Class-target contract for classes that
define __init__; the three tests that pinned it are updated.

Refs DeusData#1642

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn
mvanhorn force-pushed the fix/1642-python-constructor-calls-init branch from 904ecca to 1b64c50 Compare November 1, 2026 17:32
…tor call

pipeline_python_init_function_shares_submodule_qn pinned the old Class
target for Layout("demo"). Layout defines __init__, so the constructor
call now lands on that method. The other tests that pin the contract
were moved in the previous commit; this one reached main afterwards.

Refs DeusData#1642

Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
@mvanhorn

mvanhorn commented Nov 1, 2026

Copy link
Copy Markdown
Contributor Author

The red run on 1b64c50 was this PR: two pipeline_python_init_function_shares_submodule_qn tests (the #1548 shape) still expected Layout("demo") to hit the Class, and they landed on main after this branch started. Since Layout defines __init__, I pointed them at Layout.__init__ in aa02511, same contract as the other tests here. No resolver change. The pipeline, edge_structural, lsp_resolution_probe and py_lsp suites pass locally.

A construction call now resolves to __init__ only when the called
class declares that __init__ itself. An inherited __init__ is no
longer followed: class B(A) without its own __init__ keeps the B()
call on the Class B, as it was before this branch.

Following the bases bound every subclass construction to the base
constructor. On a django index (fb113765) that made Model.__init__
a 583-caller hub (every model instantiation), bound IntegerField()
to Field.__init__ and HttpResponseForbidden() to
HttpResponse.__init__, and degraded 625 constructor call sites
versus main (457 fell back to textual resolution, 168 lost their
row; CALLS -161). With own __init__ only, 6016 sites move to the
class own __init__, 5468 stay on the Class, none fall back to a
textual strategy, and total CALLS is +4 versus main.

Tests: the same-file and cross-file inherited cases now pin the
Class target, and a new same-file subclass test at the LSP and the
graph level covers B(A) without __init__ (stays on B, never
A.__init__) and C(A) with its own __init__ (binds to C.__init__,
never the class C or A.__init__). The strategy inventory comment in
the repro suite is updated to match.

Refs DeusData#1642

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Brings the branch up to date with main edc2301. Main only changed
CI configuration since the branch base (CodeQL action, actions/attest
and msys2/setup-msys2 pins, Dependabot grouping); no conflicts.

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 1, 2026

Copy link
Copy Markdown
Owner

Hi @mvanhorn, thank you very much for this fix and for the care you put into it. Keeping the constructor half of #1642 separate from the dunder and callback work, joining the textual call site through reason the way lsp_import_alias does, the guard for callable instances (f = Foo(); f()), the builtins guard, the cross-file tests, and a description that spelled out the contract change all made this easy to review. Thank you as well for the quick follow-up in aa02511 for the #1548 pipeline tests; that was exactly the right call.

We have now decided the open Class-versus-__init__ question: construction moves to __init__ as you proposed, but only when the class declares that __init__ itself. Since maintainer edits are enabled, we added that on top of your branch instead of asking you for another round:

  1. fix(python): bind ClassName() only to the class own __init__ (0547190): ClassName() now resolves to __init__ only when the called class defines it. A class that only inherits one (class B(A) with no __init__ of its own) keeps the call on the Class B, as before. Why we dropped the inherited binding: following the bases attributes every subclass construction to the base constructor. On a django index that turned Model.__init__ into a 583-caller hub (every model instantiation), bound IntegerField() to Field.__init__ and HttpResponseForbidden() to HttpResponse.__init__, and 625 constructor call sites lost their LSP resolution compared with main (457 fell back to textual matching, 168 lost their edge). With own __init__ only, 6,016 call sites move to the class's own __init__, 5,468 stay on the Class, none fall back, and the total CALLS count stays within +4 of main. Your same-file and cross-file inherited tests now pin the Class target (pylsp_constructor_same_file_subclass_own_init_only, pylsp_crossfile_inherited_init_stays_on_class), a new graph-level test lrp_python_subclass_constructor_own_init_only checks the same shape end to end (B() stays on B; C(A) with its own __init__ goes to C.__init__, never A.__init__), and the strategy inventory comment in the repro suite matches the new rule. Everything else in your change is kept as submitted.
  2. Merge origin/main (9e0159e): brings the branch up to date with main. Main only changed CI configuration, so there were no conflicts.

Please pull before pushing again so these commits are not lost. We will merge as soon as the required checks are green. Thanks again for a carefully built and well-tested fix!

Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Nov 2, 2026

Copy link
Copy Markdown
Owner

Thank you, @mvanhorn - this was green and ready, but a change that merged into main just before it (the Next.js route support in #2747) also edited tests/test_pipeline.c, so the earlier CI run no longer covered the tree this would merge into. We merged current main into your branch (e87a9ff, a plain signed-off merge commit; your change and our earlier fix-up are byte-for-byte the same) so CI re-runs on the up-to-date tree. It merges as soon as that run is green. If you push again, please pull first so the merge commit is kept.

@DeusData
DeusData merged commit 8ae04f4 into DeusData:main Nov 3, 2026
26 checks passed
@DeusData

DeusData commented Nov 3, 2026

Copy link
Copy Markdown
Owner

Merged - thank you very much, @mvanhorn! A Python ClassName() call now reaches the class's own __init__ whenever the class defines one, and stays on the class node otherwise, so there is no new hub of edges onto an inherited __init__. On django this moved about 6,000 constructor calls onto their own initializers with no fallbacks and no regressions. Thanks for the clear issue reference and for the patience through our variant measurements.

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.

2 participants