diff --git a/test/packaging/lint-once.test.ts b/test/packaging/lint-once.test.ts index b8aa723..9694a61 100644 --- a/test/packaging/lint-once.test.ts +++ b/test/packaging/lint-once.test.ts @@ -31,11 +31,14 @@ // `include`d makefile, a name outside those charsets, a generated file — is // 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 reached some other way is not: an `include`d -// makefile, a pattern rule, a target name outside that charset, or a `define` -// block pulled into a recipe as `$(NAME)` — that last one is variable -// expansion, and this parser expands no variables, so `$(EXTRA)` in a recipe -// is a name it cannot resolve rather than a body it declines to read. Within +// line after a `;`; a recipe built by an `include`d makefile, a pattern rule, +// or a target name outside that charset is not. A variable given a literal +// value on one line — `VAR := ...`, `VAR = ...`, `VAR ?= ...` — is substituted +// wherever the Makefile spells it `$(VAR)` or `${VAR}`, because this Makefile +// writes its recipes that way (`@$(YARN) tsc --watch`); the substitution is +// one pass over literals, so a value that itself names another variable, a +// value built by a make function (`$(shell ...)`, `$(addprefix ...)`), and the +// body of a `define`/`endef` block are not expanded and 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` @@ -140,10 +143,80 @@ const MAKE_TARGET_NAME = "[a-z][a-z0-9_-]*"; // alarm someone fixes; missing one is the failure this file exists to prevent. const MAKE_CONDITIONAL = /^\s*(?:ifeq|ifneq|ifdef|ifndef|else|endif)\b/; +// `@$(FMT)` is one token away from being a second prettier pass, and until the +// variable is resolved it reads as a line that invokes nothing: `make -n check` +// printed both `script/check` and `script/fmt-check` while this file scored +// one. `$(MAKE)` was already special-cased below, which is this same rule +// half-applied to a single name; this is the general form of it. +// +// Deliberately only the simple case: a name assigned a literal on one line. +// `:=` and `=` differ in when make expands them and `?=` in whether it assigns +// at all, but none of that changes the single value a literal can take, so all +// three are read the same way. What is not read is anything needing evaluation +// — a value containing another `$(...)`, a make function, or a `define` body — +// because expanding those means implementing make, and a half-implementation +// that resolves a variable to the wrong string would count invocations that do +// not happen. An unresolved reference is left standing verbatim instead, which +// is the same dead end as any other unfollowed edge rather than a wrong answer. +const MAKE_ASSIGNMENT = new RegExp( + `^([A-Za-z_][A-Za-z0-9_]*)\\s*(?::=|\\?=|=)\\s*(.*)$`, +); +const MAKE_VARIABLE_REFERENCE = /\$[({]([A-Za-z_][A-Za-z0-9_]*)[)}]/g; + +// `define`/`endef` bodies are skipped rather than parsed: the body is a +// multi-line value, so reading its lines as assignments would take whatever +// `=` they happen to contain and call it a variable. +const makeVariables = (text: string): Map => { + const variables = new Map(); + let inDefine = false; + for (const raw of text.split("\n")) { + if (/^\s*endef\b/.test(raw)) { + inDefine = false; + continue; + } + if (/^\s*define\b/.test(raw)) { + inDefine = true; + continue; + } + if (inDefine || raw.startsWith("\t")) continue; + const assignment = MAKE_ASSIGNMENT.exec(raw.replace(/#.*$/, "").trim()); + if (assignment === null) continue; + const [, name, value] = assignment; + if (name === undefined || value === undefined) continue; + // A value that is itself a reference is the recursive case, and is not + // resolved. Recording it would hand the substitution below a string it + // cannot finish expanding. + if (/\$[({]/.test(value)) continue; + variables.set(name, value.trim()); + } + return variables; +}; + +// One pass, and only over names that were assigned a literal: the result is +// never re-scanned for further references, so this cannot recurse or diverge. +const expandMakeVariables = ( + line: string, + variables: Map, +): string => + line.replace( + MAKE_VARIABLE_REFERENCE, + (reference, name: string) => variables.get(name) ?? reference, + ); + const parseMakefile = (text: string): Map => { const recipes = new Map(); + // Collected in a pass of their own because make reads the whole file + // before it runs anything: a recipe may spell a variable that the Makefile + // assigns further down, and a single pass would leave that one unexpanded + // purely because of where its author put it. + const variables = makeVariables(text); let current: string | null = null; - for (const raw of text.split("\n")) { + for (const line of text.split("\n")) { + // Expanded before parsing rather than only on recipe lines, so a + // prerequisite written `check: $(FMT)` resolves the same way a recipe + // line does. Assignment lines are expanded too and are unaffected: a + // literal value has nothing to substitute. + const raw = expandMakeVariables(line, variables); if (raw.startsWith("\t")) { if (current !== null) { recipes @@ -710,6 +783,78 @@ describe("the resolver reads what the shell would run", () => { }); }); + // The fifth construct that gave `make check` two prettier passes with the + // whole suite green: `FMT := script/fmt-check` and a recipe of `@$(FMT)`. + // The shipped Makefile writes recipe lines this way already + // (`@$(YARN) tsc --watch`), so an unexpanded variable was a gap in the + // house style rather than in an exotic corner of make. + it("expands a variable spelled into a recipe line", () => { + const recipes = parseMakefile( + [ + "FMT := script/fmt-check", + "check:", + "\t@$(FMT)", + "\t@${FMT}", + "\t@script/check", + ].join("\n"), + ); + // What `make -n check` prints, in order. + expect(recipes.get("check")?.recipe).toEqual([ + "script/fmt-check", + "script/fmt-check", + "script/check", + ]); + }); + + it("expands a variable spelled into a prerequisite list", () => { + expect( + parseMakefile("FMT = fmt-check\ncheck: $(FMT)").get("check") + ?.prerequisites, + ).toEqual(["fmt-check"]); + }); + + // A recipe may name a variable the Makefile assigns further down. + it("expands a variable assigned after the recipe that uses it", () => { + const recipes = parseMakefile( + ["check:", "\t@$(FMT)", "FMT ?= script/fmt-check"].join("\n"), + ); + expect(recipes.get("check")?.recipe).toEqual(["script/fmt-check"]); + }); + + // The edges of the expansion, asserted so the header's not-followed list + // is the code's behaviour rather than a claim about it. Each of these + // leaves the reference standing verbatim, which reaches nothing — the same + // dead end as any other unfollowed edge, and not a wrong resolution. + it("leaves a value it cannot resolve to a literal unexpanded", () => { + // A make function: resolving it would mean running it. + expect( + parseMakefile( + [ + "FMT := $(shell echo script/fmt-check)", + "check:", + "\t@$(FMT)", + ].join("\n"), + ).get("check")?.recipe, + ).toEqual(["$(FMT)"]); + // A `define` body is a multi-line value, not a single-line assignment. + expect( + parseMakefile( + [ + "define RUNFMT", + "@script/fmt-check", + "endef", + "check:", + "\t$(RUNFMT)", + ].join("\n"), + ).get("check")?.recipe, + ).toEqual(["$(RUNFMT)"]); + // An undefined name is not silently emptied. + expect( + parseMakefile(["check:", "\t@$(NOPE)"].join("\n")).get("check") + ?.recipe, + ).toEqual(["$(NOPE)"]); + }); + 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