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/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index fc3ad88d1..0cf71d1b7 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -130,6 +130,24 @@ jobs: echo "review_diff_path=.claude-review/review.diff" >> "$GITHUB_OUTPUT" echo "changed_files_path=.claude-review/changed_files.txt" >> "$GITHUB_OUTPUT" + # 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: | + 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 shell: bash @@ -203,7 +221,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 +234,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 diff --git a/toolchain/main.py b/toolchain/main.py index a2a0783fc..7ef74c6de 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 @@ -226,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 11077e273..a8e440e4d 100644 --- a/toolchain/mfc/cli/commands.py +++ b/toolchain/mfc/cli/commands.py @@ -903,6 +903,29 @@ ], ) +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="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).", + 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 +1497,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 new file mode 100644 index 000000000..6145f4cae --- /dev/null +++ b/toolchain/mfc/context.py @@ -0,0 +1,448 @@ +"""`./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 +import subprocess +import sys +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) + src_runs: collections.Counter = field(default_factory=collections.Counter) # separate appearances = copies + + +@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)) + + +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) + + +@dataclass +class Diff: + changed: dict = field(default_factory=dict) # absolute source path -> 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): + """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_path = src_path(line) if not line.endswith("/dev/null") else "/dev/null" + if side == "old": + cur = old_path if old_path != "/dev/null" else None + elif line.startswith("+++ "): + 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 = 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 (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": + d.checks[cur].append((old_ln, body)) + old_ln += 1 + elif kind == "+": + if side == "new": + d.changed[cur].add(new_ln) + d.checks[cur].append((new_ln, body)) + elif old_ln > 1: + 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 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): + 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(ln for ln in changed.get(r0.src_file, ()) if lo <= ln <= 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, 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 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}") + return sorted(lines) + + +def _statement_starts(changed, read_source): + """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: + src = [] + starts = {} + for ln in ls: + start = ln + while 1 < start <= len(src) and src[start - 2].rstrip().endswith("&"): + start -= 1 + starts[ln] = start + out[f] = starts + return out + + +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 diff.changed and not diff.new and not diff.deleted: + return "No changes under src/." + 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), ""] + 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") + 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 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 + } + ) + 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"])] + return "\n".join(out) + + +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 <= 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]) + 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 + + +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.""" + 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`.""" + + from . import common, context_expand # pylint: disable=import-outside-toplevel + from .state import ARG # pylint: disable=import-outside-toplevel + + 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 = 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 d.changed else ({}, []) + report = render(build_index(files), d, 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) + if ARG("output"): + Path(ARG("output")).write_text(report + "\n") + else: + sys.stdout.write(report + "\n") diff --git a/toolchain/mfc/context_expand.py b/toolchain/mfc/context_expand.py new file mode 100644 index 000000000..a23dd59dd --- /dev/null +++ b/toolchain/mfc/context_expand.py @@ -0,0 +1,90 @@ +"""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) + 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) + 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..2597f966b --- /dev/null +++ b/toolchain/mfc/test_context.py @@ -0,0 +1,340 @@ +"""Tests for ./mfc.sh context (context_expand and context).""" + +import re +from pathlib import Path + +from mfc import context, 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"]) + + +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 + + +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 q(i) = a + dir_idx\n# 10 "/r/src/simulation/m_k.fpp"\n a = q(i)\n', +) + + +def _render(idx, diff, side="new", **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(): + 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): + 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, 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 + + +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 + + +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 + + +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").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(): + 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").changed == {"/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").changed == {"/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, 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): + # 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) + + +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, + 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"])