From f4ecef882058ee76db28a3bd8c47c9a66c4ece27 Mon Sep 17 00:00:00 2001 From: clawbot Date: Fri, 4 Sep 2026 10:54:24 +0000 Subject: [PATCH] Follow make conditionals and semicolon recipes in lint-once makeRecipes() reset the current target on every non-tab line, so an ifeq/endif block ended the recipe and every tab-indented line inside it was discarded; and the target line's tail was read entirely as prerequisites, so `check: ; @script/fmt-check` split to tokens that named no target and vanished. Both gave `make check` a second prettier pass with the suite green. Conditional directives no longer end a recipe, and every branch is treated as reachable rather than evaluating the condition. The target line is split on the first `;`: what precedes it is the prerequisite list, what follows is the first recipe line. Both pinned directly, and the header's exclusion list now names what the parser actually skips. --- test/packaging/lint-once.test.ts | 89 +++++++++++++++++++++++++++++--- 1 file changed, 83 insertions(+), 6 deletions(-) diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts index b7b4af2..727c089 100644 --- a/test/packaging/lint-once.test.ts +++ b/test/packaging/lint-once.test.ts @@ -29,8 +29,11 @@ // 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. +// not followed. Recipe lines reached through a make conditional are followed, +// without evaluating the condition, and so is a recipe written on the target +// line after a `;`; a recipe built by an `include`d makefile, a pattern rule, +// or a target name outside that charset is not. 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. @@ -115,10 +118,29 @@ interface MakeTarget { // line spelled `YARN: yarn run`. const MAKE_TARGET_NAME = "[a-z][a-z0-9_-]*"; -const makeRecipes = (): Map => { +// Conditional directives are not target lines, and they are not recipe lines +// either — they are parse-time structure wrapped around the recipe that +// encloses them. Treating one as an ordinary non-tab line ends the current +// recipe, and every tab-indented line after it is discarded, so +// +// check: +// \t@script/check +// ifeq (1,1) +// \t@script/fmt-check +// endif +// +// reads as a recipe of one command while `make -n check` prints two. The +// condition is deliberately not evaluated: that would mean evaluating make +// variables, and the conservative reading — every branch is reachable, so +// every branch's lines are recipe lines — is the one that cannot lose an +// invocation. Counting a command from a branch make would skip is a false +// alarm someone fixes; missing one is the failure this file exists to prevent. +const MAKE_CONDITIONAL = /^\s*(?:ifeq|ifneq|ifdef|ifndef|else|endif)\b/; + +const parseMakefile = (text: string): Map => { const recipes = new Map(); let current: string | null = null; - for (const raw of read("Makefile").split("\n")) { + for (const raw of text.split("\n")) { if (raw.startsWith("\t")) { if (current !== null) { recipes @@ -127,14 +149,26 @@ const makeRecipes = (): Map => { } continue; } + if (MAKE_CONDITIONAL.test(raw)) continue; const target = new RegExp(`^(${MAKE_TARGET_NAME})\\s*:(?!=)(.*)$`).exec( raw, ); current = target === null ? null : (target[1] ?? null); if (target === null || current === null) continue; + // `target: prereqs ; command` puts the first recipe line on the target + // line itself. Read as prerequisites the whole of it, `;` and the + // command split into tokens that name no target and are filtered out, + // and the invocation disappears — `check: ; @script/fmt-check` was a + // second prettier pass this parser scored as none. + const rest = target[2] ?? ""; + const semicolon = rest.indexOf(";"); + const inlineRecipe = + semicolon === -1 ? "" : rest.slice(semicolon + 1).trim(); // Trailing `# comment` is not a prerequisite; neither is the empty // string a split leaves behind. - const prerequisites = (target[2] ?? "") + const prerequisites = ( + semicolon === -1 ? rest : rest.slice(0, semicolon) + ) .replace(/#.*$/, "") .trim() .split(/\s+/) @@ -145,11 +179,16 @@ const makeRecipes = (): Map => { } else { existing.prerequisites.push(...prerequisites); } + if (inlineRecipe !== "") { + recipes + .get(current) + ?.recipe.push(inlineRecipe.replace(/^[@-]+/, "")); + } } return recipes; }; -const recipes = makeRecipes(); +const recipes = parseMakefile(read("Makefile")); // Only prerequisites that name a target of this Makefile are followed: the // rest are filenames, or expansions of variables this parser does not @@ -630,6 +669,44 @@ describe("the resolver reads what the shell would run", () => { expect(edgesOf("make -n fmt-check")).toContain("make:fmt-check"); }); + // Two more constructs that gave `make check` a second prettier pass with + // the whole suite green, each reproduced as a Makefile mutation before + // being pinned here. + it("keeps recipe lines inside a make conditional", () => { + const recipes = parseMakefile( + [ + "check:", + "\t@script/check", + "ifeq (1,1)", + "\t@script/fmt-check", + "endif", + ].join("\n"), + ); + // What `make -n check` prints, in order. + expect(recipes.get("check")?.recipe).toEqual([ + "script/check", + "script/fmt-check", + ]); + }); + + it("reads a recipe written on the target line after a semicolon", () => { + const recipes = parseMakefile( + ["check: ; @script/fmt-check", "\t@script/check"].join("\n"), + ); + expect(recipes.get("check")?.recipe).toEqual([ + "script/fmt-check", + "script/check", + ]); + // And the `;` and the command are not mistaken for prerequisites. + expect(recipes.get("check")?.prerequisites).toEqual([]); + expect( + parseMakefile("check: lint ; @script/fmt-check").get("check"), + ).toEqual({ + prerequisites: ["lint"], + recipe: ["script/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