Skip to content

fix(python): resolve aliased from-import CALLS to real def - #1371

Open
Joseph-MingEn wants to merge 5 commits into
DeusData:mainfrom
Joseph-MingEn:fix/py-aliased-from-import-calls
Open

fix(python): resolve aliased from-import CALLS to real def#1371
Joseph-MingEn wants to merge 5 commits into
DeusData:mainfrom
Joseph-MingEn:fix/py-aliased-from-import-calls

Conversation

@Joseph-MingEn

@Joseph-MingEn Joseph-MingEn commented Jul 31, 2026

Copy link
Copy Markdown

Summary

Broader graph-shape note (maintainer call)

Plain and aliased from-imports share the same module_path ("M.f"), so the member-preference block retargets IMPORTS Module → member def for every Python from-import that resolves that way — not only aliases. That is more precise for from M import f, but it changes:

  • module-dependency / file→Module IMPORTS expectations
  • detect_changes impact propagation that walked Module IMPORTS
  • any query assuming from-import always ends on Module

This is intentionally in front of the maintainer (same mechanism class as the ~11K-edge #1000 regression). Happy to split or gate if preferred.

Evidence (not Django-scale)

  • Local: ./build/c/test-runner registry lang_contract97 passed (includes new ghost-module negative).
  • Consumer lab (Yui BASIC, mid-size Python monorepo): after patched MCP + full re-index, trace_path(submit_task) hop-1 lands on services.satori_bridge.gate.execute without changing the import execute as bridge_execute alias in source. Graph after ≈ 11549 nodes / 46879 edges. No Django-scale before/after edge-count diff available from this contributor machine.

Test plan

  • make -f Makefile.cbm lint-format (Homebrew LLVM clang-format)
  • ./build/c/test-runner registry lang_contract — 97 passed
  • CI lint + test (was skipped while lint failed) + ci-ok

@Joseph-MingEn
Joseph-MingEn requested a review from DeusData as a code owner July 31, 2026 02:18
@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.

@Joseph-MingEn

Copy link
Copy Markdown
Author

Rebased onto current DeusData/main and resolved conflicts in registry.c / test_registry.c:

  • Kept upstream #875/#979/#1000 suffix-safe bare-alias direct hit (do not swallow .method suffix).
  • Kept Yui-shaped resolve_import_map_aliased_from_import + absolute from-import member IMPORTS (pass_pkgmap) + lang_contract tests.
  • Local: ./build/c/test-runner registry lang_contract96 passed.

Ready for CI when the merge queue picks this up.

@Joseph-MingEn
Joseph-MingEn force-pushed the fix/py-aliased-from-import-calls branch from 41793cc to b81c1e2 Compare July 31, 2026 02:38
@Joseph-MingEn

Copy link
Copy Markdown
Author

Conflict recheck: rebased/force-pushed head b81c1e2. Local merge vs current main (69599b7) is a clean fast-forward (no conflict markers). If the UI still shows conflicts on registry.c / test_registry.c, please refresh mergeability — those were from the pre-rebase tip.

Joseph-MingEn added a commit to Joseph-MingEn/codebase-memory-mcp that referenced this pull request Jul 31, 2026
Web conflict editor reports conflicts too complex; local merge vs
main is a clean fast-forward. Empty commit forces recompute.

Co-authored-by: Cursor <cursoragent@cursor.com>
Joseph-MingEn added a commit to Joseph-MingEn/codebase-memory-mcp that referenced this pull request Jul 31, 2026
Web conflict editor reports conflicts too complex; local merge vs
main is a clean fast-forward. Empty commit forces recompute.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: 涂名恩 <125283161+Joseph-MingEn@users.noreply.github.com>
@Joseph-MingEn
Joseph-MingEn force-pushed the fix/py-aliased-from-import-calls branch from a4aff88 to 345a350 Compare July 31, 2026 02:50
@DeusData

Copy link
Copy Markdown
Owner

Thank you for this — and welcome. Reviewed properly rather than skimmed, because the root cause you found is a good one.

The diagnosis is right and I verified it on main. cbm_pipeline_fqn_modulefqn_compute(name=NULL)strip_file_extension() strips the last dot-segment unconditionally, so a Python from-import path pkg.mod.symbol collapses to the Module QN proj.pkg.mod. The alias then maps to the Module, a bare call builds the candidate moduleQN.g — a ghost — and resolves to nothing. Routing the sequential import map through cbm_pipeline_resolve_import_node, the same resolver the IMPORTS pass already uses, fixes the sequential and parallel legs at a single choke point. That is the right shape, and it is the right layer: pipeline base, below the LSP passes, so the hybrid-LSP work stays additive on top.

I also went looking for the failure mode that would make this dangerous — resolving to a same-named symbol in a different module — and it cannot happen. The member QN is built strictly inside the resolved module's namespace and must hit an existing node by exact QN with a targetable label, with Module and File excluded. Cross-module leakage is not reachable. Import maps are per-file, so the same alias in two files cannot collide either.

There is a nice side benefit too: from M import C; C.method() now direct-hits proj.M.C.method instead of leaning on the by-name suffix fallback, which could previously pick a different class's .method in the same module. That is a genuine accuracy win beyond the aliased case.

Two things need your hand, one trivial and one that needs the maintainer.

1. The lint failure is hiding something important. pass_pkgmap.c:1533-1534 has clang-format violations — your new import_last_segment(...) line break. Trivial in itself, but the consequence is not: because lint failed, the entire test job was auto-skipped, so your new contract tests have never actually run on CI, on any platform. The green pr-smoke checks make the PR look better-verified than it is.

Run make -f Makefile.cbm lint-ci locally to reproduce it. One caveat that will save you a wasted round trip: our clang-format is the Homebrew LLVM build — a standalone clang-format-20 produces spurious whole-file drift that looks nothing like the two real lines. Once that is fixed, CI will exercise your tests for the first time.

2. The change is wider than the title, and that part is the maintainer's call. Because plain and aliased from-imports store the same module_path ("M.f"), the member-preference block retargets the IMPORTS edge from the Module node to the member def for every Python from-import in every indexed repo — not just aliased ones. It is arguably more precise, and arguably what from M import f actually means. But it changes module-dependency queries, detect_changes impact propagation, and anything expecting file→Module IMPORTS edges.

That is a graph-shape decision rather than a bug fix, and it lands in the exact mechanism that once regressed about 11,000 edges (#1000), so I have put it in front of the maintainer instead of deciding it in review. Two things would help it along: state the broader effect explicitly in the PR description, and — if you are able — a before/after edge-count diff on a Django-scale repository. That evidence is what would derisk it.

One test I would like: a pure negative — from ghost_module import nope as alias; alias() must produce no CALLS edge at all. Your existing ghost-negative assertions cover the invented-alias case nicely; this would pin the unresolvable-module case too, which is the one where an over-eager resolver would invent an edge.

Smaller notes: the PR body says the change makes resolve_import_map prefer an exact registry hit, but that behaviour already exists on main and your registry.c diff is comment-only — the registry test is a useful adversarial pin rather than a regression test, worth relabelling. And the empty commit 345a350 is harmless; it disappears in a merge commit.

Fix the formatting and this gets a real CI run; I will come back to you on the IMPORTS-edge question either way.

Joseph-MingEn and others added 4 commits July 30, 2026 21:39
from M import f as g + g() must emit CALLS→f, not invent M.g.
Use resolve_import_node in sequential import maps, and prefer an
exact registry hit when the IMPORTS target is already the def QN.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: 涂名恩 <125283161+Joseph-MingEn@users.noreply.github.com>
resolve_module often lands on the Module for pkg.mod.symbol paths.
When the member Function/Class exists, IMPORTS must target it so
\`from M import f as g\` + g() can CALLS→f (Yui bridge_execute shape).

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: 涂名恩 <125283161+Joseph-MingEn@users.noreply.github.com>
Web conflict editor reports conflicts too complex; local merge vs
main is a clean fast-forward. Empty commit forces recompute.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: 涂名恩 <125283161+Joseph-MingEn@users.noreply.github.com>
Unblock CI lint so the test job runs. Prefer Homebrew LLVM formatting
on the member IMPORTS retarget. Add contract that
`from ghost_module import nope as alias; alias()` emits no CALLS.
Relabel registry alias pin as adversarial (behavior already on main).

Signed-off-by: 涂名恩 <125283161+Joseph-MingEn@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@Joseph-MingEn
Joseph-MingEn force-pushed the fix/py-aliased-from-import-calls branch from 345a350 to e56e965 Compare July 31, 2026 04:43
@Joseph-MingEn

Copy link
Copy Markdown
Author

Thanks for the careful review — addressed:

  1. Lint / format: pass_pkgmap.c member-prefer block reformatted with Homebrew LLVM clang-format (make -f Makefile.cbm lint-format clean). Rebased onto current main.
  2. Ghost-module negative: added contract_python_ghost_module_aliased_from_import_no_callsfrom ghost_module import nope as alias; alias() must emit no CALLS. Local registry + lang_contract97 passed.
  3. PR description: now states the broader Module→member IMPORTS retarget for all Python from-imports (not just aliases), and that this is a maintainer graph-shape call. Yui mid-size lab cited; no Django-scale edge-count diff from this machine.
  4. Registry test: relabelled as adversarial pin (exact-hit behavior already on main; registry.c remains comment-only).

Please re-check once CI runs test (previously skipped behind lint). Happy to iterate on the IMPORTS-edge scope if you want it narrowed or gated.

@Joseph-MingEn

Copy link
Copy Markdown
Author

CI note: the red test-unix (macos-15-intel) / ci-ok is unrelated to this PR’s Python alias / IMPORTS changes.

Failure site:

lock_registry_absolute_deadline_survives_repeated_wakes
FAIL tests/test_lock_registry.c:1153: ASSERT(tail_queued)

On that same job, lang_contract was 40/40 (incl. the new ghost-module negative). All other test-unix / Windows / tsan / lint / smoke matrices were green.

Re-running failed jobs now. If it flakes again we can wait for maintainer guidance or another rerun — not planning to touch lock_registry in this PR.

Upstream workflow rerun requires admin; empty signed commit to re-queue.

Signed-off-by: 涂名恩 <125283161+Joseph-MingEn@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@DeusData

Copy link
Copy Markdown
Owner

Your diagnosis is correct, and you do not need to prove it — it is already on our list.

lock_registry_absolute_deadline_survives_repeated_wakes (tests/test_lock_registry.c:1153) is one of two tests in that suite that wait on a 500 ms observation window for waiters to park, and then assert against it. That is a transient-window assertion, which under our own testing doctrine is a defect in the test rather than a property of your change: a verdict that depends on how a shared runner is scheduled is not a verdict. Its sibling lock_registry_large_queue_parks_non_head_waiters (:897) has the same shape. Both fail preferentially on the slower macos-15-intel runner, which is exactly where yours failed.

So: not yours, and you were right not to touch lock_registry in this PR. A re-run may clear it, but please read that as a coin landing the right way rather than as evidence — the fix we owe is a seam or a held gate, not a wider budget. It is recorded for us to do.

Your PR itself is still with the maintainer for the graph-shape question raised earlier; that is a direction call, not a quality one, and nothing further is needed from you while it waits. Thank you for the clean triage — reporting the failing site with the surrounding evidence (lang_contract 40/40 on the same job, everything else green) is precisely what makes a CI report actionable.

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