WIP: follow Makefile prerequisites and $(MAKE) in lint-once

test/packaging/lint-once.test.ts

@@ -16,10 +16,21 @@
 // against one file. Starting from an entrypoint, it follows every edge the repo
 // actually uses to reach another command — `run:` steps in the CI workflow,
 // `"$SCRIPT_DIR/<name>"` and `script/<name>` into other scripts, `make <target>`
-// through the Makefile shims, `yarn run <name>` through the `package.json`
+// and `$(MAKE) <target>` through the Makefile shims, the prerequisites of a
-// scripts, and `docker build -f <file>` into that Dockerfile's `RUN` steps — and
+// Makefile target, `yarn run <name>` through the `package.json` scripts, and
-// counts the prettier invocations it finds. A prettier call added anywhere in
+// `docker build -f <file>` into that Dockerfile's `RUN` steps, heredoc bodies
-// that graph is therefore caught, wherever it is added.
+// included — and counts the prettier invocations it finds.
+//
+// What that covers is worth stating exactly rather than as "anywhere", because
+// a comment claiming more than the code delivers is the same defect as an
+// assertion that cannot fail — and an earlier draft of this header did claim
+// it, while `check: fmt-check` and `@$(MAKE) fmt-check` both walked straight
+// past it. Command names are read as `[a-z][a-z0-9_-]*` for Makefile targets
+// and `script/` files, and `[A-Za-z][A-Za-z0-9_:-]*` for `package.json`
+// scripts. A command reached by some other means — `sh -c`, a shell alias, an
+// `include`d makefile, a name outside those charsets, a generated file — is
+// not followed. Within those edges, a prettier call is caught wherever it is
+// added.
 //
 // Two entrypoints are walked, because they cover different graphs: `make check`
 // is what a developer runs, and `.gitea/workflows/check.yml` is what CI runs.
@@ -83,20 +94,56 @@ const countPrettier = (line: string): number =>
 // Makefile targets are thin shims (`check:` / tab / `@script/check`), so a
 // `make <target>` edge has to resolve through them to keep "per `make check`"
 // meaning what it says. Recipe lines are the tab-indented ones.
-const makeRecipes = (): Map<string, string[]> => {
+//
-    const recipes = new Map<string, string[]>();
+// The prerequisite list is read as well, and it is not decoration: make runs a
+// target's prerequisites before its recipe, so `check: fmt-check` invokes
+// fmt-check every bit as much as a `@script/fmt-check` line in the recipe
+// would. An earlier version of this parser captured the target name and threw
+// the rest of the line away, which made `check: fmt-check` — a one-token edit,
+// and the most natural way for someone to wire the host formatting check back
+// into `make check` — a second prettier pass that this file scored as one. The
+// shipped Makefile already relies on prerequisites (`install: build-bin`), so
+// this is the file's own house style rather than a contrived evasion.
+interface MakeTarget {
+    prerequisites: string[];
+    recipe: string[];
+}
+
+// Names are lowercase-with-dashes here, plus digits and underscores. The
+// leading character is deliberately not uppercase: `YARN := yarn run` is a
+// variable assignment, not a rule, and `(?!=)` alone would not say so on a
+// line spelled `YARN: yarn run`.
+const MAKE_TARGET_NAME = "[a-z][a-z0-9_-]*";
+
+const makeRecipes = (): Map<string, MakeTarget> => {
+    const recipes = new Map<string, MakeTarget>();
     let current: string | null = null;
     for (const raw of read("Makefile").split("\n")) {
         if (raw.startsWith("\t")) {
             if (current !== null) {
-                recipes.get(current)?.push(raw.trim().replace(/^[@-]+/, ""));
+                recipes
+                    .get(current)
+                    ?.recipe.push(raw.trim().replace(/^[@-]+/, ""));
             }
             continue;
         }
-        const target = /^([a-z][a-z-]*)\s*:(?!=)/.exec(raw);
+        const target = new RegExp(`^(${MAKE_TARGET_NAME})\\s*:(?!=)(.*)$`).exec(
-        current = target === null ? null : target[1];
+            raw,
-        if (current !== null && !recipes.has(current)) {
+        );
-            recipes.set(current, []);
+        current = target === null ? null : (target[1] ?? null);
+        if (target === null || current === null) continue;
+        // Trailing `# comment` is not a prerequisite; neither is the empty
+        // string a split leaves behind.
+        const prerequisites = (target[2] ?? "")
+            .replace(/#.*$/, "")
+            .trim()
+            .split(/\s+/)
+            .filter((name) => name !== "");
+        const existing = recipes.get(current);
+        if (existing === undefined) {
+            recipes.set(current, { prerequisites, recipe: [] });
+        } else {
+            existing.prerequisites.push(...prerequisites);
         }
     }
     return recipes;
@@ -104,6 +151,18 @@ const makeRecipes = (): Map<string, string[]> => {
 
 const recipes = makeRecipes();
 
+// Only prerequisites that name a target of this Makefile are followed: the
+// rest are filenames, or expansions of variables this parser does not
+// evaluate, and neither is an invocation of anything it could read.
+const prerequisitesOf = (node: string): string[] => {
+    if (!node.startsWith("make:")) return [];
+    const target = recipes.get(node.slice("make:".length));
+    if (target === undefined) return [];
+    return target.prerequisites
+        .filter((name) => recipes.has(name))
+        .map((name) => `make:${name}`);
+};
+
 const packageScripts = (): Record<string, string> => {
     const pkg = JSON.parse(read("package.json")) as {
         scripts?: Record<string, string>;
@@ -113,14 +172,40 @@ const packageScripts = (): Record<string, string> => {
 
 const scripts = packageScripts();
 
+// A `RUN <<EOF` body is a command the image executes, and BuildKit runs it on
+// the default frontend with no `# syntax=` directive, so keeping only the
+// lines that begin `RUN ` would let a whole shell script hide one line below
+// one. The body is returned alongside the `RUN` line that opened it. An
+// unterminated heredoc is a thrown error rather than a silent truncation of
+// the rest of the file, for the same reason every other dead end here is.
+const HEREDOC_OPEN = /<<-?\s*(['"]?)([A-Za-z_][A-Za-z0-9_]*)\1/;
+
+const dockerRunCommands = (text: string): string[] => {
+    const commands: string[] = [];
+    let terminator: string | null = null;
+    for (const line of executable(text)) {
+        if (terminator !== null) {
+            if (line === terminator) terminator = null;
+            else commands.push(line);
+            continue;
+        }
+        if (!line.startsWith("RUN ")) continue;
+        const command = line.slice("RUN ".length);
+        terminator = HEREDOC_OPEN.exec(command)?.[2] ?? null;
+        commands.push(command);
+    }
+    if (terminator !== null) {
+        throw new Error(`unterminated heredoc: ${terminator}`);
+    }
+    return commands;
+};
+
 // Node keys: `script/<name>`, `docker:<Dockerfile>`, `make:<target>`,
 // `yarn:<package.json script>`, `workflow:<CI workflow file>`.
 const resolve = (node: string): string[] => {
     if (node.startsWith("script/")) return executable(read(node));
     if (node.startsWith("docker:")) {
-        return executable(read(node.slice("docker:".length)))
+        return dockerRunCommands(read(node.slice("docker:".length)));
-            .filter((line) => line.startsWith("RUN "))
-            .map((line) => line.slice("RUN ".length));
     }
     // The `run:` steps of a workflow, in file order. `uses:` steps are actions,
     // not commands, and have no edges into this repo's graph. A `run: |` block
@@ -140,7 +225,7 @@ const resolve = (node: string): string[] => {
         if (recipe === undefined) {
             throw new Error(`no such Makefile target: ${target}`);
         }
-        return recipe;
+        return recipe.recipe;
     }
     if (node.startsWith("yarn:")) {
         const name = node.slice("yarn:".length);
@@ -156,9 +241,13 @@ const resolve = (node: string): string[] => {
 // Same reasoning as the missing-target error, applied to every node kind: a
 // node that resolves to no commands contributes zero prettier invocations and
 // zero edges, which is indistinguishable from a clean result. Fail instead.
+//
+// A Makefile target with prerequisites and an empty recipe is the one case
+// that is genuinely not vacuous — it runs its prerequisites — so it is not
+// caught here.
 const commandsOf = (node: string): string[] => {
     const commands = resolve(node);
-    if (commands.length === 0) {
+    if (commands.length === 0 && prerequisitesOf(node).length === 0) {
         throw new Error(`node resolved to no commands: ${node}`);
     }
     return commands;
@@ -170,21 +259,48 @@ const edgesOf = (line: string): string[] => {
     // `"$SCRIPT_DIR/lint"`, `"$ROOT/script/lint"` and a bare `script/lint` are
     // all the same edge.
     for (const match of line.matchAll(
-        /(?:\$SCRIPT_DIR|\$\{SCRIPT_DIR\}|script)\/([a-z][a-z-]*)/g,
+        new RegExp(
+            `(?:\\$SCRIPT_DIR|\\$\\{SCRIPT_DIR\\}|script)/(${MAKE_TARGET_NAME})`,
+            "g",
+        ),
     )) {
         edges.push(`script/${match[1]}`);
     }
 
     // Only real targets: `pkg_install gnumake make make make` in
     // script/bootstrap is a package name, not an invocation of this Makefile.
-    for (const match of line.matchAll(/\bmake\s+([a-z][a-z-]*)/g)) {
+    //
+    // `$(MAKE)` and `${MAKE}` count as `make`. Recursive make is spelled that
+    // way by convention rather than as a literal `make`, and this Makefile
+    // already expands variables into recipes (`@$(YARN) tsc --watch`), so a
+    // case-sensitive literal-only match left `@$(MAKE) fmt-check` — the other
+    // one-token way to put the host prettier pass back into `make check` —
+    // unfollowed. Valueless flags between the command and the target (`make
+    // -n check`, `make -j4 check`) are skipped. A flag that takes a separate
+    // argument is not: `make -C sub build` runs `sub/Makefile`'s target, not
+    // this one's, and resolving it against these recipes would be wrong
+    // rather than merely incomplete.
+    for (const match of line.matchAll(
+        new RegExp(
+            `(?:\\$\\(MAKE\\)|\\$\\{MAKE\\}|\\bmake)(?:\\s+-\\S+)*\\s+(${MAKE_TARGET_NAME})`,
+            "g",
+        ),
+    )) {
         if (recipes.has(match[1] ?? "")) edges.push(`make:${match[1]}`);
     }
 
     // Same rule for yarn: `yarn run prettier` is the linter itself (counted,
     // not followed), `yarn run fmt-check` would be a package.json script that
     // runs it indirectly.
-    for (const match of line.matchAll(/\byarn(?:\s+run)?\s+([a-z][a-z-]*)/g)) {
+    //
+    // The name charset is wider than the Makefile's because `package.json`
+    // script names conventionally carry colons, digits and underscores
+    // (`lint:fmt`, `test:e2e`). Under the narrower charset `yarn run lint:fmt`
+    // read as `yarn run lint`, which is not a script, so it produced no edge
+    // and no count.
+    for (const match of line.matchAll(
+        /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g,
+    )) {
         if ((match[1] ?? "") in scripts) edges.push(`yarn:${match[1]}`);
     }
 
@@ -217,6 +333,11 @@ const walk = (node: string, path: string[] = [], into?: Walk): Walk => {
     }
     result.reached.add(node);
 
+    // Before the recipe, exactly as make does.
+    for (const edge of prerequisitesOf(node)) {
+        walk(edge, [...path, node], result);
+    }
+
     for (const line of commandsOf(node)) {
         result.prettier += countPrettier(line);
         for (const edge of edgesOf(line)) {
@@ -333,9 +454,13 @@ const installBranches = (): { withoutYarn: string[]; withYarn: string[] } => {
     }
     const close = lines.indexOf("}", open);
     const body = lines.slice(open + 1, close === -1 ? undefined : close);
-    const guard = body.findIndex((line) =>
+    // The guard has to be a plain positive `missing yarn` test at the front of
-        /^if\b.*\bmissing yarn\b/.test(line),
+    // the condition. `if ! missing yarn ...` is the same shape to a looser
-    );
+    // regex but swaps which branch is which, and since the two branches are
+    // asserted separately below, that would silently relabel them — a failing
+    // test would then name the wrong branch. Anything else throws the named
+    // error below, which is the loud failure this file prefers.
+    const guard = body.findIndex((line) => /^if\s+missing yarn\b/.test(line));
     const otherwise = body.indexOf("else", guard);
     const end = body.indexOf("fi", otherwise);
     if (guard === -1 || otherwise === -1 || end === -1) {
@@ -488,6 +613,52 @@ describe("the resolver reads what the shell would run", () => {
         ).toEqual(["script/lint", "script/test", "script/fmt"]);
     });
 
+    // The two edges the previous version of this file could not see, each
+    // pinned directly as well as through the graph. Both were reproduced as
+    // Makefile mutations that gave `make check` two prettier passes with the
+    // whole suite green.
+    it("follows a Makefile prerequisite as an invocation", () => {
+        // Not hypothetical: this is the shipped Makefile's own `install`.
+        expect(prerequisitesOf("make:install")).toEqual(["make:build-bin"]);
+        expect(prerequisitesOf("make:check")).toEqual([]);
+    });
+
+    it("treats $(MAKE) and ${MAKE} as make", () => {
+        expect(edgesOf("$(MAKE) fmt-check")).toContain("make:fmt-check");
+        expect(edgesOf("${MAKE} fmt-check")).toContain("make:fmt-check");
+        expect(edgesOf("make fmt-check")).toContain("make:fmt-check");
+        expect(edgesOf("make -n fmt-check")).toContain("make:fmt-check");
+    });
+
+    it("reads a package.json script name that contains a colon", () => {
+        // The lookup against `scripts` is what turns this into an edge, and no
+        // script in this repo is colon-named, so the charset is asserted on
+        // the extraction itself.
+        expect(
+            [
+                ..."yarn run lint:fmt".matchAll(
+                    /\byarn(?:\s+run)?\s+([A-Za-z][A-Za-z0-9_:-]*)/g,
+                ),
+            ].map((match) => match[1]),
+        ).toEqual(["lint:fmt"]);
+    });
+
+    it("reads the body of a BuildKit heredoc RUN step", () => {
+        expect(
+            dockerRunCommands(
+                ["FROM scratch", "RUN <<EOF", "yarn run prettier --check .", "EOF", "RUN echo done"].join(
+                    "\n",
+                ),
+            ),
+        ).toContain("yarn run prettier --check .");
+    });
+
+    it("refuses a heredoc that is never terminated", () => {
+        expect(() =>
+            dockerRunCommands("FROM scratch\nRUN <<EOF\nyarn run prettier .\n"),
+        ).toThrow(/unterminated heredoc/);
+    });
+
     it("follows a bare docker build to Dockerfile and -f to its file", () => {
         expect(edgesOf("docker build .")).toContain("docker:Dockerfile");
         expect(edgesOf("docker build -f Dockerfile.lint .")).toContain(
@@ -495,6 +666,12 @@ describe("the resolver reads what the shell would run", () => {
         );
     });
 
+    // This exact-equality assertion is also the block-scalar guard, which is
+    // not obvious from its name: a `run: |` step resolves to the bare `|`,
+    // which reaches nothing, so the prettier count would stay 1 no matter what
+    // the block contains. Pinning the resolved list is what makes such a step
+    // red. The cost is that any legitimate second `run:` step — a cache step,
+    // an `echo` — fails this test for a reason unrelated to prettier.
     it("reads the run steps of the CI workflow and not its uses steps", () => {
         expect(commandsOf("workflow:.gitea/workflows/check.yml")).toEqual([
             "script/cibuild",