From e40cb2f0e7bc3e56e08608822c5eb8b3e7d809c2 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 2 Sep 2026 21:10:02 +0000 Subject: [PATCH] fix(tracers): make an id unique, by three mechanisms instead of one The v1.2.0 manual pass found duplicated ids on ordinary third-party code -- 158 in the CPython standard library, 107 in PrimeVue, 106 in `/usr/include`, 4 in the TypeScript compiler -- and recorded them unresolved. This resolves them. An id is what a call graph refers to a function by, and `/code-flow.quality` computes `unreached` by subtracting reached ids from catalogued ones. Two functions sharing an id means reaching either marks both reached, so a genuinely unreachable function is silently absent from the findings: the exact failure mode `detectorsSkipped` and the "ran and found nothing" line exist to prevent. `derive_id` is untouched, so an ordinary function's id is the same string it has always been and a hand-written map still agrees with a traced one. What changes is `assign_ids`, which counted names where the id rule folds names: - It now counts **derived ids**. `__add__` and `add`, a Java `Builder` and its `builder()` factory, `~Widget` and `Widget`, `_M_x` and `_M_X` are all two names and one id, and all of them went unsuffixed and shared it. - Two definitions on one line get `_`, their position among that line's same-id definitions. `_l` cannot separate them and no record carries a column. 105 of PrimeVue's 107 were this. - Two files that derive one id get `_f`, each file's position among those paths sorted -- `service.cpp` beside `service.hpp`, since the rule drops the extension, or `distutils/_msvccompiler.py` beside `distutils/msvccompiler.py`, since it collapses underscore runs. The one part of the rule that looks outside a single file, so it is applied only to the ids two files both derived, never to the rest of either file. Measured over the same corpora, re-deriving the old ids from the same trace output so the two columns are two rules over one catalog: 158 -> 0, 107 -> 0, 106 -> 0, 4 -> 0. Between 2.6% and 3.8% of ids change, all of them colliding ones. `scripts/build-map.py` over PrimeVue, the run that first tripped the uniqueness assertion, now completes. The C fixture was rearranged in 1768daf to dodge the `service.cpp` / `service.hpp` collision rather than cover it; that is reverted, so the suite proves the fix on the shape that exposed it. The TypeScript tracer assigned ids per file inside the collection walk, which cannot see a cross-file collision at all -- and silently overwrote `byId` when one happened. Ids and the by-id index now come after the walk. The id rule is stated verbatim in four host templates and in the tracers' README; all five say all of this. 608 pytest, 76 node. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GfQ1KbfqUeC1JFjC2F6g8v --- CHANGELOG.md | 18 ++++ RELEASE-CHECK-2026-08-28.md | 49 +++++++++- templates/claude/code-flow.map.md | 5 +- templates/copilot/code-flow.map.prompt.md | 3 +- templates/gemini/code-flow.map.toml | 5 +- templates/shared/code-flow-map/SKILL.md | 5 +- templates/shared/tracers/README.md | 18 +++- templates/shared/tracers/_common.py | 88 ++++++++++++++---- templates/shared/tracers/trace_python.py | 12 +-- templates/shared/tracers/trace_typescript.mjs | 91 +++++++++++++++++-- test/tracer-typescript.test.js | 56 +++++++++++- tests/fixtures/c-app/src/describable.hpp | 14 --- tests/fixtures/c-app/src/service.hpp | 9 +- tests/test_tracer_lexer.py | 72 +++++++++++++++ tests/test_tracers.py | 29 +++--- 15 files changed, 397 insertions(+), 77 deletions(-) delete mode 100644 tests/fixtures/c-app/src/describable.hpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 8231e39..4791512 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -214,6 +214,24 @@ from this repository at the same version. ### Fixed +- **Two functions can no longer share one `id`.** The id rule folds a name into a slug + and the collision suffix counted *names*, so two different names that slug to one + string were both left unsuffixed and sharing it: `__add__` and `add`, a Java `Builder` + constructor and its `builder()` factory, `~Widget` and `Widget`, `_M_x` and `_M_X`. + Two definitions on one line had nothing to separate them either, and two file paths + can fold to one stem — `service.cpp` beside `service.hpp`, or + `distutils/_msvccompiler.py` beside `distutils/msvccompiler.py`. Measured over + third-party code, that was **158 duplicated ids in the CPython standard library, 107 + in PrimeVue, 106 in `/usr/include`, 4 in the TypeScript compiler** — ordinary code, + not a minifier curiosity. The damage is silent: `/code-flow.quality` computes + `unreached` by subtracting reached ids from catalogued ones, so reaching either + function marked both reached and a genuinely unreachable function simply never + appeared in the findings. `assign_ids` now counts derived ids rather than names, adds + a position suffix when a line holds two of them, and adds a `_f` suffix for the + ids two files both derived. All four counts above are now zero, and building + a whole map from tracer output over PrimeVue — the run that first tripped the + assertion — completes. The id rule in all four host templates says all of this, so a + map written by hand and a map written by a tracer still agree. - **An empty `git ls-files` no longer means "this repository has no code".** Both file listers returned null for the cases their docstrings named — not a git checkout, git not installed — and the caller walked the tree for those. Neither treated git diff --git a/RELEASE-CHECK-2026-08-28.md b/RELEASE-CHECK-2026-08-28.md index 9377fc5..5d14abe 100644 --- a/RELEASE-CHECK-2026-08-28.md +++ b/RELEASE-CHECK-2026-08-28.md @@ -126,7 +126,8 @@ The two zeroes were checked rather than accepted: The last part of step 6 — build a whole map from tracer output rather than by re-reading — was run with `scripts/build-map.py` against PrimeVue. **It did not -finish; see below.** +finish; see below.** It was re-run after the fix and completes: 5,817 functions, +no duplicate ids. ## What this pass found @@ -176,9 +177,49 @@ either one reports both as reached — so a genuinely unreachable function is silently absent from the findings, which is the exact failure mode `detectorsSkipped` and the "ran and found nothing" line exist to prevent. -**This is unresolved as of this pass.** It predates this release — the rule has -been this way since the tracers landed — and fixing class 3 means changing a -rule stated verbatim in four host templates. +**Fixed after this pass**, in the commit that follows it. `derive_id` is +untouched — the rule the four host templates state for an ordinary function is +the same string it always was — and `assign_ids` grew the two suffixes the three +mechanisms need: + +- Class 1 disappears by counting **derived ids** instead of names before + applying `_l`. Nothing else changes: the suffix is still the definition + line, still decided from the file's own contents. +- Class 2 gets `_`, the definition's 1-based position among the same-id + definitions on its line, in source order — the only thing left to order them + by, since no record carries a column. +- Class 3 gets `_f`, the file's position among the paths that derived the + id, sorted. This is the one part of the rule that looks outside a single file, + because the collision is between two of them, and it is applied only to the + ids two files **both** derived — a `.hpp` beside its `.cpp` shares a stem, but + suffixing every function in both would rename most of a C++ catalog to fix a + handful of ids. Keying the rank off the id rather than off the stem also + closes the case where two files with *different* stems still meet on one id, + which a stem-keyed rank would have walked straight past. + +Re-measured over the same corpora, from the same trace output: + +| Corpus | Functions | Duplicated ids, before → after | +|---|---|---| +| CPython standard library | 14,720 | 158 → **0** | +| `/usr/include` | 12,765 | 106 → **0** | +| PrimeVue | 5,817 | 107 → **0** | +| TypeScript 5.6 compiler | 20,707 | 4 → **0** | +| Commons Lang 3 + Gson | 4,594 | 3 → **0** | + +Each "before" is the pre-fix rule re-derived from the *same* trace output, so +the two columns are the two rules over one catalog rather than two runs that +might differ for another reason. Three of the corpora are not byte-identical to +the ones above — the Java corpus here is Commons Lang 3 and Gson without Guava, +JUnit or picocli, and the TypeScript compiler is 5.6.3 — so their totals are +smaller; the four that are identical reproduce their counts exactly. + +2.6% of ids change in the standard library, 2.7% in `/usr/include`, 3.8% in +PrimeVue — the colliding ones and the groups they belong to, and nothing else. + +The C fixture was rearranged in `1768daf` to dodge the `service.cpp` / +`service.hpp` case rather than cover it; that is reverted, so the suite now +proves the fix on the shape that first exposed it. ## Not covered by this pass diff --git a/templates/claude/code-flow.map.md b/templates/claude/code-flow.map.md index d31c744..fecdcd8 100644 --- a/templates/claude/code-flow.map.md +++ b/templates/claude/code-flow.map.md @@ -152,9 +152,8 @@ By default, **also** produce a self-contained interactive HTML page next to the Rules — follow these exactly, or the page will refuse to render: - One node per function in the diagram. **Every `edge.from` and `edge.to` MUST match a node `id`.** -- `id` is derived from the node's own `file` and function name, so that the same function always gets the same `id` in every flow and downstream tools can join flow nodes against a function catalog. Derive it exactly like this: **(1)** take the repo-relative `file` path and drop the extension from its **last segment only** — the final `.` in the filename and everything after it, so `src/v2.1/handler.py` → `src/v2.1/handler`, and a filename with no dot loses nothing; **(2)** append `_` followed by the function's **unqualified** name — `authenticate`, never `User.authenticate` — which is the same name a function catalog records for it; **(3)** lowercase the whole string, replace every remaining character outside `[a-z0-9_]` (path separators, dots, dashes, spaces, anything else) with `_`, collapse each run of `_` into a single `_`, and trim any leading or trailing `_`. Example: `src/web/views.py` + `login_view` → `src_web_views_login_view`. If **the file itself** defines more than one function with that name — same-named methods on two classes, or an overload — append `_l` and the line number of the function's own definition keyword — the -`def`, `function`, `func` or `fn` line itself, never a decorator, annotation or -comment line above it: `src_jobs_worker_run_l31` and `src_jobs_worker_run_l88`. Decide that from the file's own contents, never from which nodes happen to be in this flow: an `id` must not change depending on what else you mapped. +- `id` is derived from the node's own `file` and function name, so that the same function always gets the same `id` in every flow and downstream tools can join flow nodes against a function catalog. Derive it exactly like this: **(1)** take the repo-relative `file` path and drop the extension from its **last segment only** — the final `.` in the filename and everything after it, so `src/v2.1/handler.py` → `src/v2.1/handler`, and a filename with no dot loses nothing; **(2)** append `_` followed by the function's **unqualified** name — `authenticate`, never `User.authenticate` — which is the same name a function catalog records for it; **(3)** lowercase the whole string, replace every remaining character outside `[a-z0-9_]` (path separators, dots, dashes, spaces, anything else) with `_`, collapse each run of `_` into a single `_`, and trim any leading or trailing `_`. Example: `src/web/views.py` + `login_view` → `src_web_views_login_view`. If **the file itself** derives that same id for more than one function — same-named methods on two classes, an overload, or *two different names that slug to one string*, which is what `__add__` and `add`, a `Builder` constructor and its `builder()` factory, or `~Widget` and `Widget` all do — append `_l` and the line number of each one's own definition keyword — the `def`, `function`, `func` or `fn` line itself, never a decorator, annotation or comment line above it: `src_jobs_worker_run_l31` and `src_jobs_worker_run_l88`. Suffix every one of them, including the first. If two of them are on the **same line**, the line cannot separate them, so append `_` and the position of each among that line's same-id definitions, counting from 1 in source order: `src_ui_panel_x_l7_1`, `src_ui_panel_x_l7_2`. Decide all of that from the file's own contents, never from which nodes happen to be in this flow: an `id` must not change depending on what else you mapped. +- Two **different files** can derive the same `id` — `src/service.cpp` and `src/service.hpp`, since the rule drops the extension, or `distutils/_msvccompiler.py` and `distutils/msvccompiler.py`, since the leading `_` becomes a separator and collapses into the one before it. Where that happens, and only for the ids both files actually derived, append `_f` and each file's position among those paths, sorted, counting from 1: `src_service_describe_f1` for `src/service.cpp` and `src_service_describe_f2` for `src/service.hpp`. Leave every other function in both files alone. This is the one part of the rule that looks outside a single file, because the collision is between two of them; it is also rare, so if you are applying it to more than a handful of ids, re-read the paths. - `kind` on a **node** ∈ `entry` | `step` | `external` | `io` | `component` (default `step`). `entry` = where the flow starts; `external` = a third-party/library boundary; `io` = a DB/network/file side effect; `component` = a UI component rather than a plain function. This drives node color. - **Exactly one** node MUST have `kind: entry`. If the flow has several plausible roots — two HTTP handlers, say — pick the one the user asked about, make that the `entry`, and mark the others `step`. - `kind` on an **edge** ∈ `call` | `async` | `conditional` | `render` (default `call`). `render` is a parent component drawing a child. Set `"back": true` on any edge that closes a loop or recursion (points back to an ancestor) so it is drawn as a routed dashed curve. diff --git a/templates/copilot/code-flow.map.prompt.md b/templates/copilot/code-flow.map.prompt.md index 62e89c2..426adda 100644 --- a/templates/copilot/code-flow.map.prompt.md +++ b/templates/copilot/code-flow.map.prompt.md @@ -46,7 +46,8 @@ By default, also produce a self-contained interactive HTML page next to the mark Rules (follow exactly or the page refuses to render): - One node per function. **Every `edge.from`/`edge.to` MUST match a node `id`.** -- `id` is derived from the node's own `file` and function name, so that the same function always gets the same `id` in every flow and downstream tools can join flow nodes against a function catalog. Derive it exactly like this: **(1)** take the repo-relative `file` path and drop the extension from its **last segment only** — the final `.` in the filename and everything after it, so `src/v2.1/handler.py` → `src/v2.1/handler`, and a filename with no dot loses nothing; **(2)** append `_` followed by the function's **unqualified** name — `authenticate`, never `User.authenticate` — which is the same name a function catalog records for it; **(3)** lowercase the whole string, replace every remaining character outside `[a-z0-9_]` (path separators, dots, dashes, spaces, anything else) with `_`, collapse each run of `_` into a single `_`, and trim any leading or trailing `_`. Example: `src/web/views.py` + `login_view` → `src_web_views_login_view`. If **the file itself** defines more than one function with that name — same-named methods on two classes, or an overload — append `_l` and the line number of the function's own definition keyword — the `def`, `function`, `func` or `fn` line itself, never a decorator, annotation or comment line above it: `src_jobs_worker_run_l31` and `src_jobs_worker_run_l88`. Decide that from the file's own contents, never from which nodes happen to be in this flow: an `id` must not change depending on what else you mapped. +- `id` is derived from the node's own `file` and function name, so that the same function always gets the same `id` in every flow and downstream tools can join flow nodes against a function catalog. Derive it exactly like this: **(1)** take the repo-relative `file` path and drop the extension from its **last segment only** — the final `.` in the filename and everything after it, so `src/v2.1/handler.py` → `src/v2.1/handler`, and a filename with no dot loses nothing; **(2)** append `_` followed by the function's **unqualified** name — `authenticate`, never `User.authenticate` — which is the same name a function catalog records for it; **(3)** lowercase the whole string, replace every remaining character outside `[a-z0-9_]` (path separators, dots, dashes, spaces, anything else) with `_`, collapse each run of `_` into a single `_`, and trim any leading or trailing `_`. Example: `src/web/views.py` + `login_view` → `src_web_views_login_view`. If **the file itself** derives that same id for more than one function — same-named methods on two classes, an overload, or *two different names that slug to one string*, which is what `__add__` and `add`, a `Builder` constructor and its `builder()` factory, or `~Widget` and `Widget` all do — append `_l` and the line number of each one's own definition keyword — the `def`, `function`, `func` or `fn` line itself, never a decorator, annotation or comment line above it: `src_jobs_worker_run_l31` and `src_jobs_worker_run_l88`. Suffix every one of them, including the first. If two of them are on the **same line**, the line cannot separate them, so append `_` and the position of each among that line's same-id definitions, counting from 1 in source order: `src_ui_panel_x_l7_1`, `src_ui_panel_x_l7_2`. Decide all of that from the file's own contents, never from which nodes happen to be in this flow: an `id` must not change depending on what else you mapped. +- Two **different files** can derive the same `id` — `src/service.cpp` and `src/service.hpp`, since the rule drops the extension, or `distutils/_msvccompiler.py` and `distutils/msvccompiler.py`, since the leading `_` becomes a separator and collapses into the one before it. Where that happens, and only for the ids both files actually derived, append `_f` and each file's position among those paths, sorted, counting from 1: `src_service_describe_f1` for `src/service.cpp` and `src_service_describe_f2` for `src/service.hpp`. Leave every other function in both files alone. This is the one part of the rule that looks outside a single file, because the collision is between two of them; it is also rare, so if you are applying it to more than a handful of ids, re-read the paths. - Node `kind` ∈ `entry` | `step` | `external` | `io` | `component` (default `step`). `entry` = where the flow starts; `external` = a third-party/library boundary; `io` = a DB/network/file side effect; `component` = a UI component rather than a plain function. This drives node color. **Exactly one** node MUST have `kind: entry`; if the flow has several plausible roots, pick the one the user asked about and mark the rest `step`. Edge `kind` ∈ `call` | `async` | `conditional` | `render` (default `call`); `render` is a parent component drawing a child. Set `"back": true` on edges that close a loop/recursion. - File paths use **forward slashes**, repo-relative; `meta.root` is the absolute root, forward slashes. - `snippet` is optional (≤ ~40 lines). **Replace each `` appended when one file defines that name -more than once. A flow node and this file's entry for the same function +collapse runs, trim. A flow node and this file's entry for the same function therefore carry the same `id`, which is what lets `/code-flow-quality` join them. `idRule` names the version of that rule, so a future change to it is detectable rather than silent. +Three suffixes keep that id unique, each applied only where the one before it +left two functions sharing a string — so on code where nothing collides, every +id is the bare derivation above: + +| Suffix | When | Example | +|---|---|---| +| `_l` | one file derives the same id twice | Python's `__add__` and `add` both slug to `add`; two classes with a `get`; an overload set | +| `_` | two of those share a line, and no record carries a column | a bundled file's `function f(){}function F(){}`, `n` counting in source order | +| `_f` | two files derive the same id | `service.cpp` and `service.hpp`, since the rule drops the extension; `distutils/_msvccompiler.py` and `distutils/msvccompiler.py`, since it collapses underscore runs. `rank` sorts those two paths | + +The first two are decided from a single file's contents, so an unrelated file +moving never renames anything. The third cannot be — the collision is between +two files — so it is applied as narrowly as possible: only to the ids the two +files actually both derived, never to the rest of either file. + **`confidence`** on a call is `exact` when an import, a `self.`/`this.` receiver, a constructor binding, a header the calling file includes, or a same-file definition made the target certain, and `heuristic` when a unique name diff --git a/templates/shared/tracers/_common.py b/templates/shared/tracers/_common.py index 0d9210c..993a101 100644 --- a/templates/shared/tracers/_common.py +++ b/templates/shared/tracers/_common.py @@ -57,25 +57,81 @@ def derive_id(file: str, name: str) -> str: def assign_ids(functions: List[Dict[str, Any]]) -> None: - """Set `id` on every record in ``functions``, in place. - - A name defined more than once in one file — an overload set in C++, two - classes with a `get` in Python, a trait impl and an inherent impl in Rust — - gets `_l` appended to every one of its ids, including the first. The - collision is decided from the file's own contents, so moving an unrelated - file never renames anything, and the suffix comes from the definition line, - so two same-named functions cannot swap identities when one of them moves. + """Set a unique `id` on every record in ``functions``, in place. + + Call this once with the whole repository's functions. Three suffixes are + applied, in order, and each one is added only where the step before it left + two records sharing a string -- so on code where nothing collides, every id + is the bare `derive_id` result and looks exactly like the map templates say. + + **`_l`, when one file derives one id twice.** Two same-named methods + on two classes, an overload set, or -- and this is the part that counts by + id rather than by name -- two *different* names that derive the same id. + Python's `__add__` and `add` both slug to `add`; Java's `Builder` + constructor and its `builder()` factory both slug to `builder`; C++'s + `~Widget` and `Widget` both slug to `widget`. Counting names missed every + one of those: 158 duplicated ids in the CPython standard library alone. + Every member of the group is suffixed, including the first, so a caller + that asked about one of them still sees the same id as a caller that asked + about all of them. + + **`_`, when two of those are on the same line.** A bundled file's + `function f(){}function F(){}` is one line and two definitions, and no + record carries a column, so the line cannot separate them. `n` is the + 1-based position among the same-id definitions on that line, in source + order. + + **`_f`, when two files derive one id.** The rule drops the extension + from the path's last segment, so `service.cpp` and `service.hpp` fold to one + stem, and it collapses underscore runs, so `distutils/_msvccompiler.py` and + `distutils/msvccompiler.py` do too -- the leading `_` becomes a separator + and merges into the one before it. This is the one suffix not decided from a + single file's contents, because the collision is not inside a single file, + and it is applied only to the ids two files actually both derived: a header + beside its source shares a stem, but suffixing every function in both would + rename most of a C++ catalog to fix a handful of ids. `rank` is the file's + 1-based position among those files' paths, sorted in code-point order, so it + is fixed by the repository's file names rather than by traversal order. """ - counts: Dict[Tuple[str, str], int] = {} + _suffix_within_files(functions) + _suffix_across_files(functions) + + +def _suffix_within_files(functions: List[Dict[str, Any]]) -> None: + """Apply the `_l` and `_` suffixes, per file, per derived id.""" + groups: Dict[Tuple[str, str], List[Dict[str, Any]]] = {} for fn in functions: - key = (fn["file"], fn["name"]) - counts[key] = counts.get(key, 0) + 1 + groups.setdefault((fn["file"], derive_id(fn["file"], fn["name"])), []).append(fn) + for (_file, base), group in groups.items(): + if len(group) == 1: + group[0]["id"] = base + continue + by_line: Dict[Any, List[Dict[str, Any]]] = {} + for fn in group: + by_line.setdefault(fn["line"], []).append(fn) + for line, on_line in by_line.items(): + if len(on_line) == 1: + on_line[0]["id"] = "{}_l{}".format(base, line) + else: + for position, fn in enumerate(on_line, 1): + fn["id"] = "{}_l{}_{}".format(base, line, position) + + +def _suffix_across_files(functions: List[Dict[str, Any]]) -> None: + """Apply the `_f` suffix wherever one id was derived from two files.""" + files_by_id: Dict[str, Set[str]] = {} for fn in functions: - base = derive_id(fn["file"], fn["name"]) - if counts[(fn["file"], fn["name"])] > 1: - fn["id"] = "{}_l{}".format(base, fn["line"]) - else: - fn["id"] = base + files_by_id.setdefault(fn["id"], set()).add(fn["file"]) + ranks = { + fid: {path: rank for rank, path in enumerate(sorted(paths), 1)} + for fid, paths in files_by_id.items() + if len(paths) > 1 + } + if not ranks: + return + for fn in functions: + if fn["id"] in ranks: + fn["id"] = "{}_f{}".format(fn["id"], ranks[fn["id"]][fn["file"]]) # --- discovery ------------------------------------------------------------- diff --git a/templates/shared/tracers/trace_python.py b/templates/shared/tracers/trace_python.py index b472e94..611754c 100644 --- a/templates/shared/tracers/trace_python.py +++ b/templates/shared/tracers/trace_python.py @@ -374,14 +374,14 @@ def __init__(self, root: str, detail: str) -> None: # -- ids def assign_ids(self) -> None: - """Give every function its map `id`, applying the collision suffix. + """Give every function its map `id`, applying the collision suffixes. - Per analyzer, which is per file: the suffix is decided from one file's - own contents, never from which functions a caller happened to ask about, - which is what keeps an id stable across runs. + Once, over every analyzer's functions together. The per-file suffixes + are still decided from one file's own contents -- `assign_ids` groups by + file before it counts -- but the suffix for two *paths* that fold to one + id stem cannot be, so it needs the whole catalog in one call. """ - for analyzer in self.analyzers.values(): - common.assign_ids(analyzer.functions) + common.assign_ids([fn for a in self.analyzers.values() for fn in a.functions]) def index(self) -> None: for analyzer in self.analyzers.values(): diff --git a/templates/shared/tracers/trace_typescript.mjs b/templates/shared/tracers/trace_typescript.mjs index 6ab8f7d..3ea2758 100644 --- a/templates/shared/tracers/trace_typescript.mjs +++ b/templates/shared/tracers/trace_typescript.mjs @@ -127,6 +127,76 @@ export function deriveId(file, name) { return combined.toLowerCase().replace(/[^a-z0-9_]+/g, "_").replace(/_+/g, "_").replace(/^_|_$/g, ""); } +/** + * Set a unique `id` on every record in `functions`, in place. + * + * Called once with the whole repository's functions, and the exact counterpart + * of `assign_ids` in `_common.py` -- the five tracers have to agree on this or + * a map built from two of them joins against nothing. Three suffixes, each one + * applied only where the step before it left two records sharing a string, so + * code with no collisions gets the bare `deriveId` result the templates state: + * + * - `_l` when one file derives one id twice. Counted by derived id, not + * by name: `_render` and `render`, `F` and `f`, a getter and its setter all + * slug to one string, and counting names left 107 duplicated ids across + * PrimeVue's 5,817 functions. + * - `_` when two of those are on the same line -- a bundled file's + * `function f(){}function F(){}` is one line and two definitions, and no + * record carries a column. `n` is the 1-based position among that line's + * same-id definitions, in source order. 105 of PrimeVue's 107 were this. + * - `_f` when two files derive one id: the rule drops the extension, so + * `store.ts` and `store.js` beside each other fold to one stem, and it + * collapses underscore runs, so `ui/_Button.tsx` and `ui/Button.tsx` do too. + * The one suffix not decided from a single file's contents, because the + * collision is not inside one file, and applied only to the ids two files + * actually both derived, so a pair that shares a stem does not rename the + * functions that never collided. `rank` is the file's 1-based position among + * those files' paths, sorted in code-point order, so it is fixed by the + * repository's file names rather than by traversal order. + */ +export function assignIds(functions) { + const groups = new Map(); + for (const fn of functions) { + const key = `${fn.file}\u0000${deriveId(fn.file, fn.name)}`; + if (!groups.has(key)) groups.set(key, []); + groups.get(key).push(fn); + } + for (const group of groups.values()) { + const base = deriveId(group[0].file, group[0].name); + if (group.length === 1) { + group[0].id = base; + continue; + } + const byLine = new Map(); + for (const fn of group) { + if (!byLine.has(fn.line)) byLine.set(fn.line, []); + byLine.get(fn.line).push(fn); + } + for (const [line, onLine] of byLine) { + if (onLine.length === 1) onLine[0].id = `${base}_l${line}`; + else onLine.forEach((fn, i) => (fn.id = `${base}_l${line}_${i + 1}`)); + } + } + + const filesById = new Map(); + for (const fn of functions) { + if (!filesById.has(fn.id)) filesById.set(fn.id, new Set()); + filesById.get(fn.id).add(fn.file); + } + const ranks = new Map(); + for (const [id, paths] of filesById) { + if (paths.size < 2) continue; + const byPath = new Map(); + [...paths].sort().forEach((file, i) => byPath.set(file, i + 1)); + ranks.set(id, byPath); + } + if (ranks.size === 0) return; + for (const fn of functions) { + const byPath = ranks.get(fn.id); + if (byPath) fn.id = `${fn.id}_f${byPath.get(fn.file)}`; + } +} + // --- lexing ---------------------------------------------------------------- /** @@ -1871,12 +1941,7 @@ export function trace(rootDir, detail) { const { imports } = collectImports(src, masked); const isTest = isTestPath(rel); - // Ids, with the same-name collision suffix decided from this file alone. - const counts = new Map(); - for (const fn of functions) counts.set(fn.name, (counts.get(fn.name) || 0) + 1); for (const fn of functions) { - const base = deriveId(rel, fn.name); - fn.id = counts.get(fn.name) === 1 ? base : `${base}_l${fn.line}`; fn.role = isTest ? "test" : "source"; fn.purpose = docCommentFor(src, fn); } @@ -1893,11 +1958,6 @@ export function trace(rootDir, detail) { template, varTypes: collectVarTypes(src, masked, classes), }); - for (const fn of functions) { - repo.byId.set(fn.id, fn); - if (!repo.byName.has(fn.name)) repo.byName.set(fn.name, []); - repo.byName.get(fn.name).push(fn); - } let size = 0; try { size = fs.statSync(abs).size; @@ -1907,6 +1967,17 @@ export function trace(rootDir, detail) { repo.census.push({ path: rel, size, hash: fileHash(abs) }); } + // Ids last, and for every file at once: two of the three collision suffixes + // are decided inside one file, but the third compares one file's path against + // every other's, so nothing can be indexed by id until the walk is over. + const catalogued = [...repo.files.values()].flatMap((file) => file.functions); + assignIds(catalogued); + for (const fn of catalogued) { + repo.byId.set(fn.id, fn); + if (!repo.byName.has(fn.name)) repo.byName.set(fn.name, []); + repo.byName.get(fn.name).push(fn); + } + resolveAllCalls(repo); for (const file of repo.files.values()) { diff --git a/test/tracer-typescript.test.js b/test/tracer-typescript.test.js index ec1021e..7dd4fb4 100644 --- a/test/tracer-typescript.test.js +++ b/test/tracer-typescript.test.js @@ -8,7 +8,7 @@ import { fileURLToPath } from "node:url"; const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); const tracer = path.join(repoRoot, "templates", "shared", "tracers", "trace_typescript.mjs"); -const { maskSource, deriveId, matchBrace, trace } = await import(tracer); +const { maskSource, deriveId, assignIds, matchBrace, trace } = await import(tracer); // `tests/test_tracers.py` covers what the tracer produces for a whole fixture // repository, in the same file as the Python tracer's equivalents, because the @@ -64,6 +64,60 @@ test("deriveId implements the map's documented rule", () => { assert.equal(deriveId("bin/entrypoint", "main"), "bin_entrypoint_main"); }); +test("assignIds separates two names that derive one id", () => { + // Counted by derived id, not by name: `_render` and `render` slug to the same + // string, and counting names left both of them unsuffixed and sharing it -- + // 107 duplicated ids across PrimeVue's 5,817 functions. + const functions = [ + { file: "a.ts", name: "_render", line: 12 }, + { file: "a.ts", name: "render", line: 40 }, + { file: "a.ts", name: "mount", line: 60 }, + ]; + assignIds(functions); + assert.deepEqual(functions.map((fn) => fn.id), ["a_render_l12", "a_render_l40", "a_mount"]); +}); + +test("assignIds separates two definitions that share a line", () => { + // 105 of PrimeVue's 107 were this: a bundled file puts two definitions on one + // line, and no record carries a column, so the tie-break is source order. + const functions = [ + { file: "b.js", name: "f", line: 1 }, + { file: "b.js", name: "F", line: 1 }, + { file: "b.js", name: "f", line: 3 }, + ]; + assignIds(functions); + assert.deepEqual(functions.map((fn) => fn.id), ["b_f_l1_1", "b_f_l1_2", "b_f_l3"]); +}); + +test("assignIds separates two paths that fold to one stem", () => { + // The collision no per-file rule can see: `ui/_Button.tsx` and `ui/Button.tsx` + // derive one stem, because the leading `_` collapses into the separator. Only + // the name both files define is suffixed; rank comes from sorting the paths, + // so it does not depend on which file was walked first. + const functions = [ + { file: "ui/Button.tsx", name: "render", line: 4 }, + { file: "ui/_Button.tsx", name: "render", line: 9 }, + { file: "ui/_Button.tsx", name: "measure", line: 20 }, + ]; + assignIds(functions); + assert.deepEqual(functions.map((fn) => fn.id), [ + "ui_button_render_f1", + "ui_button_render_f2", + "ui_button_measure", + ]); +}); + +test("assignIds leaves every id bare when nothing collides", () => { + // The suffixes are the exception, not the shape. Ordinary code has to come out + // looking exactly like the rule the map templates state. + const functions = [ + { file: "src/web/views.ts", name: "loginView", line: 10 }, + { file: "src/web/models.ts", name: "User", line: 3 }, + ]; + assignIds(functions); + assert.deepEqual(functions.map((fn) => fn.id), ["src_web_views_loginview", "src_web_models_user"]); +}); + test("a brace inside a string does not move a function's boundary", () => { // The failure this whole lexer exists to prevent, asserted end to end: without // the mask, `openBrace`'s body would swallow `after` and the call inside it. diff --git a/tests/fixtures/c-app/src/describable.hpp b/tests/fixtures/c-app/src/describable.hpp deleted file mode 100644 index e4ba0d8..0000000 --- a/tests/fixtures/c-app/src/describable.hpp +++ /dev/null @@ -1,14 +0,0 @@ -#pragma once - -#include - -namespace demo { - -/// Anything that can say what it is, for the log. -class Describable { -public: - /// Returns a human-readable description. - virtual std::string describe() const { return "thing"; } -}; - -} // namespace demo diff --git a/tests/fixtures/c-app/src/service.hpp b/tests/fixtures/c-app/src/service.hpp index 19f3b90..398f427 100644 --- a/tests/fixtures/c-app/src/service.hpp +++ b/tests/fixtures/c-app/src/service.hpp @@ -2,10 +2,15 @@ #include -#include "describable.hpp" - namespace demo { +/// Anything that can say what it is, for the log. +class Describable { +public: + /// Returns a human-readable description. + virtual std::string describe() const { return "thing"; } +}; + /// Authenticates users against the C store. class UserService : public Describable { public: diff --git a/tests/test_tracer_lexer.py b/tests/test_tracer_lexer.py index 92d29da..3c7193e 100644 --- a/tests/test_tracer_lexer.py +++ b/tests/test_tracer_lexer.py @@ -183,3 +183,75 @@ def test_collision_suffixes_come_from_the_line_not_the_order() -> None: ] _common.assign_ids(functions) assert [fn["id"] for fn in functions] == ["a_get_l10", "a_get_l38", "a_new"] + + +def test_two_names_that_derive_one_id_are_a_collision() -> None: + """The suffix is decided by derived id, not by name. + + `__add__` and `add` are different names and one id, so counting names left + both of them unsuffixed and sharing it. This is the mechanism behind 158 of + the CPython standard library's duplicated ids, every C++ destructor beside + its constructor, and every Java `Builder`/`builder()` pair. + """ + functions = [ + {"file": "d.py", "name": "__add__", "line": 12}, + {"file": "d.py", "name": "add", "line": 40}, + {"file": "d.py", "name": "subtract", "line": 60}, + ] + _common.assign_ids(functions) + assert [fn["id"] for fn in functions] == ["d_add_l12", "d_add_l40", "d_subtract"] + + +def test_two_definitions_on_one_line_are_separated_by_position() -> None: + """`_l` cannot separate two definitions that share a line. + + `void set(int); void set(char);` written on one line is an overload set, and + so is a bundled file's `function f(){}function F(){}`. No record carries a + column, so the tie-break is source order on that line. 105 of PrimeVue's 107 + duplicated ids were this shape. + """ + functions = [ + {"file": "a.cpp", "name": "set", "line": 7}, + {"file": "a.cpp", "name": "set", "line": 7}, + {"file": "a.cpp", "name": "set", "line": 9}, + ] + _common.assign_ids(functions) + assert [fn["id"] for fn in functions] == ["a_set_l7_1", "a_set_l7_2", "a_set_l9"] + + +def test_two_paths_that_fold_to_one_stem_are_separated_by_rank() -> None: + """The collision the per-file suffixes cannot see. + + `distutils/_msvccompiler.py` and `distutils/msvccompiler.py` derive the same + stem — the leading `_` collapses into the separator — so every function in + one shadowed its namesake in the other. Rank comes from sorting the + colliding paths, so it does not depend on which file was walked first. + """ + functions = [ + {"file": "distutils/msvccompiler.py", "name": "link", "line": 4}, + {"file": "distutils/_msvccompiler.py", "name": "link", "line": 9}, + {"file": "distutils/ccompiler.py", "name": "link", "line": 3}, + ] + _common.assign_ids(functions) + assert [fn["id"] for fn in functions] == [ + "distutils_msvccompiler_link_f2", + "distutils_msvccompiler_link_f1", + "distutils_ccompiler_link", + ] + + +def test_a_repository_with_no_collisions_keeps_every_bare_id() -> None: + """The suffixes are the exception, not the shape. Ordinary code has to come + out of `assign_ids` looking exactly like the rule the templates state, or + every id in every hand-written map is wrong.""" + functions = [ + {"file": "src/web/views.py", "name": "login_view", "line": 10}, + {"file": "src/web/models.py", "name": "User", "line": 3}, + {"file": "src/main.py", "name": "main", "line": 1}, + ] + _common.assign_ids(functions) + assert [fn["id"] for fn in functions] == [ + "src_web_views_login_view", + "src_web_models_user", + "src_main_main", + ] diff --git a/tests/test_tracers.py b/tests/test_tracers.py index 91c1960..12b87c4 100644 --- a/tests/test_tracers.py +++ b/tests/test_tracers.py @@ -31,6 +31,7 @@ from __future__ import annotations import json +import re import shutil import subprocess import sys @@ -256,8 +257,11 @@ def test_ids_follow_the_documented_derivation(request, which: str) -> None: wrong = [] for fn in trace["functions"]: base = derive_id(fn["file"], fn["name"]) - if fn["id"] not in (base, f"{base}_l{fn['line']}"): - wrong.append(f"{fn['id']} should be {base} (or {base}_l{fn['line']})") + allowed = re.compile( + rf"^{re.escape(base)}(_l{fn['line']}(_\d+)?)?(_f\d+)?$" + ) + if not allowed.match(fn["id"]): + wrong.append(f"{fn['id']} should be {base}, with a collision suffix at most") assert not wrong, f"{which}: ids do not follow the rule: " + "; ".join(wrong) @@ -269,14 +273,12 @@ def test_ids_are_unique_within_one_trace(request, which: str) -> None: reached set against a catalogued one — so a duplicate does not degrade the map, it corrupts it: one entry silently stands in for two. - This is a canary rather than a proof. The id rule drops the extension from - the path's last segment, so `service.cpp` and `service.hpp` derive the same - stem, while `assign_ids` decides its `_l` collision suffix from one - file's own contents. Two same-named function bodies across such a pair — - an inline method in a header and another class's method in the source - beside it, which is ordinary C++ — therefore collide, and nothing in the - rule prevents it. This assertion is what makes that loud the next time a - fixture reaches it. + The C fixture is built to reach the hardest case rather than to avoid it: + the id rule drops the extension from the path's last segment, so + `service.cpp` and `service.hpp` derive one stem, and `Describable::describe` + declared inline in the header sits beside `UserService::describe` defined in + the source — ordinary C++, and the shape that made this assertion fail + before `assign_ids` grew its `_f` suffix. """ trace = _trace(request, which) seen: dict[str, dict] = {} @@ -355,7 +357,10 @@ def test_limits_are_stated_rather_than_implied(request, which: str) -> None: {"src_main_java_com_demo_userservice_describe", "src_main_java_com_demo_userstore_describe"}, ), - "c-family": ("Describable::describe", {"src_service_describe"}), + # The one family whose members are split across a header and its source, so + # the ids carry the cross-file suffix: `src/service.cpp` ranks before + # `src/service.hpp`, and both fold to the stem `src_service`. + "c-family": ("Describable::describe", {"src_service_describe_f1"}), } @@ -849,7 +854,7 @@ def test_c_tracer_catalogues_a_cpp_constructor_and_an_out_of_class_method(c_trac functions = _by_id(c_trace) assert "src_service_userservice" in functions, "a C++ constructor was not catalogued" assert functions["src_service_authenticate"]["qualname"] == "UserService::authenticate" - assert ("src_service_authenticate", "src_service_describe") in _edges(c_trace) + assert ("src_service_authenticate", "src_service_describe_f1") in _edges(c_trace) def test_c_tracer_reads_a_cpp_call_into_c(c_trace: dict) -> None: