From 0511007bff1cf511d9558e112076e27d59378637 Mon Sep 17 00:00:00 2001 From: SteveSolvesProblems Date: Fri, 25 Sep 2026 14:03:56 -0400 Subject: [PATCH] fix: carry module path segments instead of re-deriving them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01UZYZbC55c7yC2YnrL5WBZx --- BENCHMARK.md | 52 +++++++------- EXAMPLES.md | 54 +++++++------- bench/run.sh | 18 +++-- src/extract/mod.rs | 171 +++++++++++++++++++++++++++++++++------------ src/model.rs | 33 ++++++--- 5 files changed, 214 insertions(+), 114 deletions(-) diff --git a/BENCHMARK.md b/BENCHMARK.md index 538db66..c000ea3 100644 --- a/BENCHMARK.md +++ b/BENCHMARK.md @@ -53,30 +53,30 @@ callers, not a list of lines. | repo | symbol | grep lines | grep tok | ctx tok | ratio | callers | ctx says | |---|---|---|---|---|---|---|---| -| ripgrep | `new` | 1216 | 80967 | 17594 | 4.6x | 359 | name collision | -| ripgrep | `unwrap` | 1147 | 75965 | 411 | 184.8x | 5 | name collision | -| ripgrep | `arg` | 357 | 22479 | 159 | 141.4x | 2 | **complete** | -| ripgrep | `parse_low_raw` | 565 | 38372 | 108 | 355.3x | 0 | incomplete | -| requests | `get` | 180 | 11532 | 4266 | 2.7x | 89 | name collision | -| requests | `httpbin` | 156 | 10327 | 100 | 103.3x | 0 | incomplete | -| requests | `prepare` | 59 | 3625 | 2008 | 1.8x | 36 | ambiguous | -| requests | `send` | 50 | 2891 | 1206 | 2.4x | 20 | name collision | -| got | `get` | 1308 | 76569 | 285 | 268.7x | 2 | name collision | -| got | `destroy` | 189 | 10171 | 1484 | 6.9x | 33 | incomplete | -| got | `setHeader` | 159 | 9922 | 1544 | 6.4x | 33 | **complete** | -| got | `json` | 141 | 8566 | 996 | 8.6x | 23 | ambiguous | -| cobra | `executeCommand` | 307 | 20238 | 7408 | 2.7x | 193 | **complete** | -| cobra | `Flags` | 205 | 13060 | 3444 | 3.8x | 90 | **complete** | -| cobra | `AddCommand` | 158 | 8915 | 5448 | 1.6x | 135 | **complete** | -| cobra | `Name` | 119 | 7550 | 2365 | 3.2x | 65 | **complete** | -| chi | `Get` | 260 | 16267 | 3382 | 4.8x | 92 | incomplete | -| chi | `Write` | 222 | 12324 | 385 | 32.0x | 4 | incomplete | -| chi | `HandlerFunc` | 169 | 11430 | 153 | 74.7x | 2 | name collision | -| chi | `testRequest` | 154 | 10670 | 1468 | 7.3x | 40 | ambiguous | -| preact | `render` | 58 | 3161 | 272 | 11.6x | 2 | incomplete | -| preact | `createElement` | 21 | 1296 | 142 | 9.1x | 0 | incomplete | -| preact | `createSignal` | 11 | 718 | 252 | 2.8x | 3 | ambiguous | -| preact | `unstable_runWithPriority` | 7 | 454 | 223 | 2.0x | 1 | incomplete | +| ripgrep | `new` | 1216 | 35975 | 17594 | 2.0x | 359 | name collision | +| ripgrep | `unwrap` | 1147 | 33526 | 411 | 81.6x | 5 | name collision | +| ripgrep | `arg` | 357 | 9270 | 159 | 58.3x | 2 | **complete** | +| ripgrep | `parse_low_raw` | 565 | 17467 | 108 | 161.7x | 0 | incomplete | +| requests | `get` | 180 | 4812 | 4266 | 1.1x | 89 | name collision | +| requests | `httpbin` | 156 | 4503 | 100 | 45.0x | 0 | incomplete | +| requests | `prepare` | 59 | 1423 | 2008 | 0.7x | 36 | ambiguous | +| requests | `send` | 50 | 1024 | 1206 | 0.8x | 20 | name collision | +| got | `get` | 1308 | 29917 | 285 | 105.0x | 2 | name collision | +| got | `destroy` | 189 | 3430 | 1484 | 2.3x | 33 | incomplete | +| got | `setHeader` | 159 | 4251 | 1544 | 2.8x | 33 | **complete** | +| got | `json` | 141 | 3537 | 996 | 3.6x | 23 | ambiguous | +| cobra | `executeCommand` | 307 | 9084 | 7408 | 1.2x | 193 | **complete** | +| cobra | `Flags` | 205 | 5612 | 3444 | 1.6x | 90 | **complete** | +| cobra | `AddCommand` | 158 | 3174 | 5448 | 0.6x | 135 | **complete** | +| cobra | `Name` | 119 | 3227 | 2365 | 1.4x | 65 | **complete** | +| chi | `Get` | 260 | 6994 | 3382 | 2.1x | 92 | incomplete | +| chi | `Write` | 222 | 4406 | 385 | 11.4x | 4 | incomplete | +| chi | `HandlerFunc` | 169 | 5402 | 153 | 35.3x | 2 | name collision | +| chi | `testRequest` | 154 | 5177 | 1468 | 3.5x | 40 | ambiguous | +| preact | `render` | 58 | 1034 | 272 | 3.8x | 2 | incomplete | +| preact | `createElement` | 21 | 526 | 142 | 3.7x | 0 | incomplete | +| preact | `createSignal` | 11 | 315 | 252 | 1.2x | 3 | ambiguous | +| preact | `unstable_runWithPriority` | 7 | 197 | 223 | 0.9x | 1 | incomplete | **Read the last column before the ratio column.** A large ratio on an `incomplete` row is not a win — it is ctx answering cheaply *because it could not @@ -122,7 +122,7 @@ can hide a large fraction of the third quantity. That is a limitation this harness found on a third-party repository, which is the argument for running it on third-party repositories. -Over the 6 `complete` answers above, the median is **3.5x** fewer tokens than the fair grep. +Over the 6 `complete` answers above, the median is **1.5x** fewer tokens than the fair grep. The median, not the best case or the range. Individual ratios here span two orders of magnitude, and the largest of them are the least trustworthy — a ratio is only as good @@ -132,4 +132,4 @@ where ctx saw least. Read the table, not this line. --- Generated by `./bench/run.sh` from `bench/repos.tsv` (6 repositories). -ctx version: `ctx 0.23.0`. +ctx version: `ctx 0.24.0`. diff --git a/EXAMPLES.md b/EXAMPLES.md index 359f353..81cfc90 100644 --- a/EXAMPLES.md +++ b/EXAMPLES.md @@ -11,7 +11,7 @@ tree they were run against, so a version string dates them without making them current — this file had drifted a whole minor version before anyone noticed. Regenerate it in the release commit instead. -The repo under analysis: **15 Rust files, 4 Markdown files, 19 modules, 6,293 +The repo under analysis: **15 Rust files, 4 Markdown files, 19 modules, 6,338 call sites.** Small enough to read in a sitting, which makes it a fair place to check whether the answers are actually right. @@ -82,19 +82,19 @@ $ ctx trace build_graph --depth 2 ~ heuristic edge (verify) · * one branch of a dispatch fan-out extract::build_graph [src/extract/mod.rs:226] [+1 outside graph] -├─ extract::disambiguate_module_names [src/extract/mod.rs:373] +├─ extract::disambiguate_module_names [src/extract/mod.rs:372] ├─ extract::go::extract [src/extract/go.rs:9] [+1 outside graph] │ ├─ extract::go::function [src/extract/go.rs:669] (depth limit) │ ├─ extract::go::imports [src/extract/go.rs:238] (depth limit) │ ├─ extract::go::interfaces_in [src/extract/go.rs:168] (depth limit) -│ ├─ model::content_hash [src/model.rs:178] +│ ├─ model::content_hash [src/model.rs:185] │ ├─ extract::go::package_level_item [src/extract/go.rs:197] (depth limit) │ ├─ extract::go::receiver_of_method [src/extract/go.rs:499] (depth limit) │ ├─ extract::go::returns_in [src/extract/go.rs:115] (depth limit) │ ├─ extract::go::types [src/extract/go.rs:320] (depth limit) │ └─ extract::go::values [src/extract/go.rs:468] (depth limit) -├─ extract::go_modules [src/extract/mod.rs:927] -│ ├─ extract::go_segs [src/extract/mod.rs:915] +├─ extract::go_modules [src/extract/mod.rs:940] +│ ├─ extract::go_segs [src/extract/mod.rs:928] │ └─ extract::walker [src/extract/mod.rs:109] (depth limit) ├─ extract::markdown::extract [src/extract/markdown.rs:45] [+1 outside graph] │ ├─ extract::markdown::assign_lines [src/extract/markdown.rs:139] (depth limit) @@ -111,27 +111,25 @@ extract::build_graph [src/extract/mod.rs:226] [+1 outside graph] │ ├─ extract::markdown::setext_heading [src/extract/markdown.rs:296] │ ├─ extract::markdown::slug [src/extract/markdown.rs:13] │ └─ extract::markdown::strip_inline [src/extract/markdown.rs:460] -├─ model::Lang::sep [src/model.rs:165] -├─ extract::module_name [src/extract/mod.rs:608] -│ ├─ model::Lang::sep [src/model.rs:165] -│ └─ extract::safe_seg [src/extract/mod.rs:604] (depth limit) -├─ extract::nearest_go_module [src/extract/mod.rs:972] +├─ extract::module_name [src/extract/mod.rs:604] +│ └─ model::Lang::sep [src/model.rs:172] +├─ extract::nearest_go_module [src/extract/mod.rs:985] ├─ extract::needs_jsx [src/extract/mod.rs:222] ├─ extract::python::extract [src/extract/python.rs:9] [+1 outside graph] │ ├─ extract::python::module_level_item [src/extract/python.rs:35] (depth limit) │ └─ extract::python::visit [src/extract/python.rs:73] (depth limit) -├─ extract::resolve_deps [src/extract/mod.rs:683] -│ ├─ extract::apply_calls [src/extract/mod.rs:2473] (depth limit) -│ ├─ extract::build_universe [src/extract/mod.rs:1493] (depth limit) -│ ├─ extract::compute_calls [src/extract/mod.rs:2310] (depth limit) -│ ├─ extract::display_reexport [src/extract/mod.rs:2480] (depth limit) -│ ├─ extract::go_mod_index [src/extract/mod.rs:989] -│ ├─ extract::go_packages [src/extract/mod.rs:1033] (depth limit) -│ ├─ model::Module::resolve_segs~ [src/model.rs:319] (depth limit) -│ └─ extract::resolve_from [src/extract/mod.rs:1163] (depth limit) +├─ extract::resolve_deps [src/extract/mod.rs:679] +│ ├─ extract::apply_calls [src/extract/mod.rs:2486] (depth limit) +│ ├─ extract::build_universe [src/extract/mod.rs:1506] (depth limit) +│ ├─ extract::compute_calls [src/extract/mod.rs:2323] (depth limit) +│ ├─ extract::display_reexport [src/extract/mod.rs:2493] (depth limit) +│ ├─ extract::go_mod_index [src/extract/mod.rs:1002] +│ ├─ extract::go_packages [src/extract/mod.rs:1046] (depth limit) +│ ├─ model::Module::resolve_segs~ [src/model.rs:326] (depth limit) +│ └─ extract::resolve_from [src/extract/mod.rs:1176] (depth limit) ├─ extract::rust::extract [src/extract/rust.rs:9] [+1 outside graph] │ └─ extract::rust::visit [src/extract/rust.rs:25] (depth limit) -├─ extract::slash_path [src/extract/mod.rs:578] +├─ extract::slash_path [src/extract/mod.rs:574] ├─ extract::source_files [src/extract/mod.rs:142] │ ├─ extract::lang_selected [src/extract/mod.rs:131] (depth limit) │ └─ extract::walker [src/extract/mod.rs:109] (depth limit) @@ -270,9 +268,9 @@ under this root. Calls into std or a third-party crate are excluded, because no internal edge could exist for them however good the resolver gets. ## Every call site, bucketed -call sites: 6293 +call sites: 6338 internal edges: 1355 [17 heuristic (~), 0 dispatch fan-out (*)] - external (provable): 4871 (77.4%) callee defined nowhere here — std/extern + external (provable): 4916 (77.6%) callee defined nowhere here — std/extern unresolved internal: 67 (1.1%) the real misses — see below ## What ctx missed (callee names that exist here but went unpinned) @@ -313,7 +311,7 @@ grep these; every other edge in the map is one ctx could prove. 95.3% recall comes with **the exact grep list for the other 4.7%** — nine names, with counts and the modules they live in. -The denominator is honest too. 4,871 of 6,293 call sites go into `std` or a +The denominator is honest too. 4,916 of 6,338 call sites go into `std` or a third-party crate, where no internal edge could ever exist, so they're excluded rather than quietly inflating the percentage. That classification is by evidence — *is this name defined anywhere under the root?* — not a hardcoded list. @@ -338,7 +336,7 @@ Ranked by dependency centrality (PageRank); higher = more depended-upon. score in out module 0.2954 13 0 model [19 items] - 0.1525 11 1 extract [100 items] + 0.1525 11 1 extract [103 items] 0.0601 4 2 render [13 items] 0.0468 3 3 view [11 items] 0.0448 2 4 query [141 items] @@ -365,7 +363,7 @@ Jump-to-def without knowing the file, across languages. $ ctx def Universe 1 definition(s) of 'Universe': -extract::Universe [struct] src/extract/mod.rs:1474-1491 #85eadbaee90b +extract::Universe [struct] src/extract/mod.rs:1487-1504 #85eadbaee90b struct Universe { methods: MethodIndex, all_names: BTreeSet, module_segs: BTreeSet, implementors: HashMap>, fields: HashMap> } — Whole-tree symbol evidence, built once and shared by every module's ``` @@ -415,7 +413,7 @@ $ ctx context Receiver --include-source # Context: model::Receiver ## Definition -model::Receiver [enum] src/model.rs:266-295 #eaedf052414f +model::Receiver [enum] src/model.rs:273-302 #eaedf052414f pub enum Receiver { Free | SelfType | SelfField | Typed | Dyn | Returned | Unknown } — How a callee was referenced — governs how confidently a receiver method ```rust @@ -452,7 +450,7 @@ pub enum Receiver { ``` ## Referenced by — dependents (14 signature(s)) -- extract (src/extract/mod.rs:1916) +- extract (src/extract/mod.rs:1929) fn field_receiver(ty: &str, uni: &Universe) -> Receiver - extract::go (src/extract/go.rs:520) struct TypeEnv { vars: HashMap } @@ -478,7 +476,7 @@ pub enum Receiver { fn let_binding(n: Node, src: &str, env: &TypeEnv) -> Option<(String, Receiver)> - extract::rust (src/extract/rust.rs:567) fn receiver_of(v: Node, src: &str, env: &TypeEnv) -> Receiver -- model (src/model.rs:301) +- model (src/model.rs:308) pub struct RawCall { pub path: String, pub recv: Receiver, pub line: usize, pub end_line: usize } Signature references only: uses inside function BODIES are not indexed. diff --git a/bench/run.sh b/bench/run.sh index 5876ef7..6102e7d 100755 --- a/bench/run.sh +++ b/bench/run.sh @@ -45,13 +45,21 @@ tokens_of () { local n; n=$(wc -c); echo $(( n / 3 )); } # The grep a competent agent would actually write: call syntax, language # filtered, skipping the directories ctx skips. Not a naive `grep -rn name`, # because beating a strawman proves nothing. +# Searched from INSIDE the tree, against `.`, so hits read `./src/x.rs:12:` and +# not `/wherever/the/cache/lives/src/x.rs:12:`. The cost of an answer is counted +# in bytes, so an absolute path would bill grep for the length of a directory +# name that has nothing to do with either tool — and would make the figure +# depend on where the corpus happened to be cloned. Measured on ripgrep, the +# prefix alone was 9,322 tokens, 21% of that row. It also biased the comparison +# in ctx's favour, since ctx already reports paths relative to its root. fair_grep () { local dir="$1" sym="$2" - grep -rnE "\\b${sym}\\(" "$dir" \ - --include='*.rs' --include='*.py' --include='*.ts' --include='*.tsx' --include='*.go' \ - --exclude-dir=.git --exclude-dir=node_modules --exclude-dir=target \ - --exclude-dir=__pycache__ --exclude-dir=.venv --exclude-dir=venv \ - --exclude-dir=dist --exclude-dir=build 2>/dev/null || true + ( cd "$dir" 2>/dev/null || return 0 + grep -rnE "\\b${sym}\\(" . \ + --include='*.rs' --include='*.py' --include='*.ts' --include='*.tsx' --include='*.go' \ + --exclude-dir=.git --exclude-dir=node_modules --exclude-dir=target \ + --exclude-dir=__pycache__ --exclude-dir=.venv --exclude-dir=venv \ + --exclude-dir=dist --exclude-dir=build 2>/dev/null || true ) } # Fetch one pinned commit, shallow. Cached, because re-cloning on every run diff --git a/src/extract/mod.rs b/src/extract/mod.rs index 54a0b5a..8a8bb3e 100644 --- a/src/extract/mod.rs +++ b/src/extract/mod.rs @@ -289,12 +289,11 @@ pub fn build_graph(root: &Path) -> Result { b.public = false; } } - let (name, mut crate_prefix) = module_name(rel, lang); + let (name, mut crate_prefix, mut path_segs) = module_name(rel, lang); // A Go package IS its directory: every .go file in it is a peer, with no // per-file visibility and no per-file import path. Files stay separate // modules (so an item's location is its own file), but resolution keys // on the package, which is what an import names. - let mut resolve_name = String::new(); let mut go_module = Vec::new(); if lang == Lang::Go { let dir: Vec = rel @@ -311,10 +310,10 @@ pub fn build_graph(root: &Path) -> Result { // a name of its own: leaving it empty made every root-level file its // own package, so a call into a sibling file read as a miss — and // Go repos routinely put their main package at the root. - resolve_name = if dir.is_empty() { - "root".to_string() + path_segs = if dir.is_empty() { + vec!["root".to_string()] } else { - dir.join(Lang::sep(lang)) + dir.clone() }; if let Some((mod_dir, mod_path)) = nearest_go_module(&go_mods, &dir) { crate_prefix = mod_dir; @@ -323,7 +322,7 @@ pub fn build_graph(root: &Path) -> Result { } modules.push(Module { name, - resolve_name, + path_segs, heuristic_deps: Vec::new(), import_sites: Vec::new(), file: slash_path(rel), @@ -398,12 +397,9 @@ fn disambiguate_module_names(modules: &mut [Module]) { } seen.insert(candidate.clone(), 1); renames.push((name.clone(), candidate.clone(), modules[i].file.clone())); - // Display name changes; resolution identity must not. A Go module - // already carries its package as `resolve_name`, and overwriting that - // with the colliding file name would break every import into it. - if modules[i].resolve_name.is_empty() { - modules[i].resolve_name = name; - } + // Display name only. Resolution keys on `path_segs`, which is derived + // from the file's location and is deliberately not touched here — that + // separation is why a rename can no longer cost an edge. modules[i].name = candidate; } for (from, to, file) in &renames { @@ -585,27 +581,27 @@ fn slash_path(rel: &Path) -> String { /// Derive a stable module name from the file path. "src" components are /// dropped; components before the first "src" become the crate prefix /// (workspace layouts like crates/foo/src/bar.rs -> crates::foo::bar). -/// One path component, with any occurrence of the language's module separator -/// removed from it. -/// -/// A module name is segments joined by the separator, and resolution splits it -/// back apart to do path arithmetic. So a component that *contains* the -/// separator silently adds a segment that was never in the path — and for the -/// dot-separated languages that is every `foo.test.ts`, `foo.spec.js` and -/// `foo.stories.tsx` in existence. +/// The specifier `./app.js` names the module built from `app.ts`, so a real +/// source extension comes off before matching. Deliberately an allowlist: a +/// trailing-dot rule would eat the `.config` in `./app.config`, which is part +/// of the module's name and not an extension at all. Native ESM and browser +/// ESM both *require* the written extension, so this is the spec-correct form +/// rather than an edge case. +fn strip_source_ext(c: &str) -> &str { + match c.rsplit_once('.') { + Some((stem, "ts" | "tsx" | "js" | "jsx" | "mjs" | "cjs")) if !stem.is_empty() => stem, + _ => c, + } +} + +/// `(display name, crate prefix, path segments)`. /// -/// The consequence was not a cosmetic one. `src/browser/frag.test.ts` named -/// itself `browser.frag.test`, which splits into four segments rather than -/// three, so every relative import in the file resolved one directory too deep -/// and silently produced nothing: no dep, no call edges, and a module recall of -/// zero. The identical import in `plain.ts` next door resolved fine. Test files -/// are exactly where a blast radius matters most, and they were the files where -/// it was empty. -fn safe_seg(c: &str, lang: Lang) -> String { - c.replace(Lang::sep(lang), "-") -} - -fn module_name(rel: &Path, lang: Lang) -> (String, Vec) { +/// The segments are returned rather than recovered later by splitting the +/// name. A component may legitimately contain the separator — `app.config.ts`, +/// `types.gen.ts` — and once joined there is no way to tell that dot from a +/// structural one. Carrying the Vec is what lets the name keep matching the +/// filename, which is the form a human and an agent can both guess. +fn module_name(rel: &Path, lang: Lang) -> (String, Vec, Vec) { let comps: Vec<&str> = rel.iter().filter_map(|c| c.to_str()).collect(); let mut segs: Vec = Vec::new(); let mut crate_prefix: Vec = Vec::new(); @@ -623,7 +619,7 @@ fn module_name(rel: &Path, lang: Lang) -> (String, Vec) { } continue; } - segs.push(safe_seg(c, lang)); + segs.push(c.to_string()); continue; } let stem = c.rsplit_once('.').map(|(s, _)| s).unwrap_or(c); @@ -632,14 +628,14 @@ fn module_name(rel: &Path, lang: Lang) -> (String, Vec) { Lang::Python => stem == "__init__", Lang::TypeScript => stem == "index", // No filename collapses to its directory in Go: every file in a - // package is a peer, and the package itself is `resolve_name`. + // package is a peer, and the package itself is `path_segs`. Lang::Go => false, Lang::Markdown => { stem.eq_ignore_ascii_case("readme") || stem.eq_ignore_ascii_case("index") } }; if !drop { - segs.push(safe_seg(stem, lang)); + segs.push(stem.to_string()); } } @@ -651,7 +647,7 @@ fn module_name(rel: &Path, lang: Lang) -> (String, Vec) { } else { segs.join(lang.sep()) }; - (name, crate_prefix) + (name, crate_prefix, segs) } struct Ctx<'a> { @@ -813,11 +809,28 @@ fn candidates(imp: &str, m: &Module, ctx: &Ctx) -> (Vec>, bool) { } i += 1; } + // One component per path segment, exactly as `module_name` + // builds them. Splitting on `.` here is what broke + // `./app.config`: the module is one segment and the specifier + // became two, so they could never match. + // + // A source extension comes off any component, because the + // binding is `specifier/symbol` — `./app.js/boot` — so the + // filename is not reliably last. The allowlist is what makes + // that safe: `./app.config/boot` keeps its `.config`, since + // `config` is not a source extension. A trailing-dot rule + // would eat it. let rest: Vec = parts[i..] .iter() - .flat_map(|p| p.split('.')) - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) + .map(|p| p.trim()) + .filter(|p| !p.is_empty()) + .map(|p| { + if m.lang == Lang::TypeScript { + strip_source_ext(p).to_string() + } else { + p.to_string() + } + }) .collect(); let cur = m.resolve_segs(); let dir_len = if m.is_package { @@ -2704,10 +2717,12 @@ mod tests { .deps .clone() }; - // The separator is stripped from the segment, so the name stays one - // segment and the arithmetic matches the plain file's exactly. - assert_eq!(deps_of("browser.frag-test"), deps_of("browser.plain")); - assert!(deps_of("browser.frag-test").contains(&"util.dom".to_string())); + // The name still contains the dot, because it is the filename: segments + // are carried rather than recovered by splitting, so a dot in a + // component is no longer ambiguous and the arithmetic matches the plain + // file's exactly. + assert_eq!(deps_of("browser.frag.test"), deps_of("browser.plain")); + assert!(deps_of("browser.frag.test").contains(&"util.dom".to_string())); let _ = fs::remove_dir_all(&dir); } @@ -2732,7 +2747,7 @@ mod tests { .unwrap(); let g = build_graph(&dir).unwrap(); - let m = g.modules.iter().find(|m| m.name == "app-test").unwrap(); + let m = g.modules.iter().find(|m| m.name == "app.test").unwrap(); assert_eq!( m.diag.unresolved, 0, "supertest's request is not ours: {:?}", @@ -2743,6 +2758,74 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + /// A module name is segments joined by the separator, so recovering them by + /// splitting the name made any component containing a dot ambiguous. These + /// are the four specifier shapes that has to get right at once; three of + /// them were broken, in two different directions, by two different fixes. + #[test] + fn specifier_and_module_name_agree_on_dots_and_extensions() { + let dir = std::env::temp_dir().join(format!("ctx_spec_{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + for f in ["app.ts", "app.config.ts", "app.test.ts"] { + fs::write(dir.join(f), "export function boot() { return 1; }\n").unwrap(); + } + let case = |spec: &str| { + fs::write( + dir.join("c.ts"), + format!( + "import {{ boot }} from '{spec}';\nexport function b() {{ return boot(); }}\n" + ), + ) + .unwrap(); + let g = build_graph(&dir).unwrap(); + g.modules + .iter() + .find(|m| m.name == "c") + .unwrap() + .deps + .clone() + }; + // Extensionless: the form a bundler accepts, and the only one that ever + // worked. + assert_eq!(case("./app"), vec!["app".to_string()]); + // Explicit extension: mandatory in native Node and browser ESM, and the + // form TypeScript tells you to write when emitting ESM. + assert_eq!(case("./app.js"), vec!["app".to_string()]); + // A dot that is part of the NAME, not an extension. `.config` must + // survive, which is why extension stripping is an allowlist. + assert_eq!(case("./app.config"), vec!["app.config".to_string()]); + // Both at once. + assert_eq!(case("./app.test.js"), vec!["app.test".to_string()]); + let _ = fs::remove_dir_all(&dir); + } + + /// `./app.config` and `./app.js` name different files, and must not collapse + /// onto one module when both exist. + #[test] + fn a_dotted_name_and_an_extension_are_not_confused() { + let dir = std::env::temp_dir().join(format!("ctx_spec2_{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("app.ts"), "export function a() { return 1; }\n").unwrap(); + fs::write( + dir.join("app.config.ts"), + "export function c() { return 2; }\n", + ) + .unwrap(); + let u = concat!( + "import { a } from './app.js';\n", + "import { c } from './app.config';\n", + "export function b() { return a() + c(); }\n" + ); + fs::write(dir.join("u.ts"), u).unwrap(); + let g = build_graph(&dir).unwrap(); + let deps = &g.modules.iter().find(|m| m.name == "u").unwrap().deps; + assert!(deps.contains(&"app".to_string()), "got {deps:?}"); + assert!(deps.contains(&"app.config".to_string()), "got {deps:?}"); + let _ = fs::remove_dir_all(&dir); + } + #[test] fn the_fingerprint_moves_when_the_tree_does() { let dir = std::env::temp_dir().join(format!("ctx_fp_{}", std::process::id())); diff --git a/src/model.rs b/src/model.rs index 4b662dd..8c982b0 100644 --- a/src/model.rs +++ b/src/model.rs @@ -12,12 +12,19 @@ pub struct Graph { #[derive(Serialize, Clone)] pub struct Module { pub name: String, - /// The name resolution keys on, when it differs from the display `name`. - /// Set either because `name` was renamed to break a collision, or — - /// for Go — because the unit of import is the package directory rather - /// than the file. Empty means `name` is also the resolution name. + /// The path segments resolution keys on, carried rather than re-derived. + /// + /// A module name is these segments joined by the language separator, and + /// resolution needs them back. Splitting the name to recover them is what + /// made any component containing the separator ambiguous: `app.config.ts` + /// reads as two segments, so `./app.config` resolved one level too deep. + /// Keeping the Vec removes the ambiguity instead of encoding around it, + /// which is why names may contain a dot again and still match their file. + /// + /// For Go this is the package *directory*, not the file: a package is the + /// unit of import, and every `.go` file in it is a peer. #[serde(skip)] - pub resolve_name: String, + pub path_segs: Vec, pub file: String, pub lang: Lang, /// Internal modules this module imports from, with re-export facades @@ -317,12 +324,16 @@ impl Module { /// its directory — so resolving against a renamed name silently loses every /// edge. Lookups therefore key on this, while edges and labels use `name`. pub fn resolve_segs(&self) -> Vec { - let base = if self.resolve_name.is_empty() { - &self.name - } else { - &self.resolve_name - }; - base.split(self.lang.sep()).map(|s| s.to_string()).collect() + if !self.path_segs.is_empty() { + return self.path_segs.clone(); + } + // Only reachable for a module deserialized from JSON, where `path_segs` + // is skipped. Splitting the name is the old behaviour and the old bug, + // but it is strictly better than resolving against nothing. + self.name + .split(self.lang.sep()) + .map(|s| s.to_string()) + .collect() } pub fn item_count(&self) -> usize {