fix(registry): stop external package imports binding to project homonyms - #1766
fix(registry): stop external package imports binding to project homonyms#1766Yyunozor wants to merge 3 commits into
Conversation
|
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. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
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. |
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a registry-level suppression guard to prevent project-wide fallback resolution from creating fabricated CALLS edges when the caller explicitly imports a homonymous symbol from an external (non-indexed) package.
Changes:
- Introduces
cbm_suppress_external_import_shadow()to suppress fallback-resolved edges for bare identifiers that are imported from non-relative specifiers but do not bind in the import map. - Applies the guard consistently in both sequential (
pass_calls.c) and parallel (pass_parallel.c) emission paths. - Adds focused unit + pipeline tests reproducing #1355 and validating behavior for sequential vs parallel indexing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_registry.c | Adds unit tests for the new suppression predicate’s contract and edge cases. |
| tests/test_pipeline.c | Adds an end-to-end fixture + assertions to verify the regression is fixed in both sequential and parallel resolvers. |
| src/pipeline/registry.c | Implements cbm_suppress_external_import_shadow() plus helpers. |
| src/pipeline/pipeline.h | Exposes the new suppression predicate in the public pipeline header. |
| src/pipeline/pass_parallel.c | Invokes the guard in the parallel call-resolution emission path. |
| src/pipeline/pass_calls.c | Threads file_imports into resolution and invokes the guard in the sequential emission path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| char *old_workers = getenv("CBM_WORKERS"); | ||
| char *saved_workers = old_workers ? strdup(old_workers) : NULL; | ||
| char *old_single = getenv("CBM_INDEX_SINGLE_THREAD"); |
| /* Enough files that CBM_WORKERS can take the fused-parallel path; the same | ||
| * tree is then indexed by each resolver in turn, because the guard lives at | ||
| * two independent emit sites (pass_calls.c and pass_parallel.c). */ | ||
| write_external_import_shadow_fixture(tmp, 50); |
| * "/abs/x") rather than an external package. Package specifiers are everything | ||
| * else — "drizzle-orm", "rxjs/operators", "@scope/pkg". */ | ||
| static bool specifier_is_relative(const char *module_path) { | ||
| return module_path && (module_path[0] == '.' || module_path[0] == '/'); |
|
Thank you for the detailed reproduction, the sequential and parallel coverage, and for documenting the known workspace tradeoff explicitly. This changes project-wide import-resolution behavior, so we are reviewing the approach carefully and will come back with a maintainer decision before asking you to rework anything. Our review queue is currently full, so this may take a little time. Thank you for your patience and for making the tradeoff visible. |
9ce0c1c to
f78bfe2
Compare
|
Reviewed. The predicate is well built, the reproduction is convincing, and there is a standing constraint on this class of change that decides the timing rather than the merit. The predicate itselfI traced it, and the conjunction is tight in the right way: strategy limited to the four project-wide guesses, callee must be a bare identifier (no Excluding relative specifiers on purpose is the right line. A relative path names something inside the tree, so a missing And the reproduction earns its conclusion. What holds it upThis PR is one of a named cluster — #1128, #1324, #1386, #1702 and this one — and there is a standing decision that they are judged against one shared census rather than one at a time, because whichever lands first shifts the baseline the others are measured on. You already spotted the nearest neighbour yourself: #1386 guards the Python side of the same issue and lands at the same two emission sites. So this is a sequencing constraint, not a verdict. Concretely, what would clear it. A change of this shape was accepted recently on four things together, and yours currently shows two of them:
One interaction to check while you are at it: you drop Two smaller thingsYour known limitation is real and honestly stated. A bare specifier that does name in-project code but fails to materialise an Your |
A bare call whose name the calling file imports from a package outside the
indexed tree was bound to whatever project symbol shared the simple name.
An `import { eq, sql } from "drizzle-orm"` plus an unrelated local module
exporting `eq`/`sql` produced two CALLS edges from the caller into that
module, strategy unique_name — the same defect on the sequential and the
fused-parallel resolver. The TS-LSP is not involved: it declines the
external names, and the textual registry fallback fires anyway.
The package materializes no node, so resolve_import_node returns NULL, no
IMPORTS edge is written, and the per-file import map carries no key for the
name. Strategies 1-2 miss and resolve_name_lookup binds the call by simple
name. Confidence does not separate the cases: the fabricated edge lands at
0.75 whenever the file also imports anything from the target's module.
Add cbm_suppress_external_import_shadow(), a pure predicate beside the
existing perl/tsjs/cross-language guards and called at the same two emit
sites. It drops the edge only when the callee is a bare identifier, the
strategy is a project-wide guess (suffix_match / unique_name /
field_type_hint / fuzzy), the file imports that exact local name from a
non-relative specifier, and no import-map key binds it. Relative specifiers
are excluded on purpose: they name a path inside the tree, so a missing
IMPORTS edge there is an in-project resolution gap and the fallback can
still be right. A name imported both relatively and from a package keeps
its edge, independently of extraction order.
Addresses the external-import sub-case of DeusData#1355.
Signed-off-by: Yyunozor <yyunozor@icloud.com>
- specifier_is_relative() now also recognizes Windows drive-letter
(C:\ / C:/) and UNC (\\server\share) specifiers as in-tree, not
external packages. pr-smoke runs this pipeline on Windows, and the
previous POSIX-only check ('.' / '/') classified those specifiers as
external, which could suppress a real edge on that platform only.
Added external_import_shadow_windows_relative_specifier_kept
(tests/test_registry.c): red before this fix, green after.
- old_workers / old_single in the DeusData#1355 pipeline test are now
const char*, matching getenv()'s read-only contract.
- The pad_files literal (50) is now a named
EXTERNAL_IMPORT_SHADOW_PARALLEL_PAD constant with a comment tying it
to MIN_FILES_FOR_PARALLEL (a private #define in pipeline.c, not
reachable for a static_assert from tests), so a future bump to that
threshold can't silently drop the fixture back onto the
sequential-only path.
Signed-off-by: Yyunozor <yyunozor@icloud.com>
…ort guard A per-language blast-radius census on twelve public repositories found one regression class in the DeusData#1355 guard, and it is not per-language: it is per-topology. In a workspace monorepo a BARE specifier can still name code inside the indexed tree. `import { eq } from "drizzle-orm"` written inside the drizzle-orm repository is a sibling package, not a dependency. Measured on drizzle-team/drizzle-orm at b7862528, full index, both resolvers: the guard removed 2609 CALLS edges. Resolving each removed edge's specifier against the repository's own package manifests puts 1377 of them (52.8%) on the file the specifier actually names. 690 of those are strict: the specifier names a subpath, that subpath maps to one directory, and the target file is inside it. The other 687 come from the bare package root, where the check can only confirm the target is somewhere in the package, so read 690 as the defensible floor. Only 373 removed edges pointed at a third-party package, which is the defect DeusData#1355 reports. The same census on eleven other repositories (flask, scrapy, express, zustand, got, cobra, gin, chi, ripgrep, gson, jq) removed 12 CALLS edges in total: nine fabricated, one lost (a documentation example importing the repository's own published name), two weak links between a shell command and a manifest key. The regression is specific to trees that ship the package they import. Consult the pipeline package map before calling a specifier external. It is keyed by the `name` of every manifest found in the tree, so a workspace registers each of its own packages there, and a subpath specifier is walked back one slash at a time ("drizzle-orm/pg-core" -> "drizzle-orm"). A specifier the tree itself claims is now treated exactly like a relative one -- kept. The check is deliberately a NAME test rather than a resolution test. A workspace package usually points `main` at a build artifact ("./index.cjs") that is not checked in, so asking whether the entry file exists in the graph answers "no" for exactly the monorepos this has to protect. Whether the tree claims the name is decidable from the manifest alone. Passing NULL restores the previous specifier-shape-only contract, which is what the unit tests exercise. After the change the same census removes 391 edges on drizzle-orm, 375 of them CALLS, and every one resolves to a third-party package; of the other eleven repositories only got changes, recovering its self-referencing edge. Signed-off-by: Yyunozor <yyunozor@icloud.com>
f78bfe2 to
797a11a
Compare
|
Thank you for the review. I ran the census. It found a regression class in my Summary
Full census: per-repository tables, edges read by hand, the #1907 fixture, caveats3. Measured blast radius, per languageTwelve public repositories, one index with
Go, Rust, Java and C are byte-identical before and after. I also built a traced drizzle-orm is the problem. I classified all 2609 removed CALLS edges
The 1377 are not all equally strong evidence, so here is the split. In 690 the I read 14 of them by hand. The other eleven repositories removed 12 CALLS edges in total: express 3,
9 + 1 + 2 = 12. 4. Why there is no per-language gateThe census answers this better than an argument would. The blast radius is not What separates the safe repositories from drizzle-orm is not the language. It is So the correct seam is the package map, which is what you called the right The new commit consults Same census after the change:
got is the only one of the eleven that changes: the self-referencing All 375 resolve to a third-party package. I read 10: Tests: Each emission site has its own red proof. Replacing The #1907 interactionYour reasoning holds, and here is the measurement. Numbers below are from my
The absolute totals do not travel between indexing sessions. Four fresh What does hold in both sessions is the comparison, which is what the question So my drop-list naming The seam does exist. I built a fixture with Two notesThe branch was 106 commits behind, so I rebased it onto The known limitation is now narrower but still real. A bare specifier that names The "per topology, not per language" claim rests on one workspace. drizzle-orm Rebasing this changes the diff you already traced. If you would rather judge the Repository SHAs used: drizzle-orm Measurement caveats. Running the same |
|
One correction to my last note: I have since run the full suite on this branch ( Branch: 7821 passed, 1 failed, 7 skipped, 141/141 suites. Bare parent The one failure on the branch is |
Addresses the external-import sub-case of #1355: the caller's own source already says the callee is not a project symbol.
Complements #1386, which guards the Python side of the same issue with a generic-name list. This one keys on import evidence rather than on names and is not language-gated; the repro and tests here are TypeScript. Both land at the same two emission sites — happy to rebase onto #1386 whenever it lands.
Reproduction
src/queries.tsdoesimport { eq, sql } from "drizzle-orm";src/text-utils.tsexports unrelated localeq/sqlhelpers. Onmain(34d18ae):Two fabricated
CALLSedges, identical underCBM_INDEX_SINGLE_THREAD=1andCBM_WORKERS=4. The TS-LSP is not at fault:normalize, imported relatively from the very module the guess picked, resolves vialsp_ts_import@ 0.95. It declines the external names; the textual fallback fires regardless.Cause
drizzle-ormis outside the indexed tree, socbm_pipeline_resolve_import_nodereturns NULL, noIMPORTSedge is written, and the import map has no key foreq. Strategies 1-2 miss andresolve_name_lookupbinds by simple name. Confidence does not separate the cases: the fabricated edge reaches 0.75 when the file also imports from the target's module.Fix
cbm_suppress_external_import_shadow()— a pure predicate beside the existingcbm_perl_*/cbm_tsjs_*/cbm_suppress_cross_language_*guards. It drops the edge only when the callee is a bare identifier, the strategy is a project-wide guess (suffix_match/unique_name/field_type_hint/fuzzy), the file imports that name from a non-relative specifier, and no import-map key binds it.Relative specifiers are excluded on purpose: they name a path inside the tree, so a missing
IMPORTSedge there is an in-project gap and the fallback may still be right.Verification
scripts/test.sh: branch 7569 / 1 / 8, parent 7565 / 1 / 8. Delta is exactly the 4 new tests; the pre-existing failure (test_cli.c:8914) is identical on both sides.buildQuery → eqedge present, on both resolvers.workspaces,pnpm-workspace.yaml, tsconfigpaths, barrel and./x.jsimports keep their edges; Javaimport staticand Pythonfrom pkg import fare untouched.Known limitation
A bare specifier that does name in-project code but fails to materialize an
IMPORTSedge (the workspace case in #1732) now loses its same-name fallback edge instead of keeping it at reduced confidence. A natural follow-up would consult the package map first.