From 6a2537541db5e60390fefffc996824608568dd44 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:49:20 -0400 Subject: [PATCH 01/11] Expand MFC sources with the build's Fypp flags, without a build --- toolchain/mfc/context_expand.py | 89 +++++++++++++++++++++++++++++++++ toolchain/mfc/test_context.py | 46 +++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 toolchain/mfc/context_expand.py create mode 100644 toolchain/mfc/test_context.py diff --git a/toolchain/mfc/context_expand.py b/toolchain/mfc/context_expand.py new file mode 100644 index 000000000..f6661af1b --- /dev/null +++ b/toolchain/mfc/context_expand.py @@ -0,0 +1,89 @@ +"""Fypp-expand MFC sources the way the build does, without a build. Used by `./mfc.sh context`.""" + +import hashlib +import shutil +import subprocess +import sys +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path + +# Target-independent arguments of the Fypp command in cmake/Fypp.cmake; test_context checks they match. +FYPP_FLAGS = ["-m", "re", "--line-numbering", "--no-folding", "--line-length=999", "--line-numbering-mode=nocontlines"] + + +def _fypp_exe() -> str: + """The fypp beside the running interpreter (the toolchain venv), else the one on PATH.""" + beside = Path(sys.executable).parent / "fypp" + return str(beside) if beside.exists() else (shutil.which("fypp") or "fypp") + + +def fypp_command(fypp, repo, include_dir, target, src, out, compiler="NVHPC"): + common = repo / "src" / "common" + incs = [include_dir / target, repo / "src" / target / "include", common / "include", common, common / "include" / "defaults"] + defines = [f"MFC_{compiler}", f"MFC_{target.upper()}", f'MFC_COMPILER="{compiler}"', "MFC_CASE_OPTIMIZATION=False", "chemistry=False"] + return [fypp, *FYPP_FLAGS[:2], *(a for i in incs for a in ("-I", str(i))), *(a for d in defines for a in ("-D", d)), *FYPP_FLAGS[2:], str(src), str(out)] + + +def _write_includes(repo: Path, include_dir: Path, targets): + """Write what the build generates before Fypp runs: parameter includes and a default case.fpp.""" + sys.path.insert(0, str(repo / "toolchain")) + from mfc import state # pylint: disable=import-outside-toplevel + from mfc.case import Case # pylint: disable=import-outside-toplevel + from mfc.params.generators.fortran_gen import get_generated_files # pylint: disable=import-outside-toplevel + + for path, content in get_generated_files(include_dir.parent): + path.parent.mkdir(parents=True, exist_ok=True) + if not path.exists() or path.read_text() != content: + path.write_text(content) + state.gARG.setdefault("case_optimization", False) # get_fpp reads it; nothing is case-optimized here + for t in targets: + (include_dir / t).mkdir(parents=True, exist_ok=True) + case = include_dir / t / "case.fpp" + content = Case({}).get_fpp(t, print=False) + if not case.exists() or case.read_text() != content: + case.write_text(content) + + +def _sources(repo: Path, target: str): + return sorted((repo / "src" / "common").glob("*.fpp")) + sorted((repo / "src" / target).glob("*.fpp")) + + +def _key(paths) -> str: + h = hashlib.sha256() + for p in paths: + h.update(str(p).encode()) + h.update(p.read_bytes()) + return h.hexdigest()[:16] + + +def expand(repo, targets, cache, compiler="NVHPC", write_includes=True): + """Expand each target's sources into cache//.f90. Returns (files per target, errors).""" + repo, cache = Path(repo).resolve(), Path(cache).resolve() + include_dir = cache / "include" + if write_includes: + _write_includes(repo, include_dir, targets) + fypp = _fypp_exe() + inc_key = _key(sorted(repo.glob("src/*/include/**/*.fpp")) + sorted(include_dir.glob("*/*.fpp"))) + jobs = [] + for t in targets: + for src in _sources(repo, t): + # Keyed by source and include content, so unchanged sources are not re-expanded. + jobs.append((t, src, cache / t / f"{src.name}.{_key([src])}{inc_key}.f90", cache / t / f"{src.name}.f90")) + + def run(job): + t, src, keyed, final = job + if not keyed.exists(): + keyed.parent.mkdir(parents=True, exist_ok=True) + r = subprocess.run(fypp_command(fypp, repo, include_dir, t, src, keyed, compiler), capture_output=True, text=True, check=False) + if r.returncode: + keyed.unlink(missing_ok=True) + return f"could not expand {src.relative_to(repo)}: {(r.stderr.strip().splitlines() or ['?'])[-1]}" + if not final.is_symlink() or final.resolve() != keyed: + final.unlink(missing_ok=True) + final.symlink_to(keyed.name) + return None + + with ThreadPoolExecutor(16) as ex: + errors = [e for e in ex.map(run, jobs) if e] + files = {t: [f for tt, _, _, f in jobs if tt == t and f.exists()] for t in targets} + return files, errors diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py new file mode 100644 index 000000000..94f7b0788 --- /dev/null +++ b/toolchain/mfc/test_context.py @@ -0,0 +1,46 @@ +"""Tests for ./mfc.sh context (context_expand and context).""" + +import re +from pathlib import Path + +from mfc import context_expand + +REPO = Path(__file__).resolve().parents[2] + + +def test_fypp_flags_match_cmake(): + text = (REPO / "cmake" / "Fypp.cmake").read_text() + start = text.index("COMMAND ${FYPP_EXE}") + block = text[start : text.index("DEPENDS", start)] + cmake_flags = sorted(re.sub(r"\s+", " ", f) for f in re.findall(r"(--[\w-]+(?:=\S+)?|-m\s+\w+)", block)) + flags = context_expand.FYPP_FLAGS + ours = sorted([f for f in flags if f.startswith("--")] + [f"{flags[0]} {flags[1]}"]) + assert cmake_flags == ours + + +def test_expand_one_target_produces_markers(tmp_path): + files, errors = context_expand.expand(REPO, ["post_process"], tmp_path) + assert errors == [] + names = {p.name for p in files["post_process"]} + assert "m_derived_variables.fpp.f90" in names and "m_helper_basic.fpp.f90" in names + text = next(p for p in files["post_process"] if p.name == "m_helper_basic.fpp.f90").read_text() + assert re.search(r'^# \d+ ".*src/common/m_helper_basic\.fpp"', text, re.M) + + +def test_expand_cache_is_reused(tmp_path): + files, _ = context_expand.expand(REPO, ["post_process"], tmp_path) + p = files["post_process"][0].resolve() + mtime = p.stat().st_mtime_ns + files2, _ = context_expand.expand(REPO, ["post_process"], tmp_path) + assert files2["post_process"][0].resolve() == p and p.stat().st_mtime_ns == mtime + + +def test_expand_reports_fypp_error_and_continues(tmp_path): + fake = tmp_path / "repo" + (fake / "src" / "common" / "include").mkdir(parents=True) + (fake / "src" / "post_process").mkdir(parents=True) + (fake / "src" / "common" / "m_ok.fpp").write_text("module m_ok\nend module m_ok\n") + (fake / "src" / "post_process" / "m_bad.fpp").write_text("#:if\nmodule m_bad\nend module m_bad\n") + files, errors = context_expand.expand(fake, ["post_process"], tmp_path / "cache", write_includes=False) + assert any("m_bad.fpp" in e for e in errors) + assert any(p.name == "m_ok.fpp.f90" for p in files["post_process"]) From 49a5e464dfe4831d3d99888ef3a456b10a7f7556 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:50:08 -0400 Subject: [PATCH 02/11] Index Fypp-expanded sources by routine through line markers --- toolchain/mfc/context.py | 114 ++++++++++++++++++++++++++++++++++ toolchain/mfc/test_context.py | 74 +++++++++++++++++++++- 2 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 toolchain/mfc/context.py diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py new file mode 100644 index 000000000..8c3ed5346 --- /dev/null +++ b/toolchain/mfc/context.py @@ -0,0 +1,114 @@ +"""`./mfc.sh context`: what to re-read to check a diff to MFC's Fortran. + +Works on Fypp-expanded sources (context_expand), whose '# N "file"' markers map every expanded line back to its +source line, so a changed source line is traced to every generated copy of it. +""" + +import collections +import re +from dataclasses import dataclass, field +from pathlib import Path + +MARKER = re.compile(r'^#\s*(\d+)\s+"([^"]+)"') +ROUTINE = re.compile(r"^\s*(?:(?:pure|elemental|impure|recursive|module)\s+)*(?:(?:real|integer|logical|complex|type\s*\([^)]*\))\S*\s+)?(subroutine|function)\s+(\w+)", re.I) +END_ROUTINE = re.compile(r"^\s*end\s+(subroutine|function)\b", re.I) +MODULE = re.compile(r"^\s*module\s+(?!procedure\b)(\w+)\s*$", re.I) +TYPE_START = re.compile(r"^\s*type\s*(?:,[^:]*)?(?:::)?\s*(\w+)\s*$", re.I) +TYPE_END = re.compile(r"^\s*end\s*type\b", re.I) +CONTAINS = re.compile(r"^\s*contains\s*$", re.I) +DECL = re.compile(r"^\s*(real|integer|logical|complex|character|type\s*\()[^!]*::\s*(.*)$", re.I) +USE = re.compile(r"^\s*use\s+(\w+)\s*(?:,\s*only\s*:\s*(.*))?$", re.I) +CALL = re.compile(r"\bcall\s+(\w+)", re.I) +FUNC_REF = re.compile(r"\b(f_\w+)\s*\(", re.I) +ASSIGN = re.compile(r"^\s*(\w+)\s*(?:\([^=]*\))?\s*(?:%[\w%()]*)?\s*=(?!=)") + + +def _code(text: str) -> str: + return text if "!$" in text else text.split("!")[0] + + +def _decl_names(rhs: str) -> list: + rhs = re.sub(r"\([^()]*\)", "", re.sub(r"\([^()]*\)", "", rhs)) + return [m.group(1).lower() for tok in rhs.split(",") if (m := re.match(r"\s*(\w+)", tok))] + + +def read_expanded(path): + """(text, source file, source line) for each expanded line, from the Fypp line markers.""" + rows, src, ln = [], "", 0 + for text in Path(path).read_text(errors="replace").splitlines(): + if m := MARKER.match(text): + ln, src = int(m.group(1)), m.group(2) + continue + rows.append((text, src, ln)) + ln += 1 + return rows + + +@dataclass +class Routine: # pylint: disable=too-many-instance-attributes + name: str + target: str + module: str + rows: list = field(default_factory=list) + calls: set = field(default_factory=set) + idents: set = field(default_factory=set) + assigns: set = field(default_factory=set) + locals: set = field(default_factory=set) + src_file: str = "" + src_range: tuple = (0, 0) + src_count: collections.Counter = field(default_factory=collections.Counter) + + +@dataclass +class Index: + routines: list + module_vars: dict # name -> modules declaring it + module_uses: dict # (target, module) -> {used module: only-list, or None for all} + + +def _finish(r: Routine): + code = [_code(t) for t, _, _ in r.rows] + body = "\n".join(code) + r.calls = {c.lower() for c in CALL.findall(body)} | {c.lower() for c in FUNC_REF.findall(body)} + r.idents = {w.lower() for w in re.findall(r"(?")[-1].strip().lower() for n in u.group(2).split(",")} if u.group(2) else None + module_uses[(target, module)][u.group(1).lower()] = only + if TYPE_START.match(code): + in_type += 1 + elif TYPE_END.match(code): + in_type = max(0, in_type - 1) + elif in_spec and not stack and not in_type and (d := DECL.match(code)) and "parameter" not in code.lower().split("::")[0]: + for n in _decl_names(d.group(2)): + module_vars.setdefault(n, set()).add(module) + if (m := ROUTINE.match(code)) and not re.match(r"\s*end\b", code, re.I): + stack.append(Routine(m.group(2), target, module or "")) + for r in stack: + r.rows.append((text, src, ln)) + if END_ROUTINE.match(code) and stack: + r = stack.pop() + _finish(r) + routines.append(r) + return Index(routines, module_vars, dict(module_uses)) diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 94f7b0788..8467df945 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -3,7 +3,7 @@ import re from pathlib import Path -from mfc import context_expand +from mfc import context, context_expand REPO = Path(__file__).resolve().parents[2] @@ -44,3 +44,75 @@ def test_expand_reports_fypp_error_and_continues(tmp_path): files, errors = context_expand.expand(fake, ["post_process"], tmp_path / "cache", write_includes=False) assert any("m_bad.fpp" in e for e in errors) assert any(p.name == "m_ok.fpp.f90" for p in files["post_process"]) + + +# --- context: index of expanded files --------------------------------------------------------------------------- + +EXPANDED = """# 1 "/r/src/common/m_g.fpp" +module m_g + implicit none + integer :: dir_idx, nsteps + type t_box + real :: a, b + end type t_box +contains + subroutine s_set_dir(d) + integer, intent(in) :: d + dir_idx = d + end subroutine s_set_dir +end module m_g +# 1 "/r/src/simulation/m_k.fpp" +module m_k + use m_g, only: dir_idx + implicit none +contains + subroutine s_kernel(q) + real, intent(inout) :: q(:) + integer :: i + real :: a + do i = 1, 10 + a = q(i) + q(i) = a + dir_idx + end do + call s_helper(q) + contains + subroutine s_helper(x) + real, intent(inout) :: x(:) + x = 0. + end subroutine s_helper + end subroutine s_kernel +end module m_k +""" + + +def _write(tmp_path, text, target="simulation"): + p = tmp_path / target / "x.f90" + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + return p + + +def _index(tmp_path, text=EXPANDED, target="simulation"): + return context.build_index({target: [_write(tmp_path, text, target)]}) + + +def test_markers_map_lines(tmp_path): + rows = context.read_expanded(_write(tmp_path, EXPANDED)) + assert rows[0] == ("module m_g", "/r/src/common/m_g.fpp", 1) + assert ("module m_k", "/r/src/simulation/m_k.fpp", 1) in rows + + +def test_type_components_are_not_module_vars(tmp_path): + idx = _index(tmp_path) + assert {"dir_idx", "nsteps"} <= set(idx.module_vars) + assert "a" not in idx.module_vars and "b" not in idx.module_vars + + +def test_routine_locals_calls_and_use_only(tmp_path): + idx = _index(tmp_path) + k = next(r for r in idx.routines if r.name == "s_kernel") + assert {"i", "a", "q"} <= k.locals + assert "s_helper" in k.calls + assert k.module == "m_k" and k.src_file == "/r/src/simulation/m_k.fpp" + assert idx.module_uses[("simulation", "m_k")] == {"m_g": {"dir_idx"}} + assert "dir_idx" in next(r for r in idx.routines if r.name == "s_set_dir").assigns From 8e1d0fbe28117436005771b608dff37ef7a71221 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:51:00 -0400 Subject: [PATCH 03/11] Render what to re-read for a diff: routines, copies, callers, callees, globals --- toolchain/mfc/context.py | 113 ++++++++++++++++++++++++++++++++++ toolchain/mfc/test_context.py | 62 +++++++++++++++++++ 2 files changed, 175 insertions(+) diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index 8c3ed5346..16266693c 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -112,3 +112,116 @@ def build_index(files): _finish(r) routines.append(r) return Index(routines, module_vars, dict(module_uses)) + + +CAP = 15 +KERNEL_START, KERNEL_END = "GPU_PARALLEL_LOOP(", "END_GPU_PARALLEL_LOOP" +# Routines that only read, default or broadcast inputs; listing them as writers of a global is noise. +SETUP = re.compile(r"(s_read_input_file|bcast|default|s_initialize_global_parameters_module|s_read_serial)", re.I) + + +def parse_diff(diff_text, repo, side): + """Changed source lines per file (side 'new' or 'old'), changed text per file, and files the diff adds.""" + changed, text, new = collections.defaultdict(set), collections.defaultdict(list), [] + cur, old_is_null = None, False + for line in diff_text.splitlines(): + if line.startswith("--- "): + old_is_null = line.endswith("/dev/null") + if side == "old": + cur = None if old_is_null else str(Path(repo) / line[6:]) + elif line.startswith("+++ "): + path = None if line.endswith("/dev/null") else str(Path(repo) / line[6:]) + if old_is_null and path: + new.append(path) + if side == "new": + cur = path + if cur and "/src/" not in cur: + cur = None + elif line.startswith("@@") and cur: + m = re.search(r"\+(\d+)(?:,(\d+))?" if side == "new" else r"-(\d+)(?:,(\d+))?", line) + start, n = int(m.group(1)), int(m.group(2) or 1) + changed[cur].update(range(start, start + max(n, 1))) + elif cur and line[:1] in "+-": + text[cur].append(line[1:]) + new = [p for p in new if "/src/" in p] + for p in new: + changed.pop(p, None) + return dict(changed), dict(text), new + + +def _rel(path, repo): + try: + return str(Path(path).relative_to(Path(repo))) + except ValueError: + return str(path) + + +def _cap(items): + return items[:CAP] + ([f"- +{len(items) - CAP} more"] if len(items) > CAP else []) + + +def _inline(r0, changed, read_source, repo): + """The routine's source, or the kernel(s) holding the change when the routine is long.""" + lo, hi = r0.src_range + src = read_source(r0.src_file) + mine = sorted(l for l in changed.get(r0.src_file, ()) if lo <= l <= hi) + a, b, note = lo, hi, [] + if hi - lo > 300 and mine: + a = max((i for i in range(lo, mine[0] + 1) if KERNEL_START in src[i - 1]), default=max(lo, mine[0] - 40)) + b = min((i for i in range(mine[-1], hi + 1) if KERNEL_END in src[i - 1]), default=min(hi, mine[-1] + 40)) + note = [f"(routine is {hi - lo + 1} lines; lines {a}-{b} hold the change, the rest is at {_rel(r0.src_file, repo)}:{lo}-{hi})"] + return [*note, "```fortran", *src[a - 1 : b], "```"] + + +def _globals(idx, names, by_name, repo): + lines = set() + for n in names: + r = by_name[n][0] + uses = idx.module_uses.get((r.target, r.module), {}) + for v in sorted((r.idents - r.locals) & set(idx.module_vars)): + owners = {m for m in idx.module_vars[v] if m == r.module or (m in uses and (uses[m] is None or v in uses[m]))} + if not owners: + continue + writers = sorted({f"`{w.name}` ({_rel(w.src_file, repo)}:{w.src_range[0]})" for w in idx.routines if w.target == r.target and v in w.assigns and v not in w.locals and w.name.lower() not in names and not SETUP.search(w.name)}) + if writers: + more = f", +{len(writers) - 4} more" if len(writers) > 4 else "" + lines.add(f"- `{v}` ({', '.join(sorted(owners))}) is set by {', '.join(writers[:4])}{more}") + return sorted(lines) + + +def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_source=None): # pylint: disable=too-many-arguments,too-many-locals + """Markdown: what to re-read to check the diff.""" + read_source = read_source or (lambda f: Path(f).read_text().splitlines()) + if not changed and not new_files: + return "No changes under src/." + by_name = collections.defaultdict(list) + for r in idx.routines: + by_name[r.name.lower()].append(r) + names = sorted({r.name.lower() for r in idx.routines if any((f, l) in r.src_count for f, ls in changed.items() for l in ls)}) + out = ["# What to re-read for this diff", ""] + if new_files: + out += ["## New files: read in full", *(f"- {_rel(p, repo)}" for p in new_files), ""] + out += macro_sections(changed, changed_text, repo) + if not names: + return "\n".join(out) + out.append("## Changed routines") + for k, n in enumerate(names): + r0 = by_name[n][0] + targets = ", ".join(sorted({r.target for r in by_name[n]})) + out.append(f"\n### `{r0.name}` {_rel(r0.src_file, repo)}:{r0.src_range[0]}-{r0.src_range[1]} (in {targets})") + for l in sorted(changed.get(r0.src_file, ())): + if (copies := r0.src_count.get((r0.src_file, l), 0)) > 1: + out.append(f"- line {l} is emitted {copies} times (Fypp loop): check every copy, each kernel has its own clauses") + break + out += ["(not shown inline; read the range above)"] if k >= max_inline else _inline(r0, changed, read_source, repo) + tgts = {r.target for n in names for r in by_name[n]} + callers = sorted({f"- `{r.name}` {_rel(r.src_file, repo)}:{r.src_range[0]}" for r in idx.routines if r.calls & set(names) and r.name.lower() not in names}) + callees = sorted({f"- `{c}` {_rel(x.src_file, repo)}:{x.src_range[0]}" for n in names for r in by_name[n] for c in r.calls if c not in names for x in by_name.get(c, [])[:1] if x.target in tgts}) + out += ["\n## Callers", *(_cap(callers) or ["- none found"])] + out += ["\n## Callees", *(_cap(callees) or ["- none found"])] + out += ["\n## Module variables used here and set elsewhere at run time", *(_cap(_globals(idx, names, by_name, repo)) or ["- none"])] + return "\n".join(out) + + +def macro_sections(changed, changed_text, repo): # pylint: disable=unused-argument + return [] diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 8467df945..305bd3320 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -116,3 +116,65 @@ def test_routine_locals_calls_and_use_only(tmp_path): assert k.module == "m_k" and k.src_file == "/r/src/simulation/m_k.fpp" assert idx.module_uses[("simulation", "m_k")] == {"m_g": {"dir_idx"}} assert "dir_idx" in next(r for r in idx.routines if r.name == "s_set_dir").assigns + + +# --- context: diff mapping and report --------------------------------------------------------------------------- + +EXPANDED_SRC = [ln for ln in EXPANDED.splitlines() if not ln.startswith("#")] + +DIFF_KERNEL = """diff --git a/src/simulation/m_k.fpp b/src/simulation/m_k.fpp +--- a/src/simulation/m_k.fpp ++++ b/src/simulation/m_k.fpp +@@ -10 +10 @@ +- a = q(i) ++ a = 2*q(i) +""" + +COPIES = EXPANDED.replace( + " do i = 1, 10\n a = q(i)\n", + ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', +) + + +def _render(idx, diff, side="new", **kw): + changed, text, new = context.parse_diff(diff, Path("/r"), side) + return context.render(idx, changed, text, new, Path("/r"), read_source=lambda f: EXPANDED_SRC, **kw) + + +def test_parse_diff_sides(): + changed, text, new = context.parse_diff(DIFF_KERNEL, Path("/r"), "new") + assert changed == {"/r/src/simulation/m_k.fpp": {10}} and new == [] + assert any("2*q(i)" in t for t in text["/r/src/simulation/m_k.fpp"]) + assert context.parse_diff(DIFF_KERNEL, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} + + +def test_report_changed_routine_callee_and_global(tmp_path): + out = _render(_index(tmp_path), DIFF_KERNEL) + assert "`s_kernel`" in out + assert "`s_helper`" in out.split("## Callees")[1] + globals_ = out.split("## Module variables")[1] + assert "`dir_idx`" in globals_ and "`s_set_dir`" in globals_ + assert "`a`" not in globals_ and "`i`" not in globals_ + + +def test_report_flags_generated_copies(tmp_path): + assert "emitted 2 times" in _render(_index(tmp_path, COPIES), DIFF_KERNEL) + + +def test_no_src_changes(): + diff = "diff --git a/docs/x.md b/docs/x.md\n--- a/docs/x.md\n+++ b/docs/x.md\n@@ -1 +1 @@\n-a\n+b\n" + assert _render(context.Index([], {}, {}), diff).startswith("No changes under src/.") + + +def test_new_file_is_listed_not_dropped(): + diff = "diff --git a/src/simulation/m_new.fpp b/src/simulation/m_new.fpp\nnew file mode 100644\n--- /dev/null\n+++ b/src/simulation/m_new.fpp\n@@ -0,0 +1,2 @@\n+module m_new\n+end module m_new\n" + out = _render(context.Index([], {}, {}), diff, side="old") + assert "New files" in out and "src/simulation/m_new.fpp" in out + + +def test_inline_is_capped(tmp_path): + many = "".join(f'# 1 "/r/src/simulation/m_{i}.fpp"\nmodule m_{i}\ncontains\n subroutine s_{i}()\n x = 1\n end subroutine s_{i}\nend module m_{i}\n' for i in range(8)) + idx = _index(tmp_path, many) + changed = {f"/r/src/simulation/m_{i}.fpp": {4} for i in range(8)} + out = context.render(idx, changed, {}, [], Path("/r"), read_source=lambda f: ["module m", "contains", "subroutine s", "x = 1", "end subroutine", "end module"]) + assert out.count("```fortran") == 5 and "not shown inline" in out From e2bc51d6afbbc9d07b8b91981c4623dbfdbf5ec0 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:51:34 -0400 Subject: [PATCH 04/11] List call sites of a changed Fypp macro, filtered by the arguments it touches --- toolchain/mfc/context.py | 74 ++++++++++++++++++++++++++++++++++- toolchain/mfc/test_context.py | 18 +++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index 16266693c..b8dfa4178 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -223,5 +223,75 @@ def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_sourc return "\n".join(out) -def macro_sections(changed, changed_text, repo): # pylint: disable=unused-argument - return [] +DEF = re.compile(r"^\s*#:def\s+(\w+)") + + +def _defs(lines): + out, start, name = [], None, None + for i, t in enumerate(lines, 1): + if m := DEF.match(t): + start, name = i, m.group(1) + elif re.match(r"\s*#:enddef", t) and start: + out.append((name, start, i)) + start = None + return out + + +def _joined(text): + """Lines with '&' continuations joined, so a macro call or signature is one line.""" + return re.sub(r"&\s*\n\s*&?", " ", text).splitlines() + + +def _callers_of(name, incs): + """The macro and every macro that calls it, directly or indirectly.""" + found, todo = {name}, [name] + while todo: + cur = todo.pop() + for g in incs: + outer = None + for t in g.read_text().splitlines(): + if m := DEF.match(t): + outer = m.group(1) + elif outer and outer not in found and re.search(rf"\b{cur}\s*\(", t): + found.add(outer) + todo.append(outer) + return found + + +def macro_sections(changed, changed_text, repo): + """Call sites of each changed Fypp macro. + + Fypp line markers record where a macro is defined, not where it expands, so call sites are found by name, + following wrapper macros (GPU_PARALLEL_LOOP -> OMP_PARALLEL_LOOP). When the change mentions some of the + macro's arguments, only call sites that pass them are kept. + """ + repo = Path(repo) + incs = sorted(repo.glob("src/*/include/*.fpp")) + out = [] + for f, ls in changed.items(): + if not f.endswith(".fpp") or not Path(f).exists(): + continue + src = Path(f).read_text().splitlines() + for name, a, b in _defs(src): + if not any(a <= l <= b for l in ls): + continue + callers = _callers_of(name, incs) + sig = re.match(r"\s*#:def\s+\w+\s*\(([^)]*)\)", _joined("\n".join(src[a - 1 : b]))[0]) + args = [x.split("=")[0].strip() for x in sig.group(1).split(",")] if sig else [] + touched = [x for x in args if x and not x.startswith("*") and any(re.search(rf"\b{x}\b", t) for t in changed_text.get(f, []))] + pat = re.compile(r"\b(" + "|".join(sorted(callers)) + r")\s*\(") + byfile = collections.Counter() + for g in sorted(repo.glob("src/**/*.fpp")): + if "/include/" in str(g): + continue + calls = [t for t in _joined(g.read_text()) if re.search(r"[$@]:", t) and pat.search(t)] + if touched: + calls = [t for t in calls if any(re.search(rf"\b{x}\s*=", t) for x in touched)] + if calls: + byfile[_rel(g, repo)] = len(calls) + via = ", ".join(sorted(callers - {name})) or "no other macro" + scope = f" passing {', '.join(touched)}" if touched else "" + out.append(f"## Changed macro `{name}` ({_rel(f, repo)}:{a}-{b})") + out.append(f"Used directly or through {via}: {sum(byfile.values())} call sites{scope} in {len(byfile)} files.") + out += [*_cap([f"- {k}: {v}" for k, v in byfile.most_common()]), ""] + return out diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 305bd3320..f4495e229 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -178,3 +178,21 @@ def test_inline_is_capped(tmp_path): changed = {f"/r/src/simulation/m_{i}.fpp": {4} for i in range(8)} out = context.render(idx, changed, {}, [], Path("/r"), read_source=lambda f: ["module m", "contains", "subroutine s", "x = 1", "end subroutine", "end module"]) assert out.count("```fortran") == 5 and "not shown inline" in out + + +# --- context: changed macros ------------------------------------------------------------------------------------ + + +def test_macro_sites_through_wrapper_filtered_by_argument(tmp_path): + inc = tmp_path / "src" / "common" / "include" + inc.mkdir(parents=True) + (tmp_path / "src" / "simulation").mkdir(parents=True) + (inc / "omp.fpp").write_text("#:def OMP_LOOP(collapse=None, reduction=None, &\n & copy=None)\n #:set red = reduction\n#:enddef\n") + (inc / "par.fpp").write_text("#:def GPU_LOOP(**kw)\n $:OMP_LOOP(**kw)\n#:enddef\n") + (tmp_path / "src" / "simulation" / "m_a.fpp").write_text("$:GPU_LOOP(collapse=3)\n$:GPU_LOOP(reduction='[[s]]', reductionOp='[+]')\n") + (tmp_path / "src" / "simulation" / "m_b.fpp").write_text("$:GPU_LOOP(collapse=2, &\n & reduction='[[t]]')\n") + f = str(inc / "omp.fpp") + out = "\n".join(context.macro_sections({f: {3}}, {f: [" #:set red = reduction"]}, tmp_path)) + assert "OMP_LOOP" in out and "GPU_LOOP" in out + assert "2 call sites passing reduction" in out + assert "src/simulation/m_a.fpp: 1" in out and "src/simulation/m_b.fpp: 1" in out From d4be51d4a6ecd735d82abeb71581603f014b9504 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:52:22 -0400 Subject: [PATCH 05/11] Add ./mfc.sh context and point the re-read rule at it --- .claude/CLAUDE.md | 2 +- toolchain/main.py | 6 +++++- toolchain/mfc/cli/commands.py | 22 +++++++++++++++++++ toolchain/mfc/context.py | 40 +++++++++++++++++++++++++++++++++++ toolchain/mfc/test_context.py | 13 ++++++++++++ 5 files changed, 81 insertions(+), 2 deletions(-) diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index 3f204b1e0..f9b8e2649 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -19,7 +19,7 @@ It holds policy and pointers only; facts live in the linked docs, and `./mfc.sh * Comments should be as short as possible without sacrificing value * GPU macros should follow the source's existing GPU macro principles and patterns * Functions/subroutines/modules shorter is better while being correct, fast, and separating concerns - * When a fix fails twice, stop and re-read in full the code you are changing, its callers and callees, and where each global it uses is set, before the next attempt. Repeated failures usually come from a detail remembered wrong + * When a fix fails twice, stop and re-read in full the code you are changing, its callers and callees, and where each global it uses is set, before the next attempt (`./mfc.sh context` lists them for your diff). Repeated failures usually come from a detail remembered wrong ## Where to look diff --git a/toolchain/main.py b/toolchain/main.py index a2a0783fc..3235180e7 100644 --- a/toolchain/main.py +++ b/toolchain/main.py @@ -124,7 +124,7 @@ def __print_greeting(): def __checks(): - if ARG("command") == "viz": + if ARG("command") in ("viz", "context"): return if not does_command_exist("cmake"): raise MFCException("CMake is required to build MFC but couldn't be located on your system. Please ensure it installed and discoverable (e.g in your system's $PATH).") @@ -162,6 +162,10 @@ def __run(): from mfc import count count.count_diff() + elif cmd == "context": + from mfc import context + + context.context() elif cmd == "clean": from mfc import clean diff --git a/toolchain/mfc/cli/commands.py b/toolchain/mfc/cli/commands.py index 11077e273..75b102c99 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -903,6 +903,27 @@ ], ) +CONTEXT_COMMAND = Command( + name="context", + help="List what to re-read to check a diff: changed routines and their Fypp copies, changed-macro call sites, callers, callees, and module variables set elsewhere.", + include_common=["targets"], + arguments=[ + Argument(name="base", help="Diff the working tree against REV (default: the merge base with origin/master).", type=str, default=None, metavar="REV"), + Argument(name="diff", help="Read the diff from FILE instead of running git diff.", type=str, default=None, metavar="FILE"), + Argument( + name="expand", + help="worktree: expand the working tree and map the diff's new side. base: expand the checked-out base and map the old side (CI).", + type=str, + default="worktree", + choices=["worktree", "base"], + ), + ], + examples=[ + Example("./mfc.sh context", "What to re-read for this branch's changes"), + Example("./mfc.sh context --base HEAD", "Only uncommitted changes"), + ], +) + FP_STABILITY_COMMAND = Command( name="fp-stability", help="Run floating-point stability tests using Verrou.", @@ -1474,6 +1495,7 @@ BENCH_DIFF_COMMAND, COUNT_COMMAND, COUNT_DIFF_COMMAND, + CONTEXT_COMMAND, FP_STABILITY_COMMAND, ], common_sets=[ diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index b8dfa4178..936f06c6b 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -295,3 +295,43 @@ def macro_sections(changed, changed_text, repo): out.append(f"Used directly or through {via}: {sum(byfile.values())} call sites{scope} in {len(byfile)} files.") out += [*_cap([f"- {k}: {v}" for k, v in byfile.most_common()]), ""] return out + + +SOLVER_TARGETS = ("pre_process", "simulation", "post_process") + + +def default_base(repo): + """The merge base with origin/master, or HEAD (with a note) when there is no such remote.""" + import subprocess # pylint: disable=import-outside-toplevel + + r = subprocess.run(["git", "merge-base", "HEAD", "origin/master"], cwd=repo, capture_output=True, text=True, check=False) + if r.returncode == 0 and r.stdout.strip(): + return r.stdout.strip(), None + return "HEAD", "no origin/master: showing uncommitted changes against HEAD" + + +def context(): + """Entry point of `./mfc.sh context`.""" + import subprocess # pylint: disable=import-outside-toplevel + import sys # pylint: disable=import-outside-toplevel + + from . import common, context_expand # pylint: disable=import-outside-toplevel + from .state import ARG # pylint: disable=import-outside-toplevel + + repo = Path(common.MFC_ROOT_DIR) + side = "old" if ARG("expand") == "base" else "new" + note = None + if ARG("diff"): + diff = Path(ARG("diff")).read_text() + else: + base, note = (ARG("base"), None) if ARG("base") else default_base(repo) + diff = subprocess.run(["git", "diff", base, "--", "src"], cwd=repo, capture_output=True, text=True, check=True).stdout + changed, text, new = parse_diff(diff, repo, side) + targets = [t for t in ARG("targets") if t in SOLVER_TARGETS] or list(SOLVER_TARGETS) + files, errors = context_expand.expand(repo, targets, repo / "build" / "context") if changed else ({}, []) + report = render(build_index(files), changed, text, new, repo) + if note: + report = f"({note})\n\n{report}" + if errors: + report += "\n\n## Not expanded\n" + "\n".join(f"- {e}" for e in errors) + sys.stdout.write(report + "\n") diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index f4495e229..7d5dedb2a 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -196,3 +196,16 @@ def test_macro_sites_through_wrapper_filtered_by_argument(tmp_path): assert "OMP_LOOP" in out and "GPU_LOOP" in out assert "2 call sites passing reduction" in out assert "src/simulation/m_a.fpp: 1" in out and "src/simulation/m_b.fpp: 1" in out + + +# --- context: command --------------------------------------------------------------------------------------------- + + +def test_default_base_falls_back_without_origin(tmp_path): + import subprocess # pylint: disable=import-outside-toplevel + + env = {"GIT_AUTHOR_NAME": "a", "GIT_AUTHOR_EMAIL": "a@b", "GIT_COMMITTER_NAME": "a", "GIT_COMMITTER_EMAIL": "a@b", "PATH": "/usr/bin:/bin"} + subprocess.run(["git", "init", "-q", str(tmp_path)], check=True) + subprocess.run(["git", "-C", str(tmp_path), "commit", "-q", "--allow-empty", "-m", "x"], check=True, env=env) + base, note = context.default_base(tmp_path) + assert base == "HEAD" and "origin/master" in note From 17eb76255cecfe6ded2ac5d9a1a6fe7992752707 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:53:57 -0400 Subject: [PATCH 06/11] Map only real changed lines from a diff, context lines included --- toolchain/mfc/context.py | 25 ++++++++++++++++++++----- toolchain/mfc/test_context.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index 936f06c6b..e9b6c73eb 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -138,11 +138,26 @@ def parse_diff(diff_text, repo, side): if cur and "/src/" not in cur: cur = None elif line.startswith("@@") and cur: - m = re.search(r"\+(\d+)(?:,(\d+))?" if side == "new" else r"-(\d+)(?:,(\d+))?", line) - start, n = int(m.group(1)), int(m.group(2) or 1) - changed[cur].update(range(start, start + max(n, 1))) - elif cur and line[:1] in "+-": - text[cur].append(line[1:]) + m = re.match(r"@@ -(\d+)(?:,(\d+))? \+(\d+)", line) + old_ln, new_ln = int(m.group(1)), int(m.group(3)) + if m.group(2) == "0": + old_ln += 1 # git numbers a pure insertion by the line it follows + elif cur and line[:1] in "+- ": + # Count only + and - lines; context lines just advance both sides. + if line[0] == "-": + if side == "old": + changed[cur].add(old_ln) + old_ln += 1 + elif line[0] == "+": + if side == "new": + changed[cur].add(new_ln) + elif old_ln > 1: + changed[cur].add(old_ln - 1) # an insertion maps to the old line it follows + new_ln += 1 + else: + old_ln, new_ln = old_ln + 1, new_ln + 1 + if line[0] != " ": + text[cur].append(line[1:]) new = [p for p in new if "/src/" in p] for p in new: changed.pop(p, None) diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 7d5dedb2a..ad7d4ccb2 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -209,3 +209,34 @@ def test_default_base_falls_back_without_origin(tmp_path): subprocess.run(["git", "-C", str(tmp_path), "commit", "-q", "--allow-empty", "-m", "x"], check=True, env=env) base, note = context.default_base(tmp_path) assert base == "HEAD" and "origin/master" in note + + +def test_parse_diff_ignores_context_lines(): + diff = """--- a/src/simulation/m_k.fpp ++++ b/src/simulation/m_k.fpp +@@ -7,7 +7,8 @@ + integer :: i + real :: a + do i = 1, 10 +- a = q(i) ++ a = 2*q(i) ++ a = a + 1 + q(i) = a + dir_idx + end do + call s_helper(q) +""" + assert context.parse_diff(diff, Path("/r"), "new")[0] == {"/r/src/simulation/m_k.fpp": {10, 11}} + assert context.parse_diff(diff, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} + + +def test_parse_diff_old_side_maps_insertions_to_preceding_line(): + diff = """--- a/src/simulation/m_k.fpp ++++ b/src/simulation/m_k.fpp +@@ -8,4 +8,5 @@ + real :: a + do i = 1, 10 + a = q(i) ++ a = a + 1 + q(i) = a + dir_idx +""" + assert context.parse_diff(diff, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} From afa0ec83049dd571d08d89c05ba689f40df204c6 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:56:26 -0400 Subject: [PATCH 07/11] Map deleted and continuation lines, count copies by separate appearances, add --output --- toolchain/main.py | 3 ++- toolchain/mfc/cli/commands.py | 1 + toolchain/mfc/context.py | 34 ++++++++++++++++++++++++++++++++-- toolchain/mfc/test_context.py | 30 +++++++++++++++++++++++++++++- 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/toolchain/main.py b/toolchain/main.py index 3235180e7..7ef74c6de 100644 --- a/toolchain/main.py +++ b/toolchain/main.py @@ -230,7 +230,8 @@ def __run(): # Auto-regenerate completion scripts if source files changed __ensure_generated_files() - __print_greeting() + if ARG("command") != "context": # its output is meant to be read or captured whole + __print_greeting() __checks() __run() diff --git a/toolchain/mfc/cli/commands.py b/toolchain/mfc/cli/commands.py index 75b102c99..ed4a6b5be 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -910,6 +910,7 @@ arguments=[ Argument(name="base", help="Diff the working tree against REV (default: the merge base with origin/master).", type=str, default=None, metavar="REV"), Argument(name="diff", help="Read the diff from FILE instead of running git diff.", type=str, default=None, metavar="FILE"), + Argument(name="output", short="o", help="Write the report to FILE instead of standard output.", type=str, default=None, metavar="FILE"), Argument( name="expand", help="worktree: expand the working tree and map the diff's new side. base: expand the checked-out base and map the old side (CI).", diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index e9b6c73eb..ed3a67f03 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -57,6 +57,7 @@ class Routine: # pylint: disable=too-many-instance-attributes src_file: str = "" src_range: tuple = (0, 0) src_count: collections.Counter = field(default_factory=collections.Counter) + src_runs: collections.Counter = field(default_factory=collections.Counter) # separate appearances = copies @dataclass @@ -75,6 +76,11 @@ def _finish(r: Routine): r.locals = {n for c in code if (d := DECL.match(c)) for n in _decl_names(d.group(2))} srcs = [(s, l) for _, s, l in r.rows if s.endswith((".fpp", ".f90")) and "/include/" not in s] r.src_count = collections.Counter(srcs) + prev = None + for _, s, l in r.rows: + if (s, l) != prev: + r.src_runs[(s, l)] += 1 + prev = (s, l) if srcs: r.src_file = collections.Counter(s for s, _ in srcs).most_common(1)[0][0] own = [l for s, l in srcs if s == r.src_file] @@ -147,6 +153,8 @@ def parse_diff(diff_text, repo, side): if line[0] == "-": if side == "old": changed[cur].add(old_ln) + else: + changed[cur].add(max(new_ln, 1)) # a deletion maps to the new line now in its place old_ln += 1 elif line[0] == "+": if side == "new": @@ -204,11 +212,30 @@ def _globals(idx, names, by_name, repo): return sorted(lines) +def _statement_starts(changed, read_source): + """Add the first line of each changed line's statement: Fypp marks only first lines of continued statements.""" + out = {} + for f, ls in changed.items(): + try: + src = read_source(f) + except OSError: + out[f] = set(ls) + continue + starts = set(ls) + for l in ls: + while 1 < l <= len(src) and src[l - 2].rstrip().endswith("&"): + l -= 1 + starts.add(l) + out[f] = starts + return out + + def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_source=None): # pylint: disable=too-many-arguments,too-many-locals """Markdown: what to re-read to check the diff.""" read_source = read_source or (lambda f: Path(f).read_text().splitlines()) if not changed and not new_files: return "No changes under src/." + changed = _statement_starts(changed, read_source) by_name = collections.defaultdict(list) for r in idx.routines: by_name[r.name.lower()].append(r) @@ -225,7 +252,7 @@ def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_sourc targets = ", ".join(sorted({r.target for r in by_name[n]})) out.append(f"\n### `{r0.name}` {_rel(r0.src_file, repo)}:{r0.src_range[0]}-{r0.src_range[1]} (in {targets})") for l in sorted(changed.get(r0.src_file, ())): - if (copies := r0.src_count.get((r0.src_file, l), 0)) > 1: + if (copies := r0.src_runs.get((r0.src_file, l), 0)) > 1: out.append(f"- line {l} is emitted {copies} times (Fypp loop): check every copy, each kernel has its own clauses") break out += ["(not shown inline; read the range above)"] if k >= max_inline else _inline(r0, changed, read_source, repo) @@ -349,4 +376,7 @@ def context(): report = f"({note})\n\n{report}" if errors: report += "\n\n## Not expanded\n" + "\n".join(f"- {e}" for e in errors) - sys.stdout.write(report + "\n") + if ARG("output"): + Path(ARG("output")).write_text(report + "\n") + else: + sys.stdout.write(report + "\n") diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index ad7d4ccb2..6ab89c0b5 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -132,7 +132,7 @@ def test_routine_locals_calls_and_use_only(tmp_path): COPIES = EXPANDED.replace( " do i = 1, 10\n a = q(i)\n", - ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', + ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n q(i) = a + dir_idx\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', ) @@ -240,3 +240,31 @@ def test_parse_diff_old_side_maps_insertions_to_preceding_line(): q(i) = a + dir_idx """ assert context.parse_diff(diff, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} + + +def test_parse_diff_new_side_maps_deletions_to_following_line(): + diff = """--- a/src/simulation/m_k.fpp ++++ b/src/simulation/m_k.fpp +@@ -9,3 +9,2 @@ + do i = 1, 10 +- a = q(i) + q(i) = a + dir_idx +""" + assert context.parse_diff(diff, Path("/r"), "new")[0] == {"/r/src/simulation/m_k.fpp": {10}} + + +def test_changed_continuation_line_maps_to_its_statement(tmp_path): + expanded = '# 1 "/r/src/simulation/m_c.fpp"\nmodule m_c\ncontains\n subroutine s_c()\n# 5 "/r/src/simulation/m_c.fpp"\n call s_x(1, 2)\n# 7 "/r/src/simulation/m_c.fpp"\n end subroutine s_c\nend module m_c\n' + source = ["module m_c", "contains", " subroutine s_c()", "", " call s_x(1, &", " & 2)", " end subroutine s_c", "end module m_c"] + idx = _index(tmp_path, expanded) + out = context.render(idx, {"/r/src/simulation/m_c.fpp": {6}}, {}, [], Path("/r"), read_source=lambda f: source) + assert "`s_c`" in out + + +def test_one_macro_expansion_is_not_reported_as_copies(tmp_path): + # A macro call expands to several lines that all map to the call's line: one copy, not several. + expanded = EXPANDED.replace( + " do i = 1, 10\n a = q(i)\n", + ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n!$acc parallel loop\n# 10 "/r/src/simulation/m_k.fpp"\n!$omp target teams loop\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', + ) + assert "emitted" not in _render(_index(tmp_path, expanded), DIFF_KERNEL) From 689fa53b155a21f2db2c22ae76ad0af5c75f139b Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:58:10 -0400 Subject: [PATCH 08/11] Give the Claude review what the diff touches beyond its hunks --- .github/workflows/claude-code-review.yml | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index fc3ad88d1..047e3df3b 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -130,6 +130,15 @@ jobs: echo "review_diff_path=.claude-review/review.diff" >> "$GITHUB_OUTPUT" echo "changed_files_path=.claude-review/changed_files.txt" >> "$GITHUB_OUTPUT" + # Base checkout only, never the PR head: expands the trusted base sources and maps the PR's diff onto them + # by old-side line numbers, so no PR code runs in this privileged job. + - name: What the diff touches beyond its hunks + continue-on-error: true + shell: bash + run: | + ./mfc.sh context --diff "${{ steps.review_input.outputs.review_diff_path }}" --expand base \ + -o .claude-review/context.md > .claude-review/context.log 2>&1 || : > .claude-review/context.md + - name: Prefetch bounded context for changed files only id: context shell: bash @@ -203,7 +212,8 @@ jobs: Scope: - Read only these files: ./.claude/CLAUDE.md, ./docs/documentation/contributing.md, - ./docs/documentation/gpuParallelization.md, the prepared inputs above, and files inside the context directory. + ./docs/documentation/gpuParallelization.md, .claude-review/context.md, the prepared inputs above, and files + inside the context directory. - Every finding must be grounded in a changed hunk. Unchanged lines in changed files are context only. - Do not comment on code outside the changed files, and do not give general repo suggestions or style nitpicks. @@ -215,7 +225,10 @@ jobs: 4) For each changed Fortran or Python file that has a context file, open it and check each changed hunk against its surroundings: declarations, loop bounds, callers and callees within the file, comments that state what the code does, and the matching code paths in other directions, targets or branches. - 5) Write .claude-review/output.md in the format below, then STOP. Do not post GitHub comments yourself. + 5) Read .claude-review/context.md: what the diff touches beyond its hunks (Fypp-generated copies of changed + lines, call sites of changed macros, callers, callees, module variables set elsewhere). Check the diff + against it; findings must still be grounded in changed hunks. + 6) Write .claude-review/output.md in the format below, then STOP. Do not post GitHub comments yourself. What to report: correctness bugs first (logic, numerics, array bounds, precision, memory, MPI, GPU macros, physics consistency, compiler portability), then code that contradicts its own comments or docs. Report a From 5288b16abbf2af6a2bf11e27faf9211871874397 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 11:58:10 -0400 Subject: [PATCH 09/11] Key routines by name and source file in ./mfc.sh context --- toolchain/mfc/context.py | 34 ++++++++++++++++++---------------- toolchain/mfc/test_context.py | 9 +++++++++ 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index ed3a67f03..148c3ef7a 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -196,16 +196,15 @@ def _inline(r0, changed, read_source, repo): return [*note, "```fortran", *src[a - 1 : b], "```"] -def _globals(idx, names, by_name, repo): - lines = set() - for n in names: - r = by_name[n][0] +def _globals(idx, changed_routines, repo): + lines, changed_names = set(), {r.name.lower() for r in changed_routines} + for r in changed_routines: uses = idx.module_uses.get((r.target, r.module), {}) for v in sorted((r.idents - r.locals) & set(idx.module_vars)): owners = {m for m in idx.module_vars[v] if m == r.module or (m in uses and (uses[m] is None or v in uses[m]))} if not owners: continue - writers = sorted({f"`{w.name}` ({_rel(w.src_file, repo)}:{w.src_range[0]})" for w in idx.routines if w.target == r.target and v in w.assigns and v not in w.locals and w.name.lower() not in names and not SETUP.search(w.name)}) + writers = sorted({f"`{w.name}` ({_rel(w.src_file, repo)}:{w.src_range[0]})" for w in idx.routines if w.target == r.target and v in w.assigns and v not in w.locals and w.name.lower() not in changed_names and not SETUP.search(w.name)}) if writers: more = f", +{len(writers) - 4} more" if len(writers) > 4 else "" lines.add(f"- `{v}` ({', '.join(sorted(owners))}) is set by {', '.join(writers[:4])}{more}") @@ -236,32 +235,35 @@ def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_sourc if not changed and not new_files: return "No changes under src/." changed = _statement_starts(changed, read_source) - by_name = collections.defaultdict(list) + # A routine is its name and source file: each target's m_global_parameters has its own same-named routines. + by_key = collections.defaultdict(list) for r in idx.routines: - by_name[r.name.lower()].append(r) - names = sorted({r.name.lower() for r in idx.routines if any((f, l) in r.src_count for f, ls in changed.items() for l in ls)}) + by_key[(r.name.lower(), r.src_file)].append(r) + keys = sorted(k for k, rs in by_key.items() if any((f, l) in rs[0].src_count for f, ls in changed.items() for l in ls)) out = ["# What to re-read for this diff", ""] if new_files: out += ["## New files: read in full", *(f"- {_rel(p, repo)}" for p in new_files), ""] out += macro_sections(changed, changed_text, repo) - if not names: + if not keys: return "\n".join(out) out.append("## Changed routines") - for k, n in enumerate(names): - r0 = by_name[n][0] - targets = ", ".join(sorted({r.target for r in by_name[n]})) + for k, key in enumerate(keys): + r0 = by_key[key][0] + targets = ", ".join(sorted({r.target for r in by_key[key]})) out.append(f"\n### `{r0.name}` {_rel(r0.src_file, repo)}:{r0.src_range[0]}-{r0.src_range[1]} (in {targets})") for l in sorted(changed.get(r0.src_file, ())): if (copies := r0.src_runs.get((r0.src_file, l), 0)) > 1: out.append(f"- line {l} is emitted {copies} times (Fypp loop): check every copy, each kernel has its own clauses") break out += ["(not shown inline; read the range above)"] if k >= max_inline else _inline(r0, changed, read_source, repo) - tgts = {r.target for n in names for r in by_name[n]} - callers = sorted({f"- `{r.name}` {_rel(r.src_file, repo)}:{r.src_range[0]}" for r in idx.routines if r.calls & set(names) and r.name.lower() not in names}) - callees = sorted({f"- `{c}` {_rel(x.src_file, repo)}:{x.src_range[0]}" for n in names for r in by_name[n] for c in r.calls if c not in names for x in by_name.get(c, [])[:1] if x.target in tgts}) + changed_routines = [r for key in keys for r in by_key[key]] + tgts = {r.target for r in changed_routines} + names = {r.name.lower() for r in changed_routines} + callers = sorted({f"- `{r.name}` {_rel(r.src_file, repo)}:{r.src_range[0]}" for r in idx.routines if r.target in tgts and r.calls & names and (r.name.lower(), r.src_file) not in keys}) + callees = sorted({f"- `{x.name}` {_rel(x.src_file, repo)}:{x.src_range[0]}" for r in changed_routines for x in idx.routines if x.target == r.target and x.name.lower() in r.calls and x.name.lower() not in names}) out += ["\n## Callers", *(_cap(callers) or ["- none found"])] out += ["\n## Callees", *(_cap(callees) or ["- none found"])] - out += ["\n## Module variables used here and set elsewhere at run time", *(_cap(_globals(idx, names, by_name, repo)) or ["- none"])] + out += ["\n## Module variables used here and set elsewhere at run time", *(_cap(_globals(idx, changed_routines, repo)) or ["- none"])] return "\n".join(out) diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 6ab89c0b5..215ee41da 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -268,3 +268,12 @@ def test_one_macro_expansion_is_not_reported_as_copies(tmp_path): ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n!$acc parallel loop\n# 10 "/r/src/simulation/m_k.fpp"\n!$omp target teams loop\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', ) assert "emitted" not in _render(_index(tmp_path, expanded), DIFF_KERNEL) + + +def test_same_named_routines_in_different_files_are_kept_apart(tmp_path): + pre = '# 1 "/r/src/pre_process/m_gp.fpp"\nmodule m_gp\ncontains\n subroutine s_defaults()\n x = 1\n end subroutine s_defaults\nend module m_gp\n' + sim = '# 1 "/r/src/simulation/m_gp.fpp"\nmodule m_gp\ncontains\n subroutine s_defaults()\n y = 2\n end subroutine s_defaults\nend module m_gp\n' + idx = context.build_index({"pre_process": [_write(tmp_path, pre, "pre_process")], "simulation": [_write(tmp_path, sim, "simulation")]}) + out = context.render(idx, {"/r/src/simulation/m_gp.fpp": {4}}, {}, [], Path("/r"), read_source=lambda f: ["module m_gp", "contains", "subroutine s_defaults()", "y = 2", "end subroutine", "end module"]) + assert "src/simulation/m_gp.fpp" in out and "src/pre_process/m_gp.fpp" not in out + assert "(in simulation)" in out From f4f7ddd4b183a574345e091e932bd90e7545e7b5 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 12:03:09 -0400 Subject: [PATCH 10/11] Lint fixes for ./mfc.sh context --- toolchain/mfc/context.py | 50 ++++++++++++++++++++++------------- toolchain/mfc/test_context.py | 26 +++++++++--------- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index 148c3ef7a..378e78c8d 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -74,16 +74,16 @@ def _finish(r: Routine): r.idents = {w.lower() for w in re.findall(r"(? 300 and mine: a = max((i for i in range(lo, mine[0] + 1) if KERNEL_START in src[i - 1]), default=max(lo, mine[0] - 40)) @@ -204,7 +204,13 @@ def _globals(idx, changed_routines, repo): owners = {m for m in idx.module_vars[v] if m == r.module or (m in uses and (uses[m] is None or v in uses[m]))} if not owners: continue - writers = sorted({f"`{w.name}` ({_rel(w.src_file, repo)}:{w.src_range[0]})" for w in idx.routines if w.target == r.target and v in w.assigns and v not in w.locals and w.name.lower() not in changed_names and not SETUP.search(w.name)}) + writers = sorted( + { + f"`{w.name}` ({_rel(w.src_file, repo)}:{w.src_range[0]})" + for w in idx.routines + if w.target == r.target and v in w.assigns and v not in w.locals and w.name.lower() not in changed_names and not SETUP.search(w.name) + } + ) if writers: more = f", +{len(writers) - 4} more" if len(writers) > 4 else "" lines.add(f"- `{v}` ({', '.join(sorted(owners))}) is set by {', '.join(writers[:4])}{more}") @@ -221,10 +227,11 @@ def _statement_starts(changed, read_source): out[f] = set(ls) continue starts = set(ls) - for l in ls: - while 1 < l <= len(src) and src[l - 2].rstrip().endswith("&"): - l -= 1 - starts.add(l) + for ln in ls: + start = ln + while 1 < start <= len(src) and src[start - 2].rstrip().endswith("&"): + start -= 1 + starts.add(start) out[f] = starts return out @@ -239,7 +246,7 @@ def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_sourc by_key = collections.defaultdict(list) for r in idx.routines: by_key[(r.name.lower(), r.src_file)].append(r) - keys = sorted(k for k, rs in by_key.items() if any((f, l) in rs[0].src_count for f, ls in changed.items() for l in ls)) + keys = sorted(k for k, rs in by_key.items() if any((f, ln) in rs[0].src_count for f, ls in changed.items() for ln in ls)) out = ["# What to re-read for this diff", ""] if new_files: out += ["## New files: read in full", *(f"- {_rel(p, repo)}" for p in new_files), ""] @@ -251,16 +258,23 @@ def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_sourc r0 = by_key[key][0] targets = ", ".join(sorted({r.target for r in by_key[key]})) out.append(f"\n### `{r0.name}` {_rel(r0.src_file, repo)}:{r0.src_range[0]}-{r0.src_range[1]} (in {targets})") - for l in sorted(changed.get(r0.src_file, ())): - if (copies := r0.src_runs.get((r0.src_file, l), 0)) > 1: - out.append(f"- line {l} is emitted {copies} times (Fypp loop): check every copy, each kernel has its own clauses") + for ln in sorted(changed.get(r0.src_file, ())): + if (copies := r0.src_runs.get((r0.src_file, ln), 0)) > 1: + out.append(f"- line {ln} is emitted {copies} times (Fypp loop): check every copy, each kernel has its own clauses") break out += ["(not shown inline; read the range above)"] if k >= max_inline else _inline(r0, changed, read_source, repo) changed_routines = [r for key in keys for r in by_key[key]] tgts = {r.target for r in changed_routines} names = {r.name.lower() for r in changed_routines} callers = sorted({f"- `{r.name}` {_rel(r.src_file, repo)}:{r.src_range[0]}" for r in idx.routines if r.target in tgts and r.calls & names and (r.name.lower(), r.src_file) not in keys}) - callees = sorted({f"- `{x.name}` {_rel(x.src_file, repo)}:{x.src_range[0]}" for r in changed_routines for x in idx.routines if x.target == r.target and x.name.lower() in r.calls and x.name.lower() not in names}) + callees = sorted( + { + f"- `{x.name}` {_rel(x.src_file, repo)}:{x.src_range[0]}" + for r in changed_routines + for x in idx.routines + if x.target == r.target and x.name.lower() in r.calls and x.name.lower() not in names + } + ) out += ["\n## Callers", *(_cap(callers) or ["- none found"])] out += ["\n## Callees", *(_cap(callees) or ["- none found"])] out += ["\n## Module variables used here and set elsewhere at run time", *(_cap(_globals(idx, changed_routines, repo)) or ["- none"])] @@ -317,7 +331,7 @@ def macro_sections(changed, changed_text, repo): continue src = Path(f).read_text().splitlines() for name, a, b in _defs(src): - if not any(a <= l <= b for l in ls): + if not any(a <= ln <= b for ln in ls): continue callers = _callers_of(name, incs) sig = re.match(r"\s*#:def\s+\w+\s*\(([^)]*)\)", _joined("\n".join(src[a - 1 : b]))[0]) diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 215ee41da..918f138f2 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -46,8 +46,6 @@ def test_expand_reports_fypp_error_and_continues(tmp_path): assert any(p.name == "m_ok.fpp.f90" for p in files["post_process"]) -# --- context: index of expanded files --------------------------------------------------------------------------- - EXPANDED = """# 1 "/r/src/common/m_g.fpp" module m_g implicit none @@ -118,8 +116,6 @@ def test_routine_locals_calls_and_use_only(tmp_path): assert "dir_idx" in next(r for r in idx.routines if r.name == "s_set_dir").assigns -# --- context: diff mapping and report --------------------------------------------------------------------------- - EXPANDED_SRC = [ln for ln in EXPANDED.splitlines() if not ln.startswith("#")] DIFF_KERNEL = """diff --git a/src/simulation/m_k.fpp b/src/simulation/m_k.fpp @@ -180,9 +176,6 @@ def test_inline_is_capped(tmp_path): assert out.count("```fortran") == 5 and "not shown inline" in out -# --- context: changed macros ------------------------------------------------------------------------------------ - - def test_macro_sites_through_wrapper_filtered_by_argument(tmp_path): inc = tmp_path / "src" / "common" / "include" inc.mkdir(parents=True) @@ -198,9 +191,6 @@ def test_macro_sites_through_wrapper_filtered_by_argument(tmp_path): assert "src/simulation/m_a.fpp: 1" in out and "src/simulation/m_b.fpp: 1" in out -# --- context: command --------------------------------------------------------------------------------------------- - - def test_default_base_falls_back_without_origin(tmp_path): import subprocess # pylint: disable=import-outside-toplevel @@ -254,7 +244,11 @@ def test_parse_diff_new_side_maps_deletions_to_following_line(): def test_changed_continuation_line_maps_to_its_statement(tmp_path): - expanded = '# 1 "/r/src/simulation/m_c.fpp"\nmodule m_c\ncontains\n subroutine s_c()\n# 5 "/r/src/simulation/m_c.fpp"\n call s_x(1, 2)\n# 7 "/r/src/simulation/m_c.fpp"\n end subroutine s_c\nend module m_c\n' + expanded = ( + '# 1 "/r/src/simulation/m_c.fpp"\nmodule m_c\ncontains\n subroutine s_c()\n' + '# 5 "/r/src/simulation/m_c.fpp"\n call s_x(1, 2)\n# 7 "/r/src/simulation/m_c.fpp"\n' + " end subroutine s_c\nend module m_c\n" + ) source = ["module m_c", "contains", " subroutine s_c()", "", " call s_x(1, &", " & 2)", " end subroutine s_c", "end module m_c"] idx = _index(tmp_path, expanded) out = context.render(idx, {"/r/src/simulation/m_c.fpp": {6}}, {}, [], Path("/r"), read_source=lambda f: source) @@ -265,7 +259,11 @@ def test_one_macro_expansion_is_not_reported_as_copies(tmp_path): # A macro call expands to several lines that all map to the call's line: one copy, not several. expanded = EXPANDED.replace( " do i = 1, 10\n a = q(i)\n", - ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n!$acc parallel loop\n# 10 "/r/src/simulation/m_k.fpp"\n!$omp target teams loop\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', + ( + ' do i = 1, 10\n# 10 "/r/src/simulation/m_k.fpp"\n!$acc parallel loop\n' + '# 10 "/r/src/simulation/m_k.fpp"\n!$omp target teams loop\n' + '# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n' + ), ) assert "emitted" not in _render(_index(tmp_path, expanded), DIFF_KERNEL) @@ -274,6 +272,8 @@ def test_same_named_routines_in_different_files_are_kept_apart(tmp_path): pre = '# 1 "/r/src/pre_process/m_gp.fpp"\nmodule m_gp\ncontains\n subroutine s_defaults()\n x = 1\n end subroutine s_defaults\nend module m_gp\n' sim = '# 1 "/r/src/simulation/m_gp.fpp"\nmodule m_gp\ncontains\n subroutine s_defaults()\n y = 2\n end subroutine s_defaults\nend module m_gp\n' idx = context.build_index({"pre_process": [_write(tmp_path, pre, "pre_process")], "simulation": [_write(tmp_path, sim, "simulation")]}) - out = context.render(idx, {"/r/src/simulation/m_gp.fpp": {4}}, {}, [], Path("/r"), read_source=lambda f: ["module m_gp", "contains", "subroutine s_defaults()", "y = 2", "end subroutine", "end module"]) + out = context.render( + idx, {"/r/src/simulation/m_gp.fpp": {4}}, {}, [], Path("/r"), read_source=lambda f: ["module m_gp", "contains", "subroutine s_defaults()", "y = 2", "end subroutine", "end module"] + ) assert "src/simulation/m_gp.fpp" in out and "src/pre_process/m_gp.fpp" not in out assert "(in simulation)" in out From dd61ce764349be0cdd3a89bfd0c09242f22e964a Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 11 Oct 2026 12:49:13 -0400 Subject: [PATCH 11/11] Review fixes for ./mfc.sh context Map CI diffs onto the PR's merge base (--root on a merge-base worktree), not the tip of master; flag hunks that do not match the source; list changed lines outside any routine, deleted files and untracked new files; read src paths relative to the repo; make git diff immune to user diff settings; do not crash on blank hunk lines; drop a file's old expansion when Fypp fails. --- .github/workflows/claude-code-review.yml | 17 ++- toolchain/mfc/cli/commands.py | 1 + toolchain/mfc/context.py | 140 +++++++++++++++-------- toolchain/mfc/context_expand.py | 1 + toolchain/mfc/test_context.py | 87 +++++++++++--- 5 files changed, 184 insertions(+), 62 deletions(-) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 047e3df3b..0cf71d1b7 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -130,14 +130,23 @@ jobs: echo "review_diff_path=.claude-review/review.diff" >> "$GITHUB_OUTPUT" echo "changed_files_path=.claude-review/changed_files.txt" >> "$GITHUB_OUTPUT" - # Base checkout only, never the PR head: expands the trusted base sources and maps the PR's diff onto them - # by old-side line numbers, so no PR code runs in this privileged job. + # Never the PR head: the tool from this trusted checkout analyzes a worktree of the PR's merge base, which is + # in the base branch's history, so the diff's old-side line numbers point at the code they came from and no PR + # code runs in this privileged job. - name: What the diff touches beyond its hunks continue-on-error: true shell: bash + env: + GH_TOKEN: ${{ github.token }} run: | - ./mfc.sh context --diff "${{ steps.review_input.outputs.review_diff_path }}" --expand base \ - -o .claude-review/context.md > .claude-review/context.log 2>&1 || : > .claude-review/context.md + PR_NUMBER="${{ steps.mode.outputs.pr_number }}" + REPO="${{ github.repository }}" + BASE_REF="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefName --jq .baseRefName)" + MERGE_BASE="$(gh api "repos/$REPO/compare/$BASE_REF...${{ steps.state.outputs.current_head_sha }}" --jq .merge_base_commit.sha)" + git worktree add --detach .claude-review/merge-base "$MERGE_BASE" > /dev/null + ./mfc.sh context --root .claude-review/merge-base --diff "${{ steps.review_input.outputs.review_diff_path }}" \ + --expand base -o .claude-review/context.md > .claude-review/context.log 2>&1 || : > .claude-review/context.md + [ -s .claude-review/context.md ] || echo "::warning::./mfc.sh context produced no output; see .claude-review/context.log" - name: Prefetch bounded context for changed files only id: context diff --git a/toolchain/mfc/cli/commands.py b/toolchain/mfc/cli/commands.py index ed4a6b5be..a8e440e4d 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -911,6 +911,7 @@ Argument(name="base", help="Diff the working tree against REV (default: the merge base with origin/master).", type=str, default=None, metavar="REV"), Argument(name="diff", help="Read the diff from FILE instead of running git diff.", type=str, default=None, metavar="FILE"), Argument(name="output", short="o", help="Write the report to FILE instead of standard output.", type=str, default=None, metavar="FILE"), + Argument(name="root", help="Analyze the source tree at DIR instead of this checkout (CI: a worktree of the PR's merge base).", type=str, default=None, metavar="DIR"), Argument( name="expand", help="worktree: expand the working tree and map the diff's new side. base: expand the checked-out base and map the old side (CI).", diff --git a/toolchain/mfc/context.py b/toolchain/mfc/context.py index 378e78c8d..6145f4cae 100644 --- a/toolchain/mfc/context.py +++ b/toolchain/mfc/context.py @@ -6,6 +6,8 @@ import collections import re +import subprocess +import sys from dataclasses import dataclass, field from pathlib import Path @@ -74,7 +76,7 @@ def _finish(r: Routine): r.idents = {w.lower() for w in re.findall(r"(? changed line numbers (on the mapped side) + text: dict = field(default_factory=dict) # absolute source path -> text of its + and - lines + new: list = field(default_factory=list) # files the diff adds + deleted: list = field(default_factory=list) # files the diff removes + checks: dict = field(default_factory=dict) # absolute source path -> [(line, text)] the mapped side should contain + + def parse_diff(diff_text, repo, side): - """Changed source lines per file (side 'new' or 'old'), changed text per file, and files the diff adds.""" - changed, text, new = collections.defaultdict(set), collections.defaultdict(list), [] - cur, old_is_null = None, False + """Map a unified diff onto source lines of one side ('new': the working tree, 'old': the base).""" + d = Diff(changed=collections.defaultdict(set), text=collections.defaultdict(list), checks=collections.defaultdict(list)) + cur, old_path = None, None + + def src_path(header): + rel = header[6:].rstrip("\t") + return None if header.endswith("/dev/null") or not rel.startswith("src/") else str(Path(repo) / rel) + for line in diff_text.splitlines(): if line.startswith("--- "): - old_is_null = line.endswith("/dev/null") + old_path = src_path(line) if not line.endswith("/dev/null") else "/dev/null" if side == "old": - cur = None if old_is_null else str(Path(repo) / line[6:]) + cur = old_path if old_path != "/dev/null" else None elif line.startswith("+++ "): - path = None if line.endswith("/dev/null") else str(Path(repo) / line[6:]) - if old_is_null and path: - new.append(path) + new_path = src_path(line) + if old_path == "/dev/null" and new_path: + d.new.append(new_path) + if line.endswith("/dev/null") and old_path not in (None, "/dev/null"): + d.deleted.append(old_path) if side == "new": - cur = path - if cur and "/src/" not in cur: - cur = None + cur = new_path elif line.startswith("@@") and cur: m = re.match(r"@@ -(\d+)(?:,(\d+))? \+(\d+)", line) old_ln, new_ln = int(m.group(1)), int(m.group(3)) if m.group(2) == "0": old_ln += 1 # git numbers a pure insertion by the line it follows - elif cur and line[:1] in "+- ": - # Count only + and - lines; context lines just advance both sides. - if line[0] == "-": + elif cur and (not line or line[0] in "+- "): + kind, body = (line[0], line[1:]) if line else (" ", "") + if kind == "-": + d.changed[cur].add(old_ln if side == "old" else max(new_ln, 1)) # a deletion maps to the line now in its place if side == "old": - changed[cur].add(old_ln) - else: - changed[cur].add(max(new_ln, 1)) # a deletion maps to the new line now in its place + d.checks[cur].append((old_ln, body)) old_ln += 1 - elif line[0] == "+": + elif kind == "+": if side == "new": - changed[cur].add(new_ln) + d.changed[cur].add(new_ln) + d.checks[cur].append((new_ln, body)) elif old_ln > 1: - changed[cur].add(old_ln - 1) # an insertion maps to the old line it follows + d.changed[cur].add(old_ln - 1) # an insertion maps to the old line it follows new_ln += 1 else: + d.checks[cur].append((new_ln if side == "new" else old_ln, body)) old_ln, new_ln = old_ln + 1, new_ln + 1 - if line[0] != " ": - text[cur].append(line[1:]) - new = [p for p in new if "/src/" in p] - for p in new: - changed.pop(p, None) - return dict(changed), dict(text), new + if kind != " ": + d.text[cur].append(body) + for p in d.new + d.deleted: + d.changed.pop(p, None) + d.changed, d.text, d.checks = dict(d.changed), dict(d.text), dict(d.checks) + return d def _rel(path, repo): @@ -218,39 +235,63 @@ def _globals(idx, changed_routines, repo): def _statement_starts(changed, read_source): - """Add the first line of each changed line's statement: Fypp marks only first lines of continued statements.""" + """Map each changed line to the first line of its statement: Fypp marks only first lines of continued statements.""" out = {} for f, ls in changed.items(): try: src = read_source(f) except OSError: - out[f] = set(ls) - continue - starts = set(ls) + src = [] + starts = {} for ln in ls: start = ln while 1 < start <= len(src) and src[start - 2].rstrip().endswith("&"): start -= 1 - starts.add(start) + starts[ln] = start out[f] = starts return out -def render(idx, changed, changed_text, new_files, repo, max_inline=5, read_source=None): # pylint: disable=too-many-arguments,too-many-locals +def _mismatches(diff, read_source, repo): + """Hunks whose lines differ from the file being mapped onto: line numbers there would point at other code.""" + out = [] + for f, checks in diff.checks.items(): + try: + src = read_source(f) + except OSError: + continue + bad = [ln for ln, body in checks if ln > len(src) or src[ln - 1].rstrip() != body.rstrip()] + if bad: + out.append(f"- {_rel(f, repo)}: the diff does not match this file at line {bad[0]} ({len(bad)} lines); results for it may point at other code") + return out + + +def render(idx, diff, repo, max_inline=5, read_source=None): # pylint: disable=too-many-locals """Markdown: what to re-read to check the diff.""" read_source = read_source or (lambda f: Path(f).read_text().splitlines()) - if not changed and not new_files: + if not diff.changed and not diff.new and not diff.deleted: return "No changes under src/." - changed = _statement_starts(changed, read_source) + starts = _statement_starts(diff.changed, read_source) + changed = {f: set(m) | set(m.values()) for f, m in starts.items()} + new_files = diff.new # A routine is its name and source file: each target's m_global_parameters has its own same-named routines. by_key = collections.defaultdict(list) for r in idx.routines: by_key[(r.name.lower(), r.src_file)].append(r) keys = sorted(k for k, rs in by_key.items() if any((f, ln) in rs[0].src_count for f, ls in changed.items() for ln in ls)) out = ["# What to re-read for this diff", ""] + if bad := _mismatches(diff, read_source, repo): + out += ["## Diff does not match the source", *bad, ""] if new_files: out += ["## New files: read in full", *(f"- {_rel(p, repo)}" for p in new_files), ""] - out += macro_sections(changed, changed_text, repo) + if diff.deleted: + out += ["## Deleted files: check what used them", *(f"- {_rel(p, repo)}" for p in diff.deleted), ""] + out += macro_sections(changed, diff.text, repo) + covered = {(f, ln) for key in keys for (f, ln) in by_key[key][0].src_count} + loose = {f: sorted(ln for ln, start in m.items() if (f, start) not in covered) for f, m in starts.items() if "/include/" not in _rel(f, repo)} + if loose := {f: ls for f, ls in loose.items() if ls}: + out += ["## Not in any routine (module declarations, or code this build configuration leaves out): read these lines directly"] + out += [f"- {_rel(f, repo)}: {', '.join(map(str, ls))}" for f, ls in sorted(loose.items())] + [""] if not keys: return "\n".join(out) out.append("## Changed routines") @@ -360,34 +401,43 @@ def macro_sections(changed, changed_text, repo): def default_base(repo): """The merge base with origin/master, or HEAD (with a note) when there is no such remote.""" - import subprocess # pylint: disable=import-outside-toplevel - r = subprocess.run(["git", "merge-base", "HEAD", "origin/master"], cwd=repo, capture_output=True, text=True, check=False) if r.returncode == 0 and r.stdout.strip(): return r.stdout.strip(), None return "HEAD", "no origin/master: showing uncommitted changes against HEAD" +def git_diff(repo, base): + """`git diff` of src/ against base, immune to user diff settings (prefixes, colour, external tools).""" + cmd = ["git", "-c", "diff.noprefix=false", "diff", "--no-color", "--no-ext-diff", "--src-prefix=a/", "--dst-prefix=b/", base, "--", "src"] + return subprocess.run(cmd, cwd=repo, capture_output=True, text=True, check=True).stdout + + +def untracked_sources(repo): + out = subprocess.run(["git", "ls-files", "--others", "--exclude-standard", "--", "src"], cwd=repo, capture_output=True, text=True, check=False).stdout + return [str(Path(repo) / p) for p in out.split()] + + def context(): """Entry point of `./mfc.sh context`.""" - import subprocess # pylint: disable=import-outside-toplevel - import sys # pylint: disable=import-outside-toplevel from . import common, context_expand # pylint: disable=import-outside-toplevel from .state import ARG # pylint: disable=import-outside-toplevel - repo = Path(common.MFC_ROOT_DIR) + repo = Path(ARG("root") or common.MFC_ROOT_DIR).resolve() side = "old" if ARG("expand") == "base" else "new" note = None if ARG("diff"): diff = Path(ARG("diff")).read_text() else: base, note = (ARG("base"), None) if ARG("base") else default_base(repo) - diff = subprocess.run(["git", "diff", base, "--", "src"], cwd=repo, capture_output=True, text=True, check=True).stdout - changed, text, new = parse_diff(diff, repo, side) + diff = git_diff(repo, base) + d = parse_diff(diff, repo, side) + if not ARG("diff"): + d.new += [p for p in untracked_sources(repo) if p not in d.new] targets = [t for t in ARG("targets") if t in SOLVER_TARGETS] or list(SOLVER_TARGETS) - files, errors = context_expand.expand(repo, targets, repo / "build" / "context") if changed else ({}, []) - report = render(build_index(files), changed, text, new, repo) + files, errors = context_expand.expand(repo, targets, repo / "build" / "context") if d.changed else ({}, []) + report = render(build_index(files), d, repo) if note: report = f"({note})\n\n{report}" if errors: diff --git a/toolchain/mfc/context_expand.py b/toolchain/mfc/context_expand.py index f6661af1b..a23dd59dd 100644 --- a/toolchain/mfc/context_expand.py +++ b/toolchain/mfc/context_expand.py @@ -77,6 +77,7 @@ def run(job): r = subprocess.run(fypp_command(fypp, repo, include_dir, t, src, keyed, compiler), capture_output=True, text=True, check=False) if r.returncode: keyed.unlink(missing_ok=True) + final.unlink(missing_ok=True) # never index the previous expansion against new line numbers return f"could not expand {src.relative_to(repo)}: {(r.stderr.strip().splitlines() or ['?'])[-1]}" if not final.is_symlink() or final.resolve() != keyed: final.unlink(missing_ok=True) diff --git a/toolchain/mfc/test_context.py b/toolchain/mfc/test_context.py index 918f138f2..2597f966b 100644 --- a/toolchain/mfc/test_context.py +++ b/toolchain/mfc/test_context.py @@ -133,15 +133,14 @@ def test_routine_locals_calls_and_use_only(tmp_path): def _render(idx, diff, side="new", **kw): - changed, text, new = context.parse_diff(diff, Path("/r"), side) - return context.render(idx, changed, text, new, Path("/r"), read_source=lambda f: EXPANDED_SRC, **kw) + return context.render(idx, context.parse_diff(diff, Path("/r"), side), Path("/r"), read_source=lambda f: EXPANDED_SRC, **kw) def test_parse_diff_sides(): - changed, text, new = context.parse_diff(DIFF_KERNEL, Path("/r"), "new") - assert changed == {"/r/src/simulation/m_k.fpp": {10}} and new == [] - assert any("2*q(i)" in t for t in text["/r/src/simulation/m_k.fpp"]) - assert context.parse_diff(DIFF_KERNEL, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} + d = context.parse_diff(DIFF_KERNEL, Path("/r"), "new") + assert d.changed == {"/r/src/simulation/m_k.fpp": {10}} and d.new == [] + assert any("2*q(i)" in t for t in d.text["/r/src/simulation/m_k.fpp"]) + assert context.parse_diff(DIFF_KERNEL, Path("/r"), "old").changed == {"/r/src/simulation/m_k.fpp": {10}} def test_report_changed_routine_callee_and_global(tmp_path): @@ -172,7 +171,7 @@ def test_inline_is_capped(tmp_path): many = "".join(f'# 1 "/r/src/simulation/m_{i}.fpp"\nmodule m_{i}\ncontains\n subroutine s_{i}()\n x = 1\n end subroutine s_{i}\nend module m_{i}\n' for i in range(8)) idx = _index(tmp_path, many) changed = {f"/r/src/simulation/m_{i}.fpp": {4} for i in range(8)} - out = context.render(idx, changed, {}, [], Path("/r"), read_source=lambda f: ["module m", "contains", "subroutine s", "x = 1", "end subroutine", "end module"]) + out = context.render(idx, context.Diff(changed=changed), Path("/r"), read_source=lambda f: ["module m", "contains", "subroutine s", "x = 1", "end subroutine", "end module"]) assert out.count("```fortran") == 5 and "not shown inline" in out @@ -215,8 +214,8 @@ def test_parse_diff_ignores_context_lines(): end do call s_helper(q) """ - assert context.parse_diff(diff, Path("/r"), "new")[0] == {"/r/src/simulation/m_k.fpp": {10, 11}} - assert context.parse_diff(diff, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} + assert context.parse_diff(diff, Path("/r"), "new").changed == {"/r/src/simulation/m_k.fpp": {10, 11}} + assert context.parse_diff(diff, Path("/r"), "old").changed == {"/r/src/simulation/m_k.fpp": {10}} def test_parse_diff_old_side_maps_insertions_to_preceding_line(): @@ -229,7 +228,7 @@ def test_parse_diff_old_side_maps_insertions_to_preceding_line(): + a = a + 1 q(i) = a + dir_idx """ - assert context.parse_diff(diff, Path("/r"), "old")[0] == {"/r/src/simulation/m_k.fpp": {10}} + assert context.parse_diff(diff, Path("/r"), "old").changed == {"/r/src/simulation/m_k.fpp": {10}} def test_parse_diff_new_side_maps_deletions_to_following_line(): @@ -240,7 +239,7 @@ def test_parse_diff_new_side_maps_deletions_to_following_line(): - a = q(i) q(i) = a + dir_idx """ - assert context.parse_diff(diff, Path("/r"), "new")[0] == {"/r/src/simulation/m_k.fpp": {10}} + assert context.parse_diff(diff, Path("/r"), "new").changed == {"/r/src/simulation/m_k.fpp": {10}} def test_changed_continuation_line_maps_to_its_statement(tmp_path): @@ -251,8 +250,9 @@ def test_changed_continuation_line_maps_to_its_statement(tmp_path): ) source = ["module m_c", "contains", " subroutine s_c()", "", " call s_x(1, &", " & 2)", " end subroutine s_c", "end module m_c"] idx = _index(tmp_path, expanded) - out = context.render(idx, {"/r/src/simulation/m_c.fpp": {6}}, {}, [], Path("/r"), read_source=lambda f: source) + out = context.render(idx, context.Diff(changed={"/r/src/simulation/m_c.fpp": {6}}), Path("/r"), read_source=lambda f: source) assert "`s_c`" in out + assert "Not in any routine" not in out def test_one_macro_expansion_is_not_reported_as_copies(tmp_path): @@ -273,7 +273,68 @@ def test_same_named_routines_in_different_files_are_kept_apart(tmp_path): sim = '# 1 "/r/src/simulation/m_gp.fpp"\nmodule m_gp\ncontains\n subroutine s_defaults()\n y = 2\n end subroutine s_defaults\nend module m_gp\n' idx = context.build_index({"pre_process": [_write(tmp_path, pre, "pre_process")], "simulation": [_write(tmp_path, sim, "simulation")]}) out = context.render( - idx, {"/r/src/simulation/m_gp.fpp": {4}}, {}, [], Path("/r"), read_source=lambda f: ["module m_gp", "contains", "subroutine s_defaults()", "y = 2", "end subroutine", "end module"] + idx, + context.Diff(changed={"/r/src/simulation/m_gp.fpp": {4}}), + Path("/r"), + read_source=lambda f: ["module m_gp", "contains", "subroutine s_defaults()", "y = 2", "end subroutine", "end module"], ) assert "src/simulation/m_gp.fpp" in out and "src/pre_process/m_gp.fpp" not in out assert "(in simulation)" in out + + +def test_blank_line_inside_a_hunk_does_not_crash(): + diff = "--- a/src/simulation/m_k.fpp\n+++ b/src/simulation/m_k.fpp\n@@ -9,3 +9,3 @@\n\n- a = q(i)\n+ a = 2*q(i)\n" + assert context.parse_diff(diff, Path("/r"), "new").changed == {"/r/src/simulation/m_k.fpp": {10}} + + +def test_src_check_uses_repo_relative_paths(): + diff = "--- a/docs/x.md\n+++ b/docs/x.md\n@@ -1 +1 @@\n-a\n+b\n" + d = context.parse_diff(diff, Path("/home/u/src/MFC"), "new") + assert d.changed == {} and d.new == [] + + +def test_deleted_file_is_listed(): + diff = "--- a/src/simulation/m_old.fpp\n+++ /dev/null\n@@ -1,2 +0,0 @@\n-module m_old\n-end module m_old\n" + out = context.render(context.Index([], {}, {}), context.parse_diff(diff, Path("/r"), "new"), Path("/r")) + assert "Deleted files" in out and "src/simulation/m_old.fpp" in out + + +def test_changed_lines_outside_routines_are_listed(tmp_path): + # Line 3 of m_g is a module variable declaration: in no routine, but must not vanish from the report. + diff = "--- a/src/common/m_g.fpp\n+++ b/src/common/m_g.fpp\n@@ -3 +3 @@\n- integer :: dir_idx, nsteps\n+ integer :: dir_idx, nsteps, extra\n" + out = _render(_index(tmp_path), diff) + assert "Not in any routine" in out and "src/common/m_g.fpp: 3" in out + + +def test_hunk_that_does_not_match_the_file_is_flagged(tmp_path): + # The diff's context says line 9 is 'do i = 1, 99' but the file has 'do i = 1, 10': mapping would be off. + diff = "--- a/src/simulation/m_k.fpp\n+++ b/src/simulation/m_k.fpp\n@@ -9,2 +9,2 @@\n do i = 1, 99\n- a = q(i)\n+ a = 2*q(i)\n" + src = EXPANDED.split('# 1 "/r/src/simulation/m_k.fpp"\n')[1].splitlines() + out = context.render(_index(tmp_path), context.parse_diff(diff, Path("/r"), "new"), Path("/r"), read_source=lambda f: src) + assert "does not match" in out + + +def test_git_diff_ignores_user_config(tmp_path): + import subprocess # pylint: disable=import-outside-toplevel + + env = {"GIT_AUTHOR_NAME": "a", "GIT_AUTHOR_EMAIL": "a@b", "GIT_COMMITTER_NAME": "a", "GIT_COMMITTER_EMAIL": "a@b", "PATH": "/usr/bin:/bin", "HOME": str(tmp_path)} + (tmp_path / "src").mkdir() + (tmp_path / "src" / "m.fpp").write_text("a\n") + for cmd in (["init", "-q"], ["config", "diff.noprefix", "true"], ["config", "color.diff", "always"], ["add", "."], ["commit", "-q", "-m", "x"]): + subprocess.run(["git", "-C", str(tmp_path), *cmd], check=True, env=env) + (tmp_path / "src" / "m.fpp").write_text("b\n") + d = context.parse_diff(context.git_diff(tmp_path, "HEAD"), tmp_path, "new") + assert d.changed == {str(tmp_path / "src" / "m.fpp"): {1}} + + +def test_failed_expansion_does_not_leave_stale_output(tmp_path): + fake = tmp_path / "repo" + (fake / "src" / "common").mkdir(parents=True) + (fake / "src" / "post_process").mkdir(parents=True) + src = fake / "src" / "post_process" / "m_x.fpp" + src.write_text("module m_x\nend module m_x\n") + files, _ = context_expand.expand(fake, ["post_process"], tmp_path / "cache", write_includes=False) + assert any(p.name == "m_x.fpp.f90" for p in files["post_process"]) + src.write_text("#:if\nmodule m_x\nend module m_x\n") + files, errors = context_expand.expand(fake, ["post_process"], tmp_path / "cache", write_includes=False) + assert errors and not any(p.name == "m_x.fpp.f90" for p in files["post_process"])