refactor(#358): move Python import edges to shared captures

[LaimaWu]

Python import targets now use the shared @import.path capture path on current main. Python-specific @import.names and paired @import.alias matches retain the existing RawBind semantics; the old Python walked extractor is removed. Plain comma-separated imports still contribute only their first dependency target, and an aliased first clause keeps its written as spelling in --deps. No cross-language binding API or Go/JS/TS behavior change is included.

Rebased onto 0e0f67c73ca332ed58bb8478677f916f7ccc1e84 after the Go and JS/TS slices landed through #385/v0.6.7. Published head: 38607f401a5b0497cff18f131239f208065a2877. Author/committer metadata was corrected to the verified GitHub noreply identity; the tree is identical to the reviewed integration candidate. All validation counts below were rerun on clean builds of this head.

The integration follows the eight review points: the struct-returning ImportReach, Python normalizer and importsFromCapture arm, the single existing two-argument importDirectiveOf, first-name query anchor, removal of both Python directiveTargetOf arms and the old bind helper, preserved Full-capture guard, updated shared vocabulary comment, and an explicit CHANGELOG entry.

The approved disclosure change is intentional: real imports at container depths 255/256 remain; a real import at 257 is cut and disclosed. A deeper container with no import below the cut, including a top-level import followed by a deep pass, no longer produces extract-partial and can be cached. This is not a claim of complete byte identity with the previous walk.

The Python gate now pins future annotations and dynamic import calls against extra Include/RawBind facts, dotted module alias c -> a.b with its module marker, and relative Include . with binding b. It checks complete binding records and counts, including the existing NameAlias fact, without coupling to the relative order of two extraction passes. Sixteen built-in corrupt-fact controls and sixteen external serialized-fact injections all reject their planted defects; the transparent wrapper control passes.

Validation on macOS 14.3.1 x86_64, 16 GiB, CMake 4.4.4 / Clang 23.1.1, clean dev builds:

• The ten requested import/deps/resolve/binding/cache gates: 448 PASS / 0 FAIL / 10 explicit arm SKIP. Expanded relevant selection: 78 gates, 3,084 PASS / 0 FAIL / 16 arm SKIP. The full functional repository suite was not run.
• Final-SHA G1 builds retain the project's address/undefined/integer/float checks, fail-fast, -O2 -g and existing ignorelist; address outline instrumentation controls compile memory. Five relevant gates: 269 PASS / 0 FAIL / 10 arm SKIP, no sanitizer reports, including both import gates.
• Thirteen representative corpora × nine operations: 117 successful raw main/head command pairs. 116/117 stdout identical; 108/117 stdout+stderr+exit identical. All nine complete-result differences are on the deep fixture: only skipped changes stdout (three extract-partial rows become one), while the debug trace moves from captureIncludes to emitCapturedImport. No output normalization was used.
• Twelve non-deep main-cache reuse cases preserve cache bytes and inode and match fresh output; both builds' fresh/cold/warm outputs agree. Deep completeness/cacheability differences are listed separately. An internal NameAlias/Import record-order change is recorded; complete facts and published results agree.
• Additional main-reference gates: 181 child-walk and nine preproc-dead stdout comparisons identical. Go/JS/TS gates and their representative raw comparisons pass.
• Diff/scope/history and retired-extractor/duplicate-overload checks pass. Clang-format 22 and clang-tidy 22 gating pass; quality-delta has gating=0, two disclosed minor increases in captureTagsFacts (complexity and verbosity).

The 16 normal arm SKIPs are ten historical red-first module-alias controls whose fixes are already on HEAD, three macroreparse historical-baseline arms, the Apple Silicon auto-detection arm on this Intel host, and the qualnew/objcfield historical-binary controls. Binoverride's failing-sentinel harness reports zero false-greens: 636 nonzero children and seven timeout children; those timeout children are not counted as functional passes. The seven sentinel timeouts are compactlegendcheck, emitescapecheck, regexguardcheck, selfcheckcheck, shapingflagcheck, type3clonecheck and vendorpatchcheck. No prior Linux/Release/full-suite results are inherited by these counts.

Persisted record format and main's extraction/quality version numbers remain unchanged; the existing facts are preserved, the approved disclosure/cacheability exception is explicit, and producer-source identity checks pass. Current-main G1, a separate Release functional run, third-party large Python repository matrix and other native platforms were not run in this round.

Ready for maintainer re-review. This updates the existing Python slice only.

Historical validation: the previous PR head fe4e681b7575c5472ee9a3e4db5cc5f6ed6ca2fc was tested on Linux against pre-integration main 60dd3b3f2246ce0284cfa4d4ed48694eafa69c2b. Those Linux, Release and full-suite results belong to the old head only and are not results for this rebased head. In particular, the historical full suite was not entirely green; no claim of a completed or green full suite is made here.

Refs #358

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: redhat-et/ripwire/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0d9dda5a-0866-4eb7-a704-12f00ec72400

📥 Commits

Reviewing files that changed from the base of the PR and between 60dd3b3 and fe4e681.

📒 Files selected for processing (6)

• CHANGELOG.md
• queries/python/tags.scm
• src/ingest_importcap.h
• src/ingest_relations.h
• src/ingest_sidecap.h
• test/pyimportprecisecheck.sh

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

Python import targets and named clauses now come from tags captures. Captured paths produce dependency facts, and named clauses produce bindings. The bounded walk remains for nesting disclosure. Regression checks cover import forms, nesting limits, and cache output consistency.

Changes

Python import capture

Layer / File(s)	Summary
Capture imports through tags queries/python/tags.scm, src/ingest_sidecap.h, src/ingest_relations.h	The Python query captures import paths, named clauses, and optional aliases. The tags pass stages names and aliases. The prior relation-walk import extraction and binding helper are removed.
Emit import facts and preserve depth disclosure src/ingest_importcap.h, src/ingest_relations.h, src/ingest_sidecap.h, test/pyimportprecisecheck.sh, CHANGELOG.md	Captured paths produce dependency facts, and captured names produce bindings. The bounded walk remains for depth disclosure. Regression checks cover import forms, nesting, and cache output consistency.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Suggested reviewers: lennix1337, joyful-ii-v-i

Merge Risk: ⚪ Minimal · up to fe4e6

This change moves Python import extraction to shared query captures and is reported to keep output identical to main. No actionable merge-blocking risk was found in the supplied context.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: moving Python import edges to shared captures.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

_{Comment @coderabbitai help to get the list of available commands.}

[joyful-ii-V-I]

@LaimaWu thank you for this, and for the very thorough validation write-up. It's a careful slice, and holding off on rebasing onto an unmerged branch was the right call.

Short answer: keep the Python-specific binding helper in #382. Just adapt it to the new shared interfaces (details below). Please don't move binding emission into the shared API in this PR; we'll do that as a separate follow-up.

Why:

• The shared binds channel is bigger than Python. Python's import bindings are one case of a general "local name → target" binding: a local name that stands for a module, type or symbol defined elsewhere. That binding is currently produced in several separate code paths:

  • C++: using declarations, using X = … aliases and typedef
  • C#: using X = …
  • TypeScript/JavaScript: import { a as b }, import * as ns and destructured require. Today these are written as RawBinds by their own pass, not from the capture.
  • Rust: use x as y
  • Go: aliased imports (also their own pass)
  • PHP: use … as

  The JS/TS and Go slices deliberately left their binds where they are for this reason. Designing one channel that serves all of these is cross-cutting work, so we'll take it on ourselves.

• Your helper is the first real user of @import.names / @import.alias. That makes it a good template for the shared design, and it's the smallest scope for you.

Merge order

Our JS/TS and Go slices go into main with our next integration batch. #382 then rebases onto main. If you'd rather not deal with the conflicts, tell us and we'll do the rebase and conflict resolution ourselves when we integrate #382, then push it back for your review. Either way is fine with us.

What the rebase looks like

I merged #382 locally on top of both slices to check this. There are 8 conflict hunks in two files: src/ingest_importcap.h (4) and src/ingest_relations.h (4). src/ingest_sidecap.h and CHANGELOG.md merge cleanly, though two comments/sentences need updating (see 7 and 8). Hints, in the order you'll hit them:

1. importContainerReach now returns a struct. It returns {depth, reachable, insideFn} from a single descent from the root, with no out-parameter. In emitCapturedPythonImportBind:

const ImportReach reach = importContainerReach( stmt, Lang::Python );
if( reach.depth > kMaxImportContainerDepth || !reach.reachable )
{
    return;
}

2. normaliseImportSpecifier. Add your arm next to the new Web and Go arms:

case DepDialect::Python:  return { std::string( raw ) };

emitCapturedImport now runs the normaliser before the depth/reach test. Your arm never returns empty for a real capture, so nothing changes for you.

3. importsFromCapture() is where the walk is switched off. Add Python there:

case DepDialect::CFamily:
case DepDialect::Web:
case DepDialect::Go:
case DepDialect::Python:
    return true;

With that in place, drop the if( lang != Lang::Python ) guard around the DISCLOSE in emitCapturedImport, and drop the "bounded walk remains" comment in captureSideFacts. See the disclosure note below for why this is a small behaviour change.

4. importDirectiveOf( DepDialect, TSNode ) needs no Python case. Its default returns the capture's parent. For both name: (_) and module_name: (_) that parent is the import_statement / import_from_statement, including when the first name is an aliased_import, so records stay at the statement byte. Please call the existing two-argument function. Don't add a second overload. Two overloads compile fine, and that is exactly how a dialect's imports get dropped silently.

5. The first-name filter can live in the query. The lang == Lang::Python && … !ts_node_eq( pathNode, fieldChild( … Name ) ) check can move out of the shared emitter and into the query, using an anchor:

(import_statement . name: (_) @import.path)
(import_from_statement module_name: (_) @import.path)

This is optional but cleaner. I checked it locally: the anchored query gives the same --deps, --skipped, callees and uses output as main, and your gate passes.

6. directiveTargetOf in src/ingest_relations.h. After the JS/TS slice, the import_statement arm is Python-only, and the TS/JS export_statement and call_expression arms are gone. Delete both Python arms, import_statement and import_from_statement. That means no lang != Lang::Python guard and no "other grammars" fallback, since no other walked grammar has that node. The other two conflicts in this file are comment blocks: just leave Python out of the walked-node list.

7. Please keep the vocabulary comment in src/ingest_importcap.h. That's the block explaining why @import.alias / @import.names are reserved by name. Please update it rather than delete it: say that Python is now the first dialect to use the other two names, for bindings only, never extra dependency targets. That comment is what the next language reads before picking a spelling.

8. CHANGELOG. The sentence about the bounded Python walk remaining for disclosure needs to change to match the next section.

Disclosure: importContainerReach is not equivalent to the retained walk (and we think that's OK)

The two disagree in exactly one case: a file nested more than 256 import-containers deep with no import below the cut.

• The walk announces it (an extract-partial row in --skipped, and the file is kept out of the cache).
• The capture path only announces when a captured import is actually cut.

I checked this with roughly 150 nested if blocks (300 containers):

file	main / #382	rebased with walk off
import a at the bottom	announced	announced
pass at the bottom	announced	not announced
import b at the top, pass at the bottom	announced	not announced

C-family already behaves the capture-path way on main: 300 nested #ifdef around int x; is not announced. The JS/TS slice states the same rule ("announce only when a captured import is actually cut"). So we'd prefer Python to follow it rather than keep a second walk alive just for disclosure.

The only gate change is the deep-nesting fixture in test/pyimportprecisecheck.sh:

assert {f.get('p') for f in skipped.findall('f') if f.get('why') == 'extract-partial'} == {
    'depth257.py'}, 'cut at 257 is disclosed; an import-free deep container is not (nothing was cut)'

Keep the 255/256 kept and 257 cut-and-announced rows as they are.

Review notes on #382 itself

• No bugs found. I compared these forms against main on --deps, --skipped, callees, uses and the map. All are byte-identical both on refactor(#358): move Python import edges to shared captures #382 alone and on the rebased stack:
  • import a.b as c, import a . b, import a, b as bb, pkg.mod as pm
  • from . import a as x, b, from .. import up, from .pkg import mod, from x import (a, b as c,\n c,), from m import *
  • if TYPE_CHECKING:, try/except ImportError/else/finally, with, for, while, match/case
  • async def, decorated defs, class bodies
  • from __future__ import …, __import__("x"), importlib.import_module("x") (no edge before or after)
  • broken/ERROR statements and py2 print/exec
• Missing negatives in the gate (cheap to add):
  • from __future__ import annotations, __import__("x") and importlib.import_module("x") should produce no Include and no binding.
  • import a.b as c should bind c → a.b with the module marker. Today only the undotted alias is pinned.
  • from . import a as b should give Include . and bind b.
• Small cost note, not blocking. Each @import.names match does its own root descent in importContainerReach, plus one or two ts_node_parent calls. So from m import (a, b, …) with N names costs N descents. That's fine for real code. If you want, compute the reach once per statement.

Verifying byte identity after the rebase

I ran this locally on the rebased stack with steps 1–6 applied and the gate change above. For your rerun:

• Gates. Build main and your head, then run RIPWIRE_BASE_BIN=<main build> test/pyimportprecisecheck.sh and test/importcapcheck.sh. Also run the resolve/deps gates you already use (depsprecisecheck, resolvecheck, nestedimportcheck, importnarrowcheck, externalvetocheck, builtinbindcheck, pymodulealiascheck, cacheidentitycheck). They all passed for me, and both import gates ran clean under ASan (no sanitizer reports).
• Diffs. Diff --deps --limit=100000 --no-cache, cached --deps, --skipped --no-cache, the map, and --callers / --callees / --impact / --uses over a few Python repos. I got 49/49 identical over 8 repos and the Python fixtures. As a sanity check against silent drops, the number of Python <inc> rows was unchanged on textual (4479), aiohttp (1496), conan (4924), meson (3378) and synapse (4776).
• Expected difference. The only one should be the import-free deep-nesting --skipped rows above. Please call it out in the CHANGELOG entry.

Thanks again! Ping us here if anything in the rebase is unclear, or just say the word and we'll take the rebase.

[LaimaWu]

@joyful-ii-V-I Thanks for the detailed review and integration guidance. I’ll keep the Python-specific binding helper scoped to #382, as suggested.

I’ll wait for the Go and JS/TS slices to land on main, then handle the rebase and rerun the differential validation. I’ll also add the missing negative regression cases and explicitly document the deep-nesting disclosure change.

Appreciate you checking the combined implementation.

[joyful-ii-V-I]

@LaimaWu a short status so you can plan. The Go and JS/TS import slices that #382 waits for are in the
integration PR for v0.6.7 (#385), which merged yesterday and shipped in v0.6.7. #382 can now rebase onto main using
the hints above, or we can do the rebase for you, as offered.

The shared binds channel (renames for the other languages) comes after v0.6.7. It does not have a date yet,
because what goes into the next train depends on measurements we have not run.

v0.6.7 also brings a new CONTRIBUTING section, Adding a language: common stays common.
For each feature family it says where the shared mechanism lives and lists claimable slices. If you'd like
to take one yourself, slice 1a (moving one more language onto @import.path) is close to what you did for
Python. Claim it on #358 first. No obligation, of course.

[LaimaWu]

@joyful-ii-V-I Thank you for the detailed integration guidance. I have rebased #382 onto main 0e0f67c73 after #385/v0.6.7, updated the existing branch to 38607f401, and revised the PR description for re-review.

The six-file Python slice follows the eight review points, retains the Python-specific binding helper, and removes the superseded walked extractor. The gate now checks future annotations, dynamic import calls, dotted module aliases (including the module marker), and relative aliases with real Include/RawBind assertions and fault-injection controls. No shared binding API or Go/JS/TS feature change is included.

Clean builds of the new head: 78 relevant gates, 3,084 PASS / 0 FAIL / 16 SKIP; five G1 gates, 269 PASS / 0 FAIL / 10 SKIP, with no sanitizer reports. The 117 successful raw baseline/head command pairs differ only on the documented deep fixture; Go/JS/TS comparisons and 12 non-deep main-cache reuse cases pass. The full repository suite was not run, and the old Linux results remain historical.

The approved disclosure exception is explicit in CHANGELOG and tests: imports at depths 255/256 stay, a real import cut at 257 is disclosed, and import-free deep containers no longer produce extract-partial (including a top-level import followed by deep pass). Ready for your re-review; thanks again for checking the combined implementation.

[LaimaWu]

@joyful-ii-V-I Thanks also for pointing me to the new CONTRIBUTING guide and the slice 1a opportunities.

As noted in my update above, #382 has now been rebased, validated, and pushed for re-review.

I'd be interested in taking on another small 1a slice once #382 is settled. I'll check what's still available and coordinate on #358 before starting.

Appreciate your guidance and the opportunity to keep contributing!

[joyful-ii-V-I]

@LaimaWu thank you for this. The rebase follows all eight hints closely, and the slice is what we hoped for: Python's import edges now come from @import.path, the walked extractor and both Python arms of directiveTargetOf are gone, and the binding helper stays Python-specific, exactly as we asked. The negative and alias arms you added go further than we asked. The cache-level check that pins the full Include/RawBind facts, together with its corrupt-fact controls, is a good model for the next slice.

We're approving this and merging it once CI is green; it ships in the next release. The notes below are small and none of them blocks the merge. Please don't push for them now (a push would restart CI); we'll fold them into our next integration batch, or you're welcome to send them as a small follow-up PR.

What we checked (base main 0e0f67c, head 38607f4)

• Byte identity: 39/39 argv identical, run with --no-cache on both binaries.
  • Corpora: aiohttp, conan, textual, flake8, pyimportprecisefix, pymodulealiasfix and pyshapefix.
  • Verbs: the map, --deps --limit=100000, --skipped, and --callers/--callees/--impact/--uses on imported symbols.
  • The Python <inc> counts are unchanged: aiohttp 1496, conan 4924, textual 4479.
• A probe of 12 sibling forms was also identical on both binaries:
  • relative aliases, parenthesised clauses with a comment between them, try/except rebinding the same alias
  • async, decorated, match/case and class-body imports
  • broken trailing import statements
• The only difference is the documented one. An import-free deep container is no longer extract-partial, and a real import cut below it is still disclosed.
• A cache written by main, read by your head, gives identical answers (22/22 argv) and is not rewritten.
• Gates: your pyimportprecisecheck is green on head. Run against main, only the deep-nesting arm goes red, as it should. Also green:
  • importcap, depsprecise, resolve, nestedimport, qschemetrip, importnarrow
  • externalveto, builtinbind, pymodulealias, cacheidentity, childwalkscale
  • preproccond, plus every gate that greps the files you touched
  • ASan (llvm@22) on the import gates: no reports.
• --quality-delta main..head gates 0. --clones shows no Python import group.

Small notes (all optional)

1. Stale references to the deleted capturePythonImportBinds. src/ingest_importcap.h ~L348-350 (the emitCapturedImport "SEVEN PARAMETERS" comment) still lists it as a live sibling, and src/graph.h ~L2623 says it "keeps the module only". grep -n capturePythonImportBinds src/ finds both. We'll update the CONTRIBUTING "common stays common" table ourselves (row 1, "Python is in review").
2. One gate arm can't fail. The "import.names and import.alias vocabulary is compiled in" arm also passes on main's binary, because Go's embedded tags.scm comment already contains both names:

   bash test/pyimportprecisecheck.sh /path/to/main/build/ripwire | grep vocabulary   # PASS on main
   

   Grepping for a Python-only pattern string (e.g. name: (dotted_name) @import.names) would make it live. Dropping it is also fine, since your fact-level arms already prove the behaviour.
3. The per-clause reach cost, measured. This is the note from last round; it is fine for real code, where conan's --deps time is unchanged. On a hostile shape, from m import (a0, …, a79999) under 250 nested ifs, head takes 3.9 s against 0.28 s on main. It is linear in names × depth, not quadratic. Matches arrive in byte order, so remembering the last statement and its ImportReach would remove it. Only if you feel like it.
4. CHANGELOG wording. "No persisted record … change is intended" is true of the record contents. The order of a file's bind records in the cache does change, though: the receiver-evidence NameAlias now comes before the Import binds. Nothing reads that order, and a main-written cache gives identical answers. Maybe: "no record content or extraction-version change (the order of a file's bind records differs; no reader depends on it)". There's also a double blank line before ## [0.6.7].
5. queries/python/tags.scm comment. "C++ imposes the old container reach/depth" could read as the C++ language. "The C++ side (ingest_importcap.h) imposes…" would be clearer.

The two le.lang == Lang::Python checks in captureTagsFacts are fine for this PR. They are the price of keeping the helper Python-specific, as we asked, and they go away when the shared binds channel (slice 2a) lands.

Thanks again! This was a careful, well-evidenced slice. If you'd like another one, slice 1a for one more language is open on #358.

[joyful-ii-V-I]

@coderabbitai review

[joyful-ii-V-I]

Merged to main (035b0bc), and it ships in the next release. Thank you, @LaimaWu! Python is now on the shared @import.path capture, one per-language extractor fewer. We'll fold the small notes from the review into our next integration train. The one red CI leg along the way was a timeout in one of our own gates on a slow runner, not your change; we're fixing the gate.