From 21e6aa2d69fdb45eae4c36c57a7ccbf1905ffe66 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 01:17:18 -0400 Subject: [PATCH 01/15] scripts: add split_rust_module.py for mechanical Rust module splits Splitting a large Rust file into child modules by hand is slow to review and painful to rebase. This tool does it from a short spec that names the items each new child module gets: - `list FILE` prints every top-level item with the selector a spec uses. - `apply SPEC` moves the named items verbatim, with their doc comments and attributes and in their original order, into new child modules. The parent keeps `mod child;` and a glob re-export, so every existing path still resolves and no caller changes. - `verify [BASE]` proves BASE...HEAD is such a move. It re-applies the same edits to every item at the merge base and requires an exact match at HEAD, ignoring whitespace and trailing commas but not string literals. The only edits to moved text are the ones that keep name resolution and reach exactly as they were: - Visibility on items, inherent-impl members and struct fields: private becomes `pub(super)` and `pub(super)` becomes `pub(in )`, which is the same reach one module deeper. - `super::` paths gain one `super::`. - Relative `include_str!`/`include_bytes!` paths gain `../` when the children sit one directory below the parent file. Each child starts with `use super::*;`, so the parent's imports, traits and macros resolve as before. Because the spec names items rather than lines, a split can be regenerated on a newer main instead of being rebased. --- scripts/split_rust_module.py | 892 ++++++++++++++++++++++++ scripts/tests/test_split_rust_module.py | 278 ++++++++ 2 files changed, 1170 insertions(+) create mode 100644 scripts/split_rust_module.py create mode 100644 scripts/tests/test_split_rust_module.py diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py new file mode 100644 index 000000000..719ad7c7f --- /dev/null +++ b/scripts/split_rust_module.py @@ -0,0 +1,892 @@ +#!/usr/bin/env python3 +"""Split one rustfmt-formatted Rust module file into child modules, mechanically. + +The split is a pure move. Every top-level item named in the spec moves verbatim +(with its doc comments and attributes, in original order) into a new child +module. The parent keeps `mod child;` plus a glob re-export, so every existing +path keeps resolving. The only edits to moved text are the ones that keep +name resolution and effective visibility exactly as they were: + +* Visibility, so each item and each inherent-impl member and struct field keeps + the same reach it had in the parent: + private / pub(self) -> pub(super) + pub(super) -> pub(in ) (pub(crate) if that is the crate root) + pub(crate), pub, pub(in crate::..) unchanged +* `super::` paths in moved code gain one `super::` (the code is one module deeper). +* Relative `include_str!`/`include_bytes!` paths gain `../` when the children + live one directory deeper than the parent file (parent is `foo.rs`). + +Each new module starts with `use super::*;`, so the parent's imports, traits and +sibling items resolve in moved code as before. + +The spec is item names, not line numbers, so it can be re-run on a newer main +instead of rebasing a split by hand. + +Usage: + split_rust_module.py list FILE # items and selectors, for writing a spec + split_rust_module.py apply SPEC.toml [--repo ROOT] [--report OUT.md] [--dry-run] + split_rust_module.py verify [BASE] [--head REV] # prove BASE...HEAD is such a move + +Spec (TOML): + file = "crates/libs/x/src/foo.rs" + [[module]] + name = "bar" + doc = "What bar holds." # one `//!` line at the top of the new file + items = ["fn parse_*", "struct Config", "impl Config", "impl Display for Config"] + # optional: reexport = "auto" | "none" | "test" (default auto) + +Selectors are fnmatch patterns over ` `; impl blocks are +`impl
` with whitespace collapsed (e.g. `impl From for Foo`). +Every pattern must match at least one item and every item at most one module. +""" +from __future__ import annotations + +import argparse +import fnmatch +import json +import os +import re +import subprocess +import sys +import tomllib +from dataclasses import dataclass, field + +# --------------------------------------------------------------------- masking + +CHAR_RE = re.compile(r"'(?:[^'\\\n]|\\(?:[nrt\\0'\"]|x[0-9a-fA-F]{2}|u\{[0-9a-fA-F_]{1,6}\}))'") + + +def mask(src: str, comment: str = " ") -> str: + """Same length as src; comments -> `comment`, string/char literal bodies -> 'x'. + + Newlines are kept, so line structure and character offsets line up with src. + """ + out = list(src) + n = len(src) + i = 0 + + def blank(a: int, b: int, ch: str) -> None: + for k in range(a, b): + if out[k] != "\n": + out[k] = ch + + while i < n: + c = src[i] + if c == "/" and i + 1 < n and src[i + 1] == "/": + j = src.find("\n", i) + if j < 0: + j = n + blank(i, j, comment) + i = j + continue + if c == "/" and i + 1 < n and src[i + 1] == "*": + depth = 1 + j = i + 2 + while j < n and depth: + if src.startswith("/*", j): + depth += 1 + j += 2 + elif src.startswith("*/", j): + depth -= 1 + j += 2 + else: + j += 1 + blank(i, j, comment) + i = j + continue + if c in "rbc" and (i == 0 or not (src[i - 1].isalnum() or src[i - 1] == "_")): + m = re.match(r"(?:br|cr|r)(#*)\"", src[i:i + 300]) + if m: + term = '"' + m.group(1) + j = src.find(term, i + m.end()) + j = n if j < 0 else j + len(term) + blank(i, j, "x") + i = j + continue + if re.match(r"(?:b|c)\"", src[i:i + 2]): + out[i] = "x" + i += 1 + c = '"' + elif src.startswith("b'", i): + m = CHAR_RE.match(src, i + 1) + if m: + blank(i, m.end(), "x") + i = m.end() + continue + if c == '"': + j = i + 1 + while j < n: + if src[j] == "\\": + j += 2 + continue + if src[j] == '"': + j += 1 + break + j += 1 + blank(i, j, "x") + i = j + continue + if c == "'": + m = CHAR_RE.match(src, i) + if m: + blank(i, m.end(), "x") + i = m.end() + continue + i += 1 + return "".join(out) + + +OPEN = "([{" +CLOSE = ")]}" + +# ------------------------------------------------------------------- items + +VIS = r"(?:pub(?:\s*\(\s*(?:crate|self|super|in\s+[A-Za-z0-9_:\s]+)\s*\))?\s+)?" +QUAL = r"(?:(?:const|async|unsafe|default|extern(?:\s+\"[^\"]*\")?)\s+)*" +ITEM_RE = re.compile( + r"^(?P" + VIS + r")" + QUAL + + r"(?Pfn|struct|enum|union|trait|impl|type|const|static|mod|use|extern\s+crate|auto\s+trait)\b|(?Pmacro_rules!)" +) +NAME_RE = re.compile(r"\s*(?:r#)?([A-Za-z_][A-Za-z0-9_]*)") +MACRO_CALL_RE = re.compile(r"^(?P[A-Za-z_][A-Za-z0-9_:]*)\s*!") +UNMOVABLE = {"use", "extern crate"} + + +@dataclass +class Item: + kind: str + name: str + start: int # first line of the item's code (0-based) + end: int # last line (inclusive) + lead: int # first line of its leading trivia (attributes, comments) + vis: str + cfg: list[str] = field(default_factory=list) + + @property + def selector(self) -> str: + if self.kind == "impl": + return "impl" + (self.name if self.name.startswith("<") else " " + self.name) + return f"{self.kind} {self.name}" + + +def line_depth_delta(mline: str) -> int: + d = 0 + for ch in mline: + if ch in OPEN: + d += 1 + elif ch in CLOSE: + d -= 1 + return d + + +def collapse(s: str) -> str: + return re.sub(r"\s+", " ", s).strip() + + +def parse_items(src: str) -> tuple[list[Item], int]: + """Return (items, header_end) where header_end is the first line after the header. + + Relies on rustfmt layout: top-level items start in column 0 at depth 0. + """ + lines = src.split("\n") + mlines = mask(src).split("\n") + n = len(lines) + # Header: leading //! docs, #![...] inner attributes, and blank lines. + i = 0 + header_end = 0 + depth = 0 + while i < n: + s = lines[i].strip() + if s.startswith("//!") or s == "": + i += 1 + if s: + header_end = i + continue + if s.startswith("#!["): + d = line_depth_delta(mlines[i]) + i += 1 + while d > 0 and i < n: + d += line_depth_delta(mlines[i]) + i += 1 + header_end = i + continue + break + items: list[Item] = [] + i = header_end + lead = None + while i < n: + raw, m = lines[i], mlines[i] + s = raw.strip() + if depth != 0: + raise SystemExit(f"internal: depth {depth} at line {i + 1}") + if s == "": + i += 1 + continue + if m.strip() == "": # comment-only line + if lead is None: + lead = i + i += 1 + continue + if raw[0].isspace(): + raise SystemExit(f"line {i + 1}: top-level code not in column 0 (run rustfmt first)") + if s.startswith("#["): + if lead is None: + lead = i + d = line_depth_delta(m) + i += 1 + while d > 0 and i < n: + d += line_depth_delta(mlines[i]) + i += 1 + continue + # item start + start = i + d = 0 + while True: + d += line_depth_delta(mlines[i]) + tail = mlines[i].rstrip() + if d == 0 and (tail.endswith("}") or tail.endswith(";")): + break + i += 1 + if i >= n: + raise SystemExit(f"line {start + 1}: item never ends") + end = i + i += 1 + text_m = "\n".join(mlines[start:end + 1]) + it = classify(lines, mlines, start, end, text_m) + it.lead = lead if lead is not None else start + it.cfg = [collapse(x) for x in attrs_of(lines, it.lead, start) if collapse(x).startswith("#[cfg(")] + items.append(it) + lead = None + return items, header_end + + +def attrs_of(lines: list[str], lead: int, start: int) -> list[str]: + out, cur = [], None + for k in range(lead, start): + s = lines[k].strip() + if s.startswith("#["): + cur = s + if cur.count("[") <= cur.count("]"): + out.append(cur) + cur = None + elif cur is not None: + cur += " " + s + if cur.count("[") <= cur.count("]"): + out.append(cur) + cur = None + return out + + +def classify(lines, mlines, start, end, text_m) -> Item: + first = lines[start] + mm = ITEM_RE.match(first) + if mm and mm.group("mr"): + nm = NAME_RE.match(first[mm.end():]) + return Item("macro_rules!", nm.group(1) if nm else "?", start, end, start, "") + if mm: + kind = collapse(mm.group("kind")) + vis = collapse(mm.group("vis")) + rest = first[mm.end():] + if kind == "impl": + mtext = "\n".join(mlines[start:end + 1]) + cut = mtext.find("{") + header = collapse("\n".join(lines[start:end + 1])[: cut if cut >= 0 else None]) + name = re.sub(r"^(?:unsafe\s+)?impl\s*", "", header).strip() + return Item("impl", name, start, end, start, vis) + if kind in ("use", "extern crate"): + return Item(kind, collapse(" ".join(lines[start:end + 1]))[:120], start, end, start, vis) + nm = NAME_RE.match(rest) + name = nm.group(1) if nm else "?" + if kind == "mod": + kind = "mod" if text_m.rstrip().endswith("}") else "mod-decl" + return Item(kind, name, start, end, start, vis) + mc = MACRO_CALL_RE.match(first.strip()) + if mc: + return Item("macro", mc.group("path"), start, end, start, "") + raise SystemExit(f"line {start + 1}: cannot classify top-level item: {lines[start][:80]!r}") + + +# ----------------------------------------------------------- visibility map + +def module_path_of(rel_file: str) -> list[str]: + """Module path segments of a file inside a crate's src/ (crate root -> []).""" + p = rel_file.replace(os.sep, "/") + if "/src/" not in p: + raise SystemExit(f"{rel_file}: expected a path under /src/") + tail = p.split("/src/", 1)[1] + parts = tail[:-3].split("/") if tail.endswith(".rs") else tail.split("/") + if parts[-1] in ("lib", "main", "mod"): + parts = parts[:-1] + if parts and parts[0] == "bin": # src/bin/x.rs is its own crate root + parts = parts[2:] + return parts + + +def map_vis(vis: str, parent_path: list[str]) -> str | None: + """New visibility for an item moved from module P (path parent_path) into P::child. + + Returns None when the visibility is unchanged. + """ + v = collapse(vis) + if v in ("", "pub(self)"): + return "pub(super) " + if v == "pub(super)": + if not parent_path: + raise SystemExit("pub(super) in a crate root") + gp = parent_path[:-1] + return "pub(crate) " if not gp else f"pub(in crate::{'::'.join(gp)}) " + if v.startswith("pub(in ") and not v.startswith("pub(in crate"): + raise SystemExit(f"relative visibility {v!r} not supported; edit by hand") + return None + + +VIS_AT = re.compile(r"pub(?:\s*\(\s*(?:crate|self|super|in\s+[A-Za-z0-9_:\s]+)\s*\))?\s+") + + +def skip_ws_attrs(mt: str, p: int) -> int: + """Skip whitespace (comments are already blank in masked text) and #[...] attributes.""" + n = len(mt) + while p < n: + if mt[p].isspace(): + p += 1 + elif mt.startswith("#[", p) or mt.startswith("#![", p): + d = 0 + while p < n: + if mt[p] == "[": + d += 1 + elif mt[p] == "]": + d -= 1 + if d == 0: + p += 1 + break + p += 1 + else: + break + return p + + +def find_body_open(mt: str, p: int) -> int: + """From after the item name, skip generics and where clauses; return offset of the body + delimiter `{` or `(`, or -1 for a unit item ending in `;`.""" + n = len(mt) + if p < n and mt[p] == "<": + d = 0 + while p < n: + ch = mt[p] + if ch == "<": + d += 1 + elif ch == ">" and mt[p - 1] != "-": + d -= 1 + if d == 0: + p += 1 + break + p += 1 + while p < n and mt[p].isspace(): + p += 1 + if p < n and mt[p] == "(": + return p + depth = 0 + while p < n: + ch = mt[p] + if ch in "([": + depth += 1 + elif ch in ")]": + depth -= 1 + elif ch == "{" and depth == 0: + return p + elif ch == ";" and depth == 0: + return -1 + p += 1 + return -1 + + +def split_top(mt: str, open_pos: int) -> list[tuple[int, int]]: + """Segments [a, b) separated by depth-1 commas inside the delimiter at open_pos. + + Angle brackets count as nesting (`HashMap`), except the `>` of `->` and `=>`. + """ + segs, depth, angle, a = [], 0, 0, open_pos + 1 + for p in range(open_pos, len(mt)): + ch = mt[p] + if ch in OPEN: + depth += 1 + elif ch in CLOSE: + depth -= 1 + if depth == 0: + segs.append((a, p)) + return segs + elif ch == "<" and depth >= 1: + angle += 1 + elif ch == ">" and depth >= 1 and mt[p - 1] not in "-=" and angle > 0: + angle -= 1 + elif ch == "," and depth == 1 and angle == 0: + segs.append((a, p)) + a = p + 1 + return segs + + +MEMBER_RE = re.compile(r"(" + VIS + r")" + QUAL + r"(fn|const|type)\b") + + +def transform_item(text: str, kind: str, parent_path: list[str], deeper_dir: bool) -> tuple[str, dict]: + """Apply the visibility/path edits to one moved item's text (lead trivia included).""" + counts: dict = {} + mt = mask(text) + edits: list[tuple[int, int, str]] = [] # (pos, len_old, new) applied right-to-left + + def vis_edit(pos: int) -> None: + m = VIS_AT.match(text, pos) + old = m.group(0) if m else "" + new = map_vis(old, parent_path) + if new is not None: + edits.append((pos, len(old), new)) + counts["vis"] = counts.get("vis", 0) + 1 + + # 1. the item's own visibility: first code offset after attributes + p0 = skip_ws_attrs(mt, 0) + if kind not in ("impl", "macro"): + vis_edit(p0) + # 2. members + if kind in ("struct", "union"): + km = re.compile(VIS + QUAL + r"(struct|union)\s+").match(mt, p0) + nm = NAME_RE.match(mt, km.end()) if km else None + if nm: + ob = find_body_open(mt, nm.end()) + if ob >= 0: + for a, b in split_top(mt, ob): + q = skip_ws_attrs(mt, a) + if q < b: + vis_edit(q) + elif kind == "impl": + header_end = mt.find("{", p0) + header = re.sub(r"\bfor\s*<[^<>]*>", "", mt[p0:header_end]) + header = re.sub(r"<[^<>]*>", "", header) + while re.search(r"<[^<>]*>", header): + header = re.sub(r"<[^<>]*>", "", header) + inherent = not re.search(r"\bfor\s+(?!<)", header) + if inherent: + # members start after the body brace or after a depth-1 `}`/`;` + depth, p, starts = 0, header_end, [] + while p < len(mt): + ch = mt[p] + if ch in OPEN: + depth += 1 + if depth == 1 and ch == "{": + starts.append(p + 1) + elif ch in CLOSE: + depth -= 1 + if depth == 1 and ch == "}": + starts.append(p + 1) + if depth == 0: + break + elif ch == ";" and depth == 1: + starts.append(p + 1) + p += 1 + for s in starts: + q = skip_ws_attrs(mt, s) + if MEMBER_RE.match(mt, q): + vis_edit(q) + # 3. super:: paths (code only) + for m in re.finditer(r"(? dict: + mt = mask(text) + return {k: len(r.findall(mt if k != "instrument_attrs" else text)) for k, r in OBS.items()} + + +# ------------------------------------------------------------------ apply + +RANK = {"none": -1, "private": 0, "in": 1, "crate": 2, "pub": 3} +TYPE_NS = {"struct", "enum", "union", "trait", "type", "mod", "mod-decl"} + + +def new_vis_rank(orig_vis: str, parent_path: list[str]) -> tuple[int, str]: + """Rank of an item's visibility after the move, for choosing the re-export.""" + v = collapse(orig_vis) + mapped = map_vis(v, parent_path) + v = collapse(mapped) if mapped is not None else v + if v == "pub": + return RANK["pub"], "" + if v == "pub(crate)": + return RANK["crate"], "" + if v.startswith("pub(in "): + return RANK["in"], v + return RANK["private"], "" # pub(super): reaches the parent only + + +def code_lines(text: str) -> int: + return sum(1 for ln in mask(text).split("\n") if ln.strip()) + + +def strip_super(sel: str) -> str: + return re.sub(r"\bsuper::", "", collapse(sel)) + + +def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: + spec = tomllib.load(open(spec_path, "rb")) + rel = spec["file"] + path = os.path.join(repo, rel) + src = open(path, encoding="utf-8").read() + items, header_end = parse_items(src) + lines = src.split("\n") + parent_path = module_path_of(rel) + base = os.path.basename(rel) + deeper = base not in ("lib.rs", "main.rs", "mod.rs") + child_dir = os.path.join(os.path.dirname(path), base[:-3]) if deeper else os.path.dirname(path) + mods = spec["module"] + names = [m["name"] for m in mods] + if len(set(names)) != len(names): + raise SystemExit("duplicate module names in spec") + + assign: dict[int, str] = {} + for mod in mods: + name = mod["name"] + if not re.fullmatch(r"[a-z][a-z0-9_]*", name): + raise SystemExit(f"bad module name {name!r}") + for pat in mod["items"]: + hits = [k for k, it in enumerate(items) if fnmatch.fnmatchcase(it.selector, pat)] + if not hits: + raise SystemExit(f"module {name}: pattern {pat!r} matches no item") + for k in hits: + it = items[k] + if it.kind in UNMOVABLE or it.kind == "mod-decl": + raise SystemExit(f"{it.selector}: use/mod declarations stay in the parent") + if it.kind == "macro_rules!": + raise SystemExit(f"{it.selector}: macro_rules! stays in the parent (textual scope)") + if k in assign and assign[k] != name: + raise SystemExit(f"{it.selector} matched by {assign[k]} and {name}") + assign[k] = name + for it in items: + if it.kind in TYPE_NS and it.name in names: + raise SystemExit(f"module name {it.name!r} collides with {it.selector}") + + results = {} + for mod in mods: + name = mod["name"] + ks = [k for k in range(len(items)) if assign.get(k) == name] + if not ks: + raise SystemExit(f"module {name}: no items") + chunks, counts, obs, best = [], {}, {k: 0 for k in OBS}, (RANK["none"], "") + for k in ks: + it = items[k] + text = "\n".join(lines[it.lead:it.end + 1]).lstrip("\n") + new, c = transform_item(text, it.kind, parent_path, deeper) + for key, v in c.items(): + counts[key] = counts.get(key, 0) + v + for key, v in observables(text).items(): + obs[key] += v + chunks.append(new) + if it.kind == "macro": + counts["macro_items"] = counts.get("macro_items", 0) + 1 + best = max(best, (RANK["private"], "")) + elif it.kind != "impl": + best = max(best, new_vis_rank(it.vis, parent_path)) + doc = mod.get("doc", "").strip() + body = "" + if doc: + body += "".join(f"//! {d}\n" if d.strip() else "//!\n" for d in doc.split("\n")) + "\n" + body += "use super::*;\n\n" + "\n\n".join(chunks) + "\n" + cfgs = [set(items[k].cfg) for k in ks] + common_cfg = sorted(set.intersection(*cfgs)) + reexport = mod.get("reexport", "auto") + if best[0] == RANK["none"]: + reexport = "none" # only impl blocks: nothing to name + use_vis = {RANK["pub"]: "pub ", RANK["crate"]: "pub(crate) ", RANK["in"]: best[1] + " "}.get(best[0], "") + results[name] = dict(items=[items[k].selector for k in ks], text=body, counts=counts, obs=obs, + cfg=common_cfg, use_vis=use_vis, reexport=reexport, + code=code_lines(body), path=os.path.join(child_dir, name + ".rs")) + + # declarations for the parent + decls = [] + for mod in mods: + r = results[mod["name"]] + decls.append("".join(c + "\n" for c in r["cfg"]) + f"mod {mod['name']};") + uses = [] + for mod in mods: + r = results[mod["name"]] + if r["reexport"] == "none": + continue + gate = "".join(c + "\n" for c in r["cfg"]) + ("#[cfg(test)]\n" if r["reexport"] == "test" else "") + uses.append(f"{gate}{r['use_vis']}use {mod['name']}::*;") + decl_text = "\n".join(decls) + ("\n\n" + "\n".join(uses) if uses else "") + + # parent remainder: kept items in order; keep original spacing between items + # that were adjacent, one blank line where moved items were removed. + keep = [k for k in range(len(items)) if k not in assign] + lead_end = -1 + for idx, k in enumerate(keep): + if items[k].kind in ("use", "extern crate", "mod-decl"): + lead_end = idx + else: + break + macro_last = max([idx for idx, k in enumerate(keep) if items[k].kind == "macro_rules!"], default=-1) + decl_after = max(lead_end, macro_last) + out = lines[:header_end] + while out and not out[-1].strip(): + out.pop() + + def sep() -> None: + if out and out[-1].strip(): + out.append("") + + if decl_after == -1: + sep() + out.extend(decl_text.split("\n")) + prev = None + for idx, k in enumerate(keep): + it = items[k] + if prev is not None and prev == k - 1: + out.extend(lines[items[prev].end + 1:it.lead]) # original gap (blank lines) + else: + sep() + out.extend(lines[it.lead:it.end + 1]) + if idx == decl_after: + sep() + out.extend(decl_text.split("\n")) + prev = None + else: + prev = k + trailing = lines[items[-1].end + 1:] if items else [] + while trailing and not trailing[-1].strip(): + trailing.pop() + if trailing: + sep() + out.extend(t for t in trailing) + parent_text = "\n".join(out).rstrip("\n") + "\n" + + # self-check: re-parse everything and compare item sequences + for name, r in results.items(): + got = [strip_super(it.selector) for it in parse_items(r["text"])[0] if it.kind != "use"] + want = [strip_super(x) for x in r["items"]] + if got != want: + raise SystemExit(f"self-check failed for {name}: {got} != {want}") + new_decl = {f"mod-decl {n}" for n in names} + got = [it.selector for it in parse_items(parent_text)[0] + if it.selector not in new_decl and not re.fullmatch( + r"use (?:pub(?:\([^)]*\))? )?use (" + "|".join(names) + r")::\*;", "use " + it.name)] + want = [items[k].selector for k in keep] + if got != want: + missing = set(want) - set(got) + extra = set(got) - set(want) + raise SystemExit(f"self-check failed for parent: missing {missing}, extra {extra}") + + summary = { + "file": rel, + "parent_code_before": code_lines(src), + "parent_code_after": code_lines(parent_text), + "modules": {n: {k: v for k, v in r.items() if k != "text"} for n, r in results.items()}, + "kept": [items[k].selector for k in keep if items[k].kind not in ("use", "mod-decl")], + } + if not dry: + for name, r in results.items(): + if os.path.exists(r["path"]): + raise SystemExit(f"{r['path']} already exists") + os.makedirs(child_dir, exist_ok=True) + for name, r in results.items(): + with open(r["path"], "w", encoding="utf-8") as f: + f.write(r["text"]) + with open(path, "w", encoding="utf-8") as f: + f.write(parent_text) + print(json.dumps(summary, indent=1, default=str)) + if report: + write_report(report, summary, repo) + return 0 + + +# ------------------------------------------------------------------ verify + +def normalize(text: str) -> str: + """Whitespace- and trailing-comma-insensitive form of an item, for comparing a move + across a rustfmt run. String and char literals are kept byte for byte.""" + m = mask(text, comment="c") + out: list[str] = [] + for ch, mk in zip(text, m): + if mk == "x": + out.append(ch) + elif ch.isspace(): + continue + else: + if mk != "c" and ch in CLOSE and out and out[-1] == ",": + out.pop() + out.append(ch) + return "".join(out) + + +def git(repo: str, *args: str) -> str: + return subprocess.run(["git", "-C", repo, *args], capture_output=True, text=True, check=True).stdout + + +def git_show(repo: str, rev: str, path: str) -> str | None: + r = subprocess.run(["git", "-C", repo, "show", f"{rev}:{path}"], capture_output=True, text=True) + return r.stdout if r.returncode == 0 else None + + +def item_records(src: str) -> list[tuple[Item, str]]: + items, _ = parse_items(src) + lines = src.split("\n") + return [(it, "\n".join(lines[it.lead:it.end + 1]).lstrip("\n")) for it in items] + + +def parent_candidates(child: str) -> list[str]: + d = os.path.dirname(child) + out = [d + ".rs", os.path.join(d, "mod.rs")] + if os.path.basename(d) == "src": + out = [os.path.join(d, "lib.rs"), os.path.join(d, "main.rs")] + return out + + +def verify(repo: str, base: str, head: str) -> int: + """Prove BASE...HEAD only moves top-level items into child modules the way `apply` does.""" + merge_base = git(repo, "merge-base", base, head).strip() + status = [ln.split("\t") for ln in git(repo, "diff", "--name-status", "--no-renames", + f"{merge_base}..{head}", "--", "*.rs").splitlines()] + added = {p for st, p in status if st == "A"} + modified = {p for st, p in status if st == "M"} + deleted = {p for st, p in status if st == "D"} + pool: dict[str, list[tuple[Item, str]]] = {} + for p in modified | deleted: + pool[p] = item_records(git_show(repo, merge_base, p) or "") + unmatched_base = {p: list(v) for p, v in pool.items()} + residual_added: list[str] = [] + moved = 0 + new_mods = {os.path.splitext(os.path.basename(p))[0] for p in added} + + def take(p: str, want: str) -> bool: + for idx, (it, text) in enumerate(unmatched_base.get(p, [])): + if normalize(text) == want: + unmatched_base[p].pop(idx) + return True + return False + + for p in sorted(modified): + for it, text in item_records(git_show(repo, head, p) or ""): + if take(p, normalize(text)): + continue + if it.kind == "mod-decl" and it.name in new_mods: + continue + if it.kind == "use" and re.search(r"\buse\s+(" + "|".join(map(re.escape, new_mods)) + r")::\*;", it.name): + continue + residual_added.append(f"{p}: {it.selector} (line {it.start + 1})") + for p in sorted(added): + parent = next((c for c in parent_candidates(p) if c in pool), None) + recs = item_records(git_show(repo, head, p) or "") + for it, text in recs: + if it.kind == "use" and collapse(text) == "use super::*;": + continue + if parent is None: + residual_added.append(f"{p}: {it.selector} (new file with no changed parent module)") + continue + ppath = module_path_of(parent) + deeper = os.path.basename(parent) not in ("lib.rs", "main.rs", "mod.rs") + want = normalize(text) + hit = False + for idx, (bit, btext) in enumerate(unmatched_base[parent]): + if bit.kind != it.kind: + continue + try: + exp, _ = transform_item(btext, bit.kind, ppath, deeper) + except SystemExit: + continue + if normalize(exp) == want: + unmatched_base[parent].pop(idx) + hit = True + moved += 1 + break + if not hit: + residual_added.append(f"{p}: {it.selector} (line {it.start + 1})") + residual_removed = [f"{p}: {it.selector} (base line {it.start + 1})" + for p, v in unmatched_base.items() for it, _ in v] + print(f"moved items: {moved} unmatched at base: {len(residual_removed)} unmatched at head: {len(residual_added)}") + for title, rows in (("Base items not found at head (removed or edited):", residual_removed), + ("Head items that are not a move (added or edited):", residual_added)): + if rows: + print("\n" + title) + for r in rows: + print(" " + r) + return 1 if residual_removed or residual_added else 0 + + +def write_report(out: str, s: dict, repo: str) -> None: + rows = [] + for name, m in s["modules"].items(): + rows.append(f"| `{name}` | {m['code']} | {len(m['items'])} | " + f"{m['counts'].get('vis', 0)} | {m['counts'].get('super', 0)} | " + f"{m['counts'].get('include', 0)} | {m['obs']['tracing_events_without_target']} | " + f"{m['obs']['spans_without_target'] + m['obs']['instrument_attrs']} | " + f"{m['obs']['sql_locators']} | {m['obs']['type_name']} |") + with open(out, "w") as f: + f.write(f"Split of `{s['file']}`: parent {s['parent_code_before']} -> " + f"{s['parent_code_after']} code lines.\n\n") + f.write("| module | code lines | items | vis edits | super edits | include edits | " + "events w/o target | spans/instrument | sql! | type_name |\n|--|--:|--:|--:|--:|--:|--:|--:|--:|--:|\n") + f.write("\n".join(rows) + "\n") + + +def list_items(path: str) -> int: + src = open(path, encoding="utf-8").read() + items, _ = parse_items(src) + lines = src.split("\n") + for it in items: + text = "\n".join(lines[it.lead:it.end + 1]) + cfg = (" " + " ".join(it.cfg)) if it.cfg else "" + print(f"{it.start + 1:6}-{it.end + 1:<6} {code_lines(text):5} {it.selector}{cfg}") + return 0 + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + sub = ap.add_subparsers(dest="cmd", required=True) + a = sub.add_parser("list") + a.add_argument("file") + b = sub.add_parser("apply") + b.add_argument("spec") + b.add_argument("--repo", default=".") + b.add_argument("--dry-run", action="store_true") + b.add_argument("--report") + c = sub.add_parser("verify", help="prove BASE...HEAD is a move made the way `apply` makes it") + c.add_argument("base", nargs="?", default="origin/main") + c.add_argument("--head", default="HEAD") + c.add_argument("--repo", default=".") + args = ap.parse_args() + if args.cmd == "list": + return list_items(args.file) + if args.cmd == "verify": + return verify(args.repo, args.base, args.head) + return apply(args.spec, args.repo, args.dry_run, args.report) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py new file mode 100644 index 000000000..a5fd07a9b --- /dev/null +++ b/scripts/tests/test_split_rust_module.py @@ -0,0 +1,278 @@ +"""split_rust_module.py moves items into child modules without changing what they mean. + +Each test builds a small crate in a temporary git repository, runs `apply`, and checks +the generated text and that `verify` accepts the result. The visibility edits must keep +every item's reach exactly what it was in the parent. +""" + +from __future__ import annotations + +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(SCRIPTS_DIR)) + +import split_rust_module as srm # noqa: E402 + +TOOL = SCRIPTS_DIR / "split_rust_module.py" + +PARENT = textwrap.dedent( + """\ + //! Parent docs. + + use std::collections::HashMap; + use std::fmt; + + mod existing; + + macro_rules! noop { + () => {}; + } + + /// A config. + #[derive(Debug)] + pub struct Config { + pub name: String, + inner: u32, + pub(super) shared: bool, + routes: HashMap>, + } + + struct Tuple(u32, pub String, #[allow(dead_code)] Box u8>); + + impl Config { + pub fn new() -> Self { + noop!(); + Self { name: String::new(), inner: 0, shared: false, routes: HashMap::new() } + } + + fn private_helper(&self) -> u32 { + let s = "fn not_a_member() {}"; + self.inner + } + + pub(super) const LIMIT: u32 = 5; + } + + impl fmt::Display for Config { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.name) + } + } + + impl Holder + where + F: for<'a> Fn(&'a u8), + { + fn call(&self) {} + } + + pub struct Holder { + f: F, + } + + // Parsing. + + fn parse(x: &str) -> u32 { + let _ = super::outer(); + let _ = include_str!("data.txt"); + x.len() as u32 + } + + pub(super) fn reach_up() {} + + #[cfg(feature = "k8s")] + fn k8s_only() {} + + fn stays() {} + + #[cfg(test)] + mod tests; + """ +) + +SPEC = textwrap.dedent( + """\ + file = "crate/src/a/foo.rs" + [[module]] + name = "config" + doc = "The configuration type." + items = ["struct Config", "impl Config", "impl fmt::Display for Config", "struct Tuple"] + [[module]] + name = "parsing" + items = ["fn parse", "fn reach_up", "impl Holder *", "struct Holder"] + [[module]] + name = "k8s" + items = ["fn k8s_only"] + """ +) + + +def git(repo: Path, *args: str) -> str: + return subprocess.run(["git", "-C", str(repo), *args], check=True, capture_output=True, text=True).stdout + + +def run(*args: str, cwd: Path) -> subprocess.CompletedProcess: + return subprocess.run([sys.executable, str(TOOL), *args], cwd=cwd, capture_output=True, text=True) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + src = tmp_path / "crate/src/a" + src.mkdir(parents=True) + (src / "foo.rs").write_text(PARENT) + (tmp_path / "spec.toml").write_text(SPEC) + git(tmp_path, "init", "-q") + git(tmp_path, "-c", "user.email=t@example.com", "-c", "user.name=t", "add", "crate") + git(tmp_path, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qm", "base") + return tmp_path + + +def apply_and_commit(repo: Path) -> None: + r = run("apply", "spec.toml", cwd=repo) + assert r.returncode == 0, r.stderr + git(repo, "add", "crate") + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qm", "split") + + +@pytest.mark.parametrize( + ("vis", "parent", "expected"), + [ + ("", ["a", "foo"], "pub(super) "), + ("pub(self) ", ["a", "foo"], "pub(super) "), + ("pub(super) ", ["a", "foo"], "pub(in crate::a) "), + ("pub(super) ", ["foo"], "pub(crate) "), + ("pub(crate) ", ["a", "foo"], None), + ("pub ", ["a", "foo"], None), + ("pub(in crate::a) ", ["a", "foo"], None), + ("", [], "pub(super) "), + ], +) +def test_visibility_keeps_the_same_reach(vis, parent, expected): + assert srm.map_vis(vis, parent) == expected + + +def test_relative_pub_in_is_refused(): + with pytest.raises(SystemExit): + srm.map_vis("pub(in super::x) ", ["a", "foo"]) + + +@pytest.mark.parametrize( + ("path", "segments"), + [ + ("crates/x/src/lib.rs", []), + ("crates/x/src/a.rs", ["a"]), + ("crates/x/src/a/mod.rs", ["a"]), + ("crates/x/src/a/b.rs", ["a", "b"]), + ], +) +def test_module_path_of(path, segments): + assert srm.module_path_of(path) == segments + + +def test_apply_writes_children_and_keeps_parent_paths(repo): + apply_and_commit(repo) + parent = (repo / "crate/src/a/foo.rs").read_text() + config = (repo / "crate/src/a/foo/config.rs").read_text() + parsing = (repo / "crate/src/a/foo/parsing.rs").read_text() + k8s = (repo / "crate/src/a/foo/k8s.rs").read_text() + + # Parent: declarations after the macro (textual scope), glob re-exports at each + # module's widest visibility, cfg gates carried to the declaration. + assert parent.index("macro_rules! noop") < parent.index("mod config;") + assert "pub use config::*;" in parent + assert "pub use parsing::*;" in parent + assert '#[cfg(feature = "k8s")]\nmod k8s;' in parent + assert '#[cfg(feature = "k8s")]\nuse k8s::*;' in parent + assert "fn stays() {}" in parent and "#[cfg(test)]\nmod tests;" in parent + assert "struct Config" not in parent + + assert config.startswith("//! The configuration type.\n\nuse super::*;\n") + # Fields and inherent-impl members keep the parent's reach; trait impls are untouched. + assert " pub name: String,\n pub(super) inner: u32,\n pub(in crate::a) shared: bool," in config + assert " pub(super) routes: HashMap>," in config + assert "pub(super) struct Tuple(pub(super) u32, pub String, #[allow(dead_code)] pub(super) Box u8>);" in config + assert " pub(super) fn private_helper(&self) -> u32 {" in config + assert ' let s = "fn not_a_member() {}";' in config + assert " pub(in crate::a) const LIMIT: u32 = 5;" in config + assert " fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {" in config + + # A higher-ranked bound is not a trait impl; super:: and include paths go one level deeper. + assert " pub(super) fn call(&self) {}" in parsing + assert " pub(super) f: F," in parsing + assert "// Parsing.\n\npub(super) fn parse(x: &str) -> u32 {" in parsing + assert "super::super::outer()" in parsing + assert 'include_str!("../data.txt")' in parsing + assert "pub(in crate::a) fn reach_up() {}" in parsing + assert '#[cfg(feature = "k8s")]\npub(super) fn k8s_only() {}' in k8s + + +def test_verify_accepts_the_move_and_rejects_an_edit(repo): + apply_and_commit(repo) + ok = run("verify", "HEAD~1", cwd=repo) + assert ok.returncode == 0, ok.stdout + assert "moved items: 9" in ok.stdout + + config = repo / "crate/src/a/foo/config.rs" + config.write_text(config.read_text().replace(" self.inner\n", " self.inner + 1\n")) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 + assert "impl Config" in bad.stdout + + +def test_verify_ignores_rustfmt_rewraps(repo): + apply_and_commit(repo) + parsing = repo / "crate/src/a/foo/parsing.rs" + parsing.write_text(parsing.read_text().replace("pub(super) fn parse(x: &str) -> u32 {", + "pub(super) fn parse(\n x: &str,\n) -> u32 {")) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "fmt") + assert run("verify", "HEAD~2", cwd=repo).returncode == 0 + + +def test_verify_sees_edits_inside_string_literals(repo): + apply_and_commit(repo) + config = repo / "crate/src/a/foo/config.rs" + config.write_text(config.read_text().replace('"fn not_a_member() {}"', '"fn not_a_member(){}"')) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") + assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + + +@pytest.mark.parametrize( + ("items", "message"), + [ + ('["fn missing"]', "matches no item"), + ('["macro_rules! noop"]', "macro_rules"), + ('["mod-decl existing"]', "stay in the parent"), + ('["fn stays", "fn stay*"]', None), + ], +) +def test_spec_errors(repo, items, message): + (repo / "spec.toml").write_text(f'file = "crate/src/a/foo.rs"\n[[module]]\nname = "m"\nitems = {items}\n') + r = run("apply", "spec.toml", "--dry-run", cwd=repo) + if message is None: + assert r.returncode == 0, r.stderr + else: + assert r.returncode != 0 + assert message in r.stderr + + +def test_an_item_cannot_go_to_two_modules(repo): + (repo / "spec.toml").write_text( + 'file = "crate/src/a/foo.rs"\n' + '[[module]]\nname = "one"\nitems = ["fn stays"]\n' + '[[module]]\nname = "two"\nitems = ["fn stay*"]\n' + ) + r = run("apply", "spec.toml", "--dry-run", cwd=repo) + assert r.returncode != 0 and "matched by one and two" in r.stderr + + +def test_module_name_may_not_shadow_an_existing_module(repo): + (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "existing"\nitems = ["fn stays"]\n') + r = run("apply", "spec.toml", "--dry-run", cwd=repo) + assert r.returncode != 0 and "collides" in r.stderr From 1edd3340b194cd3390e7cf12c4de230eb1d55f7f Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 01:27:12 -0400 Subject: [PATCH 02/15] scripts: let split_rust_module.py move members of an inherent impl A large file is often one big inherent impl, so whole-item moves cannot shrink it. A selector of the form 'impl
:: fn ' now moves chosen members into an impl block with the same attributes and header in the child module. The parent's block keeps the rest and disappears once it is empty. The impl's doc comments stay with the parent's block, or go to the first child that receives members when none are left. Trait impls still move whole. verify compares inherent impls member by member, keyed by the impl's attributes and header, so a split impl is proved the same way as a whole item. apply now also refuses a parent file loaded through #[path], whose children would resolve elsewhere, and reports archcheck pattern-ratchet matches each new module would hold, so their baseline entries can move with the code. --- scripts/split_rust_module.py | 424 ++++++++++++++++++++---- scripts/tests/test_split_rust_module.py | 91 ++++- 2 files changed, 451 insertions(+), 64 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 719ad7c7f..27f1f26d3 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -37,7 +37,15 @@ Selectors are fnmatch patterns over ` `; impl blocks are `impl
` with whitespace collapsed (e.g. `impl From for Foo`). +`impl
:: ` selects members of an inherent impl: they move into +an impl block with the same attributes and header in the child, and the parent's +block keeps the rest (or disappears when none are left). Trait impls move whole. Every pattern must match at least one item and every item at most one module. + +Refused, to be done by hand: moving `use`, `mod x;` or `macro_rules!` items (they +stay in the parent, above the new `mod` lines, so children see the macros), relative +`pub(in super::..)` visibility, `#[path]` inside moved items, and a parent file that +is itself loaded through `#[path]`. """ from __future__ import annotations @@ -529,8 +537,124 @@ def observables(text: str) -> dict: # ------------------------------------------------------------------ apply +@dataclass +class Member: + kind: str + name: str + lead: int # absolute line indices + start: int + end: int + + @property + def selector(self) -> str: + return f"{self.kind} {self.name}" + + +@dataclass +class ImplLayout: + body_open: int # line holding the impl's opening `{` + members: list[Member] + close: int # line holding the closing `}` + + +MEMBER_LINE_RE = re.compile(r"^\s+(?P" + VIS + r")" + QUAL + r"(?Pfn|const|type)\b") +MEMBER_MACRO_RE = re.compile(r"^\s+(?P[A-Za-z_][A-Za-z0-9_:]*)\s*!") + + +def is_inherent_header(mheader: str) -> bool: + header = re.sub(r"\bfor\s*<[^<>]*>", "", mheader) + while re.search(r"<[^<>]*>", header): + header = re.sub(r"<[^<>]*>", "", header) + return not re.search(r"\bfor\s+(?!<)", header) + + +def impl_layout(lines: list[str], mlines: list[str], it: Item) -> ImplLayout | None: + """Members of an inherent impl item; None for a trait impl or a one-line impl.""" + body_open, header = None, "" + for i in range(it.start, it.end + 1): + pos = mlines[i].find("{") + if pos >= 0: + header += mlines[i][:pos] + body_open = i + break + header += mlines[i] + "\n" + if body_open is None or not is_inherent_header(header): + return None + if body_open == it.end or not mlines[body_open].rstrip().endswith("{"): + return None + members: list[Member] = [] + i, lead = body_open + 1, None + while i < it.end: + raw, m = lines[i], mlines[i] + s = raw.strip() + if s == "": + i += 1 + continue + if m.strip() == "": + lead = i if lead is None else lead + i += 1 + continue + if s.startswith("#["): + lead = i if lead is None else lead + d = line_depth_delta(m) + i += 1 + while d > 0 and i < it.end: + d += line_depth_delta(mlines[i]) + i += 1 + continue + start, d = i, 0 + while True: + d += line_depth_delta(mlines[i]) + tail = mlines[i].rstrip() + if d == 0 and (tail.endswith("}") or tail.endswith(";")): + break + i += 1 + if i >= it.end: + raise SystemExit(f"line {start + 1}: impl member never ends") + mm = MEMBER_LINE_RE.match(raw) + if mm: + nm = NAME_RE.match(raw[mm.end():]) + kind, name = mm.group("kind"), (nm.group(1) if nm else "?") + else: + mc = MEMBER_MACRO_RE.match(raw) + if not mc: + raise SystemExit(f"line {start + 1}: cannot classify impl member: {raw.strip()[:80]!r}") + kind, name = "macro", mc.group("path") + members.append(Member(kind, name, lead if lead is not None else start, start, i)) + lead = None + i += 1 + return ImplLayout(body_open, members, it.end) + + +def rewrite_super(text: str) -> str: + mt = mask(text) + for m in reversed(list(re.finditer(r"(? list[str]: + """The attribute lines (whole multi-line attributes) in an item's leading trivia.""" + out, k = [], lead + mlines = mask("\n".join(lines[lead:start])).split("\n") + while k < start: + s = lines[k].strip() + if s.startswith("#["): + d = line_depth_delta(mlines[k - lead]) + out.append(lines[k]) + k += 1 + while d > 0 and k < start: + d += line_depth_delta(mlines[k - lead]) + out.append(lines[k]) + k += 1 + continue + k += 1 + return out + + RANK = {"none": -1, "private": 0, "in": 1, "crate": 2, "pub": 3} TYPE_NS = {"struct", "enum", "union", "trait", "type", "mod", "mod-decl"} +DEFS = {"struct", "enum", "union", "trait", "impl", "type", "const", "static", "macro"} def new_vis_rank(orig_vis: str, parent_path: list[str]) -> tuple[int, str]: @@ -555,6 +679,33 @@ def strip_super(sel: str) -> str: return re.sub(r"\bsuper::", "", collapse(sel)) +def impl_block(lines: list[str], it: Item, lay: ImplLayout, mids: list[int], lead_lines: list[str], + child: bool, trailing: bool, parent_path: list[str], deeper: bool, counts: dict) -> str: + """An impl block holding the members `mids` (indices into lay.members), in order.""" + header = "\n".join(lines[it.start:lay.body_open + 1]) + out = list(lead_lines) + out.extend((rewrite_super(header) if child else header).split("\n")) + prev = None + for mi in mids: + m = lay.members[mi] + if prev is not None: + if prev == mi - 1: + out.extend(lines[lay.members[prev].end + 1:m.lead]) + else: + out.append("") + text = "\n".join(lines[m.lead:m.end + 1]) + if child: + text, c = transform_item(text, "macro" if m.kind == "macro" else "fn", parent_path, deeper) + for key, v in c.items(): + counts[key] = counts.get(key, 0) + v + out.extend(text.split("\n")) + prev = mi + if trailing: + out.extend(lines[lay.members[-1].end + 1:lay.close]) # trailing comments in the body + out.append(lines[lay.close]) + return "\n".join(out) + + def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: spec = tomllib.load(open(spec_path, "rb")) rel = spec["file"] @@ -562,21 +713,49 @@ def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: src = open(path, encoding="utf-8").read() items, header_end = parse_items(src) lines = src.split("\n") + mlines = mask(src).split("\n") parent_path = module_path_of(rel) base = os.path.basename(rel) deeper = base not in ("lib.rs", "main.rs", "mod.rs") child_dir = os.path.join(os.path.dirname(path), base[:-3]) if deeper else os.path.dirname(path) + loaded_by_path = path_attribute_users(path) + if loaded_by_path: + raise SystemExit(f"{rel} is loaded through #[path] ({loaded_by_path}); its child modules would " + "not resolve under the directory this tool writes. Remove the attribute first.") mods = spec["module"] names = [m["name"] for m in mods] if len(set(names)) != len(names): raise SystemExit("duplicate module names in spec") + layouts: dict[int, ImplLayout | None] = {} + + def layout(k: int) -> ImplLayout | None: + if k not in layouts: + layouts[k] = impl_layout(lines, mlines, items[k]) if items[k].kind == "impl" else None + return layouts[k] assign: dict[int, str] = {} + massign: dict[tuple[int, int], str] = {} for mod in mods: name = mod["name"] if not re.fullmatch(r"[a-z][a-z0-9_]*", name): raise SystemExit(f"bad module name {name!r}") for pat in mod["items"]: + if " :: " in pat: + ipat, mpat = pat.split(" :: ", 1) + hits = [] + for k, it in enumerate(items): + if it.kind == "impl" and fnmatch.fnmatchcase(it.selector, ipat): + lay = layout(k) + if lay is None: + raise SystemExit(f"{it.selector}: only inherent impls can be split by member") + hits += [(k, mi) for mi, m in enumerate(lay.members) if fnmatch.fnmatchcase(m.selector, mpat)] + if not hits: + raise SystemExit(f"module {name}: pattern {pat!r} matches no impl member") + for km in hits: + if km in massign and massign[km] != name: + raise SystemExit(f"{pat}: member matched by {massign[km]} and {name}") + massign[km] = name + continue hits = [k for k, it in enumerate(items) if fnmatch.fnmatchcase(it.selector, pat)] if not hits: raise SystemExit(f"module {name}: pattern {pat!r} matches no item") @@ -589,43 +768,76 @@ def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: if k in assign and assign[k] != name: raise SystemExit(f"{it.selector} matched by {assign[k]} and {name}") assign[k] = name + split_impls = sorted({k for k, _ in massign}) + for k in split_impls: + if k in assign: + raise SystemExit(f"{items[k].selector}: selected whole and by member") for it in items: if it.kind in TYPE_NS and it.name in names: raise SystemExit(f"module name {it.name!r} collides with {it.selector}") + # Where a split impl's doc comments go: with the members the parent keeps, else with the + # first module (in spec order) that receives members. + lead_owner: dict[int, str | None] = {} + for k in split_impls: + lay = layout(k) + kept = [mi for mi in range(len(lay.members)) if (k, mi) not in massign] + lead_owner[k] = None if kept else next(n for n in names if any(massign.get((k, mi)) == n + for mi in range(len(lay.members)))) + results = {} for mod in mods: name = mod["name"] - ks = [k for k in range(len(items)) if assign.get(k) == name] - if not ks: + chunks, counts, obs, best, sels = [], {}, {k: 0 for k in OBS}, (RANK["none"], ""), [] + for k, it in enumerate(items): + if assign.get(k) == name: + text = "\n".join(lines[it.lead:it.end + 1]).lstrip("\n") + new, c = transform_item(text, it.kind, parent_path, deeper) + for key, v in c.items(): + counts[key] = counts.get(key, 0) + v + for key, v in observables(text).items(): + obs[key] += v + chunks.append(new) + sels.append(it.selector) + if it.kind == "macro": + counts["macro_items"] = counts.get("macro_items", 0) + 1 + best = max(best, (RANK["private"], "")) + elif it.kind != "impl": + best = max(best, new_vis_rank(it.vis, parent_path)) + elif k in split_impls: + lay = layout(k) + mids = [mi for mi in range(len(lay.members)) if massign.get((k, mi)) == name] + if not mids: + continue + lead_lines = (lines[it.lead:it.start] if lead_owner[k] == name + else attr_lines(lines, it.lead, it.start)) + while lead_lines and not lead_lines[0].strip(): + lead_lines = lead_lines[1:] + block = impl_block(lines, it, lay, mids, lead_lines, True, lead_owner[k] == name, + parent_path, deeper, counts) + for mi in mids: + m = lay.members[mi] + for key, v in observables("\n".join(lines[m.lead:m.end + 1])).items(): + obs[key] += v + counts["members"] = counts.get("members", 0) + len(mids) + chunks.append(block) + sels.append(it.selector) + if not chunks: raise SystemExit(f"module {name}: no items") - chunks, counts, obs, best = [], {}, {k: 0 for k in OBS}, (RANK["none"], "") - for k in ks: - it = items[k] - text = "\n".join(lines[it.lead:it.end + 1]).lstrip("\n") - new, c = transform_item(text, it.kind, parent_path, deeper) - for key, v in c.items(): - counts[key] = counts.get(key, 0) + v - for key, v in observables(text).items(): - obs[key] += v - chunks.append(new) - if it.kind == "macro": - counts["macro_items"] = counts.get("macro_items", 0) + 1 - best = max(best, (RANK["private"], "")) - elif it.kind != "impl": - best = max(best, new_vis_rank(it.vis, parent_path)) doc = mod.get("doc", "").strip() body = "" if doc: body += "".join(f"//! {d}\n" if d.strip() else "//!\n" for d in doc.split("\n")) + "\n" body += "use super::*;\n\n" + "\n\n".join(chunks) + "\n" + ks = [k for k in range(len(items)) if assign.get(k) == name or + (k in split_impls and any(massign.get((k, mi)) == name for mi in range(len(layout(k).members))))] cfgs = [set(items[k].cfg) for k in ks] - common_cfg = sorted(set.intersection(*cfgs)) + common_cfg = sorted(set.intersection(*cfgs)) if cfgs else [] reexport = mod.get("reexport", "auto") if best[0] == RANK["none"]: reexport = "none" # only impl blocks: nothing to name use_vis = {RANK["pub"]: "pub ", RANK["crate"]: "pub(crate) ", RANK["in"]: best[1] + " "}.get(best[0], "") - results[name] = dict(items=[items[k].selector for k in ks], text=body, counts=counts, obs=obs, + results[name] = dict(items=sels, text=body, counts=counts, obs=obs, cfg=common_cfg, use_vis=use_vis, reexport=reexport, code=code_lines(body), path=os.path.join(child_dir, name + ".rs")) @@ -643,17 +855,22 @@ def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: uses.append(f"{gate}{r['use_vis']}use {mod['name']}::*;") decl_text = "\n".join(decls) + ("\n\n" + "\n".join(uses) if uses else "") - # parent remainder: kept items in order; keep original spacing between items - # that were adjacent, one blank line where moved items were removed. - keep = [k for k in range(len(items)) if k not in assign] - lead_end = -1 + # parent remainder: kept items in order; keep original spacing between items that were + # adjacent, one blank line where moved items were removed. + keep = [] + for k in range(len(items)): + if k in assign: + continue + if k in split_impls and lead_owner[k] is not None: + continue # every member moved + keep.append(k) + first_def = next((k for k, it in enumerate(items) if it.kind in DEFS), len(items)) + decl_after = -1 for idx, k in enumerate(keep): - if items[k].kind in ("use", "extern crate", "mod-decl"): - lead_end = idx - else: - break - macro_last = max([idx for idx, k in enumerate(keep) if items[k].kind == "macro_rules!"], default=-1) - decl_after = max(lead_end, macro_last) + if k < first_def and items[k].kind in ("use", "extern crate", "mod-decl"): + decl_after = idx + if items[k].kind == "macro_rules!": + decl_after = max(decl_after, idx) out = lines[:header_end] while out and not out[-1].strip(): out.pop() @@ -672,7 +889,13 @@ def sep() -> None: out.extend(lines[items[prev].end + 1:it.lead]) # original gap (blank lines) else: sep() - out.extend(lines[it.lead:it.end + 1]) + if k in split_impls: + lay = layout(k) + kept_m = [mi for mi in range(len(lay.members)) if (k, mi) not in massign] + out.extend(impl_block(lines, it, lay, kept_m, lines[it.lead:it.start], False, True, + parent_path, deeper, {}).split("\n")) + else: + out.extend(lines[it.lead:it.end + 1]) if idx == decl_after: sep() out.extend(decl_text.split("\n")) @@ -684,7 +907,7 @@ def sep() -> None: trailing.pop() if trailing: sep() - out.extend(t for t in trailing) + out.extend(trailing) parent_text = "\n".join(out).rstrip("\n") + "\n" # self-check: re-parse everything and compare item sequences @@ -703,11 +926,13 @@ def sep() -> None: extra = set(got) - set(want) raise SystemExit(f"self-check failed for parent: missing {missing}, extra {extra}") + ratchets = ratchet_hits(repo, results) summary = { "file": rel, "parent_code_before": code_lines(src), "parent_code_after": code_lines(parent_text), "modules": {n: {k: v for k, v in r.items() if k != "text"} for n, r in results.items()}, + "pattern_ratchet_lines_moved": ratchets, "kept": [items[k].selector for k in keep if items[k].kind not in ("use", "mod-decl")], } if not dry: @@ -726,11 +951,43 @@ def sep() -> None: return 0 +def path_attribute_users(path: str) -> str: + """A `#[path = "..."]` in the crate that names this file, if any.""" + crate_src = path.split(os.sep + "src" + os.sep, 1)[0] + os.sep + "src" + target = os.path.realpath(path) + for d, _, files in os.walk(crate_src): + for f in files: + if not f.endswith(".rs"): + continue + fp = os.path.join(d, f) + for m in re.finditer(r'#\s*\[\s*path\s*=\s*"([^"]+)"\s*\]', open(fp, encoding="utf-8").read()): + if os.path.realpath(os.path.join(d, m.group(1))) == target: + return os.path.relpath(fp, crate_src) + return "" + + +def ratchet_hits(repo: str, results: dict) -> dict: + """Lines matching an archcheck pattern_ratchet that a module would now hold. A ratchet + counts lines per file, so a moved match needs its baseline entry moved too.""" + arch = os.path.join(repo, "ARCHITECTURE.toml") + if not os.path.exists(arch): + return {} + rules = tomllib.load(open(arch, "rb")).get("rules", {}).get("pattern_ratchet", {}) + hits: dict = {} + for inst, cfg in rules.items(): + pats = [re.compile(p) for p in cfg.get("patterns", [])] + for name, r in results.items(): + n = sum(1 for ln in r["text"].split("\n") if any(p.search(ln) for p in pats)) + if n: + hits.setdefault(inst, {})[name] = n + return hits + + # ------------------------------------------------------------------ verify def normalize(text: str) -> str: - """Whitespace- and trailing-comma-insensitive form of an item, for comparing a move - across a rustfmt run. String and char literals are kept byte for byte.""" + """Whitespace- and trailing-comma-insensitive form of code, for comparing a move across a + rustfmt run. String and char literals are kept byte for byte.""" m = mask(text, comment="c") out: list[str] = [] for ch, mk in zip(text, m): @@ -754,10 +1011,39 @@ def git_show(repo: str, rev: str, path: str) -> str | None: return r.stdout if r.returncode == 0 else None -def item_records(src: str) -> list[tuple[Item, str]]: +@dataclass +class Record: + """One unit a move must conserve: a whole item, one member of an inherent impl (keyed by + the impl's attributes and header), or the non-attribute trivia of an inherent impl.""" + kind: str # "item" | "member" | "impl-trivia" + item_kind: str + selector: str + key: str # normalized impl attrs + header for members, else "" + text: str + line: int + + +def records(src: str) -> list[Record]: items, _ = parse_items(src) lines = src.split("\n") - return [(it, "\n".join(lines[it.lead:it.end + 1]).lstrip("\n")) for it in items] + mlines = mask(src).split("\n") + out: list[Record] = [] + for it in items: + lay = impl_layout(lines, mlines, it) if it.kind == "impl" else None + if lay is None or not lay.members: + text = "\n".join(lines[it.lead:it.end + 1]).lstrip("\n") + out.append(Record("item", it.kind, it.selector, "", text, it.start)) + continue + attrs = attr_lines(lines, it.lead, it.start) + header = "\n".join(attrs + lines[it.start:lay.body_open + 1]) + trivia = [ln for ln in lines[it.lead:it.start] if ln.strip() and ln not in attrs] + trivia += lines[lay.members[-1].end + 1:lay.close] + if any(t.strip() for t in trivia): + out.append(Record("impl-trivia", "impl", it.selector, "", "\n".join(trivia), it.start)) + for m in lay.members: + out.append(Record("member", m.kind, f"{it.selector} :: {m.selector}", header, + "\n".join(lines[m.lead:m.end + 1]), m.start)) + return out def parent_candidates(child: str) -> list[str]: @@ -769,67 +1055,79 @@ def parent_candidates(child: str) -> list[str]: def verify(repo: str, base: str, head: str) -> int: - """Prove BASE...HEAD only moves top-level items into child modules the way `apply` does.""" + """Prove BASE...HEAD only moves items (or inherent-impl members) into child modules the + way `apply` does, plus the `mod`/`use` declarations `apply` writes.""" merge_base = git(repo, "merge-base", base, head).strip() status = [ln.split("\t") for ln in git(repo, "diff", "--name-status", "--no-renames", f"{merge_base}..{head}", "--", "*.rs").splitlines()] added = {p for st, p in status if st == "A"} modified = {p for st, p in status if st == "M"} deleted = {p for st, p in status if st == "D"} - pool: dict[str, list[tuple[Item, str]]] = {} - for p in modified | deleted: - pool[p] = item_records(git_show(repo, merge_base, p) or "") - unmatched_base = {p: list(v) for p, v in pool.items()} + pool = {p: records(git_show(repo, merge_base, p) or "") for p in modified | deleted} + new_mods = {os.path.splitext(os.path.basename(p))[0] for p in added} residual_added: list[str] = [] moved = 0 - new_mods = {os.path.splitext(os.path.basename(p))[0] for p in added} - def take(p: str, want: str) -> bool: - for idx, (it, text) in enumerate(unmatched_base.get(p, [])): - if normalize(text) == want: - unmatched_base[p].pop(idx) + def take(p: str, rec: Record, want: str, key: str) -> bool: + for idx, b in enumerate(pool.get(p, [])): + if b.kind == rec.kind and normalize(b.key) == key and normalize(b.text) == want: + pool[p].pop(idx) return True return False + trivia_head: list[tuple[str, Record]] = [] for p in sorted(modified): - for it, text in item_records(git_show(repo, head, p) or ""): - if take(p, normalize(text)): + for rec in records(git_show(repo, head, p) or ""): + if rec.kind == "impl-trivia": + trivia_head.append((p, rec)) + continue + if take(p, rec, normalize(rec.text), normalize(rec.key)): continue - if it.kind == "mod-decl" and it.name in new_mods: + if rec.item_kind == "mod-decl" and rec.selector.split(" ", 1)[1] in new_mods: continue - if it.kind == "use" and re.search(r"\buse\s+(" + "|".join(map(re.escape, new_mods)) + r")::\*;", it.name): + if rec.item_kind == "use" and new_mods and re.search( + r"\buse\s+(" + "|".join(map(re.escape, new_mods)) + r")::\*;", rec.text): continue - residual_added.append(f"{p}: {it.selector} (line {it.start + 1})") + residual_added.append(f"{p}: {rec.selector} (line {rec.line + 1})") for p in sorted(added): parent = next((c for c in parent_candidates(p) if c in pool), None) - recs = item_records(git_show(repo, head, p) or "") - for it, text in recs: - if it.kind == "use" and collapse(text) == "use super::*;": + for rec in records(git_show(repo, head, p) or ""): + if rec.kind == "item" and rec.item_kind == "use" and collapse(rec.text) == "use super::*;": + continue + if rec.kind == "impl-trivia": + trivia_head.append((p, rec)) continue if parent is None: - residual_added.append(f"{p}: {it.selector} (new file with no changed parent module)") + residual_added.append(f"{p}: {rec.selector} (new file with no changed parent module)") continue ppath = module_path_of(parent) deeper = os.path.basename(parent) not in ("lib.rs", "main.rs", "mod.rs") - want = normalize(text) + want, key = normalize(rec.text), normalize(rec.key) hit = False - for idx, (bit, btext) in enumerate(unmatched_base[parent]): - if bit.kind != it.kind: + for idx, b in enumerate(pool[parent]): + if b.kind != rec.kind or b.item_kind != rec.item_kind: + continue + if b.kind == "member" and normalize(rewrite_super(b.key)) != key: continue try: - exp, _ = transform_item(btext, bit.kind, ppath, deeper) + tkind = b.item_kind if b.kind == "item" else ("macro" if b.item_kind == "macro" else "fn") + exp, _ = transform_item(b.text, tkind, ppath, deeper) except SystemExit: continue if normalize(exp) == want: - unmatched_base[parent].pop(idx) + pool[parent].pop(idx) hit = True moved += 1 break if not hit: - residual_added.append(f"{p}: {it.selector} (line {it.start + 1})") - residual_removed = [f"{p}: {it.selector} (base line {it.start + 1})" - for p, v in unmatched_base.items() for it, _ in v] - print(f"moved items: {moved} unmatched at base: {len(residual_removed)} unmatched at head: {len(residual_added)}") + residual_added.append(f"{p}: {rec.selector} (line {rec.line + 1})") + for p, rec in trivia_head: # an impl's doc comments may land with any of its blocks + src_p = next((c for c in [p] + parent_candidates(p) if c in pool), None) + cands = [c for c in ([src_p] if src_p else []) + list(pool) if c] + if not any(take(c, rec, normalize(rec.text), "") for c in dict.fromkeys(cands)): + residual_added.append(f"{p}: doc/comments of {rec.selector} (line {rec.line + 1})") + residual_removed = [f"{p}: {b.selector} (base line {b.line + 1})" for p, v in pool.items() for b in v] + print(f"moved: {moved} unmatched at base: {len(residual_removed)} unmatched at head: {len(residual_added)}") for title, rows in (("Base items not found at head (removed or edited):", residual_removed), ("Head items that are not a move (added or edited):", residual_added)): if rows: diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index a5fd07a9b..7c4a07865 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -216,7 +216,7 @@ def test_verify_accepts_the_move_and_rejects_an_edit(repo): apply_and_commit(repo) ok = run("verify", "HEAD~1", cwd=repo) assert ok.returncode == 0, ok.stdout - assert "moved items: 9" in ok.stdout + assert "moved: 11 unmatched at base: 0 unmatched at head: 0" in ok.stdout config = repo / "crate/src/a/foo/config.rs" config.write_text(config.read_text().replace(" self.inner\n", " self.inner + 1\n")) @@ -276,3 +276,92 @@ def test_module_name_may_not_shadow_an_existing_module(repo): (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "existing"\nitems = ["fn stays"]\n') r = run("apply", "spec.toml", "--dry-run", cwd=repo) assert r.returncode != 0 and "collides" in r.stderr + + +MEMBER_SPEC = textwrap.dedent( + """\ + file = "crate/src/a/foo.rs" + [[module]] + name = "helpers" + doc = "Config helpers." + items = ["impl Config :: fn private_helper", "impl Config :: const LIMIT"] + """ +) + + +def test_members_of_an_inherent_impl_move_into_their_own_impl_block(repo): + (repo / "spec.toml").write_text(MEMBER_SPEC) + apply_and_commit(repo) + parent = (repo / "crate/src/a/foo.rs").read_text() + helpers = (repo / "crate/src/a/foo/helpers.rs").read_text() + assert "impl Config {\n pub fn new() -> Self {" in parent + assert "private_helper" not in parent and "LIMIT" not in parent + assert helpers == textwrap.dedent( + """\ + //! Config helpers. + + use super::*; + + impl Config { + pub(super) fn private_helper(&self) -> u32 { + let s = "fn not_a_member() {}"; + self.inner + } + + pub(in crate::a) const LIMIT: u32 = 5; + } + """ + ) + # No re-export: an impl block has nothing to name. + assert "use helpers::*;" not in parent and "mod helpers;" in parent + ok = run("verify", "HEAD~1", cwd=repo) + assert ok.returncode == 0, ok.stdout + + +def test_an_impl_split_across_modules_keeps_its_attributes_on_every_block(repo): + (repo / "crate/src/a/foo.rs").write_text(PARENT.replace( + "impl Config {", "/// Config methods.\n#[allow(dead_code)]\nimpl Config {")) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "attrs") + (repo / "spec.toml").write_text(textwrap.dedent( + """\ + file = "crate/src/a/foo.rs" + [[module]] + name = "one" + items = ["impl Config :: fn new"] + [[module]] + name = "two" + items = ["impl Config :: *"] + """ + ).replace('"impl Config :: *"', '"impl Config :: fn private_helper", "impl Config :: const LIMIT"')) + apply_and_commit(repo) + one = (repo / "crate/src/a/foo/one.rs").read_text() + two = (repo / "crate/src/a/foo/two.rs").read_text() + parent = (repo / "crate/src/a/foo.rs").read_text() + assert "impl Config" not in parent.replace("impl fmt::Display for Config", "") + assert "/// Config methods.\n#[allow(dead_code)]\nimpl Config {" in one + assert "#[allow(dead_code)]\nimpl Config {" in two and "/// Config methods." not in two + assert run("verify", "HEAD~1", cwd=repo).returncode == 0 + + +def test_trait_impl_members_cannot_be_split(repo): + (repo / "spec.toml").write_text( + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "m"\nitems = ["impl fmt::Display for Config :: fn fmt"]\n') + r = run("apply", "spec.toml", "--dry-run", cwd=repo) + assert r.returncode != 0 and "only inherent impls" in r.stderr + + +def test_verify_rejects_a_member_edited_in_its_new_block(repo): + (repo / "spec.toml").write_text(MEMBER_SPEC) + apply_and_commit(repo) + helpers = repo / "crate/src/a/foo/helpers.rs" + helpers.write_text(helpers.read_text().replace("const LIMIT: u32 = 5;", "const LIMIT: u32 = 6;")) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "LIMIT" in bad.stdout + + +def test_a_file_loaded_through_a_path_attribute_is_refused(repo): + (repo / "crate/src/lib.rs").write_text('#[path = "a/foo.rs"]\nmod renamed;\n') + (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "m"\nitems = ["fn stays"]\n') + r = run("apply", "spec.toml", "--dry-run", cwd=repo) + assert r.returncode != 0 and "#[path]" in r.stderr From d17e5f3f652e79fb7555c35fa0b56856f1ab46de Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 01:42:31 -0400 Subject: [PATCH 03/15] scripts: split_rust_module.py fixes from a workspace-wide compile run Moving every item of 19 large files into child modules compiled and passed clippy, doctests and the affected crates' tests apart from these cases, now handled: - `const _` takes no visibility; giving it pub(super) is a warning. - A macro_rules! that emits pub(super) or pub(self) means something else when it is expanded one module deeper, so apply refuses to move code that invokes it. - verify ignores the trailing comma rustfmt adds inside generics when a visibility pushes a signature past the line width. - The #[path] check ignores attributes written in comments. --- scripts/split_rust_module.py | 25 ++++++++++++++++---- scripts/tests/test_split_rust_module.py | 31 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 27f1f26d3..36aa8a831 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -452,8 +452,8 @@ def vis_edit(pos: int) -> None: # 1. the item's own visibility: first code offset after attributes p0 = skip_ws_attrs(mt, 0) - if kind not in ("impl", "macro"): - vis_edit(p0) + if kind not in ("impl", "macro") and not re.compile(VIS + r"const\s+_\s*:").match(mt, p0): + vis_edit(p0) # `const _` takes no visibility # 2. members if kind in ("struct", "union"): km = re.compile(VIS + QUAL + r"(struct|union)\s+").match(mt, p0) @@ -768,6 +768,19 @@ def layout(k: int) -> ImplLayout | None: if k in assign and assign[k] != name: raise SystemExit(f"{it.selector} matched by {assign[k]} and {name}") assign[k] = name + # A macro_rules! that stays in the parent and emits a relative visibility (`pub(super)`) + # would mean something else when expanded one module deeper. + rel_vis_macros = [it.name for it in items if it.kind == "macro_rules!" and re.search( + r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self))\b", mask("\n".join(lines[it.start:it.end + 1])))] + if rel_vis_macros: + moved_text = "\n".join("\n".join(mlines[items[k].start:items[k].end + 1]) for k in assign) + moved_text += "\n".join("\n".join(mlines[layout(k).members[mi].start:layout(k).members[mi].end + 1]) + for k, mi in massign) + for name in rel_vis_macros: + if re.search(r"(? str: if not f.endswith(".rs"): continue fp = os.path.join(d, f) - for m in re.finditer(r'#\s*\[\s*path\s*=\s*"([^"]+)"\s*\]', open(fp, encoding="utf-8").read()): + text = open(fp, encoding="utf-8").read() + masked = mask(text) + for m in re.finditer(r'#\s*\[\s*path\s*=\s*"([^"]+)"\s*\]', text): + if masked[m.start()] != "#": + continue # inside a comment if os.path.realpath(os.path.join(d, m.group(1))) == target: return os.path.relpath(fp, crate_src) return "" @@ -996,7 +1013,7 @@ def normalize(text: str) -> str: elif ch.isspace(): continue else: - if mk != "c" and ch in CLOSE and out and out[-1] == ",": + if mk != "c" and (ch in CLOSE or ch == ">") and out and out[-1] == ",": out.pop() out.append(ch) return "".join(out) diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index 7c4a07865..8988a6cc7 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -365,3 +365,34 @@ def test_a_file_loaded_through_a_path_attribute_is_refused(repo): (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "m"\nitems = ["fn stays"]\n') r = run("apply", "spec.toml", "--dry-run", cwd=repo) assert r.returncode != 0 and "#[path]" in r.stderr + + +def test_const_underscore_takes_no_visibility(repo): + src = repo / "crate/src/a/foo.rs" + src.write_text(src.read_text() + "\nconst _: () = assert!(1 < 2);\n") + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "assert") + (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "m"\nitems = ["const _", "fn stays"]\n') + apply_and_commit(repo) + m = (repo / "crate/src/a/foo/m.rs").read_text() + assert "\nconst _: () = assert!(1 < 2);" in m and "pub(super) fn stays" in m + assert run("verify", "HEAD~1", cwd=repo).returncode == 0 + + +def test_moved_callers_of_a_macro_that_emits_relative_visibility_are_refused(repo): + src = repo / "crate/src/a/foo.rs" + src.write_text(src.read_text().replace( + "macro_rules! noop {\n () => {};\n}", + "macro_rules! noop {\n () => {};\n}\n\nmacro_rules! helper {\n ($n:ident) => {\n pub(super) fn $n() {}\n };\n}\n" + ) + "\nimpl Holder {\n helper!(made);\n}\n") + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "macro") + (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "m"\nitems = ["impl Holder"]\n') + r = run("apply", "spec.toml", "--dry-run", cwd=repo) + assert r.returncode != 0 and "macro_rules! helper" in r.stderr + + +def test_verify_ignores_a_trailing_comma_rustfmt_adds_in_generics(repo): + apply_and_commit(repo) + parsing = repo / "crate/src/a/foo/parsing.rs" + parsing.write_text(parsing.read_text().replace("impl Holder", "impl<\n F,\n> Holder")) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "fmt") + assert run("verify", "HEAD~2", cwd=repo).returncode == 0 From a71335c05f876c94b99c4d95db4fbde858d3a5a4 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 01:51:17 -0400 Subject: [PATCH 04/15] scripts: make split_rust_module.py verify compare file headers File headers (//! docs and #![...] inner attributes) are not items, so verify did not compare them, and an added #![cfg(...)] or #![allow(...)] would have passed. A modified file's header must now equal its base, and a new child module's header may hold only //! docs. --- scripts/split_rust_module.py | 12 ++++++++++++ scripts/tests/test_split_rust_module.py | 16 ++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 36aa8a831..8b81ab523 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -1092,6 +1092,18 @@ def take(p: str, rec: Record, want: str, key: str) -> bool: return True return False + # File headers (`//!` docs, `#![...]` inner attributes) are not items, so compare them + # directly: unchanged in modified files, docs only in new ones. + for p in sorted(modified): + b_src, h_src = git_show(repo, merge_base, p) or "", git_show(repo, head, p) or "" + b_end, h_end = parse_items(b_src)[1], parse_items(h_src)[1] + if normalize("\n".join(b_src.split("\n")[:b_end])) != normalize("\n".join(h_src.split("\n")[:h_end])): + residual_added.append(f"{p}: file header (//! docs or #![...] attributes) changed") + for p in sorted(added): + h_src = git_show(repo, head, p) or "" + h_end = parse_items(h_src)[1] + if any(ln.strip() and not ln.strip().startswith("//!") for ln in h_src.split("\n")[:h_end]): + residual_added.append(f"{p}: new file header has more than //! docs") trivia_head: list[tuple[str, Record]] = [] for p in sorted(modified): for rec in records(git_show(repo, head, p) or ""): diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index 8988a6cc7..db98d92e5 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -396,3 +396,19 @@ def test_verify_ignores_a_trailing_comma_rustfmt_adds_in_generics(repo): parsing.write_text(parsing.read_text().replace("impl Holder", "impl<\n F,\n> Holder")) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "fmt") assert run("verify", "HEAD~2", cwd=repo).returncode == 0 + + +@pytest.mark.parametrize( + ("path", "old", "new"), + [ + ("crate/src/a/foo.rs", "//! Parent docs.\n", "//! Parent docs.\n#![allow(dead_code)]\n"), + ("crate/src/a/foo/config.rs", "//! The configuration type.\n", "#![cfg(test)]\n"), + ], +) +def test_verify_rejects_header_edits(repo, path, old, new): + apply_and_commit(repo) + f = repo / path + f.write_text(f.read_text().replace(old, new, 1)) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "header") + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "header" in bad.stdout From 2f31d02b06f733d65b40e9467005246dd291a6fc Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 02:11:13 -0400 Subject: [PATCH 05/15] scripts: make split_rust_module.py verify regenerate the split, and harden apply Review found that verify's item matching accepted changes that are not moves: a dropped mod or re-export line, an import added next to the generated ones, a header edit, an edit a whitespace-insensitive comparison erases ((x,) to (x), a newline inside a string), doc comments swapped between impls, and items reordered in the parent. verify now proves something simpler and stronger: it derives the spec from the diff (which items each new child module holds), regenerates the split from the merge base with apply, and compares every parent and child file byte for byte after rustfmt. Any other Rust change, a symlink, or a diff with no split in it fails. apply fixes from the same review and from a rustc-built test crate: - Pick the glob re-export's visibility from each item's reach, so pub(in crate::a) items are no longer re-exported more narrowly, and re-export modules of macro invocations at pub, which a glob clamps. - Count angle brackets only directly inside a struct body and skip const-generic blocks when finding a body or impl brace, so fields after [u8; 1 << 3] and impls with where T: Tr<{ 1 + 1 }> are widened. - Match extern "C" members, treat src/bin/*.rs as crate roots and a nested main.rs as an ordinary module, keep cfg text byte for byte on the new mod lines, rewrite super:: in attributes copied onto split impl blocks, and rebase plain include! paths. - Refuse instead of guessing: a macro name defined twice, macro chains that emit pub(super), nested relative visibility, inline modules that name super, self::super, super:: inside string literals, other include arguments, keyword or import-colliding module names, and an attribute sharing a line with its item. - glob_import = false omits use super::*; for a module that needs nothing from its parent. The reexport option is gone, so a dropped re-export cannot pass verify. - The #[path] scan stays inside the crate. Tests: 63, including rejection cases for each false accept above and a compile oracle that builds and runs a small crate with rustc before and after a split, with callers at every visibility boundary. --- scripts/split_rust_module.py | 700 +++++++++++++++--------- scripts/tests/test_split_rust_module.py | 379 ++++++++++++- 2 files changed, 805 insertions(+), 274 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 8b81ab523..23b0f97ff 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -13,9 +13,6 @@ pub(super) -> pub(in ) (pub(crate) if that is the crate root) pub(crate), pub, pub(in crate::..) unchanged * `super::` paths in moved code gain one `super::` (the code is one module deeper). -* Relative `include_str!`/`include_bytes!` paths gain `../` when the children - live one directory deeper than the parent file (parent is `foo.rs`). - Each new module starts with `use super::*;`, so the parent's imports, traits and sibling items resolve in moved code as before. @@ -23,9 +20,17 @@ instead of rebasing a split by hand. Usage: - split_rust_module.py list FILE # items and selectors, for writing a spec + split_rust_module.py list FILE # items, impl members and their selectors split_rust_module.py apply SPEC.toml [--repo ROOT] [--report OUT.md] [--dry-run] - split_rust_module.py verify [BASE] [--head REV] # prove BASE...HEAD is such a move + split_rust_module.py verify [BASE] [--head REV] [--print-spec] + +`verify` proves that BASE...HEAD's Rust changes are exactly what `apply` generates: it +derives the spec from the diff (which items each new child module holds), regenerates +from the merge base, and compares every parent and child file byte for byte after +rustfmt. Any other Rust change, a missing or altered `mod`/`use` line, a dropped item, an +edit inside a moved item or a comment, or a symlink fails it. It checks the generator, +not the generator's semantics: those rest on the edits listed above, the refusals below, +and compiling and testing the result. Spec (TOML): file = "crates/libs/x/src/foo.rs" @@ -33,7 +38,8 @@ name = "bar" doc = "What bar holds." # one `//!` line at the top of the new file items = ["fn parse_*", "struct Config", "impl Config", "impl Display for Config"] - # optional: reexport = "auto" | "none" | "test" (default auto) + # optional: glob_import = false (omit `use super::*;` when the module needs nothing + # from its parent, which `-D warnings` would otherwise reject as unused) Selectors are fnmatch patterns over ` `; impl blocks are `impl
` with whitespace collapsed (e.g. `impl From for Foo`). @@ -42,10 +48,21 @@ block keeps the rest (or disappears when none are left). Trait impls move whole. Every pattern must match at least one item and every item at most one module. -Refused, to be done by hand: moving `use`, `mod x;` or `macro_rules!` items (they -stay in the parent, above the new `mod` lines, so children see the macros), relative -`pub(in super::..)` visibility, `#[path]` inside moved items, and a parent file that -is itself loaded through `#[path]`. +* Relative `include!`/`include_str!`/`include_bytes!` paths gain `../` when the + children live one directory deeper than the parent file (parent is `foo.rs`). + +Refused, to be done by hand (the tool stops rather than guess): +* moving `use`, `mod x;`, `extern` blocks or `macro_rules!` items (macros stay in the + parent, above the new `mod` lines, so children still see them); +* a parent file loaded through `#[path]`, or a `#[path]` inside moved code; +* moved code that invokes a parent macro emitting `pub(super)`/`pub(self)` (directly or + through another macro), or a macro name defined twice; +* relative visibility the edits cannot remap: `pub(in super::..)`, a `pub(super)` + nested inside a moved item, an inline module naming `super`, `self::super`; +* a `super::` path inside a string literal (e.g. an attribute argument), and include + arguments other than a plain string literal; +* a module name that is a keyword or collides with an item or import in the parent; +* an attribute on the same line as its item (so items are never mis-split). """ from __future__ import annotations @@ -150,14 +167,17 @@ def blank(a: int, b: int, ch: str) -> None: # ------------------------------------------------------------------- items VIS = r"(?:pub(?:\s*\(\s*(?:crate|self|super|in\s+[A-Za-z0-9_:\s]+)\s*\))?\s+)?" -QUAL = r"(?:(?:const|async|unsafe|default|extern(?:\s+\"[^\"]*\")?)\s+)*" +QUAL = r"(?:(?:const|async|unsafe|default|extern(?:\s+(?:\"[^\"]*\"|x+))?)\s+)*" ITEM_RE = re.compile( r"^(?P" + VIS + r")" + QUAL + r"(?Pfn|struct|enum|union|trait|impl|type|const|static|mod|use|extern\s+crate|auto\s+trait)\b|(?Pmacro_rules!)" ) NAME_RE = re.compile(r"\s*(?:r#)?([A-Za-z_][A-Za-z0-9_]*)") MACRO_CALL_RE = re.compile(r"^(?P[A-Za-z_][A-Za-z0-9_:]*)\s*!") -UNMOVABLE = {"use", "extern crate"} +UNMOVABLE = {"use", "extern crate", "extern-block"} +KEYWORDS = set("""as async await break const continue crate dyn else enum extern false fn for if impl in let +loop match mod move mut pub ref return self static struct super trait true type union unsafe use where while +abstract become box do final gen macro override priv try typeof unsized virtual yield""".split()) @dataclass @@ -237,7 +257,7 @@ def parse_items(src: str) -> tuple[list[Item], int]: continue if raw[0].isspace(): raise SystemExit(f"line {i + 1}: top-level code not in column 0 (run rustfmt first)") - if s.startswith("#["): + if s.startswith("#[") and not attr_then_code(m): if lead is None: lead = i d = line_depth_delta(m) @@ -246,6 +266,8 @@ def parse_items(src: str) -> tuple[list[Item], int]: d += line_depth_delta(mlines[i]) i += 1 continue + if s.startswith("#["): + raise SystemExit(f"line {i + 1}: an attribute and an item share a line; put the item on its own line") # item start start = i d = 0 @@ -262,26 +284,42 @@ def parse_items(src: str) -> tuple[list[Item], int]: text_m = "\n".join(mlines[start:end + 1]) it = classify(lines, mlines, start, end, text_m) it.lead = lead if lead is not None else start - it.cfg = [collapse(x) for x in attrs_of(lines, it.lead, start) if collapse(x).startswith("#[cfg(")] + it.cfg = [x for x in attrs_of(lines, it.lead, start) if re.match(r"#\[\s*cfg\s*\(", x)] items.append(it) lead = None return items, header_end +def attr_then_code(mline: str) -> bool: + """Whether a masked line holds a complete `#[...]` followed by more code.""" + s = mline.lstrip() + d = 0 + for k, ch in enumerate(s): + if ch == "[": + d += 1 + elif ch == "]": + d -= 1 + if d == 0: + return bool(s[k + 1:].strip()) + return False + + def attrs_of(lines: list[str], lead: int, start: int) -> list[str]: - out, cur = [], None - for k in range(lead, start): - s = lines[k].strip() - if s.startswith("#["): - cur = s - if cur.count("[") <= cur.count("]"): - out.append(cur) - cur = None - elif cur is not None: - cur += " " + s - if cur.count("[") <= cur.count("]"): - out.append(cur) - cur = None + """Attributes in an item's leading trivia, each with its exact original text.""" + out, k = [], lead + mlines = mask("\n".join(lines[lead:start])).split("\n") + while k < start: + if lines[k].strip().startswith("#["): + d = line_depth_delta(mlines[k - lead]) + group = [lines[k]] + k += 1 + while d > 0 and k < start: + d += line_depth_delta(mlines[k - lead]) + group.append(lines[k]) + k += 1 + out.append("\n".join(group)) + continue + k += 1 return out @@ -297,17 +335,19 @@ def classify(lines, mlines, start, end, text_m) -> Item: rest = first[mm.end():] if kind == "impl": mtext = "\n".join(mlines[start:end + 1]) - cut = mtext.find("{") + cut = find_brace(mtext, 0) header = collapse("\n".join(lines[start:end + 1])[: cut if cut >= 0 else None]) name = re.sub(r"^(?:unsafe\s+)?impl\s*", "", header).strip() return Item("impl", name, start, end, start, vis) if kind in ("use", "extern crate"): return Item(kind, collapse(" ".join(lines[start:end + 1]))[:120], start, end, start, vis) - nm = NAME_RE.match(rest) + nm = NAME_RE.match(re.sub(r"^\s*mut\s+", " ", rest) if kind == "static" else rest) name = nm.group(1) if nm else "?" if kind == "mod": kind = "mod" if text_m.rstrip().endswith("}") else "mod-decl" return Item(kind, name, start, end, start, vis) + if re.match(r"^(?:unsafe\s+)?extern\s+(?:\"[^\"]*\"\s*)?\{", first): + return Item("extern-block", collapse(first.rstrip("{ ")), start, end, start, "") mc = MACRO_CALL_RE.match(first.strip()) if mc: return Item("macro", mc.group("path"), start, end, start, "") @@ -316,18 +356,30 @@ def classify(lines, mlines, start, end, text_m) -> Item: # ----------------------------------------------------------- visibility map -def module_path_of(rel_file: str) -> list[str]: - """Module path segments of a file inside a crate's src/ (crate root -> []).""" +def file_layout(rel_file: str) -> tuple[list[str], bool]: + """(module path inside its crate, whether children live beside the file). + + Children of a crate root (`src/lib.rs`, `src/main.rs`, `src/bin/x.rs`, + `src/bin/x/main.rs`) and of a `mod.rs` sit in the same directory; children of + `foo.rs` sit in `foo/`. + """ p = rel_file.replace(os.sep, "/") - if "/src/" not in p: - raise SystemExit(f"{rel_file}: expected a path under /src/") - tail = p.split("/src/", 1)[1] - parts = tail[:-3].split("/") if tail.endswith(".rs") else tail.split("/") - if parts[-1] in ("lib", "main", "mod"): - parts = parts[:-1] - if parts and parts[0] == "bin": # src/bin/x.rs is its own crate root + if "/src/" not in p or not p.endswith(".rs"): + raise SystemExit(f"{rel_file}: expected a .rs file under /src/") + parts = p.split("/src/", 1)[1][:-3].split("/") + if parts in (["lib"], ["main"]): + return [], True + if parts[0] == "bin": + if len(parts) == 2 or (len(parts) == 3 and parts[2] == "main"): + return [], True parts = parts[2:] - return parts + if parts[-1] == "mod": + return parts[:-1], True + return parts, False + + +def module_path_of(rel_file: str) -> list[str]: + return file_layout(rel_file)[0] def map_vis(vis: str, parent_path: list[str]) -> str | None: @@ -373,9 +425,36 @@ def skip_ws_attrs(mt: str, p: int) -> int: return p +def find_brace(mt: str, p: int) -> int: + """Offset of the first `{` at nesting depth 0 from p, skipping (), [] and generic <> + (the `>` of `->`/`=>` excepted); -1 if a depth-0 `;` comes first.""" + depth = angle = 0 + n = len(mt) + while p < n: + ch = mt[p] + if ch in "([": + depth += 1 + elif ch in ")]": + depth -= 1 + elif ch == "<" and depth == 0: + angle += 1 + elif ch == ">" and depth == 0 and angle > 0 and mt[p - 1] not in "-=": + angle -= 1 + elif ch == "{" and depth == 0 and angle == 0: + return p + elif ch == "{": + depth += 1 # a const-generic block expression + elif ch == "}" and depth > 0: + depth -= 1 + elif ch == ";" and depth == 0 and angle == 0: + return -1 + p += 1 + return -1 + + def find_body_open(mt: str, p: int) -> int: - """From after the item name, skip generics and where clauses; return offset of the body - delimiter `{` or `(`, or -1 for a unit item ending in `;`.""" + """From after a struct/union name, skip generics and where clauses; return the offset of + the body delimiter `{` or `(`, or -1 for a unit item ending in `;`.""" n = len(mt) if p < n and mt[p] == "<": d = 0 @@ -383,7 +462,7 @@ def find_body_open(mt: str, p: int) -> int: ch = mt[p] if ch == "<": d += 1 - elif ch == ">" and mt[p - 1] != "-": + elif ch == ">" and mt[p - 1] not in "-=": d -= 1 if d == 0: p += 1 @@ -393,25 +472,14 @@ def find_body_open(mt: str, p: int) -> int: p += 1 if p < n and mt[p] == "(": return p - depth = 0 - while p < n: - ch = mt[p] - if ch in "([": - depth += 1 - elif ch in ")]": - depth -= 1 - elif ch == "{" and depth == 0: - return p - elif ch == ";" and depth == 0: - return -1 - p += 1 - return -1 + return find_brace(mt, p) def split_top(mt: str, open_pos: int) -> list[tuple[int, int]]: - """Segments [a, b) separated by depth-1 commas inside the delimiter at open_pos. + """Segments [a, b) separated by top-level commas inside the delimiter at open_pos. - Angle brackets count as nesting (`HashMap`), except the `>` of `->` and `=>`. + Generic angle brackets count as nesting only directly inside the delimiter, so + `HashMap` stays one segment while `[u8; 1 << 3]` (inside brackets) is ignored. """ segs, depth, angle, a = [], 0, 0, open_pos + 1 for p in range(open_pos, len(mt)): @@ -423,9 +491,9 @@ def split_top(mt: str, open_pos: int) -> list[tuple[int, int]]: if depth == 0: segs.append((a, p)) return segs - elif ch == "<" and depth >= 1: + elif ch == "<" and depth == 1: angle += 1 - elif ch == ">" and depth >= 1 and mt[p - 1] not in "-=" and angle > 0: + elif ch == ">" and depth == 1 and mt[p - 1] not in "-=" and angle > 0: angle -= 1 elif ch == "," and depth == 1 and angle == 0: segs.append((a, p)) @@ -436,11 +504,17 @@ def split_top(mt: str, open_pos: int) -> list[tuple[int, int]]: MEMBER_RE = re.compile(r"(" + VIS + r")" + QUAL + r"(fn|const|type)\b") +REL_VIS = re.compile(r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self)\b)") +INCLUDE = re.compile(r"\binclude(?:_str|_bytes)?\s*!\s*\(") +PLAIN_INCLUDE = re.compile(r"(\binclude(?:_str|_bytes)?\s*!\s*\(\s*)\"([^\"\\]*)\"\s*\)") + + def transform_item(text: str, kind: str, parent_path: list[str], deeper_dir: bool) -> tuple[str, dict]: """Apply the visibility/path edits to one moved item's text (lead trivia included).""" counts: dict = {} mt = mask(text) edits: list[tuple[int, int, str]] = [] # (pos, len_old, new) applied right-to-left + edited_rel: set[int] = set() def vis_edit(pos: int) -> None: m = VIS_AT.match(text, pos) @@ -449,6 +523,8 @@ def vis_edit(pos: int) -> None: if new is not None: edits.append((pos, len(old), new)) counts["vis"] = counts.get("vis", 0) + 1 + if old: + edited_rel.add(pos) # 1. the item's own visibility: first code offset after attributes p0 = skip_ws_attrs(mt, 0) @@ -466,13 +542,8 @@ def vis_edit(pos: int) -> None: if q < b: vis_edit(q) elif kind == "impl": - header_end = mt.find("{", p0) - header = re.sub(r"\bfor\s*<[^<>]*>", "", mt[p0:header_end]) - header = re.sub(r"<[^<>]*>", "", header) - while re.search(r"<[^<>]*>", header): - header = re.sub(r"<[^<>]*>", "", header) - inherent = not re.search(r"\bfor\s+(?!<)", header) - if inherent: + header_end = find_brace(mt, p0) + if header_end >= 0 and is_inherent_header(mt[p0:header_end]): # members start after the body brace or after a depth-1 `}`/`;` depth, p, starts = 0, header_end, [] while p < len(mt): @@ -494,21 +565,41 @@ def vis_edit(pos: int) -> None: q = skip_ws_attrs(mt, s) if MEMBER_RE.match(mt, q): vis_edit(q) + elif kind == "mod" and re.search(r"\bsuper\b", mt): + raise SystemExit("an inline module that names `super` moves one level deeper; edit by hand") + # Any relative visibility the edits above did not rewrite (nested items, macro bodies) + # would change reach one module deeper. + for m in REL_VIS.finditer(mt): + if m.start() not in edited_rel: + raise SystemExit("moved code has a nested pub(super)/pub(self) the tool cannot remap; edit by hand") + if re.search(r"\bself\s*::\s*super\b", mt): + raise SystemExit("`self::super` in moved code changes meaning one module deeper; edit by hand") + for m in re.finditer(r"\bsuper\s*::", text): + if mt[m.start()] == "x": # inside a string literal, e.g. #[serde(default = "super::f")] + raise SystemExit("a string literal in moved code names a `super::` path (an attribute argument?); " + "the tool cannot tell whether it resolves relative to the module. Edit by hand.") # 3. super:: paths (code only) for m in re.finditer(r"(? bool: def impl_layout(lines: list[str], mlines: list[str], it: Item) -> ImplLayout | None: """Members of an inherent impl item; None for a trait impl or a one-line impl.""" - body_open, header = None, "" - for i in range(it.start, it.end + 1): - pos = mlines[i].find("{") - if pos >= 0: - header += mlines[i][:pos] - body_open = i - break - header += mlines[i] + "\n" - if body_open is None or not is_inherent_header(header): + joined = "\n".join(mlines[it.start:it.end + 1]) + cut = find_brace(joined, 0) + if cut < 0: + return None + header = joined[:cut] + body_open = it.start + header.count("\n") + if not is_inherent_header(header): return None if body_open == it.end or not mlines[body_open].rstrip().endswith("{"): return None @@ -657,6 +746,42 @@ def attr_lines(lines: list[str], lead: int, start: int) -> list[str]: DEFS = {"struct", "enum", "union", "trait", "impl", "type", "const", "static", "macro"} +def new_reach(orig_vis: str, parent_path: list[str]) -> str: + """An item's reach after the move: "pub", "crate", "private" (the parent module only), + or the crate-relative module path of a `pub(in crate::..)`.""" + v = collapse(orig_vis) + mapped = map_vis(v, parent_path) + v = collapse(mapped) if mapped is not None else v + if v == "pub": + return "pub" + if v == "pub(crate)": + return "crate" + m = re.fullmatch(r"pub\(\s*in\s+crate((?:\s*::\s*\w+)*)\s*\)", v) + if m: + return re.sub(r"\s+", "", m.group(1)).lstrip(":") or "crate" + return "private" + + +def reexport_vis(reach: set[str]) -> str: + """The narrowest glob re-export that narrows no item: a glob re-exports each item at the + smaller of its own visibility and the glob's, so the glob must reach as far as the widest + item.""" + if "pub" in reach: + return "pub " + if "crate" in reach: + return "pub(crate) " + paths = [r.split("::") for r in reach if r not in ("private",)] + if not paths: + return "" + common = paths[0] + for q in paths[1:]: + n = 0 + while n < min(len(common), len(q)) and common[n] == q[n]: + n += 1 + common = common[:n] + return "pub(crate) " if not common else f"pub(in crate::{'::'.join(common)}) " + + def new_vis_rank(orig_vis: str, parent_path: list[str]) -> tuple[int, str]: """Rank of an item's visibility after the move, for choosing the re-export.""" v = collapse(orig_vis) @@ -683,7 +808,8 @@ def impl_block(lines: list[str], it: Item, lay: ImplLayout, mids: list[int], lea child: bool, trailing: bool, parent_path: list[str], deeper: bool, counts: dict) -> str: """An impl block holding the members `mids` (indices into lay.members), in order.""" header = "\n".join(lines[it.start:lay.body_open + 1]) - out = list(lead_lines) + lead = "\n".join(lead_lines) + out = (rewrite_super(lead) if child else lead).split("\n") if lead_lines else [] out.extend((rewrite_super(header) if child else header).split("\n")) prev = None for mi in mids: @@ -714,11 +840,10 @@ def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: items, header_end = parse_items(src) lines = src.split("\n") mlines = mask(src).split("\n") - parent_path = module_path_of(rel) - base = os.path.basename(rel) - deeper = base not in ("lib.rs", "main.rs", "mod.rs") - child_dir = os.path.join(os.path.dirname(path), base[:-3]) if deeper else os.path.dirname(path) - loaded_by_path = path_attribute_users(path) + parent_path, beside = file_layout(rel) + deeper = not beside + child_dir = os.path.join(os.path.dirname(path), os.path.basename(rel)[:-3]) if deeper else os.path.dirname(path) + loaded_by_path = path_attribute_users(repo, rel) if loaded_by_path: raise SystemExit(f"{rel} is loaded through #[path] ({loaded_by_path}); its child modules would " "not resolve under the directory this tool writes. Remove the attribute first.") @@ -737,7 +862,7 @@ def layout(k: int) -> ImplLayout | None: massign: dict[tuple[int, int], str] = {} for mod in mods: name = mod["name"] - if not re.fullmatch(r"[a-z][a-z0-9_]*", name): + if not re.fullmatch(r"[a-z][a-z0-9_]*", name) or name in KEYWORDS: raise SystemExit(f"bad module name {name!r}") for pat in mod["items"]: if " :: " in pat: @@ -768,26 +893,52 @@ def layout(k: int) -> ImplLayout | None: if k in assign and assign[k] != name: raise SystemExit(f"{it.selector} matched by {assign[k]} and {name}") assign[k] = name - # A macro_rules! that stays in the parent and emits a relative visibility (`pub(super)`) - # would mean something else when expanded one module deeper. - rel_vis_macros = [it.name for it in items if it.kind == "macro_rules!" and re.search( - r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self))\b", mask("\n".join(lines[it.start:it.end + 1])))] - if rel_vis_macros: - moved_text = "\n".join("\n".join(mlines[items[k].start:items[k].end + 1]) for k in assign) - moved_text += "\n".join("\n".join(mlines[layout(k).members[mi].start:layout(k).members[mi].end + 1]) - for k, mi in massign) - for name in rel_vis_macros: - if re.search(r"(? bool: + return re.search(r"(? 1}: + if invokes(moved_text, name): + raise SystemExit(f"macro_rules! {name} is defined more than once in the parent and moved code " + "invokes it; which definition it binds to would change. Edit by hand.") + bodies = {it.name: "\n".join(mlines[it.start:it.end + 1]) for it in macro_defs} + relative = {n for n, b in bodies.items() if REL_VIS.search(b)} + while True: + more = {n for n, b in bodies.items() if n not in relative and any(invokes(b, r) for r in relative)} + if not more: + break + relative |= more + for name in sorted(relative): + if invokes(moved_text, name): + raise SystemExit(f"moved code invokes macro_rules! {name}, which emits a relative visibility " + "(pub(super)/pub(self)), directly or through another macro; its expansion would " + "change reach. Keep its callers in the parent or edit the macro by hand.") split_impls = sorted({k for k, _ in massign}) for k in split_impls: if k in assign: raise SystemExit(f"{items[k].selector}: selected whole and by member") + imported: set[str] = set() + for it in items: + if it.kind in ("use", "extern crate"): + body = mask("\n".join(lines[it.start:it.end + 1])) + imported |= set(re.findall(r"\b(\w+)\s*(?=[,;}])", body)) + imported |= set(re.findall(r"\bas\s+(\w+)", body)) + imported |= set(re.findall(r"\b(\w+)\s*::\s*\{\s*self\b", body)) for it in items: if it.kind in TYPE_NS and it.name in names: raise SystemExit(f"module name {it.name!r} collides with {it.selector}") + for n in names: + if n in imported: + raise SystemExit(f"module name {n!r} collides with a name the parent imports") # Where a split impl's doc comments go: with the members the parent keeps, else with the # first module (in spec order) that receives members. @@ -795,13 +946,14 @@ def layout(k: int) -> ImplLayout | None: for k in split_impls: lay = layout(k) kept = [mi for mi in range(len(lay.members)) if (k, mi) not in massign] - lead_owner[k] = None if kept else next(n for n in names if any(massign.get((k, mi)) == n - for mi in range(len(lay.members)))) + first = min(mi for kk, mi in massign if kk == k) + lead_owner[k] = None if kept else massign[(k, first)] results = {} for mod in mods: name = mod["name"] - chunks, counts, obs, best, sels = [], {}, {k: 0 for k in OBS}, (RANK["none"], ""), [] + chunks, counts, obs, sels = [], {}, {k: 0 for k in OBS}, [] + reach: set[str] = set() # "pub", "crate", "private", or a crate-relative path for pub(in ..) for k, it in enumerate(items): if assign.get(k) == name: text = "\n".join(lines[it.lead:it.end + 1]).lstrip("\n") @@ -813,10 +965,12 @@ def layout(k: int) -> ImplLayout | None: chunks.append(new) sels.append(it.selector) if it.kind == "macro": + # What a macro invocation emits is unknown: re-export at most `pub`, which a + # glob clamps to each emitted item's own visibility, so nothing widens. counts["macro_items"] = counts.get("macro_items", 0) + 1 - best = max(best, (RANK["private"], "")) + reach.add("pub") elif it.kind != "impl": - best = max(best, new_vis_rank(it.vis, parent_path)) + reach.add(new_reach(it.vis, parent_path)) elif k in split_impls: lay = layout(k) mids = [mi for mi in range(len(lay.members)) if massign.get((k, mi)) == name] @@ -841,15 +995,16 @@ def layout(k: int) -> ImplLayout | None: body = "" if doc: body += "".join(f"//! {d}\n" if d.strip() else "//!\n" for d in doc.split("\n")) + "\n" - body += "use super::*;\n\n" + "\n\n".join(chunks) + "\n" + glob = mod.get("glob_import", True) + if not isinstance(glob, bool): + raise SystemExit(f"module {name}: glob_import must be true or false") + body += ("use super::*;\n\n" if glob else "") + "\n\n".join(chunks) + "\n" ks = [k for k in range(len(items)) if assign.get(k) == name or (k in split_impls and any(massign.get((k, mi)) == name for mi in range(len(layout(k).members))))] cfgs = [set(items[k].cfg) for k in ks] common_cfg = sorted(set.intersection(*cfgs)) if cfgs else [] - reexport = mod.get("reexport", "auto") - if best[0] == RANK["none"]: - reexport = "none" # only impl blocks: nothing to name - use_vis = {RANK["pub"]: "pub ", RANK["crate"]: "pub(crate) ", RANK["in"]: best[1] + " "}.get(best[0], "") + reexport = "auto" if reach else "none" # only impl blocks: nothing to name + use_vis = reexport_vis(reach) results[name] = dict(items=sels, text=body, counts=counts, obs=obs, cfg=common_cfg, use_vis=use_vis, reexport=reexport, code=code_lines(body), path=os.path.join(child_dir, name + ".rs")) @@ -864,7 +1019,7 @@ def layout(k: int) -> ImplLayout | None: r = results[mod["name"]] if r["reexport"] == "none": continue - gate = "".join(c + "\n" for c in r["cfg"]) + ("#[cfg(test)]\n" if r["reexport"] == "test" else "") + gate = "".join(c + "\n" for c in r["cfg"]) uses.append(f"{gate}{r['use_vis']}use {mod['name']}::*;") decl_text = "\n".join(decls) + ("\n\n" + "\n".join(uses) if uses else "") @@ -964,21 +1119,21 @@ def sep() -> None: return 0 -def path_attribute_users(path: str) -> str: +def path_attribute_users(repo: str, rel: str) -> str: """A `#[path = "..."]` in the crate that names this file, if any.""" - crate_src = path.split(os.sep + "src" + os.sep, 1)[0] + os.sep + "src" - target = os.path.realpath(path) + crate_src = os.path.join(repo, rel.replace(os.sep, "/").split("/src/", 1)[0], "src") + target = os.path.realpath(os.path.join(repo, rel)) for d, _, files in os.walk(crate_src): for f in files: if not f.endswith(".rs"): continue fp = os.path.join(d, f) - text = open(fp, encoding="utf-8").read() + text = open(fp, encoding="utf-8", errors="replace").read() masked = mask(text) - for m in re.finditer(r'#\s*\[\s*path\s*=\s*"([^"]+)"\s*\]', text): + for m in re.finditer(r'#\s*\[\s*path\s*=\s*r?(#*)"([^"]+)"\1\s*\]', text): if masked[m.start()] != "#": continue # inside a comment - if os.path.realpath(os.path.join(d, m.group(1))) == target: + if os.path.realpath(os.path.join(d, m.group(2))) == target: return os.path.relpath(fp, crate_src) return "" @@ -1002,23 +1157,6 @@ def ratchet_hits(repo: str, results: dict) -> dict: # ------------------------------------------------------------------ verify -def normalize(text: str) -> str: - """Whitespace- and trailing-comma-insensitive form of code, for comparing a move across a - rustfmt run. String and char literals are kept byte for byte.""" - m = mask(text, comment="c") - out: list[str] = [] - for ch, mk in zip(text, m): - if mk == "x": - out.append(ch) - elif ch.isspace(): - continue - else: - if mk != "c" and (ch in CLOSE or ch == ">") and out and out[-1] == ",": - out.pop() - out.append(ch) - return "".join(out) - - def git(repo: str, *args: str) -> str: return subprocess.run(["git", "-C", repo, *args], capture_output=True, text=True, check=True).stdout @@ -1028,142 +1166,158 @@ def git_show(repo: str, rev: str, path: str) -> str | None: return r.stdout if r.returncode == 0 else None -@dataclass -class Record: - """One unit a move must conserve: a whole item, one member of an inherent impl (keyed by - the impl's attributes and header), or the non-attribute trivia of an inherent impl.""" - kind: str # "item" | "member" | "impl-trivia" - item_kind: str - selector: str - key: str # normalized impl attrs + header for members, else "" - text: str - line: int - - -def records(src: str) -> list[Record]: - items, _ = parse_items(src) - lines = src.split("\n") - mlines = mask(src).split("\n") - out: list[Record] = [] - for it in items: - lay = impl_layout(lines, mlines, it) if it.kind == "impl" else None - if lay is None or not lay.members: - text = "\n".join(lines[it.lead:it.end + 1]).lstrip("\n") - out.append(Record("item", it.kind, it.selector, "", text, it.start)) - continue - attrs = attr_lines(lines, it.lead, it.start) - header = "\n".join(attrs + lines[it.start:lay.body_open + 1]) - trivia = [ln for ln in lines[it.lead:it.start] if ln.strip() and ln not in attrs] - trivia += lines[lay.members[-1].end + 1:lay.close] - if any(t.strip() for t in trivia): - out.append(Record("impl-trivia", "impl", it.selector, "", "\n".join(trivia), it.start)) - for m in lay.members: - out.append(Record("member", m.kind, f"{it.selector} :: {m.selector}", header, - "\n".join(lines[m.lead:m.end + 1]), m.start)) - return out - - def parent_candidates(child: str) -> list[str]: d = os.path.dirname(child) - out = [d + ".rs", os.path.join(d, "mod.rs")] if os.path.basename(d) == "src": - out = [os.path.join(d, "lib.rs"), os.path.join(d, "main.rs")] - return out - + return [os.path.join(d, "lib.rs"), os.path.join(d, "main.rs")] + return [d + ".rs", os.path.join(d, "mod.rs")] + + +def crate_edition(repo: str, rev: str, rel: str) -> str: + d = os.path.dirname(rel) + while d: + manifest = git_show(repo, rev, f"{d}/Cargo.toml") + if manifest is not None: + pkg = tomllib.loads(manifest).get("package", {}) + ed = pkg.get("edition") + if isinstance(ed, str): + return ed + break + d = os.path.dirname(d) + root = tomllib.loads(git_show(repo, rev, "Cargo.toml") or "") + return root.get("workspace", {}).get("package", {}).get("edition", "2024") + + +def rustfmt(repo: str, text: str, edition: str) -> str: + r = subprocess.run(["rustfmt", "--edition", edition, "--emit", "stdout", "--quiet"], cwd=repo, + input=text, capture_output=True, text=True) + if r.returncode != 0: + raise SystemExit(f"rustfmt failed: {r.stderr.strip()[:400]}") + return r.stdout + + +def header_key(sel: str) -> str: + """An impl selector without `super::`, whitespace or trailing commas, so a header matches + its base however rustfmt wrapped it.""" + return re.sub(r",(?=[>)\]}])", "", re.sub(r"\s+", "", strip_super(sel))) + + +def escape_glob(sel: str) -> str: + return re.sub(r"([*?\[])", r"[\1]", sel) + + +def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str]) -> str: + """The spec `apply` would need to turn the base parent into the head files.""" + base_items = parse_items(git_show(repo, base, parent) or "")[0] + head_parent = git_show(repo, head, parent) or "" + hp_items = parse_items(head_parent)[0] + hp_lines = head_parent.split("\n") + order = [it.name for it in hp_items if it.kind == "mod-decl"] + names = sorted(children, key=lambda n: order.index(n) if n in order else len(order)) + out = [f"file = {json.dumps(parent)}"] + for name in names: + child = os.path.join(os.path.dirname(parent), os.path.basename(parent)[:-3], f"{name}.rs") \ + if not file_layout(parent)[1] else os.path.join(os.path.dirname(parent), f"{name}.rs") + src = git_show(repo, head, child) or "" + items, hdr = parse_items(src) + lines, mlines = src.split("\n"), mask(src).split("\n") + doc = "\n".join(re.sub(r"^//! ?", "", ln.strip()) for ln in lines[:hdr] if ln.strip().startswith("//!")) + sels: list[str] = [] + for it in items: + if it.kind == "use" and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;": + continue + lay = impl_layout(lines, mlines, it) if it.kind == "impl" else None + if lay and lay.members: + base_sel = next((b.selector for b in base_items if b.kind == "impl" + and header_key(b.selector) == header_key(it.selector)), it.selector) + sels += [f"{escape_glob(base_sel)} :: {escape_glob(m.selector)}" for m in lay.members] + else: + sels.append(escape_glob(it.selector)) + has_glob = any(it.kind == "use" and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;" + for it in items) + out.append(f"\n[[module]]\nname = {json.dumps(name)}") + if doc: + out.append(f"doc = {json.dumps(doc)}") + if not has_glob: + out.append("glob_import = false") + out.append("items = [\n" + "".join(f" {json.dumps(s)},\n" for s in dict.fromkeys(sels)) + "]") + return "\n".join(out) + "\n" + + +def verify(repo: str, base: str, head: str, print_spec: bool = False) -> int: + """Prove BASE...HEAD's Rust changes are exactly what `apply` generates: derive the spec from + the diff, regenerate from the merge base, and compare byte for byte after rustfmt.""" + import contextlib + import difflib + import io + import shutil + import tempfile -def verify(repo: str, base: str, head: str) -> int: - """Prove BASE...HEAD only moves items (or inherent-impl members) into child modules the - way `apply` does, plus the `mod`/`use` declarations `apply` writes.""" merge_base = git(repo, "merge-base", base, head).strip() - status = [ln.split("\t") for ln in git(repo, "diff", "--name-status", "--no-renames", - f"{merge_base}..{head}", "--", "*.rs").splitlines()] - added = {p for st, p in status if st == "A"} - modified = {p for st, p in status if st == "M"} - deleted = {p for st, p in status if st == "D"} - pool = {p: records(git_show(repo, merge_base, p) or "") for p in modified | deleted} - new_mods = {os.path.splitext(os.path.basename(p))[0] for p in added} - residual_added: list[str] = [] - moved = 0 - - def take(p: str, rec: Record, want: str, key: str) -> bool: - for idx, b in enumerate(pool.get(p, [])): - if b.kind == rec.kind and normalize(b.key) == key and normalize(b.text) == want: - pool[p].pop(idx) - return True - return False - - # File headers (`//!` docs, `#![...]` inner attributes) are not items, so compare them - # directly: unchanged in modified files, docs only in new ones. - for p in sorted(modified): - b_src, h_src = git_show(repo, merge_base, p) or "", git_show(repo, head, p) or "" - b_end, h_end = parse_items(b_src)[1], parse_items(h_src)[1] - if normalize("\n".join(b_src.split("\n")[:b_end])) != normalize("\n".join(h_src.split("\n")[:h_end])): - residual_added.append(f"{p}: file header (//! docs or #![...] attributes) changed") - for p in sorted(added): - h_src = git_show(repo, head, p) or "" - h_end = parse_items(h_src)[1] - if any(ln.strip() and not ln.strip().startswith("//!") for ln in h_src.split("\n")[:h_end]): - residual_added.append(f"{p}: new file header has more than //! docs") - trivia_head: list[tuple[str, Record]] = [] - for p in sorted(modified): - for rec in records(git_show(repo, head, p) or ""): - if rec.kind == "impl-trivia": - trivia_head.append((p, rec)) - continue - if take(p, rec, normalize(rec.text), normalize(rec.key)): - continue - if rec.item_kind == "mod-decl" and rec.selector.split(" ", 1)[1] in new_mods: - continue - if rec.item_kind == "use" and new_mods and re.search( - r"\buse\s+(" + "|".join(map(re.escape, new_mods)) + r")::\*;", rec.text): - continue - residual_added.append(f"{p}: {rec.selector} (line {rec.line + 1})") - for p in sorted(added): - parent = next((c for c in parent_candidates(p) if c in pool), None) - for rec in records(git_show(repo, head, p) or ""): - if rec.kind == "item" and rec.item_kind == "use" and collapse(rec.text) == "use super::*;": - continue - if rec.kind == "impl-trivia": - trivia_head.append((p, rec)) - continue - if parent is None: - residual_added.append(f"{p}: {rec.selector} (new file with no changed parent module)") + raw = git(repo, "diff", "--raw", "-z", "--no-renames", "--no-abbrev", f"{merge_base}..{head}", "--", "*.rs") + fields = raw.split("\0") + status, modes = [], {} + for k in range(0, len(fields) - 1, 2): + meta, path = fields[k].split(), fields[k + 1] + status.append((meta[4], path)) + modes[path] = (meta[0].lstrip(":"), meta[1]) + irregular = {p for p, (old, new) in modes.items() + if new not in ("100644", "100755", "000000") or old not in ("100644", "100755", "000000")} + added = sorted(p for st, p in status if st == "A" and p not in irregular) + modified = {p for st, p in status if st == "M" and p not in irregular} + problems: list[str] = [f"{p}: deleted" for st, p in status if st == "D"] + problems += [f"{p}: {st}" for st, p in status if st not in ("A", "M", "D")] + problems += [f"{p}: not a regular file (mode {modes[p][0]} -> {modes[p][1]})" for p in sorted(irregular)] + groups: dict[str, list[str]] = {} + for p in added: + parent = next((c for c in parent_candidates(p) if c in modified), None) + if parent is None: + problems.append(f"{p}: new file whose parent module is not part of the change") + continue + groups.setdefault(parent, []).append(os.path.splitext(os.path.basename(p))[0]) + for p in sorted(modified - set(groups)): + problems.append(f"{p}: changed, but no child module was split out of it") + checked = 0 + for parent, children in groups.items(): + spec = derive_spec(repo, merge_base, head, parent, children) + if print_spec: + print(f"# derived spec for {parent}\n{spec}") + crate_src = parent.split("/src/", 1)[0] + "/src" + tmp = tempfile.mkdtemp(prefix="split-verify-") + try: + paths = [crate_src] + (["ARCHITECTURE.toml"] if git_show(repo, merge_base, "ARCHITECTURE.toml") else []) + archive = subprocess.run(["git", "-C", repo, "archive", merge_base, *paths], + capture_output=True, check=True).stdout + subprocess.run(["tar", "-x", "-C", tmp], input=archive, check=True) + spec_path = os.path.join(tmp, "spec.toml") + with open(spec_path, "w") as f: + f.write(spec) + try: + with contextlib.redirect_stdout(io.StringIO()): + apply(spec_path, tmp, False, None) + except SystemExit as e: + problems.append(f"{parent}: regenerating from the derived spec failed: {e}") continue - ppath = module_path_of(parent) - deeper = os.path.basename(parent) not in ("lib.rs", "main.rs", "mod.rs") - want, key = normalize(rec.text), normalize(rec.key) - hit = False - for idx, b in enumerate(pool[parent]): - if b.kind != rec.kind or b.item_kind != rec.item_kind: - continue - if b.kind == "member" and normalize(rewrite_super(b.key)) != key: - continue - try: - tkind = b.item_kind if b.kind == "item" else ("macro" if b.item_kind == "macro" else "fn") - exp, _ = transform_item(b.text, tkind, ppath, deeper) - except SystemExit: - continue - if normalize(exp) == want: - pool[parent].pop(idx) - hit = True - moved += 1 - break - if not hit: - residual_added.append(f"{p}: {rec.selector} (line {rec.line + 1})") - for p, rec in trivia_head: # an impl's doc comments may land with any of its blocks - src_p = next((c for c in [p] + parent_candidates(p) if c in pool), None) - cands = [c for c in ([src_p] if src_p else []) + list(pool) if c] - if not any(take(c, rec, normalize(rec.text), "") for c in dict.fromkeys(cands)): - residual_added.append(f"{p}: doc/comments of {rec.selector} (line {rec.line + 1})") - residual_removed = [f"{p}: {b.selector} (base line {b.line + 1})" for p, v in pool.items() for b in v] - print(f"moved: {moved} unmatched at base: {len(residual_removed)} unmatched at head: {len(residual_added)}") - for title, rows in (("Base items not found at head (removed or edited):", residual_removed), - ("Head items that are not a move (added or edited):", residual_added)): - if rows: - print("\n" + title) - for r in rows: - print(" " + r) - return 1 if residual_removed or residual_added else 0 + edition = crate_edition(repo, merge_base, parent) + beside = file_layout(parent)[1] + cdir = os.path.dirname(parent) if beside else parent[:-3] + for rel in [parent] + [f"{cdir}/{c}.rs" for c in children]: + want = rustfmt(repo, open(os.path.join(tmp, rel), encoding="utf-8").read(), edition) + got = rustfmt(repo, git_show(repo, head, rel) or "", edition) + checked += 1 + if want != got: + diff = list(difflib.unified_diff(want.split("\n"), got.split("\n"), "regenerated/" + rel, + "head/" + rel, lineterm="", n=2)) + problems.append(f"{rel}: differs from what apply generates\n " + "\n ".join(diff[:40])) + finally: + shutil.rmtree(tmp, ignore_errors=True) + if not groups and not problems: + problems.append("no split found: no new child module next to a changed parent") + print(f"regenerated and compared {checked} files from {len(groups)} split parents: " + f"{'identical' if not problems else str(len(problems)) + ' problems'}") + for p in problems: + print(" " + p) + return 1 if problems else 0 def write_report(out: str, s: dict, repo: str) -> None: @@ -1186,10 +1340,15 @@ def list_items(path: str) -> int: src = open(path, encoding="utf-8").read() items, _ = parse_items(src) lines = src.split("\n") + mlines = mask(src).split("\n") for it in items: text = "\n".join(lines[it.lead:it.end + 1]) - cfg = (" " + " ".join(it.cfg)) if it.cfg else "" + cfg = (" " + " ".join(collapse(c) for c in it.cfg)) if it.cfg else "" print(f"{it.start + 1:6}-{it.end + 1:<6} {code_lines(text):5} {it.selector}{cfg}") + lay = impl_layout(lines, mlines, it) if it.kind == "impl" else None + for m in (lay.members if lay else []): + mtext = "\n".join(lines[m.lead:m.end + 1]) + print(f"{m.start + 1:6}-{m.end + 1:<6} {code_lines(mtext):5} {it.selector} :: {m.selector}") return 0 @@ -1207,11 +1366,12 @@ def main() -> int: c.add_argument("base", nargs="?", default="origin/main") c.add_argument("--head", default="HEAD") c.add_argument("--repo", default=".") + c.add_argument("--print-spec", action="store_true", help="print the spec derived from the diff") args = ap.parse_args() if args.cmd == "list": return list_items(args.file) if args.cmd == "verify": - return verify(args.repo, args.base, args.head) + return verify(args.repo, args.base, args.head, args.print_spec) return apply(args.spec, args.repo, args.dry_run, args.report) diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index db98d92e5..077f30412 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -7,6 +7,7 @@ from __future__ import annotations +import os import subprocess import sys import textwrap @@ -216,14 +217,14 @@ def test_verify_accepts_the_move_and_rejects_an_edit(repo): apply_and_commit(repo) ok = run("verify", "HEAD~1", cwd=repo) assert ok.returncode == 0, ok.stdout - assert "moved: 11 unmatched at base: 0 unmatched at head: 0" in ok.stdout + assert "regenerated and compared 4 files from 1 split parents: identical" in ok.stdout config = repo / "crate/src/a/foo/config.rs" config.write_text(config.read_text().replace(" self.inner\n", " self.inner + 1\n")) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") bad = run("verify", "HEAD~2", cwd=repo) assert bad.returncode == 1 - assert "impl Config" in bad.stdout + assert "foo/config.rs: differs from what apply generates" in bad.stdout and "self.inner + 1" in bad.stdout def test_verify_ignores_rustfmt_rewraps(repo): @@ -357,7 +358,7 @@ def test_verify_rejects_a_member_edited_in_its_new_block(repo): helpers.write_text(helpers.read_text().replace("const LIMIT: u32 = 5;", "const LIMIT: u32 = 6;")) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") bad = run("verify", "HEAD~2", cwd=repo) - assert bad.returncode == 1 and "LIMIT" in bad.stdout + assert bad.returncode == 1 and "const LIMIT: u32 = 6;" in bad.stdout def test_a_file_loaded_through_a_path_attribute_is_refused(repo): @@ -411,4 +412,374 @@ def test_verify_rejects_header_edits(repo, path, old, new): f.write_text(f.read_text().replace(old, new, 1)) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "header") bad = run("verify", "HEAD~2", cwd=repo) - assert bad.returncode == 1 and "header" in bad.stdout + assert bad.returncode == 1 and "differs from what apply generates" in bad.stdout + + +# --- Adversarial cases: verify must reject anything that is not what apply generates. --- + + +def commit_all(repo: Path, msg: str) -> None: + git(repo, "add", "-A") + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qm", msg) + + +@pytest.mark.parametrize( + ("path", "old", "new"), + [ + # dropping a re-export or a module declaration silently drops items + ("crate/src/a/foo.rs", "pub use config::*;\n", ""), + ("crate/src/a/foo.rs", "mod parsing;\n", ""), + # an unrelated import smuggled next to the generated lines + ("crate/src/a/foo.rs", "pub use config::*;\n", "pub use config::*;\nuse std::process::exit; // use parsing::*;\n"), + # syntax a whitespace-insensitive comparison would erase + ("crate/src/a/foo/config.rs", 'let s = "fn not_a_member() {}";', 'let s = "fn not_a_member()\n{}";'), + ("crate/src/a/foo/config.rs", "self.inner\n", "(self.inner,).0\n"), + # a comment edited inside a moved item + ("crate/src/a/foo/parsing.rs", "// Parsing.", "// Parsing, edited."), + ], +) +def test_verify_rejects_edits_a_loose_comparison_would_miss(repo, path, old, new): + apply_and_commit(repo) + f = repo / path + assert old in f.read_text() + f.write_text(f.read_text().replace(old, new, 1)) + commit_all(repo, "edit") + assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + + +def test_verify_rejects_doc_comments_swapped_between_impls(repo): + (repo / "spec.toml").write_text(MEMBER_SPEC) + src = repo / "crate/src/a/foo.rs" + src.write_text(src.read_text().replace("impl Config {", "/// Config docs.\nimpl Config {") + .replace("impl Holder", "/// Holder docs.\nimpl Holder")) + commit_all(repo, "docs") + apply_and_commit(repo) + text = src.read_text() + src.write_text(text.replace("/// Config docs.", "/// TMP").replace("/// Holder docs.", "/// Config docs.") + .replace("/// TMP", "/// Holder docs.")) + commit_all(repo, "swap") + assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + + +def write_parent(repo: Path, text: str, spec: str) -> subprocess.CompletedProcess: + (repo / "crate/src/a/foo.rs").write_text(textwrap.dedent(text)) + (repo / "spec.toml").write_text(textwrap.dedent(spec)) + commit_all(repo, "parent") + return run("apply", "spec.toml", cwd=repo) + + +def test_a_shadowed_macro_used_by_moved_code_is_refused(repo): + r = write_parent(repo, """\ + macro_rules! m { + () => { + 1 + }; + } + + fn f() -> u8 { + m!() + } + + macro_rules! m { + () => { + 2 + }; + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "defined more than once" in r.stderr + + +def test_a_macro_chain_that_emits_relative_visibility_is_refused(repo): + r = write_parent(repo, """\ + macro_rules! helper { + ($n:ident) => { + pub(super) fn $n() {} + }; + } + + macro_rules! outer { + ($n:ident) => { + helper!($n); + }; + } + + struct S; + + impl S { + outer!(made); + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["impl S"]\n') + assert r.returncode != 0 and "macro_rules! outer" in r.stderr + + +def test_the_reexport_reaches_as_far_as_the_widest_item(repo): + r = write_parent(repo, """\ + pub(in crate::a) struct Wide; + + pub(super) struct Narrow; + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["struct Wide", "struct Narrow"]\n') + assert r.returncode == 0, r.stderr + parent = (repo / "crate/src/a/foo.rs").read_text() + child = (repo / "crate/src/a/foo/x.rs").read_text() + assert "pub(in crate::a) use x::*;" in parent + assert "pub(in crate::a) struct Wide;" in child and "pub(in crate::a) struct Narrow;" in child + + +def test_fields_after_const_expressions_and_where_clause_blocks_are_widened(repo): + r = write_parent(repo, """\ + trait Tr {} + + struct S { + a: [u8; 1 << 3], + b: u8, + } + + struct G + where + T: Tr<{ 1 + 1 }>, + { + t: T, + } + + struct E; + + impl E { + extern "C" fn ffi() {} + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["struct S", "struct G", "impl E"]\n') + assert r.returncode == 0, r.stderr + child = (repo / "crate/src/a/foo/x.rs").read_text() + assert " pub(super) a: [u8; 1 << 3],\n pub(super) b: u8," in child + assert " T: Tr<{ 1 + 1 }>,\n{\n pub(super) t: T," in child + assert ' pub(super) extern "C" fn ffi() {}' in child + + +@pytest.mark.parametrize( + ("rel", "segments", "beside"), + [ + ("c/src/bin/tool.rs", [], True), + ("c/src/bin/tool/main.rs", [], True), + ("c/src/bin/tool/helper.rs", ["helper"], False), + ("c/src/a/main.rs", ["a", "main"], False), + ("c/src/a/lib.rs", ["a", "lib"], False), + ("c/src/a/mod.rs", ["a"], True), + ], +) +def test_file_layout(rel, segments, beside): + assert srm.file_layout(rel) == (segments, beside) + + +def test_include_paths_are_rebased_or_refused(repo): + ok = write_parent(repo, """\ + fn data() -> &'static str { + include_str!("data.txt") + } + + fn items() { + include!("gen.rs"); + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn data", "fn items"]\n') + assert ok.returncode == 0, ok.stderr + child = (repo / "crate/src/a/foo/x.rs").read_text() + assert 'include_str!("../data.txt")' in child and 'include!("../gen.rs")' in child + git(repo, "reset", "-q", "--hard", "HEAD") + git(repo, "clean", "-qfd") + bad = write_parent(repo, """\ + fn raw() -> &'static str { + include_str!(r"data.txt") + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "y"\nitems = ["fn raw"]\n') + assert bad.returncode != 0 and "cannot rebase" in bad.stderr + + +def test_split_impl_attributes_follow_the_header_rewrite(repo): + r = write_parent(repo, """\ + struct S; + + #[super::attr] + impl S { + fn a() {} + + fn b() {} + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["impl S :: fn b"]\n') + assert r.returncode == 0, r.stderr + assert "#[super::super::attr]\nimpl S {" in (repo / "crate/src/a/foo/x.rs").read_text() + assert "#[super::attr]\nimpl S {" in (repo / "crate/src/a/foo.rs").read_text() + + +def test_cfg_gates_on_declarations_keep_their_exact_text(repo): + r = write_parent(repo, """\ + #[cfg(custom = "a b")] + fn gated() {} + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn gated"]\n') + assert r.returncode == 0, r.stderr + assert '#[cfg(custom = "a b")]\nmod x;' in (repo / "crate/src/a/foo.rs").read_text() + + +def test_module_name_may_not_shadow_an_import(repo): + r = write_parent(repo, """\ + use crate::other::child; + + fn f() {} + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "child"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "imports" in r.stderr + + +def test_nested_relative_visibility_is_refused(repo): + r = write_parent(repo, """\ + fn f() { + mod inner { + pub(super) fn g() {} + } + } + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "nested" in r.stderr + + +# --- Compile oracle: the split crate must build and behave as the original did. --- + +ORACLE = { + "lib.rs": """\ + pub mod a; + + #[test] + fn reach_is_unchanged() { + assert_eq!(a::from_grandparent(), 7); + assert_eq!(a::foo::from_parent(), 12); + } + """, + "a.rs": """\ + pub mod foo; + + pub fn from_grandparent() -> u32 { + // pub(super) items of foo, and pub(super) fields, reach here. + foo::shared() + foo::Pair::new().wide + } + """, + "a/foo.rs": """\ + mod sibling; + + pub struct Pair { + narrow: u32, + pub(super) wide: u32, + } + + impl Pair { + pub fn new() -> Self { + Self { narrow: 2, wide: 3 } + } + + fn narrow(&self) -> u32 { + self.narrow + } + } + + pub(super) fn shared() -> u32 { + 4 + } + + fn private_helper() -> u32 { + 5 + } + + pub fn from_parent() -> u32 { + private_helper() + Pair::new().narrow() + Pair::new().narrow + sibling::calls() + } + """, + "a/foo/sibling.rs": """\ + use super::*; + + pub(super) fn calls() -> u32 { + private_helper() - 2 + } + """, +} + + +def rustc() -> str | None: + import shutil + + found = shutil.which("rustc") + if found is None and os.environ.get("SPACE_CI_REQUIRE_TEST_DEPS") == "1": + pytest.fail("rustc is required for the compile oracle in CI") + return found + + +def build_and_test(root: Path) -> subprocess.CompletedProcess: + exe = root / "oracle-test" + b = subprocess.run([rustc(), "--edition", "2021", "--test", "-D", "warnings", "-o", str(exe), str(root / "src/lib.rs")], + capture_output=True, text=True) + if b.returncode: + return b + return subprocess.run([str(exe)], capture_output=True, text=True) + + +@pytest.mark.skipif(rustc() is None, reason="rustc not installed") +def test_split_crate_compiles_and_keeps_behavior(tmp_path): + src = tmp_path / "oracle/src" + for rel, text in ORACLE.items(): + (src / rel).parent.mkdir(parents=True, exist_ok=True) + (src / rel).write_text(textwrap.dedent(text)) + assert build_and_test(tmp_path / "oracle").returncode == 0 + (tmp_path / "spec.toml").write_text( + 'file = "oracle/src/a/foo.rs"\n' + '[[module]]\nname = "pair"\nglob_import = false\nitems = ["struct Pair", "impl Pair :: fn narrow"]\n' + '[[module]]\nname = "helpers"\nglob_import = false\nitems = ["fn shared", "fn private_helper"]\n' + ) + r = run("apply", "spec.toml", cwd=tmp_path) + assert r.returncode == 0, r.stderr + after = build_and_test(tmp_path / "oracle") + assert after.returncode == 0, after.stdout + after.stderr + + +@pytest.mark.parametrize( + ("text", "message"), + [ + ('#[derive(Default)]\nstruct D {\n #[serde(default = "super::f")]\n a: u8,\n}\n', "string literal"), + ("fn f() {\n self::super::g();\n}\n", "self::super"), + ], +) +def test_paths_the_tool_cannot_rewrite_are_refused(repo, text, message): + name = "struct D" if "struct D" in text else "fn f" + r = write_parent(repo, text, f'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["{name}"]\n') + assert r.returncode != 0 and message in r.stderr + + +def test_keyword_module_names_are_refused(repo): + (repo / "spec.toml").write_text('file = "crate/src/a/foo.rs"\n[[module]]\nname = "type"\nitems = ["fn stays"]\n') + assert run("apply", "spec.toml", "--dry-run", cwd=repo).returncode != 0 + + +def test_extern_blocks_parse_and_stay(repo): + r = write_parent(repo, 'extern "C" {\n fn abs(x: i32) -> i32;\n}\n\nfn f() {}\n', + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + assert 'extern "C" {' in (repo / "crate/src/a/foo.rs").read_text() + + +def test_an_attribute_sharing_a_line_with_its_item_is_refused(repo): + r = write_parent(repo, "#[rustfmt::skip] const T: u8 = 1;\n\nfn f() {}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "share a line" in r.stderr + + +def test_verify_rejects_a_symlinked_child_and_an_empty_change(repo): + assert run("verify", "HEAD", cwd=repo).returncode == 1 # nothing split + apply_and_commit(repo) + child = repo / "crate/src/a/foo/k8s.rs" + real = repo / "elsewhere.rs" + real.write_text(child.read_text()) + child.unlink() + child.symlink_to(real) + commit_all(repo, "symlink") + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "not a regular file" in bad.stdout + + +def test_the_path_attribute_scan_stays_inside_the_crate(tmp_path): + (tmp_path / "other/src").mkdir(parents=True) + (tmp_path / "other/src/lib.rs").write_text('#[path = "../../repo/c/src/a.rs"]\nmod x;\n') + (tmp_path / "repo/c/src").mkdir(parents=True) + (tmp_path / "repo/c/src/a.rs").write_text("fn f() {}\n") + assert srm.path_attribute_users(str(tmp_path / "repo"), "c/src/a.rs") == "" From c99d3ca438622e611b0b5ef28ad632b65cc95cde Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 02:12:24 -0400 Subject: [PATCH 06/15] scripts: split_rust_module.py refuses to move stringify!(super::..) --- scripts/split_rust_module.py | 2 ++ scripts/tests/test_split_rust_module.py | 1 + 2 files changed, 3 insertions(+) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 23b0f97ff..3e7861067 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -572,6 +572,8 @@ def vis_edit(pos: int) -> None: for m in REL_VIS.finditer(mt): if m.start() not in edited_rel: raise SystemExit("moved code has a nested pub(super)/pub(self) the tool cannot remap; edit by hand") + if re.search(r"\b(?:stringify|concat_idents)\s*!\s*\([^()]*\bsuper\b", mt): + raise SystemExit("moved code stringifies a `super` path; its text would change. Edit by hand.") if re.search(r"\bself\s*::\s*super\b", mt): raise SystemExit("`self::super` in moved code changes meaning one module deeper; edit by hand") for m in re.finditer(r"\bsuper\s*::", text): diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index 077f30412..12eedf2b1 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -738,6 +738,7 @@ def test_split_crate_compiles_and_keeps_behavior(tmp_path): [ ('#[derive(Default)]\nstruct D {\n #[serde(default = "super::f")]\n a: u8,\n}\n', "string literal"), ("fn f() {\n self::super::g();\n}\n", "self::super"), + ('fn f() -> &\'static str {\n stringify!(super::g)\n}\n', "stringifies"), ], ) def test_paths_the_tool_cannot_rewrite_are_refused(repo, text, message): From e087382fe6a85afc36c21a5d77571e128a09478d Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 02:31:51 -0400 Subject: [PATCH 07/15] scripts: close the second round of split_rust_module.py review findings verify: - Format only the regenerated side and compare it with HEAD's bytes. Comparing two rustfmt outputs let an edit rustfmt erases (a trailing comma inside a macro call) pass. - Find a new file's parent from the same layout rules apply uses, so a split of a src/bin/ crate root round-trips. - The glob import is never optional: a module whose items need nothing from the parent sets allow_unused_glob, which keeps `use super::*;` under #[allow(unused_imports)]. Removing the import, which can change what a name resolves to, now fails verify. apply now refuses, rather than changing meaning: - moved code that invokes a parent macro whose expansion names a super/self path, directly or through another macro; - moved code that invokes a macro the parent defines only later in the file (it bound to an outer macro of that name); - top-level macro invocations, unless the module sets allow_macro_items (their items keep visibilities the tool cannot remap); - inline modules nested inside moved code that names super; - stringify! of a super path with any delimiter; - an include inside a split impl's header or attributes; - an attribute sharing a line with an impl member. apply also deepens every path-leading super (use super as x, not only super::), rebases include paths written with {} or [] delimiters, wraps generated //! docs at 100 columns, and counts the two kinds of visibility edit separately. --- scripts/split_rust_module.py | 154 +++++++++++++++++++----- scripts/tests/test_split_rust_module.py | 124 +++++++++++++++++-- 2 files changed, 233 insertions(+), 45 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 3e7861067..46b8508ba 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -504,9 +504,41 @@ def split_top(mt: str, open_pos: int) -> list[tuple[int, int]]: MEMBER_RE = re.compile(r"(" + VIS + r")" + QUAL + r"(fn|const|type)\b") +def macro_group(mt: str, pos: int) -> tuple[int, int] | None: + """The span of the delimited token group starting at the first (, [ or { at or after pos.""" + m = re.compile(r"\s*([(\[{])").match(mt, pos) + if not m: + return None + depth, p = 0, m.start(1) + while p < len(mt): + if mt[p] in OPEN: + depth += 1 + elif mt[p] in CLOSE: + depth -= 1 + if depth == 0: + return m.start(1), p + 1 + p += 1 + return None + + +# `super` that starts a path or a use tree (`super::x`, `use super as y`, `use super;`), not +# the `super` of a `pub(super)` / `pub(in super..)` visibility. +SUPER_PATH = re.compile(r"(? list[int]: + out = [] + for m in SUPER_PATH.finditer(mt): + before = mt[max(0, m.start() - 12):m.start()] + if re.search(r"pub\s*\(\s*(?:in\s+)?$", before): + continue + out.append(m.start()) + return out + + REL_VIS = re.compile(r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self)\b)") -INCLUDE = re.compile(r"\binclude(?:_str|_bytes)?\s*!\s*\(") -PLAIN_INCLUDE = re.compile(r"(\binclude(?:_str|_bytes)?\s*!\s*\(\s*)\"([^\"\\]*)\"\s*\)") +INCLUDE = re.compile(r"\binclude(?:_str|_bytes)?\s*!\s*[(\[{]") +PLAIN_INCLUDE = re.compile(r"(\binclude(?:_str|_bytes)?\s*!\s*[(\[{]\s*)\"([^\"\\]*)\"\s*[)\]}]") def transform_item(text: str, kind: str, parent_path: list[str], deeper_dir: bool) -> tuple[str, dict]: @@ -523,6 +555,8 @@ def vis_edit(pos: int) -> None: if new is not None: edits.append((pos, len(old), new)) counts["vis"] = counts.get("vis", 0) + 1 + kind_key = "vis_super" if collapse(old) == "pub(super)" else "vis_private" + counts[kind_key] = counts.get(kind_key, 0) + 1 if old: edited_rel.add(pos) @@ -572,8 +606,13 @@ def vis_edit(pos: int) -> None: for m in REL_VIS.finditer(mt): if m.start() not in edited_rel: raise SystemExit("moved code has a nested pub(super)/pub(self) the tool cannot remap; edit by hand") - if re.search(r"\b(?:stringify|concat_idents)\s*!\s*\([^()]*\bsuper\b", mt): - raise SystemExit("moved code stringifies a `super` path; its text would change. Edit by hand.") + for m in re.finditer(r"\b(?:stringify|concat_idents)\s*!", mt): + g = macro_group(mt, m.end()) + if g and re.search(r"\bsuper\b", mt[g[0]:g[1]]): + raise SystemExit("moved code stringifies a `super` path; its text would change. Edit by hand.") + if re.search(r"\bmod\s+\w+\s*\{", mt) and re.search(r"\bsuper\b", mt): + raise SystemExit("moved code has an inline module and names `super`; which module each `super` " + "means depends on nesting the tool does not track. Edit by hand.") if re.search(r"\bself\s*::\s*super\b", mt): raise SystemExit("`self::super` in moved code changes meaning one module deeper; edit by hand") for m in re.finditer(r"\bsuper\s*::", text): @@ -581,8 +620,8 @@ def vis_edit(pos: int) -> None: raise SystemExit("a string literal in moved code names a `super::` path (an attribute argument?); " "the tool cannot tell whether it resolves relative to the module. Edit by hand.") # 3. super:: paths (code only) - for m in re.finditer(r"(? ImplLayout | N lead = i if lead is None else lead i += 1 continue + if s.startswith("#[") and attr_then_code(m): + raise SystemExit(f"line {i + 1}: an attribute and an impl member share a line; put the member on its own line") if s.startswith("#["): lead = i if lead is None else lead d = line_depth_delta(m) @@ -719,8 +760,8 @@ def impl_layout(lines: list[str], mlines: list[str], it: Item) -> ImplLayout | N def rewrite_super(text: str) -> str: mt = mask(text) - for m in reversed(list(re.finditer(r"(? list[str]: DEFS = {"struct", "enum", "union", "trait", "impl", "type", "const", "static", "macro"} +def doc_comment(doc: str) -> str: + """`//!` lines for a module doc, paragraphs separated by blank lines, wrapped at 100 columns.""" + import textwrap + + paras = [" ".join(p.split()) for p in doc.strip().split("\n\n") if p.strip()] + if not paras: + return "" + out = [] + for k, para in enumerate(paras): + if k: + out.append("//!") + out += textwrap.wrap(para, width=100, initial_indent="//! ", subsequent_indent="//! ", + break_long_words=False, break_on_hyphens=False) + return "\n".join(out) + "\n\n" + + def new_reach(orig_vis: str, parent_path: list[str]) -> str: """An item's reach after the move: "pub", "crate", "private" (the parent module only), or the crate-relative module path of a `pub(in crate::..)`.""" @@ -811,6 +868,9 @@ def impl_block(lines: list[str], it: Item, lay: ImplLayout, mids: list[int], lea """An impl block holding the members `mids` (indices into lay.members), in order.""" header = "\n".join(lines[it.start:lay.body_open + 1]) lead = "\n".join(lead_lines) + if child and deeper and INCLUDE.search(mask(lead + "\n" + header)): + raise SystemExit(f"{it.selector}: an include in a split impl's header or attributes would resolve " + "from a different directory. Edit by hand.") out = (rewrite_super(lead) if child else lead).split("\n") if lead_lines else [] out.extend((rewrite_super(header) if child else header).split("\n")) prev = None @@ -913,7 +973,9 @@ def invokes(text: str, name: str) -> bool: raise SystemExit(f"macro_rules! {name} is defined more than once in the parent and moved code " "invokes it; which definition it binds to would change. Edit by hand.") bodies = {it.name: "\n".join(mlines[it.start:it.end + 1]) for it in macro_defs} - relative = {n for n, b in bodies.items() if REL_VIS.search(b)} + # A macro whose expansion names `super`/`self` (a path or a visibility) resolves from the + # module it is expanded in, so its meaning changes one module deeper. + relative = {n for n, b in bodies.items() if REL_VIS.search(b) or re.search(r"\b(?:super|self)\s*::", b)} while True: more = {n for n, b in bodies.items() if n not in relative and any(invokes(b, r) for r in relative)} if not more: @@ -921,9 +983,24 @@ def invokes(text: str, name: str) -> bool: relative |= more for name in sorted(relative): if invokes(moved_text, name): - raise SystemExit(f"moved code invokes macro_rules! {name}, which emits a relative visibility " - "(pub(super)/pub(self)), directly or through another macro; its expansion would " - "change reach. Keep its callers in the parent or edit the macro by hand.") + raise SystemExit(f"moved code invokes macro_rules! {name}, whose expansion names `super`/`self` " + "(a path or pub(super)/pub(self)), directly or through another macro; it would " + "resolve differently one module deeper. Keep its callers in the parent or edit by hand.") + # Children are declared after every parent macro. A moved item that invokes a macro the + # parent defines only later bound to an outer macro of that name; in the child it would + # bind to the parent's definition instead. + first_def = {} + for it in macro_defs: + first_def.setdefault(it.name, it.start) + moved_units = [(items[k].start, "\n".join(mlines[items[k].start:items[k].end + 1])) for k in assign] + moved_units += [(layout(k).members[mi].start, + "\n".join(mlines[layout(k).members[mi].start:layout(k).members[mi].end + 1])) + for k, mi in massign] + for start, text in moved_units: + for name, line in first_def.items(): + if line > start and invokes(text, name): + raise SystemExit(f"moved code at line {start + 1} invokes {name}! before the parent defines it " + f"(line {line + 1}); its binding would change. Edit by hand.") split_impls = sorted({k for k, _ in massign}) for k in split_impls: if k in assign: @@ -967,8 +1044,13 @@ def invokes(text: str, name: str) -> bool: chunks.append(new) sels.append(it.selector) if it.kind == "macro": - # What a macro invocation emits is unknown: re-export at most `pub`, which a - # glob clamps to each emitted item's own visibility, so nothing widens. + # What a macro invocation emits is unknown: its items keep the visibility the + # macro gives them, which the tool cannot remap. Moving one needs an explicit + # opt-in; re-export at most `pub`, which a glob clamps, so nothing widens. + if not mod.get("allow_macro_items", False): + raise SystemExit(f"{it.selector}: a top-level macro invocation emits items whose visibility " + "the tool cannot remap; set allow_macro_items = true on the module if " + "every item it emits is pub or used only inside the module") counts["macro_items"] = counts.get("macro_items", 0) + 1 reach.add("pub") elif it.kind != "impl": @@ -993,14 +1075,13 @@ def invokes(text: str, name: str) -> bool: sels.append(it.selector) if not chunks: raise SystemExit(f"module {name}: no items") - doc = mod.get("doc", "").strip() - body = "" - if doc: - body += "".join(f"//! {d}\n" if d.strip() else "//!\n" for d in doc.split("\n")) + "\n" - glob = mod.get("glob_import", True) - if not isinstance(glob, bool): - raise SystemExit(f"module {name}: glob_import must be true or false") - body += ("use super::*;\n\n" if glob else "") + "\n\n".join(chunks) + "\n" + body = doc_comment(mod.get("doc", "")) + allow_unused = mod.get("allow_unused_glob", False) + if "glob_import" in mod or not isinstance(allow_unused, bool): + raise SystemExit(f"module {name}: the parent glob import is always kept; use " + "allow_unused_glob = true for a module that needs nothing from its parent") + glob = "#[allow(unused_imports)]\nuse super::*;\n\n" if allow_unused else "use super::*;\n\n" + body += glob + "\n\n".join(chunks) + "\n" ks = [k for k in range(len(items)) if assign.get(k) == name or (k in split_impls and any(massign.get((k, mi)) == name for mi in range(len(layout(k).members))))] cfgs = [set(items[k].cfg) for k in ks] @@ -1223,7 +1304,8 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str src = git_show(repo, head, child) or "" items, hdr = parse_items(src) lines, mlines = src.split("\n"), mask(src).split("\n") - doc = "\n".join(re.sub(r"^//! ?", "", ln.strip()) for ln in lines[:hdr] if ln.strip().startswith("//!")) + doc_lines = [re.sub(r"^//! ?", "", ln.strip()) for ln in lines[:hdr] if ln.strip().startswith("//!")] + doc = "\n".join(doc_lines).replace("\n\n", "\0").replace("\n", " ").replace("\0", "\n\n") sels: list[str] = [] for it in items: if it.kind == "use" and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;": @@ -1235,13 +1317,15 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str sels += [f"{escape_glob(base_sel)} :: {escape_glob(m.selector)}" for m in lay.members] else: sels.append(escape_glob(it.selector)) - has_glob = any(it.kind == "use" and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;" - for it in items) + glob_lead = next(("\n".join(lines[it.lead:it.start]) for it in items if it.kind == "use" + and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;"), "") out.append(f"\n[[module]]\nname = {json.dumps(name)}") if doc: out.append(f"doc = {json.dumps(doc)}") - if not has_glob: - out.append("glob_import = false") + if "allow(unused_imports)" in glob_lead: + out.append("allow_unused_glob = true") + if any(it.kind == "macro" for it in items): + out.append("allow_macro_items = true") out.append("items = [\n" + "".join(f" {json.dumps(s)},\n" for s in dict.fromkeys(sels)) + "]") return "\n".join(out) + "\n" @@ -1270,13 +1354,17 @@ def verify(repo: str, base: str, head: str, print_spec: bool = False) -> int: problems: list[str] = [f"{p}: deleted" for st, p in status if st == "D"] problems += [f"{p}: {st}" for st, p in status if st not in ("A", "M", "D")] problems += [f"{p}: not a regular file (mode {modes[p][0]} -> {modes[p][1]})" for p in sorted(irregular)] + def children_dir(parent: str) -> str: + return os.path.dirname(parent) if file_layout(parent)[1] else parent[:-3] + groups: dict[str, list[str]] = {} for p in added: - parent = next((c for c in parent_candidates(p) if c in modified), None) - if parent is None: - problems.append(f"{p}: new file whose parent module is not part of the change") + owners = [m for m in sorted(modified) if "/src/" in m and children_dir(m) == os.path.dirname(p)] + if len(owners) != 1: + problems.append(f"{p}: new file whose parent module is not part of the change" if not owners + else f"{p}: more than one changed file could own it: {owners}") continue - groups.setdefault(parent, []).append(os.path.splitext(os.path.basename(p))[0]) + groups.setdefault(owners[0], []).append(os.path.splitext(os.path.basename(p))[0]) for p in sorted(modified - set(groups)): problems.append(f"{p}: changed, but no child module was split out of it") checked = 0 @@ -1304,8 +1392,10 @@ def verify(repo: str, base: str, head: str, print_spec: bool = False) -> int: beside = file_layout(parent)[1] cdir = os.path.dirname(parent) if beside else parent[:-3] for rel in [parent] + [f"{cdir}/{c}.rs" for c in children]: + # Format only the regenerated side: head must be exactly `apply` + rustfmt, so an + # edit rustfmt would erase (a trailing comma inside a macro call) still differs. want = rustfmt(repo, open(os.path.join(tmp, rel), encoding="utf-8").read(), edition) - got = rustfmt(repo, git_show(repo, head, rel) or "", edition) + got = git_show(repo, head, rel) or "" checked += 1 if want != got: diff = list(difflib.unified_diff(want.split("\n"), got.split("\n"), "regenerated/" + rel, diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index 12eedf2b1..fbe8915fb 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -8,6 +8,7 @@ from __future__ import annotations import os +import re import subprocess import sys import textwrap @@ -134,9 +135,18 @@ def repo(tmp_path: Path) -> Path: return tmp_path +def rustfmt_all(repo: Path) -> None: + """What `cargo fmt` does to a real split before it is committed.""" + for f in sorted((repo / "crate").rglob("*.rs")): + out = subprocess.run(["rustfmt", "--edition", "2024", "--emit", "stdout", "--quiet"], + input=f.read_text(), capture_output=True, text=True, check=True).stdout + f.write_text(out) + + def apply_and_commit(repo: Path) -> None: r = run("apply", "spec.toml", cwd=repo) assert r.returncode == 0, r.stderr + rustfmt_all(repo) git(repo, "add", "crate") git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qm", "split") @@ -197,7 +207,8 @@ def test_apply_writes_children_and_keeps_parent_paths(repo): # Fields and inherent-impl members keep the parent's reach; trait impls are untouched. assert " pub name: String,\n pub(super) inner: u32,\n pub(in crate::a) shared: bool," in config assert " pub(super) routes: HashMap>," in config - assert "pub(super) struct Tuple(pub(super) u32, pub String, #[allow(dead_code)] pub(super) Box u8>);" in config + tuple = re.sub(r"\s+", " ", config[config.index("pub(super) struct Tuple("):]) + assert tuple.startswith("pub(super) struct Tuple( pub(super) u32, pub String, #[allow(dead_code)] pub(super) Box u8>, );") assert " pub(super) fn private_helper(&self) -> u32 {" in config assert ' let s = "fn not_a_member() {}";' in config assert " pub(in crate::a) const LIMIT: u32 = 5;" in config @@ -227,13 +238,21 @@ def test_verify_accepts_the_move_and_rejects_an_edit(repo): assert "foo/config.rs: differs from what apply generates" in bad.stdout and "self.inner + 1" in bad.stdout -def test_verify_ignores_rustfmt_rewraps(repo): +def test_verify_rejects_a_head_that_is_not_rustfmt_formatted(repo): apply_and_commit(repo) parsing = repo / "crate/src/a/foo/parsing.rs" parsing.write_text(parsing.read_text().replace("pub(super) fn parse(x: &str) -> u32 {", "pub(super) fn parse(\n x: &str,\n) -> u32 {")) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "fmt") - assert run("verify", "HEAD~2", cwd=repo).returncode == 0 + assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + + +def test_verify_rejects_a_token_edit_rustfmt_would_erase(repo): + apply_and_commit(repo) + parsing = repo / "crate/src/a/foo/parsing.rs" + parsing.write_text(parsing.read_text().replace("x.len() as u32", "vec![x.len() as u32,][0]")) + git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") + assert run("verify", "HEAD~2", cwd=repo).returncode == 1 def test_verify_sees_edits_inside_string_literals(repo): @@ -391,14 +410,6 @@ def test_moved_callers_of_a_macro_that_emits_relative_visibility_are_refused(rep assert r.returncode != 0 and "macro_rules! helper" in r.stderr -def test_verify_ignores_a_trailing_comma_rustfmt_adds_in_generics(repo): - apply_and_commit(repo) - parsing = repo / "crate/src/a/foo/parsing.rs" - parsing.write_text(parsing.read_text().replace("impl Holder", "impl<\n F,\n> Holder")) - git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "fmt") - assert run("verify", "HEAD~2", cwd=repo).returncode == 0 - - @pytest.mark.parametrize( ("path", "old", "new"), [ @@ -724,8 +735,8 @@ def test_split_crate_compiles_and_keeps_behavior(tmp_path): assert build_and_test(tmp_path / "oracle").returncode == 0 (tmp_path / "spec.toml").write_text( 'file = "oracle/src/a/foo.rs"\n' - '[[module]]\nname = "pair"\nglob_import = false\nitems = ["struct Pair", "impl Pair :: fn narrow"]\n' - '[[module]]\nname = "helpers"\nglob_import = false\nitems = ["fn shared", "fn private_helper"]\n' + '[[module]]\nname = "pair"\nallow_unused_glob = true\nitems = ["struct Pair", "impl Pair :: fn narrow"]\n' + '[[module]]\nname = "helpers"\nallow_unused_glob = true\nitems = ["fn shared", "fn private_helper"]\n' ) r = run("apply", "spec.toml", cwd=tmp_path) assert r.returncode == 0, r.stderr @@ -784,3 +795,90 @@ def test_the_path_attribute_scan_stays_inside_the_crate(tmp_path): (tmp_path / "repo/c/src").mkdir(parents=True) (tmp_path / "repo/c/src/a.rs").write_text("fn f() {}\n") assert srm.path_attribute_users(str(tmp_path / "repo"), "c/src/a.rs") == "" + + +# --- Round-2 review cases. --- + + +def test_verify_rejects_a_removed_glob_import(repo): + apply_and_commit(repo) + child = repo / "crate/src/a/foo/k8s.rs" + child.write_text(child.read_text().replace("use super::*;\n\n", "")) + commit_all(repo, "drop glob") + assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + + +@pytest.mark.parametrize( + ("text", "items", "message"), + [ + # a parent macro whose expansion names a super:: path + ("macro_rules! m {\n () => {\n super::V\n };\n}\n\nfn f() -> u8 {\n m!()\n}\n", + '["fn f"]', "expansion names"), + # an ancestor macro of the same name would be shadowed by the later local definition + ("fn f() -> u8 {\n m!()\n}\n\nmacro_rules! m {\n () => {\n 2\n };\n}\n", + '["fn f"]', "before the parent defines it"), + # a top-level macro invocation without opt-in + ("make!();\n\nfn g() {}\n", '["macro make"]', "allow_macro_items"), + # an inline module nested inside a moved function, with super + ("fn f() -> u8 {\n mod i {\n pub fn g() -> u8 {\n super::super::V\n }\n }\n i::g()\n}\n", + '["fn f"]', "inline module"), + # stringify with braces + ("fn f() -> &'static str {\n stringify! {super::hello}\n}\n", '["fn f"]', "stringifies"), + # an include in a split impl's header + ("struct S;\n\nimpl S<{ include_str!(\"size.txt\").len() }> {\n fn a() {}\n\n fn b() {}\n}\n", + '["impl S<* :: fn b"]', "include"), + # one-line attribute on an impl member + ("struct S;\n\nimpl S {\n #[inline] fn a() {}\n\n fn b() {}\n}\n", '["impl S :: fn b"]', "share a line"), + ], +) +def test_round_two_refusals(repo, text, items, message): + r = write_parent(repo, text, f'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = {items}\n') + assert r.returncode != 0 and message in r.stderr, r.stderr + + +def test_terminal_super_in_use_trees_is_deepened(repo): + r = write_parent(repo, "fn f() -> u8 {\n use super as outer;\n outer::V\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + assert "use super::super as outer;" in (repo / "crate/src/a/foo/x.rs").read_text() + + +def test_include_with_brace_delimiters_is_rebased(repo): + r = write_parent(repo, "fn f() -> &'static str {\n include_str! {\"data.txt\"}\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + assert 'include_str! {"../data.txt"}' in (repo / "crate/src/a/foo/x.rs").read_text() + + +def test_macro_items_move_with_opt_in(repo): + r = write_parent(repo, "make!();\n\nfn g() {}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nallow_macro_items = true\nitems = ["macro make"]\n') + assert r.returncode == 0, r.stderr + assert "pub use x::*;" in (repo / "crate/src/a/foo.rs").read_text() + + +def test_module_docs_wrap_at_100_columns_and_verify_round_trips(repo): + long = "A long module description " * 8 + (repo / "spec.toml").write_text(SPEC.replace('doc = "The configuration type."', f'doc = "{long.strip()}"')) + apply_and_commit(repo) + config = (repo / "crate/src/a/foo/config.rs").read_text() + header = [ln for ln in config.split("\n") if ln.startswith("//!")] + assert len(header) > 1 and all(len(ln) <= 100 for ln in header) + assert run("verify", "HEAD~1", cwd=repo).returncode == 0 + + +def test_a_binary_crate_root_round_trips_through_verify(tmp_path): + root = tmp_path / "r" + (root / "c/src/bin").mkdir(parents=True) + (root / "c/src/bin/tool.rs").write_text("fn helper() -> u8 {\n 1\n}\n\nfn main() {\n helper();\n}\n") + git(root, "init", "-q") + commit_all(root, "base") + (root / "spec.toml").write_text('file = "c/src/bin/tool.rs"\n[[module]]\nname = "helpers"\nitems = ["fn helper"]\n') + assert run("apply", "spec.toml", cwd=root).returncode == 0 + assert (root / "c/src/bin/helpers.rs").exists() + for f in (root / "c").rglob("*.rs"): + f.write_text(subprocess.run(["rustfmt", "--edition", "2024", "--emit", "stdout", "--quiet"], + input=f.read_text(), capture_output=True, text=True, check=True).stdout) + commit_all(root, "split") + v = run("verify", "HEAD~1", cwd=root) + assert v.returncode == 0, v.stdout From d2d6c596a464a106e6857a81f15efff885ecab9b Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 02:43:45 -0400 Subject: [PATCH 08/15] scripts: close the remaining split_rust_module.py review findings verify: - Read file bytes without newline translation, and reject a carriage return. rustc ends a // comment only at \n, so a lone CR after a comment turns the next line into comment text that Python's newline translation would hide. - Map every impl, not only inherent ones, back to its base selector, and derive a whole-impl selector when one child holds all of an impl's members. A whole move of an impl with super:: in its header or #[doc = include_str!] on it no longer fails as a false reject. - Write the derived spec without ASCII escaping, so non-ASCII docs round-trip. - Resolve the repository root, so verify works from a subdirectory. - List the non-Rust files a change also touches, which verify does not check. apply: - Refuse include! of Rust code, whose paths resolve from the module it is expanded in. include_str!/include_bytes! paths are still rebased. - Refuse a file with a carriage return. - Refuse a src/bin/.rs crate root: its children would sit in src/bin/, where Cargo builds every file as its own binary. - Refuse a module name the parent already uses as a path (a crate or a glob-imported module), which the new module would shadow. - Ignore #[...] inside block comments when reading an item's attributes, and refuse a multi-line attribute that ends on its item's line. - Leave const _ out of the re-export choice, so a module of only impls and const _ gets no unused glob re-export. Remove dead ranking code. Tests: 82, with message checks on the rejection tests and cases for each finding above. --- scripts/split_rust_module.py | 86 ++++++++++------- scripts/tests/test_split_rust_module.py | 121 +++++++++++++++++++----- 2 files changed, 149 insertions(+), 58 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 46b8508ba..82f66d11f 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -265,6 +265,9 @@ def parse_items(src: str) -> tuple[list[Item], int]: while d > 0 and i < n: d += line_depth_delta(mlines[i]) i += 1 + if d == 0 and i - 1 > lead and re.search(r"\][^\]]*\S", mlines[i - 1]) and \ + not mlines[i - 1].rstrip().endswith("]"): + raise SystemExit(f"line {i}: an attribute and an item share a line; put the item on its own line") continue if s.startswith("#["): raise SystemExit(f"line {i + 1}: an attribute and an item share a line; put the item on its own line") @@ -309,7 +312,7 @@ def attrs_of(lines: list[str], lead: int, start: int) -> list[str]: out, k = [], lead mlines = mask("\n".join(lines[lead:start])).split("\n") while k < start: - if lines[k].strip().startswith("#["): + if mlines[k - lead].strip().startswith("#["): d = line_depth_delta(mlines[k - lead]) group = [lines[k]] k += 1 @@ -537,8 +540,8 @@ def super_path_positions(mt: str) -> list[int]: REL_VIS = re.compile(r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self)\b)") -INCLUDE = re.compile(r"\binclude(?:_str|_bytes)?\s*!\s*[(\[{]") -PLAIN_INCLUDE = re.compile(r"(\binclude(?:_str|_bytes)?\s*!\s*[(\[{]\s*)\"([^\"\\]*)\"\s*[)\]}]") +INCLUDE = re.compile(r"\binclude_(?:str|bytes)\s*!\s*[(\[{]") +PLAIN_INCLUDE = re.compile(r"(\binclude_(?:str|bytes)\s*!\s*[(\[{]\s*)\"([^\"\\]*)\"\s*[)\]}]") def transform_item(text: str, kind: str, parent_path: list[str], deeper_dir: bool) -> tuple[str, dict]: @@ -613,6 +616,9 @@ def vis_edit(pos: int) -> None: if re.search(r"\bmod\s+\w+\s*\{", mt) and re.search(r"\bsuper\b", mt): raise SystemExit("moved code has an inline module and names `super`; which module each `super` " "means depends on nesting the tool does not track. Edit by hand.") + if re.search(r"\binclude\s*!\s*[(\[{]", mt): + raise SystemExit("moved code uses include!; the included Rust code resolves its paths from the module " + "it is expanded in, which the tool cannot rewrite. Edit by hand.") if re.search(r"\bself\s*::\s*super\b", mt): raise SystemExit("`self::super` in moved code changes meaning one module deeper; edit by hand") for m in re.finditer(r"\bsuper\s*::", text): @@ -784,7 +790,6 @@ def attr_lines(lines: list[str], lead: int, start: int) -> list[str]: return out -RANK = {"none": -1, "private": 0, "in": 1, "crate": 2, "pub": 3} TYPE_NS = {"struct", "enum", "union", "trait", "type", "mod", "mod-decl"} DEFS = {"struct", "enum", "union", "trait", "impl", "type", "const", "static", "macro"} @@ -841,20 +846,6 @@ def reexport_vis(reach: set[str]) -> str: return "pub(crate) " if not common else f"pub(in crate::{'::'.join(common)}) " -def new_vis_rank(orig_vis: str, parent_path: list[str]) -> tuple[int, str]: - """Rank of an item's visibility after the move, for choosing the re-export.""" - v = collapse(orig_vis) - mapped = map_vis(v, parent_path) - v = collapse(mapped) if mapped is not None else v - if v == "pub": - return RANK["pub"], "" - if v == "pub(crate)": - return RANK["crate"], "" - if v.startswith("pub(in "): - return RANK["in"], v - return RANK["private"], "" # pub(super): reaches the parent only - - def code_lines(text: str) -> int: return sum(1 for ln in mask(text).split("\n") if ln.strip()) @@ -898,11 +889,18 @@ def apply(spec_path: str, repo: str, dry: bool, report: str | None) -> int: spec = tomllib.load(open(spec_path, "rb")) rel = spec["file"] path = os.path.join(repo, rel) - src = open(path, encoding="utf-8").read() + with open(path, encoding="utf-8", newline="") as fh: + src = fh.read() + if "\r" in src: + raise SystemExit(f"{rel} contains a carriage return; rustc treats a lone CR differently from a newline. " + "Normalize line endings first.") items, header_end = parse_items(src) lines = src.split("\n") mlines = mask(src).split("\n") parent_path, beside = file_layout(rel) + if beside and re.search(r"/src/bin/[^/]+\.rs$", rel.replace(os.sep, "/")): + raise SystemExit(f"{rel}: children of a src/bin/*.rs crate root would sit in src/bin/, where Cargo builds " + "every file as its own binary. Move it to src/bin//main.rs first.") deeper = not beside child_dir = os.path.join(os.path.dirname(path), os.path.basename(rel)[:-3]) if deeper else os.path.dirname(path) loaded_by_path = path_attribute_users(repo, rel) @@ -1015,9 +1013,13 @@ def invokes(text: str, name: str) -> bool: for it in items: if it.kind in TYPE_NS and it.name in names: raise SystemExit(f"module name {it.name!r} collides with {it.selector}") + masked_src = mask(src) for n in names: if n in imported: raise SystemExit(f"module name {n!r} collides with a name the parent imports") + if re.search(r"(? bool: "every item it emits is pub or used only inside the module") counts["macro_items"] = counts.get("macro_items", 0) + 1 reach.add("pub") - elif it.kind != "impl": + elif it.kind != "impl" and not (it.kind == "const" and it.name == "_"): reach.add(new_reach(it.vis, parent_path)) elif k in split_impls: lay = layout(k) @@ -1245,8 +1247,9 @@ def git(repo: str, *args: str) -> str: def git_show(repo: str, rev: str, path: str) -> str | None: - r = subprocess.run(["git", "-C", repo, "show", f"{rev}:{path}"], capture_output=True, text=True) - return r.stdout if r.returncode == 0 else None + """A file's bytes at a revision, decoded without newline translation (a lone CR stays a CR).""" + r = subprocess.run(["git", "-C", repo, "show", f"{rev}:{path}"], capture_output=True) + return r.stdout.decode("utf-8") if r.returncode == 0 else None def parent_candidates(child: str) -> list[str]: @@ -1291,13 +1294,15 @@ def escape_glob(sel: str) -> str: def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str]) -> str: """The spec `apply` would need to turn the base parent into the head files.""" - base_items = parse_items(git_show(repo, base, parent) or "")[0] + base_src = git_show(repo, base, parent) or "" + base_items = parse_items(base_src)[0] + base_lines, base_mlines = base_src.split("\n"), mask(base_src).split("\n") head_parent = git_show(repo, head, parent) or "" hp_items = parse_items(head_parent)[0] hp_lines = head_parent.split("\n") order = [it.name for it in hp_items if it.kind == "mod-decl"] names = sorted(children, key=lambda n: order.index(n) if n in order else len(order)) - out = [f"file = {json.dumps(parent)}"] + out = [f"file = {json.dumps(parent, ensure_ascii=False)}"] for name in names: child = os.path.join(os.path.dirname(parent), os.path.basename(parent)[:-3], f"{name}.rs") \ if not file_layout(parent)[1] else os.path.join(os.path.dirname(parent), f"{name}.rs") @@ -1310,23 +1315,32 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str for it in items: if it.kind == "use" and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;": continue - lay = impl_layout(lines, mlines, it) if it.kind == "impl" else None - if lay and lay.members: - base_sel = next((b.selector for b in base_items if b.kind == "impl" - and header_key(b.selector) == header_key(it.selector)), it.selector) - sels += [f"{escape_glob(base_sel)} :: {escape_glob(m.selector)}" for m in lay.members] - else: + if it.kind != "impl": sels.append(escape_glob(it.selector)) + continue + base = [b for b in base_items if b.kind == "impl" and header_key(b.selector) == header_key(it.selector)] + base_sel = base[0].selector if base else it.selector + lay = impl_layout(lines, mlines, it) + if not (lay and lay.members): + sels.append(escape_glob(base_sel)) + continue + moved = [m.selector for m in lay.members] + whole = any(sorted(m.selector for m in (impl_layout(base_lines, base_mlines, b) or ImplLayout(0, [], 0)).members) + == sorted(moved) for b in base) + if whole and len(base) == 1: + sels.append(escape_glob(base_sel)) + else: + sels += [f"{escape_glob(base_sel)} :: {escape_glob(m)}" for m in moved] glob_lead = next(("\n".join(lines[it.lead:it.start]) for it in items if it.kind == "use" and collapse("\n".join(lines[it.start:it.end + 1])) == "use super::*;"), "") out.append(f"\n[[module]]\nname = {json.dumps(name)}") if doc: - out.append(f"doc = {json.dumps(doc)}") + out.append(f"doc = {json.dumps(doc, ensure_ascii=False)}") if "allow(unused_imports)" in glob_lead: out.append("allow_unused_glob = true") if any(it.kind == "macro" for it in items): out.append("allow_macro_items = true") - out.append("items = [\n" + "".join(f" {json.dumps(s)},\n" for s in dict.fromkeys(sels)) + "]") + out.append("items = [\n" + "".join(f" {json.dumps(x, ensure_ascii=False)},\n" for x in dict.fromkeys(sels)) + "]") return "\n".join(out) + "\n" @@ -1339,8 +1353,11 @@ def verify(repo: str, base: str, head: str, print_spec: bool = False) -> int: import shutil import tempfile + repo = git(repo, "rev-parse", "--show-toplevel").strip() merge_base = git(repo, "merge-base", base, head).strip() raw = git(repo, "diff", "--raw", "-z", "--no-renames", "--no-abbrev", f"{merge_base}..{head}", "--", "*.rs") + other = [p for p in git(repo, "diff", "--name-only", "--no-renames", f"{merge_base}..{head}").splitlines() + if not p.endswith(".rs")] fields = raw.split("\0") status, modes = [], {} for k in range(0, len(fields) - 1, 2): @@ -1367,6 +1384,9 @@ def children_dir(parent: str) -> str: groups.setdefault(owners[0], []).append(os.path.splitext(os.path.basename(p))[0]) for p in sorted(modified - set(groups)): problems.append(f"{p}: changed, but no child module was split out of it") + for p in sorted(set(added) | modified): + if "\r" in (git_show(repo, head, p) or ""): + problems.append(f"{p}: contains a carriage return") checked = 0 for parent, children in groups.items(): spec = derive_spec(repo, merge_base, head, parent, children) @@ -1409,6 +1429,8 @@ def children_dir(parent: str) -> str: f"{'identical' if not problems else str(len(problems)) + ' problems'}") for p in problems: print(" " + p) + if other: + print("not checked by verify (not Rust): " + ", ".join(other)) return 1 if problems else 0 diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index fbe8915fb..b0fc68c13 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -244,7 +244,8 @@ def test_verify_rejects_a_head_that_is_not_rustfmt_formatted(repo): parsing.write_text(parsing.read_text().replace("pub(super) fn parse(x: &str) -> u32 {", "pub(super) fn parse(\n x: &str,\n) -> u32 {")) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "fmt") - assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "parsing.rs: differs from what apply generates" in bad.stdout def test_verify_rejects_a_token_edit_rustfmt_would_erase(repo): @@ -252,7 +253,8 @@ def test_verify_rejects_a_token_edit_rustfmt_would_erase(repo): parsing = repo / "crate/src/a/foo/parsing.rs" parsing.write_text(parsing.read_text().replace("x.len() as u32", "vec![x.len() as u32,][0]")) git(repo, "-c", "user.email=t@example.com", "-c", "user.name=t", "commit", "-qam", "edit") - assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "parsing.rs: differs from what apply generates" in bad.stdout def test_verify_sees_edits_inside_string_literals(repo): @@ -455,7 +457,8 @@ def test_verify_rejects_edits_a_loose_comparison_would_miss(repo, path, old, new assert old in f.read_text() f.write_text(f.read_text().replace(old, new, 1)) commit_all(repo, "edit") - assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and ("differs from what apply generates" in bad.stdout or "regenerating" in bad.stdout), bad.stdout + bad.stderr def test_verify_rejects_doc_comments_swapped_between_impls(repo): @@ -469,7 +472,8 @@ def test_verify_rejects_doc_comments_swapped_between_impls(repo): src.write_text(text.replace("/// Config docs.", "/// TMP").replace("/// Holder docs.", "/// Config docs.") .replace("/// TMP", "/// Holder docs.")) commit_all(repo, "swap") - assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "differs from what apply generates" in bad.stdout def write_parent(repo: Path, text: str, spec: str) -> subprocess.CompletedProcess: @@ -580,27 +584,22 @@ def test_file_layout(rel, segments, beside): assert srm.file_layout(rel) == (segments, beside) -def test_include_paths_are_rebased_or_refused(repo): +def test_include_str_paths_are_rebased_and_include_is_refused(repo): ok = write_parent(repo, """\ fn data() -> &'static str { include_str!("data.txt") } - - fn items() { - include!("gen.rs"); - } - """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn data", "fn items"]\n') + """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn data"]\n') assert ok.returncode == 0, ok.stderr - child = (repo / "crate/src/a/foo/x.rs").read_text() - assert 'include_str!("../data.txt")' in child and 'include!("../gen.rs")' in child - git(repo, "reset", "-q", "--hard", "HEAD") - git(repo, "clean", "-qfd") - bad = write_parent(repo, """\ - fn raw() -> &'static str { - include_str!(r"data.txt") - } - """, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "y"\nitems = ["fn raw"]\n') - assert bad.returncode != 0 and "cannot rebase" in bad.stderr + assert 'include_str!("../data.txt")' in (repo / "crate/src/a/foo/x.rs").read_text() + for text, message in [ + ("fn items() {\n include!(\"gen.rs\");\n}\n", "include!"), + ("fn raw() -> &'static str {\n include_str!(r\"data.txt\")\n}\n", "cannot rebase"), + ]: + git(repo, "reset", "-q", "--hard", "HEAD") + git(repo, "clean", "-qfd") + bad = write_parent(repo, text, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "y"\nitems = ["fn *"]\n') + assert bad.returncode != 0 and message in bad.stderr, bad.stderr def test_split_impl_attributes_follow_the_header_rewrite(repo): @@ -805,7 +804,8 @@ def test_verify_rejects_a_removed_glob_import(repo): child = repo / "crate/src/a/foo/k8s.rs" child.write_text(child.read_text().replace("use super::*;\n\n", "")) commit_all(repo, "drop glob") - assert run("verify", "HEAD~2", cwd=repo).returncode == 1 + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "k8s.rs: differs from what apply generates" in bad.stdout @pytest.mark.parametrize( @@ -867,18 +867,87 @@ def test_module_docs_wrap_at_100_columns_and_verify_round_trips(repo): assert run("verify", "HEAD~1", cwd=repo).returncode == 0 -def test_a_binary_crate_root_round_trips_through_verify(tmp_path): +def bin_repo(tmp_path: Path, rel: str) -> Path: root = tmp_path / "r" - (root / "c/src/bin").mkdir(parents=True) - (root / "c/src/bin/tool.rs").write_text("fn helper() -> u8 {\n 1\n}\n\nfn main() {\n helper();\n}\n") + (root / rel).parent.mkdir(parents=True) + (root / rel).write_text("fn helper() -> u8 {\n 1\n}\n\nfn main() {\n helper();\n}\n") git(root, "init", "-q") commit_all(root, "base") - (root / "spec.toml").write_text('file = "c/src/bin/tool.rs"\n[[module]]\nname = "helpers"\nitems = ["fn helper"]\n') + (root / "spec.toml").write_text(f'file = "{rel}"\n[[module]]\nname = "helpers"\nitems = ["fn helper"]\n') + return root + + +def test_a_src_bin_file_root_is_refused(tmp_path): + root = bin_repo(tmp_path, "c/src/bin/tool.rs") + r = run("apply", "spec.toml", cwd=root) + assert r.returncode != 0 and "own binary" in r.stderr + + +def test_a_src_bin_directory_root_round_trips_through_verify(tmp_path): + root = bin_repo(tmp_path, "c/src/bin/tool/main.rs") assert run("apply", "spec.toml", cwd=root).returncode == 0 - assert (root / "c/src/bin/helpers.rs").exists() + assert (root / "c/src/bin/tool/helpers.rs").exists() for f in (root / "c").rglob("*.rs"): f.write_text(subprocess.run(["rustfmt", "--edition", "2024", "--emit", "stdout", "--quiet"], input=f.read_text(), capture_output=True, text=True, check=True).stdout) commit_all(root, "split") v = run("verify", "HEAD~1", cwd=root) assert v.returncode == 0, v.stdout + + +def test_a_carriage_return_is_refused_and_rejected(repo): + src = repo / "crate/src/a/foo.rs" + src.write_bytes(src.read_bytes().replace(b"// Parsing.\n", b"// Parsing.\r\n")) + commit_all(repo, "cr") + r = run("apply", "spec.toml", cwd=repo) + assert r.returncode != 0 and "carriage return" in r.stderr + git(repo, "reset", "-q", "--hard", "HEAD~1") + (repo / "spec.toml").write_text(SPEC) + apply_and_commit(repo) + child = repo / "crate/src/a/foo/parsing.rs" + child.write_bytes(child.read_bytes().replace(b"// Parsing.\n", b"// Parsing.\r")) + commit_all(repo, "cr edit") + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "carriage return" in bad.stdout + + +def test_an_attribute_inside_a_block_comment_is_not_an_attribute(repo): + r = write_parent(repo, "/*\n#[cfg(any())]\n*/\nfn f() {}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + assert "#[cfg(any())]\nmod x;" not in (repo / "crate/src/a/foo.rs").read_text() + + +def test_a_module_name_already_used_as_a_path_is_refused(repo): + r = write_parent(repo, "use crate::prelude::*;\n\nfn f() -> u8 {\n util::one()\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "util"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "already used as a path" in r.stderr + + +def test_verify_round_trips_tricky_but_valid_splits(repo): + (repo / "crate/src/a/foo.rs").write_text(textwrap.dedent("""\ + trait T {} + + struct S; + + impl super::Marker for S {} + + #[doc = include_str!("doc.md")] + impl S { + fn a() {} + } + + const _: () = (); + """)) + commit_all(repo, "tricky") + (repo / "spec.toml").write_text( + 'file = "crate/src/a/foo.rs"\n' + '[[module]]\nname = "impls"\ndoc = "Impls \u2014 with a non-ASCII dash."\n' + 'items = ["impl super::Marker for S", "impl S", "const _"]\n' + ) + apply_and_commit(repo) + parent = (repo / "crate/src/a/foo.rs").read_text() + assert "mod impls;" in parent and "use impls::*;" not in parent # nothing nameable to re-export + assert "impl super::super::Marker for S {}" in (repo / "crate/src/a/foo/impls.rs").read_text() + v = run("verify", "HEAD~1", cwd=repo) + assert v.returncode == 0, v.stdout From 8c927f571c328038cf2ca969696c0a3ef5716b5e Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 02:49:50 -0400 Subject: [PATCH 09/15] scripts: document allow_unused_glob and allow_macro_items in split_rust_module.py --- scripts/split_rust_module.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 82f66d11f..19ce3cb09 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -38,8 +38,10 @@ name = "bar" doc = "What bar holds." # one `//!` line at the top of the new file items = ["fn parse_*", "struct Config", "impl Config", "impl Display for Config"] - # optional: glob_import = false (omit `use super::*;` when the module needs nothing - # from its parent, which `-D warnings` would otherwise reject as unused) + # optional: allow_unused_glob = true (the module needs nothing from its parent: keep + # `use super::*;` under #[allow(unused_imports)], which -D warnings would reject) + # optional: allow_macro_items = true (move top-level macro invocations; their emitted + # items keep the visibility the macro gives them) Selectors are fnmatch patterns over ` `; impl blocks are `impl
` with whitespace collapsed (e.g. `impl From for Foo`). From 795b60e0a9a4f8d1c812cf637f80f5992f0628d4 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 02:57:03 -0400 Subject: [PATCH 10/15] scripts: close split_rust_module.py's third-round review findings - Check macro textual scope transitively: a moved item that reaches a parent macro through another parent macro (relay!() expanding to value!()) is refused when that macro is defined later in the parent or more than once, like a direct invocation. - Detect a terminal super (use super as x) in parent macro bodies, as in moved code. - Read the attributes copied onto split impl blocks from masked text, so a #[cfg(..)] inside a block comment is not copied as a real one. - Refuse a multi-line impl-member attribute that ends on its member's line, which would otherwise attach the member to the next one. Tests: 86. --- scripts/split_rust_module.py | 30 +++++++++++++++++++++---- scripts/tests/test_split_rust_module.py | 30 +++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 19ce3cb09..399003e04 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -736,11 +736,14 @@ def impl_layout(lines: list[str], mlines: list[str], it: Item) -> ImplLayout | N raise SystemExit(f"line {i + 1}: an attribute and an impl member share a line; put the member on its own line") if s.startswith("#["): lead = i if lead is None else lead + first_attr = i d = line_depth_delta(m) i += 1 while d > 0 and i < it.end: d += line_depth_delta(mlines[i]) i += 1 + if d == 0 and i - 1 > first_attr and not mlines[i - 1].rstrip().endswith("]"): + raise SystemExit(f"line {i}: an attribute and an impl member share a line; put the member on its own line") continue start, d = i, 0 while True: @@ -774,11 +777,12 @@ def rewrite_super(text: str) -> str: def attr_lines(lines: list[str], lead: int, start: int) -> list[str]: - """The attribute lines (whole multi-line attributes) in an item's leading trivia.""" + """The attribute lines (whole multi-line attributes) in an item's leading trivia; an + attribute written inside a comment is not one.""" out, k = [], lead mlines = mask("\n".join(lines[lead:start])).split("\n") while k < start: - s = lines[k].strip() + s = mlines[k - lead].strip() if s.startswith("#["): d = line_depth_delta(mlines[k - lead]) out.append(lines[k]) @@ -975,7 +979,8 @@ def invokes(text: str, name: str) -> bool: bodies = {it.name: "\n".join(mlines[it.start:it.end + 1]) for it in macro_defs} # A macro whose expansion names `super`/`self` (a path or a visibility) resolves from the # module it is expanded in, so its meaning changes one module deeper. - relative = {n for n, b in bodies.items() if REL_VIS.search(b) or re.search(r"\b(?:super|self)\s*::", b)} + relative = {n for n, b in bodies.items() + if REL_VIS.search(b) or super_path_positions(b) or re.search(r"\bself\s*::", b)} while True: more = {n for n, b in bodies.items() if n not in relative and any(invokes(b, r) for r in relative)} if not more: @@ -996,9 +1001,26 @@ def invokes(text: str, name: str) -> bool: moved_units += [(layout(k).members[mi].start, "\n".join(mlines[layout(k).members[mi].start:layout(k).members[mi].end + 1])) for k, mi in massign] + def reaches(text: str) -> set[str]: + """Parent macros a moved unit invokes, directly or through other parent macros.""" + seen: set[str] = set() + todo = [n for n in first_def if invokes(text, n)] + while todo: + n = todo.pop() + if n in seen: + continue + seen.add(n) + todo += [m for m in first_def if m not in seen and invokes(bodies.get(n, ""), m)] + return seen + + dup = {n for n in macro_names if macro_names.count(n) > 1} for start, text in moved_units: + used = reaches(text) + for name in sorted(used & dup): + raise SystemExit(f"moved code at line {start + 1} reaches macro_rules! {name}, which the parent defines " + "more than once; which definition it binds to would change. Edit by hand.") for name, line in first_def.items(): - if line > start and invokes(text, name): + if line > start and name in used: raise SystemExit(f"moved code at line {start + 1} invokes {name}! before the parent defines it " f"(line {line + 1}); its binding would change. Edit by hand.") split_impls = sorted({k for k, _ in massign}) diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index b0fc68c13..a3ac57fef 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -951,3 +951,33 @@ def test_verify_round_trips_tricky_but_valid_splits(repo): assert "impl super::super::Marker for S {}" in (repo / "crate/src/a/foo/impls.rs").read_text() v = run("verify", "HEAD~1", cwd=repo) assert v.returncode == 0, v.stdout + + +# --- Round-3 review cases. --- + + +@pytest.mark.parametrize( + ("text", "items", "message"), + [ + # a moved function reaches a later-defined macro through another parent macro + ("macro_rules! relay {\n () => {\n value!()\n };\n}\n\npub fn answer() -> u8 {\n relay!()\n}\n\n" + "macro_rules! value {\n () => {\n 2\n };\n}\n", '["fn answer"]', "before the parent defines it"), + # a parent macro whose body names a terminal `super` + ("const V: u8 = 2;\n\nmacro_rules! value {\n () => {{\n use super as outer;\n outer::V\n }};\n}\n\n" + "pub fn answer() -> u8 {\n value!()\n}\n", '["fn answer"]', "expansion names"), + # a multi-line member attribute ending on the member's line + ("struct S;\n\n#[rustfmt::skip]\nimpl S {\n #[\n cfg(all())\n ] fn answer() -> u8 { 1 }\n fn stay() {}\n fn kept() {}\n}\n", + '["impl S :: fn stay"]', "share a line"), + ], +) +def test_round_three_refusals(repo, text, items, message): + r = write_parent(repo, text, f'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = {items}\n') + assert r.returncode != 0 and message in r.stderr, r.stderr + + +def test_a_commented_attribute_is_not_copied_onto_split_impl_blocks(repo): + r = write_parent(repo, "struct S;\n\n/*\n#[cfg(any())]\n*/\nimpl S {\n fn answer() -> u8 {\n 1\n }\n\n fn stay() {}\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["impl S :: fn answer"]\n') + assert r.returncode == 0, r.stderr + child = (repo / "crate/src/a/foo/x.rs").read_text() + assert "#[cfg(any())]" not in child.split("impl S {")[0].split("use super::*;")[1] From b77a14af0c594f1a485d1a027616eb7c86b5a99c Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 03:04:53 -0400 Subject: [PATCH 11/15] scripts: close split_rust_module.py's last review findings - Deepen super passed as a macro argument (call!(super)); the visibility check already excludes pub(super). - Refuse a raw-identifier inline module (mod r#i { .. }) naming super, like any inline module. - Treat #[macro_use] items like macro_rules! when placing the new mod lines, so children still see the macros they bring in. - Require allow_macro_items to move a macro invocation that is an inherent-impl member, as for top-level invocations. - Scan macro_rules! definitions elsewhere in the crate (and use-aliases of them) for super/self paths, not only the parent's. - Look for a new module name used as a path in the parent's existing child modules too, so it cannot shadow a crate they name. - Refuse a code block in a module doc, which rustdoc would run. - Remove dead code. Tests: 93. --- scripts/split_rust_module.py | 47 +++++++++++++++++------- scripts/tests/test_split_rust_module.py | 48 +++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 399003e04..96a584334 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -528,7 +528,7 @@ def macro_group(mt: str, pos: int) -> tuple[int, int] | None: # `super` that starts a path or a use tree (`super::x`, `use super as y`, `use super;`), not # the `super` of a `pub(super)` / `pub(in super..)` visibility. -SUPER_PATH = re.compile(r"(? list[int]: @@ -615,7 +615,7 @@ def vis_edit(pos: int) -> None: g = macro_group(mt, m.end()) if g and re.search(r"\bsuper\b", mt[g[0]:g[1]]): raise SystemExit("moved code stringifies a `super` path; its text would change. Edit by hand.") - if re.search(r"\bmod\s+\w+\s*\{", mt) and re.search(r"\bsuper\b", mt): + if re.search(r"\bmod\s+(?:r#)?\w+\s*\{", mt) and re.search(r"\bsuper\b", mt): raise SystemExit("moved code has an inline module and names `super`; which module each `super` " "means depends on nesting the tool does not track. Edit by hand.") if re.search(r"\binclude\s*!\s*[(\[{]", mt): @@ -804,6 +804,9 @@ def doc_comment(doc: str) -> str: """`//!` lines for a module doc, paragraphs separated by blank lines, wrapped at 100 columns.""" import textwrap + if "```" in doc or "~~~" in doc: + raise SystemExit("a module doc may not contain a code block (rustdoc would run it as a doctest)") + paras = [" ".join(p.split()) for p in doc.strip().split("\n\n") if p.strip()] if not paras: return "" @@ -986,6 +989,25 @@ def invokes(text: str, name: str) -> bool: if not more: break relative |= more + # Macros defined elsewhere in the crate expand in the moved code's module too. + crate_src = os.path.join(repo, rel.replace(os.sep, "/").split("/src/", 1)[0], "src") + for d, _, files in os.walk(crate_src): + for fn in files: + fp = os.path.join(d, fn) + if not fn.endswith(".rs") or os.path.realpath(fp) == os.path.realpath(path): + continue + other = mask(open(fp, encoding="utf-8", errors="replace").read()) + for m in re.finditer(r"\bmacro_rules!\s*(?:r#)?(\w+)", other): + g = macro_group(other, m.end()) + body = other[g[0]:g[1]] if g else "" + if REL_VIS.search(body) or super_path_positions(body) or re.search(r"\bself\s*::", body): + relative.add(m.group(1)) + for it in items: + if it.kind == "use": + body = mask("\n".join(lines[it.start:it.end + 1])) + for src_name, alias in re.findall(r"\b(\w+)\s+as\s+(\w+)", body): + if src_name in relative: + relative.add(alias) for name in sorted(relative): if invokes(moved_text, name): raise SystemExit(f"moved code invokes macro_rules! {name}, whose expansion names `super`/`self` " @@ -1041,7 +1063,12 @@ def reaches(text: str) -> set[str]: for n in names: if n in imported: raise SystemExit(f"module name {n!r} collides with a name the parent imports") - if re.search(r"(? set[str]: else attr_lines(lines, it.lead, it.start)) while lead_lines and not lead_lines[0].strip(): lead_lines = lead_lines[1:] + if any(lay.members[mi].kind == "macro" for mi in mids) and not mod.get("allow_macro_items", False): + raise SystemExit(f"{it.selector}: a macro invocation inside the impl emits members whose visibility " + "the tool cannot remap; set allow_macro_items = true on the module to move it") block = impl_block(lines, it, lay, mids, lead_lines, True, lead_owner[k] == name, parent_path, deeper, counts) for mi in mids: @@ -1146,7 +1176,8 @@ def reaches(text: str) -> set[str]: for idx, k in enumerate(keep): if k < first_def and items[k].kind in ("use", "extern crate", "mod-decl"): decl_after = idx - if items[k].kind == "macro_rules!": + if items[k].kind == "macro_rules!" or any(re.match(r"#\[\s*macro_use\b", a) for a in attrs_of( + lines, items[k].lead, items[k].start)): decl_after = max(decl_after, idx) out = lines[:header_end] while out and not out[-1].strip(): @@ -1276,13 +1307,6 @@ def git_show(repo: str, rev: str, path: str) -> str | None: return r.stdout.decode("utf-8") if r.returncode == 0 else None -def parent_candidates(child: str) -> list[str]: - d = os.path.dirname(child) - if os.path.basename(d) == "src": - return [os.path.join(d, "lib.rs"), os.path.join(d, "main.rs")] - return [d + ".rs", os.path.join(d, "mod.rs")] - - def crate_edition(repo: str, rev: str, rel: str) -> str: d = os.path.dirname(rel) while d: @@ -1323,7 +1347,6 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str base_lines, base_mlines = base_src.split("\n"), mask(base_src).split("\n") head_parent = git_show(repo, head, parent) or "" hp_items = parse_items(head_parent)[0] - hp_lines = head_parent.split("\n") order = [it.name for it in hp_items if it.kind == "mod-decl"] names = sorted(children, key=lambda n: order.index(n) if n in order else len(order)) out = [f"file = {json.dumps(parent, ensure_ascii=False)}"] diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index a3ac57fef..0027c08ce 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -981,3 +981,51 @@ def test_a_commented_attribute_is_not_copied_onto_split_impl_blocks(repo): assert r.returncode == 0, r.stderr child = (repo / "crate/src/a/foo/x.rs").read_text() assert "#[cfg(any())]" not in child.split("impl S {")[0].split("use super::*;")[1] + + +@pytest.mark.parametrize( + ("text", "spec_extra", "items", "message"), + [ + # an impl-member macro needs the opt-in + ("struct S;\n\nimpl S {\n describe!();\n\n fn stay() {}\n}\n", "", '["impl S :: macro describe"]', "allow_macro_items"), + # super passed as a macro argument is deepened, not skipped: refuse via crate macro check is not needed; + # a raw-identifier inline module with super inside a moved fn + ("fn f() -> u8 {\n mod r#i {\n pub fn g() -> u8 {\n super::super::V\n }\n }\n r#i::g()\n}\n", + "", '["fn f"]', "inline module"), + # a fenced code block in a module doc would become a doctest + ("fn f() {}\n", 'doc = "Example:\\n\\n```\\nassert!(false);\\n```"\n', '["fn f"]', "code block"), + ], +) +def test_round_three_claude_refusals(repo, text, spec_extra, items, message): + r = write_parent(repo, text, f'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\n{spec_extra}items = {items}\n') + assert r.returncode != 0 and message in r.stderr, r.stderr + + +def test_super_passed_to_a_macro_is_deepened(repo): + r = write_parent(repo, "fn f() -> u8 {\n call!(super)\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + assert "call!(super::super)" in (repo / "crate/src/a/foo/x.rs").read_text() + + +def test_a_crate_macro_whose_body_names_super_is_refused(repo): + (repo / "crate/src/lib.rs").write_text("macro_rules! up {\n () => {\n super::which()\n };\n}\n") + r = write_parent(repo, "fn f() {\n up!();\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "macro_rules! up" in r.stderr, r.stderr + + +def test_macro_use_modules_are_declared_before_new_children(repo): + r = write_parent(repo, "fn early() {}\n\n#[macro_use]\nmod inner {\n macro_rules! answer {\n () => {\n 1\n };\n }\n}\n\nfn f() -> u8 {\n answer!()\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + parent = (repo / "crate/src/a/foo.rs").read_text() + assert parent.index("mod inner") < parent.index("mod x;") + + +def test_a_module_name_used_as_a_path_in_an_existing_child_is_refused(repo): + (repo / "crate/src/a/foo").mkdir(parents=True, exist_ok=True) + (repo / "crate/src/a/foo/existing.rs").write_text("use super::*;\n\nfn g() -> u8 {\n util::one()\n}\n") + r = write_parent(repo, "mod existing;\n\nfn f() {}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "util"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "already used as a path" in r.stderr From f47f71ed1f3a82117d7f8bf935f39752f832f342 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 03:06:44 -0400 Subject: [PATCH 12/15] scripts: add allow_unused_reexport to split_rust_module.py A module whose moved items are named only inside the module (and its tests) makes the parent's private use child::*; unused in normal builds, which -D warnings rejects. Dropping the re-export is not safe, because a sibling's glob import could then resolve the name to a different item. allow_unused_reexport = true keeps the re-export under #[allow(unused_imports)]; verify derives it from the head like the other module options. --- scripts/split_rust_module.py | 11 +++++++++++ scripts/tests/test_split_rust_module.py | 8 ++++++++ 2 files changed, 19 insertions(+) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 96a584334..e84003e8f 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -40,6 +40,8 @@ items = ["fn parse_*", "struct Config", "impl Config", "impl Display for Config"] # optional: allow_unused_glob = true (the module needs nothing from its parent: keep # `use super::*;` under #[allow(unused_imports)], which -D warnings would reject) + # optional: allow_unused_reexport = true (the parent's `use child::*;` is kept, under + # #[allow(unused_imports)], when nothing outside the module names its items) # optional: allow_macro_items = true (move top-level macro invocations; their emitted # items keep the visibility the macro gives them) @@ -1159,6 +1161,11 @@ def reaches(text: str) -> set[str]: if r["reexport"] == "none": continue gate = "".join(c + "\n" for c in r["cfg"]) + allow = mod.get("allow_unused_reexport", False) + if not isinstance(allow, bool): + raise SystemExit(f"module {mod['name']}: allow_unused_reexport must be true or false") + if allow: + gate += "#[allow(unused_imports)]\n" uses.append(f"{gate}{r['use_vis']}use {mod['name']}::*;") decl_text = "\n".join(decls) + ("\n\n" + "\n".join(uses) if uses else "") @@ -1385,6 +1392,10 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str out.append(f"doc = {json.dumps(doc, ensure_ascii=False)}") if "allow(unused_imports)" in glob_lead: out.append("allow_unused_glob = true") + for k, it in enumerate(hp_items): + if it.kind == "use" and re.search(r"\buse\s+" + re.escape(name) + r"::\*;", it.name) and \ + "allow(unused_imports)" in "\n".join(head_parent.split("\n")[it.lead:it.start]): + out.append("allow_unused_reexport = true") if any(it.kind == "macro" for it in items): out.append("allow_macro_items = true") out.append("items = [\n" + "".join(f" {json.dumps(x, ensure_ascii=False)},\n" for x in dict.fromkeys(sels)) + "]") diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index 0027c08ce..b017d7980 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -1029,3 +1029,11 @@ def test_a_module_name_used_as_a_path_in_an_existing_child_is_refused(repo): r = write_parent(repo, "mod existing;\n\nfn f() {}\n", 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "util"\nitems = ["fn f"]\n') assert r.returncode != 0 and "already used as a path" in r.stderr + + +def test_an_unused_reexport_is_kept_under_an_allowance_and_round_trips(repo): + (repo / "spec.toml").write_text(SPEC.replace('name = "k8s"\n', 'name = "k8s"\nallow_unused_reexport = true\n')) + apply_and_commit(repo) + parent = (repo / "crate/src/a/foo.rs").read_text() + assert '#[allow(unused_imports)]\nuse k8s::*;' in parent + assert run("verify", "HEAD~1", cwd=repo).returncode == 0 From 310ebf9bd19e5e2c777d3b4e5251052c65e5885f Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 03:22:04 -0400 Subject: [PATCH 13/15] scripts: refuse the macro forms split_rust_module.py cannot analyze The fourth review round found the remaining meaning changes all come from macros, which the tool cannot expand. Refuse them instead: - Treat a macro_rules! anywhere in the crate as relative when its body names a super/self path or relative visibility, or reaches such a macro through other macros or a use-alias, and refuse moved code that invokes one, bare or path-qualified. - Refuse passing super in a user macro's arguments: the macro can match or stringify those tokens, so deepening them may change its expansion. format-, assert- and tracing-style macros stay allowed. - Refuse any split of a parent with a #[macro_use] item, whose textual scope the new mod lines would change. - Scan the parent's child modules recursively when checking that a new module name is not already used as a path. The workspace has no #[macro_use] and no super passed to a user macro, so no real split loses anything. Docstring updated; tests: 98. --- scripts/split_rust_module.py | 81 +++++++++++++++++-------- scripts/tests/test_split_rust_module.py | 44 +++++++++++--- 2 files changed, 92 insertions(+), 33 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index e84003e8f..5d7ecdac6 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -57,16 +57,22 @@ Refused, to be done by hand (the tool stops rather than guess): * moving `use`, `mod x;`, `extern` blocks or `macro_rules!` items (macros stay in the - parent, above the new `mod` lines, so children still see them); + parent, above the new `mod` lines, so children still see them), and any split of a + parent with a `#[macro_use]` item; * a parent file loaded through `#[path]`, or a `#[path]` inside moved code; -* moved code that invokes a parent macro emitting `pub(super)`/`pub(self)` (directly or - through another macro), or a macro name defined twice; -* relative visibility the edits cannot remap: `pub(in super::..)`, a `pub(super)` - nested inside a moved item, an inline module naming `super`, `self::super`; -* a `super::` path inside a string literal (e.g. an attribute argument), and include - arguments other than a plain string literal; -* a module name that is a keyword or collides with an item or import in the parent; -* an attribute on the same line as its item (so items are never mis-split). +* moved code reaching a macro whose expansion names `super`/`self` paths or emits + `pub(super)`/`pub(self)`: any macro_rules! in the crate, directly, through other macros, + path-qualified or through a `use .. as` alias; a macro invoked before the parent defines + it, or defined twice; `super` passed in a user macro's arguments (format/tracing-style + macros are fine); `stringify!` of `super`; `include!`; +* top-level or impl-member macro invocations, unless the module sets allow_macro_items; +* relative visibility the edits cannot remap: `pub(in super::..)`, a `pub(super)` nested + inside a moved item, an inline module naming `super`, `self::super`, a `super::` path + inside a string literal, include arguments other than a plain string literal; +* a module name that is a keyword, collides with an item or import of the parent, or is + already used as a path in the parent or its child modules; +* an attribute sharing a line with its item or impl member; a carriage return; a + `src/bin/.rs` crate root; a code block in a module doc. """ from __future__ import annotations @@ -543,6 +549,13 @@ def super_path_positions(mt: str) -> list[int]: return out +# Macros whose arguments are ordinary expressions, paths or format arguments: a deeper +# `super::` path inside them means what it meant before. +KNOWN_MACROS = set("""format format_args print println eprint eprintln write writeln vec assert assert_eq +assert_ne debug_assert debug_assert_eq debug_assert_ne matches panic unreachable todo unimplemented dbg +trace debug info warn error event trace_span debug_span info_span warn_span error_span span anyhow bail +ensure sql json""".split()) + REL_VIS = re.compile(r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self)\b)") INCLUDE = re.compile(r"\binclude_(?:str|bytes)\s*!\s*[(\[{]") PLAIN_INCLUDE = re.compile(r"(\binclude_(?:str|bytes)\s*!\s*[(\[{]\s*)\"([^\"\\]*)\"\s*[)\]}]") @@ -620,6 +633,13 @@ def vis_edit(pos: int) -> None: if re.search(r"\bmod\s+(?:r#)?\w+\s*\{", mt) and re.search(r"\bsuper\b", mt): raise SystemExit("moved code has an inline module and names `super`; which module each `super` " "means depends on nesting the tool does not track. Edit by hand.") + for m in re.finditer(r"(? ImplLayout | None: for k, mi in massign) def invokes(text: str, name: str) -> bool: - return re.search(r"(? 1}: if invokes(moved_text, name): raise SystemExit(f"macro_rules! {name} is defined more than once in the parent and moved code " @@ -991,25 +1015,31 @@ def invokes(text: str, name: str) -> bool: if not more: break relative |= more - # Macros defined elsewhere in the crate expand in the moved code's module too. + # Macros defined anywhere in the crate expand in the moved code's module too. A macro is + # relative if its body names a super/self path or a relative visibility, or invokes a + # relative macro (transitively); a `use x as y` of one, anywhere in the crate, is one too. crate_src = os.path.join(repo, rel.replace(os.sep, "/").split("/src/", 1)[0], "src") + all_bodies: dict[str, str] = dict(bodies) + aliases: list[tuple[str, str]] = [] for d, _, files in os.walk(crate_src): for fn in files: - fp = os.path.join(d, fn) - if not fn.endswith(".rs") or os.path.realpath(fp) == os.path.realpath(path): + if not fn.endswith(".rs"): continue + fp = os.path.join(d, fn) other = mask(open(fp, encoding="utf-8", errors="replace").read()) for m in re.finditer(r"\bmacro_rules!\s*(?:r#)?(\w+)", other): g = macro_group(other, m.end()) - body = other[g[0]:g[1]] if g else "" - if REL_VIS.search(body) or super_path_positions(body) or re.search(r"\bself\s*::", body): - relative.add(m.group(1)) - for it in items: - if it.kind == "use": - body = mask("\n".join(lines[it.start:it.end + 1])) - for src_name, alias in re.findall(r"\b(\w+)\s+as\s+(\w+)", body): - if src_name in relative: - relative.add(alias) + all_bodies[m.group(1)] = all_bodies.get(m.group(1), "") + "\n" + (other[g[0]:g[1]] if g else "") + for m in re.finditer(r"\buse\b[^;]*;", other): + aliases += re.findall(r"\b(\w+)\s+as\s+(\w+)", m.group(0)) + relative |= {n for n, b in all_bodies.items() + if REL_VIS.search(b) or super_path_positions(b) or re.search(r"\bself\s*::", b)} + while True: + more = {n for n, b in all_bodies.items() if n not in relative and any(invokes(b, r) for r in relative)} + more |= {alias for src_name, alias in aliases if src_name in relative and alias not in relative} + if not more: + break + relative |= more for name in sorted(relative): if invokes(moved_text, name): raise SystemExit(f"moved code invokes macro_rules! {name}, whose expansion names `super`/`self` " @@ -1067,9 +1097,10 @@ def reaches(text: str) -> set[str]: raise SystemExit(f"module name {n!r} collides with a name the parent imports") siblings = "" if os.path.isdir(child_dir): - for fn in sorted(os.listdir(child_dir)): - if fn.endswith(".rs"): - siblings += mask(open(os.path.join(child_dir, fn), encoding="utf-8", errors="replace").read()) + for d, _, files in os.walk(child_dir): + for fn in sorted(files): + if fn.endswith(".rs"): + siblings += mask(open(os.path.join(d, fn), encoding="utf-8", errors="replace").read()) if re.search(r"(? u8 {\n mod r#i {\n pub fn g() -> u8 {\n super::super::V\n }\n }\n r#i::g()\n}\n", "", '["fn f"]', "inline module"), @@ -1001,11 +1000,16 @@ def test_round_three_claude_refusals(repo, text, spec_extra, items, message): assert r.returncode != 0 and message in r.stderr, r.stderr -def test_super_passed_to_a_macro_is_deepened(repo): +def test_super_passed_to_a_user_macro_is_refused_and_to_format_is_deepened(repo): r = write_parent(repo, "fn f() -> u8 {\n call!(super)\n}\n", 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') - assert r.returncode == 0, r.stderr - assert "call!(super::super)" in (repo / "crate/src/a/foo/x.rs").read_text() + assert r.returncode != 0 and "passes `super` to call!" in r.stderr, r.stderr + git(repo, "reset", "-q", "--hard", "HEAD") + git(repo, "clean", "-qfd") + ok = write_parent(repo, "fn f() -> String {\n format!(\"{}\", super::V)\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "y"\nitems = ["fn f"]\n') + assert ok.returncode == 0, ok.stderr + assert 'format!("{}", super::super::V)' in (repo / "crate/src/a/foo/y.rs").read_text() def test_a_crate_macro_whose_body_names_super_is_refused(repo): @@ -1015,12 +1019,36 @@ def test_a_crate_macro_whose_body_names_super_is_refused(repo): assert r.returncode != 0 and "macro_rules! up" in r.stderr, r.stderr -def test_macro_use_modules_are_declared_before_new_children(repo): +def test_a_parent_with_macro_use_is_refused(repo): r = write_parent(repo, "fn early() {}\n\n#[macro_use]\nmod inner {\n macro_rules! answer {\n () => {\n 1\n };\n }\n}\n\nfn f() -> u8 {\n answer!()\n}\n", 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') - assert r.returncode == 0, r.stderr - parent = (repo / "crate/src/a/foo.rs").read_text() - assert parent.index("mod inner") < parent.index("mod x;") + assert r.returncode != 0 and "macro_use" in r.stderr, r.stderr + + +@pytest.mark.parametrize( + ("root", "call"), + [ + # a relay macro elsewhere in the crate that reaches a macro naming super + ("macro_rules! up {\n () => {\n super::which()\n };\n}\n\nmacro_rules! relay {\n () => {\n up!()\n };\n}\n", "relay!()"), + # a path-qualified call to a crate macro naming super + ("macro_rules! up {\n () => {\n super::which()\n };\n}\n\npub(crate) use up;\n", "crate::up!()"), + # an alias, defined in another file, of a macro naming super + ("macro_rules! up {\n () => {\n super::which()\n };\n}\n\npub(crate) use up as lifted;\n", "lifted!()"), + ], +) +def test_crate_macros_that_expand_relative_paths_are_refused(repo, root, call): + (repo / "crate/src/lib.rs").write_text(root) + r = write_parent(repo, f"fn f() {{\n {call};\n}}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "expansion names" in r.stderr, r.stderr + + +def test_the_shadowing_scan_covers_nested_children(repo): + (repo / "crate/src/a/foo/other").mkdir(parents=True, exist_ok=True) + (repo / "crate/src/a/foo/other/mod.rs").write_text("use super::*;\n\nfn g() -> u8 {\n util::one()\n}\n") + r = write_parent(repo, "mod other;\n\nfn f() {}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "util"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "already used as a path" in r.stderr def test_a_module_name_used_as_a_path_in_an_existing_child_is_refused(repo): From dcfd1ae230db3453800e227e522be5b97995bbb8 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 03:22:53 -0400 Subject: [PATCH 14/15] scripts: drop an unused loop variable in split_rust_module.py --- scripts/split_rust_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 5d7ecdac6..5372013e3 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -1423,7 +1423,7 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str out.append(f"doc = {json.dumps(doc, ensure_ascii=False)}") if "allow(unused_imports)" in glob_lead: out.append("allow_unused_glob = true") - for k, it in enumerate(hp_items): + for it in hp_items: if it.kind == "use" and re.search(r"\buse\s+" + re.escape(name) + r"::\*;", it.name) and \ "allow(unused_imports)" in "\n".join(head_parent.split("\n")[it.lead:it.start]): out.append("allow_unused_reexport = true") From a097b250108875adce498f8b0db4b815514abd45 Mon Sep 17 00:00:00 2001 From: ac-pplx Date: Fri, 16 Oct 2026 12:23:49 -0400 Subject: [PATCH 15/15] scripts: close the upstream review findings in split_rust_module.py - Refuse super passed to a path-qualified user macro (crate::m!, $crate::m!, other::m!), and to a crate macro_rules! that shares a name with a known format/assert/tracing/anyhow macro: the exemption now applies only when the crate defines no macro of that name. - Keep plain comments (a license or SPDX line) above the module's inner docs in the parent header, and recognize /*! */ inner docs. - Refuse moved code whose doc comments or #[doc] attributes name a super:: or self:: path: an intra-doc link would point one module off. - Place the new mod lines above the first fn when the parent defines no types, instead of after a trailing mod tests;. - verify fails on non-Rust changes inside the split crate (a manifest, data or build input) unless --allow-non-rust is given. - Document that macros from other crates, and proc and attribute macros, are not analyzed; fix the comment on split-impl doc placement. All eight open split branches still verify identical with this tool. Tests: 108. --- scripts/split_rust_module.py | 101 ++++++++++++++++++++---- scripts/tests/test_split_rust_module.py | 83 +++++++++++++++++++ 2 files changed, 169 insertions(+), 15 deletions(-) diff --git a/scripts/split_rust_module.py b/scripts/split_rust_module.py index 5372013e3..f8e40068b 100644 --- a/scripts/split_rust_module.py +++ b/scripts/split_rust_module.py @@ -22,13 +22,15 @@ Usage: split_rust_module.py list FILE # items, impl members and their selectors split_rust_module.py apply SPEC.toml [--repo ROOT] [--report OUT.md] [--dry-run] - split_rust_module.py verify [BASE] [--head REV] [--print-spec] + split_rust_module.py verify [BASE] [--head REV] [--print-spec] [--allow-non-rust] `verify` proves that BASE...HEAD's Rust changes are exactly what `apply` generates: it derives the spec from the diff (which items each new child module holds), regenerates from the merge base, and compares every parent and child file byte for byte after rustfmt. Any other Rust change, a missing or altered `mod`/`use` line, a dropped item, an -edit inside a moved item or a comment, or a symlink fails it. It checks the generator, +edit inside a moved item or a comment, or a symlink fails it, and so does a non-Rust change +inside the split crate (a manifest, data or build input) unless `--allow-non-rust` is +given; non-Rust changes elsewhere are listed, not checked. It checks the generator, not the generator's semantics: those rest on the edits listed above, the refusals below, and compiling and testing the result. @@ -63,9 +65,12 @@ * moved code reaching a macro whose expansion names `super`/`self` paths or emits `pub(super)`/`pub(self)`: any macro_rules! in the crate, directly, through other macros, path-qualified or through a `use .. as` alias; a macro invoked before the parent defines - it, or defined twice; `super` passed in a user macro's arguments (format/tracing-style - macros are fine); `stringify!` of `super`; `include!`; + it, or defined twice; `super` passed in a user macro's arguments, path-qualified or not + (format/tracing-style macros are fine unless the crate defines a macro of the same + name); `stringify!` of `super`; `include!`; * top-level or impl-member macro invocations, unless the module sets allow_macro_items; +* a doc comment in moved code naming a super::/self:: path (an intra-doc link would point + one module off); * relative visibility the edits cannot remap: `pub(in super::..)`, a `pub(super)` nested inside a moved item, an inline module naming `super`, `self::super`, a `super::` path inside a string literal, include arguments other than a plain string literal; @@ -73,6 +78,10 @@ already used as a path in the parent or its child modules; * an attribute sharing a line with its item or impl member; a carriage return; a `src/bin/.rs` crate root; a code block in a module doc. + +Not analyzed: macros defined outside the crate being split (other workspace crates, +dependencies, proc and attribute macros). One that emits `pub(super)` or a `super::` path +expands one module deeper without a refusal; compiling and testing the result is the check. """ from __future__ import annotations @@ -229,16 +238,30 @@ def parse_items(src: str) -> tuple[list[Item], int]: lines = src.split("\n") mlines = mask(src).split("\n") n = len(lines) - # Header: leading //! docs, #![...] inner attributes, and blank lines. + # Header: everything up to the last inner doc (`//!`, `/*! */`) or inner attribute + # (`#![...]`) that precedes the first item, including plain comments and blank lines + # before it (a license line above the module docs stays with the parent). i = 0 header_end = 0 depth = 0 + in_inner_block = False while i < n: - s = lines[i].strip() - if s.startswith("//!") or s == "": + raw = lines[i] + s = raw.strip() + if in_inner_block: + if "*/" in s: + in_inner_block = False + i += 1 + header_end = i + continue + if s.startswith("//!"): i += 1 - if s: - header_end = i + header_end = i + continue + if s.startswith("/*!"): + in_inner_block = "*/" not in s[3:] + i += 1 + header_end = i continue if s.startswith("#!["): d = line_depth_delta(mlines[i]) @@ -248,6 +271,9 @@ def parse_items(src: str) -> tuple[list[Item], int]: i += 1 header_end = i continue + if s == "" or mlines[i].strip() == "": + i += 1 # blank or plain comment: header only if an inner doc/attribute follows + continue break items: list[Item] = [] i = header_end @@ -556,11 +582,43 @@ def super_path_positions(mt: str) -> list[int]: trace debug info warn error event trace_span debug_span info_span warn_span error_span span anyhow bail ensure sql json""".split()) +# macro_rules! names defined in the crate being split (set by apply): a crate macro that +# shares a name with a KNOWN_MACROS entry gets no exemption. +CRATE_MACROS: set[str] = set() + REL_VIS = re.compile(r"\bpub\s*\(\s*(?:super|self|in\s+(?:super|self)\b)") INCLUDE = re.compile(r"\binclude_(?:str|bytes)\s*!\s*[(\[{]") PLAIN_INCLUDE = re.compile(r"(\binclude_(?:str|bytes)\s*!\s*[(\[{]\s*)\"([^\"\\]*)\"\s*[)\]}]") +def doc_texts(text: str, mt: str) -> list[str]: + """The doc comments (`///`, `//!`, `/** */`, `/*! */`) and `#[doc ..]` attributes in text.""" + mc = mask(text, "\0") + out = [] + for m in re.finditer(r"/[/*]", text): + p = m.start() + if mc[p] != "\0" or (p and mc[p - 1] == "\0"): + continue # not a comment, or inside one + if text[p + 1] == "/": + e = text.find("\n", p) + body = text[p:len(text) if e < 0 else e] + doc = re.match(r"///(?!/)|//!", body) + else: + depth, e = 1, p + 2 + while e < len(text) and depth: + depth += 1 if text.startswith("/*", e) else -1 if text.startswith("*/", e) else 0 + e += 2 if text.startswith(("/*", "*/"), e) else 1 + body = text[p:e] + doc = re.match(r"/\*\*(?![*/])|/\*!", body) + if doc: + out.append(body) + for m in re.finditer(r"#\s*!?\s*\[\s*doc\b", mt): + g = macro_group(mt, m.start() + mt[m.start():].index("[")) + if g: + out.append(text[g[0]:g[1]]) + return out + + def transform_item(text: str, kind: str, parent_path: list[str], deeper_dir: bool) -> tuple[str, dict]: """Apply the visibility/path edits to one moved item's text (lead trivia included).""" counts: dict = {} @@ -633,13 +691,16 @@ def vis_edit(pos: int) -> None: if re.search(r"\bmod\s+(?:r#)?\w+\s*\{", mt) and re.search(r"\bsuper\b", mt): raise SystemExit("moved code has an inline module and names `super`; which module each `super` " "means depends on nesting the tool does not track. Edit by hand.") - for m in re.finditer(r"(? bool: all_bodies[m.group(1)] = all_bodies.get(m.group(1), "") + "\n" + (other[g[0]:g[1]] if g else "") for m in re.finditer(r"\buse\b[^;]*;", other): aliases += re.findall(r"\b(\w+)\s+as\s+(\w+)", m.group(0)) + CRATE_MACROS.clear() + CRATE_MACROS.update(all_bodies) relative |= {n for n, b in all_bodies.items() if REL_VIS.search(b) or super_path_positions(b) or re.search(r"\bself\s*::", b)} while True: @@ -1106,7 +1169,7 @@ def reaches(text: str) -> set[str]: "module?); a new module of that name would shadow it") # Where a split impl's doc comments go: with the members the parent keeps, else with the - # first module (in spec order) that receives members. + # module that receives the impl's first (lowest-indexed) moved member. lead_owner: dict[int, str | None] = {} for k in split_impls: lay = layout(k) @@ -1209,7 +1272,8 @@ def reaches(text: str) -> set[str]: if k in split_impls and lead_owner[k] is not None: continue # every member moved keep.append(k) - first_def = next((k for k, it in enumerate(items) if it.kind in DEFS), len(items)) + first_def = next((k for k, it in enumerate(items) if it.kind in DEFS), + next((k for k, it in enumerate(items) if it.kind in ("fn", "mod")), len(items))) decl_after = -1 for idx, k in enumerate(keep): if k < first_def and items[k].kind in ("use", "extern crate", "mod-decl"): @@ -1433,7 +1497,7 @@ def derive_spec(repo: str, base: str, head: str, parent: str, children: list[str return "\n".join(out) + "\n" -def verify(repo: str, base: str, head: str, print_spec: bool = False) -> int: +def verify(repo: str, base: str, head: str, print_spec: bool = False, allow_non_rust: bool = False) -> int: """Prove BASE...HEAD's Rust changes are exactly what `apply` generates: derive the spec from the diff, regenerate from the merge base, and compare byte for byte after rustfmt.""" import contextlib @@ -1514,6 +1578,11 @@ def children_dir(parent: str) -> str: shutil.rmtree(tmp, ignore_errors=True) if not groups and not problems: problems.append("no split found: no new child module next to a changed parent") + crates = {g.split("/src/", 1)[0] + "/" for g in groups} + inside = [p for p in other if any(p.startswith(c) for c in crates)] + if inside and not allow_non_rust: + problems.append("non-Rust files changed inside the split crate (manifest, data, build inputs); " + "a pure move changes none. Pass --allow-non-rust if they are intended: " + ", ".join(inside)) print(f"regenerated and compared {checked} files from {len(groups)} split parents: " f"{'identical' if not problems else str(len(problems)) + ' problems'}") for p in problems: @@ -1570,11 +1639,13 @@ def main() -> int: c.add_argument("--head", default="HEAD") c.add_argument("--repo", default=".") c.add_argument("--print-spec", action="store_true", help="print the spec derived from the diff") + c.add_argument("--allow-non-rust", action="store_true", + help="accept non-Rust changes inside the split crate (they are listed, not checked)") args = ap.parse_args() if args.cmd == "list": return list_items(args.file) if args.cmd == "verify": - return verify(args.repo, args.base, args.head, args.print_spec) + return verify(args.repo, args.base, args.head, args.print_spec, args.allow_non_rust) return apply(args.spec, args.repo, args.dry_run, args.report) diff --git a/scripts/tests/test_split_rust_module.py b/scripts/tests/test_split_rust_module.py index f1ffe87b1..f2261abc6 100644 --- a/scripts/tests/test_split_rust_module.py +++ b/scripts/tests/test_split_rust_module.py @@ -1065,3 +1065,86 @@ def test_an_unused_reexport_is_kept_under_an_allowance_and_round_trips(repo): parent = (repo / "crate/src/a/foo.rs").read_text() assert '#[allow(unused_imports)]\nuse k8s::*;' in parent assert run("verify", "HEAD~1", cwd=repo).returncode == 0 + + +# --- Upstream review cases. --- + + +@pytest.mark.parametrize( + ("root", "call", "name"), + [ + # a path-qualified user macro + ("macro_rules! call {\n ($e:path) => {\n $e()\n };\n}\n\npub(crate) use call;\n", "crate::call!(super::g)", "call"), + # a crate macro that shares a name with a known std/tracing/anyhow macro + ("macro_rules! ensure {\n ($e:expr) => {\n assert!($e)\n };\n}\n", "ensure!(super::ok())", "ensure"), + ], +) +def test_super_passed_to_a_qualified_or_shadowing_macro_is_refused(repo, root, call, name): + (repo / "crate/src/lib.rs").write_text(root) + r = write_parent(repo, f"fn f() {{\n {call};\n}}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and f"passes `super` to {name}!" in r.stderr, r.stderr + + +@pytest.mark.parametrize( + "header", + [ + "// SPDX-License-Identifier: Apache-2.0\n\n//! Docs.\n", + "// Copyright notice.\n/*!\n * Block docs.\n */\n", + ], +) +def test_comments_above_inner_docs_stay_in_the_parent_header(repo, header): + r = write_parent(repo, header + "\nstruct S;\n\nfn stay() {}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["struct S"]\n') + assert r.returncode == 0, r.stderr + assert (repo / "crate/src/a/foo.rs").read_text().startswith(header) + child = (repo / "crate/src/a/foo/x.rs").read_text() + assert "Docs" not in child and "notice" not in child and "SPDX" not in child + rustfmt_all(repo) + commit_all(repo, "split") + v = run("verify", "HEAD~1", cwd=repo) + assert v.returncode == 0, v.stdout + + +@pytest.mark.parametrize( + "text", + [ + "/// See [`super::V`].\nfn f() {}\n", + "/** Calls [`self::g`]. */\nfn f() {}\n\nfn g() {}\n", + '#[doc = "Reads [`super::V`]."]\nfn f() {}\n', + ], +) +def test_doc_links_through_relative_paths_are_refused(repo, text): + r = write_parent(repo, text, 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode != 0 and "doc comment" in r.stderr, r.stderr + + +def test_relative_paths_in_code_and_plain_comments_are_not_doc_links(repo): + r = write_parent(repo, "fn f() -> u8 {\n // Scales super::V.\n 3\n * super::V\n}\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn f"]\n') + assert r.returncode == 0, r.stderr + assert "* super::super::V" in (repo / "crate/src/a/foo/x.rs").read_text() + + +def test_declarations_go_above_the_first_fn_when_the_parent_has_no_types(repo): + r = write_parent(repo, "use std::fmt;\n\nfn a() {}\n\nfn b() {}\n\n#[cfg(test)]\nmod tests;\n", + 'file = "crate/src/a/foo.rs"\n[[module]]\nname = "x"\nitems = ["fn b"]\n') + assert r.returncode == 0, r.stderr + parent = (repo / "crate/src/a/foo.rs").read_text() + assert parent.index("use std::fmt;") < parent.index("mod x;") < parent.index("fn a()") < parent.index("mod tests;") + + +def test_verify_rejects_non_rust_changes_inside_the_split_crate(repo): + apply_and_commit(repo) + (repo / "crate/Cargo.toml").write_text('[package]\nname = "c"\n') + (repo / "NOTES.md").write_text("Outside the crate.\n") + commit_all(repo, "manifest") + bad = run("verify", "HEAD~2", cwd=repo) + assert bad.returncode == 1 and "crate/Cargo.toml" in bad.stdout and "inside the split crate" in bad.stdout + assert "NOTES.md" not in bad.stdout.split("not checked by verify")[0] + ok = run("verify", "HEAD~2", "--allow-non-rust", cwd=repo) + assert ok.returncode == 0, ok.stdout + git(repo, "reset", "-q", "--hard", "HEAD~1") + (repo / "NOTES.md").write_text("Outside the crate.\n") + commit_all(repo, "notes") + assert run("verify", "HEAD~2", cwd=repo).returncode == 0