Repository navigation
fix: carry module path segments instead of re-deriving them - #40
Merged
Merged
Conversation
A module name is segments joined by the language separator, and resolution needs them back. Recovering them by splitting the name is what made any component containing the separator ambiguous, and it produced two bugs pointing in opposite directions: - `./app.js` never resolved. The written extension is mandatory in native Node ESM and browser ESM, and it is what TypeScript tells you to emit, so this was every spec-correct ESM project. Pre-existing. - `./app.config` stopped resolving in the unreleased 0.24.0. `safe_seg` renamed the module to `app-config` while the specifier still split to ["app","config"], so the two transforms no longer agreed. `safe_seg` is deleted rather than patched. Mangling the name encoded around the ambiguity and charged the reader for it: `app.config.ts` became module `app-config`, a translation rule a human and an agent both have to learn and neither can guess, with nothing in the output to explain it. `module_name` already built the segments and threw them away one line later, so they are now carried on `Module::path_segs` and returned by `resolve_segs` directly. Names match filenames again, and the whole class of separator-in-a-component bugs stops being expressible rather than being fixed one instance at a time. `resolve_name` is gone, subsumed. It meant two things — Go's package directory, and "the name before collision renaming" — and both were always really "the segments to resolve against". Disambiguation now touches the display name only, because resolution no longer reads it. The specifier side strips a source extension from any component, since the binding is `specifier/symbol` and the filename is not reliably last. It is an allowlist (ts, tsx, js, jsx, mjs, cjs) and not "text after the last dot", which is exactly what lets `./app.config` keep its `.config` while `./app.js` loses its `.js`. Recall is unchanged on all six benchmark repos, so the gains this class of fix bought earlier are intact: preact 78.1%, got 78.9%, ripgrep 65.4%, requests 83.6%, cobra 99.8%, chi 89.6%. BENCH: the grep figures were not reproducible. `fair_grep` searched an absolute path, and cost is counted in bytes, so every hit carried the clone directory's name and the result depended on where the corpus happened to sit. On ripgrep the prefix alone was 9,322 tokens, 21% of that row. It also biased the comparison toward ctx, which already reports paths relative to its root. Searching from inside the tree makes the output byte-identical across cache paths — verified by running the harness twice from two different ones — and honestly shrinks ctx's headline advantage: ripgrep `new` 4.6x -> 2.0x, requests `get` 2.7x -> 1.4x. BENCHMARK.md regenerated; recall figures did not move, only the cost ratios. EXAMPLES.md regenerated against a clean build; all eight non-pinned blocks diffed byte-for-byte. Module names in it are unchanged, since no file here has a dot in its name — which is also why this repo's own figures could not have caught either bug. Fixes #38. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01UZYZbC55c7yC2YnrL5WBZx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #38. Blocks the 0.24.0 release — I cancelled the tag for this.
A module name is segments joined by the language separator, and resolution needs them back. Recovering them by splitting the name made any component containing the separator ambiguous, which produced two bugs pointing in opposite directions:
./app.js./app.configsafe_segrenamed the module toapp-config, but the specifier still split to["app","config"]The second is a regression in the unreleased 0.24.0, caught before it shipped — diagnosed by the session that wrote the original dotted-filename fix.
safe_segis deleted, not patchedPatching it would have kept the mangled names.
app.config.tsbecoming moduleapp-configis a translation rule a human and an agent each have to learn and neither can guess —ctx def app.configwouldn't match the file in front of them, and nothing in the output explains why.module_namealready built the segments and joined them away one line later. They're now carried onModule::path_segsand returned byresolve_segsdirectly. Names match filenames again, and the whole class of separator-in-a-component bugs stops being expressible rather than being fixed one instance at a time.resolve_nameis gone, subsumed. It meant two things — Go's package directory, and "the name before collision renaming" — and both were always really "the segments to resolve against". Disambiguation now touches the display name only, because resolution no longer reads it.The specifier side strips a source extension from any component, because the binding is
specifier/symbol(./app.js/boot) so the filename isn't reliably last. It's an allowlist —ts, tsx, js, jsx, mjs, cjs— not "text after the last dot". That distinction is the whole fix: it's what lets./app.configkeep.configwhile./app.jsloses.js.Four new regression tests cover exactly that matrix, plus one asserting
./app.configand./app.jsresolve to different modules when both files exist.Recall is unchanged on every benchmark repo
The gains this class of fix bought earlier are fully intact:
Also: the benchmark's grep figures were not reproducible
fair_grepsearched an absolute path, and cost is counted in bytes — so every hit carried the clone directory's name, and the numbers depended on where the corpus happened to sit. On ripgrep the path prefix alone was 9,322 tokens, 21% of that row.It also biased the comparison toward ctx, which already reports paths relative to its root. Searching from inside the tree fixes both. Verified by running the harness twice from two different cache paths — now byte-identical.
This shrinks ctx's headline advantage, which is the honest direction:
newunwrapgetBENCHMARK.md regenerated. Recall figures did not move; only the cost ratios.
Release-note consequence
Drop the "module names change for files with a dot" line. It's no longer true — naming reverts to what 0.23.0 already produced, so anyone upgrading 0.23.0 → 0.24.0 sees no naming change at all. That's why this belongs before the tag rather than in a 0.24.1: otherwise names would change twice across two releases for no user-visible reason.
Verified
cargo fmt --all --check— cleancargo clippy --all-targets --lockedunderRUSTFLAGS=-D warnings— cleancargo test --all-targets --locked— 172 passed, 0 failed (4 new)cargo +1.90 check --all-targets --locked— cleancargo audit— 0 vulnerabilitiesModule names in EXAMPLES.md are unchanged, since no file in this repo has a dot in its name — which is also why this repo's own figures could never have caught either bug.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UZYZbC55c7yC2YnrL5WBZx