diff --git a/Cargo.lock b/Cargo.lock index 96ff310..01526f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1229,7 +1229,7 @@ checksum = "0ceec5bc11778974d1bcb055b18002eba7f4b3518b6a0081b3af5f21666da9ad" [[package]] name = "makefile-lossless" version = "0.3.40" -source = "git+https://github.com/leynos/makefile-lossless.git?rev=8dd35801b75b332c2ac2f995ae398ef8238559fa#8dd35801b75b332c2ac2f995ae398ef8238559fa" +source = "git+https://github.com/leynos/makefile-lossless.git?rev=2ae7134beb04416851ab18c8a5d5893348fbe26c#2ae7134beb04416851ab18c8a5d5893348fbe26c" dependencies = [ "log", "rowan", diff --git a/Cargo.toml b/Cargo.toml index 36191ae..f56a986 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,7 +28,7 @@ sha2 = "0.11.0" thiserror = "2.0.18" [patch.crates-io] -makefile-lossless = { git = "https://github.com/leynos/makefile-lossless.git", rev = "8dd35801b75b332c2ac2f995ae398ef8238559fa" } +makefile-lossless = { git = "https://github.com/leynos/makefile-lossless.git", rev = "2ae7134beb04416851ab18c8a5d5893348fbe26c" } [dev-dependencies] assert_cmd = "2.2.2" diff --git a/docs/adrs/0002-bare-export-directive-representation.md b/docs/adrs/0002-bare-export-directive-representation.md new file mode 100644 index 0000000..4c25036 --- /dev/null +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -0,0 +1,139 @@ +# ADR-0002: Represent bare `export` directives as valueless variable facts + +## Status + +Accepted + +## Date + +2026-08-13 + +## Context and Problem Statement + +GNU Make's `export` keyword serves two roles. It can modify a definition, as in +`export FOO := bar`, or stand alone as a directive naming variables assigned +elsewhere, as in `export FOO BAR`. The directive form carries no assignment +operator and no value. + +`makeutil` originally treated an absent assignment operator as a broken syntax +tree and returned a missing-field error, which surfaced as a fatal +`parse-internal` message and exit code 2. Because facts are collected before +diagnostics, one such line destroyed the report for the whole file: every rule, +variable, and include was lost. Downstream consumers therefore recorded an +operational error for an entire repository rather than a set of facts with one +gap. The directive form is common in real Makefiles, so this was a blocking +defect rather than an edge case. + +Schema version 1 sets `"additionalProperties": false` at every level, and +consumers pin `makeutil` by commit SHA and validate reports against the +published schema. Any new key is therefore a breaking change requiring a schema +version 2 and a coordinated re-pin by every consumer. + +Consumers also fail closed on any `parse.status` other than `complete`, so a +representation that forces `recovered` leaves them just as blocked as an abort +did. + +## Decision + +A bare `export NAME` directive is reported as an entry in the existing +`variables` array, using the operator enum's existing empty-string variant, +with an empty `raw_value`, `exported` true, and `define_block` false. A +directive naming several variables yields one entry per name, each carrying the +span of the whole directive. + +The discriminating predicate for consumers is +`operator == "" && define_block == false`, which identifies an export directive +rather than an assignment. The empty operator is shared with `define` blocks, +which is why `define_block` is part of the predicate. + +Neither `schema_version` nor `schemas/makeutil.parse.v1.schema.json` changes, +because the empty operator was already in the enum for `define` blocks and no +new key or enum member is introduced. + +A line the parser can name nothing on — a bare `export` with no names, which +means "export every variable", `export define NAME`, or a line whose only name +is one the parser treats as a keyword — yields no entry rather than an invented +one, together with a diagnostic of `makeutil`'s own. The diagnostic is emitted +rather than relying on the parser to emit one, because the parser does not +always do so: `override export override` is dropped upstream without any error. +Without it such a line would leave a report claiming `complete` with the +construct silently missing, which the honesty rule forbids. + +The names on a directive line are read from the definition node's identifier +tokens, anchored on the name the parser itself reports rather than by skipping +identifiers whose text matches a directive keyword. A variable may legitimately +be called `unexport`, and keyword-text filtering would silently discard it. + +`unexport` remains unrepresented and is documented as a known gap. + +## Alternatives considered + +### Add a new top-level `exports` array + +Rejected. This is the most honest representation, but `additionalProperties` is +`false` on the root object, so every consumer validating a report against the +version 1 schema would reject reports containing it. That is a breaking change +requiring a schema version 2 and a coordinated re-pin by every consumer — +disproportionate to a crash fix, and it would leave consumers blocked until +they moved. + +### Record the directive only as a recoverable diagnostic + +Rejected. It stops the abort but forces `status: "recovered"`, and consumers +fail closed on any status other than `complete`. Every Makefile containing a +bare export would remain effectively unparsable downstream, which fails the +purpose of the change. + +### Add a new operator enum variant for the directive form + +Rejected. The operator enum is a closed set in the version 1 schema, so a new +member is as breaking as a new key. + +## Consequences + +### Positive + +- No form of `export` or `unexport` aborts the parse, so one directive line can + no longer destroy the report for a whole file. +- A Makefile that assigns variables and then exports them by name reports + `complete` with exit code 0, unblocking consumers immediately. +- No schema file changes and no consumer re-pin is required beyond moving to a + commit that carries the fix. + +### Negative + +- Conflation: a consumer treating every entry in `variables` as an assignment + now sees extra entries for export directives, and a name may appear twice — + once for its assignment and once for the directive that exports it. This is + mitigated by the documented predicate in the users' guide and pinned by a + test. +- The representation cannot express "export every variable", so that form is + reported as an absence plus a diagnostic rather than as a fact. +- Supporting the multi-name form required a change to the parser fork and a + revision bump of the `[patch.crates-io]` pin, so this decision is not + confined to `makeutil` after all. The published `parser_version` is + unaffected. +- A variable whose name is `export`, `override` or `define` is unrepresentable, + because the parser's name accessor skips those texts. The adapter preserves + every nameable fact and emits a recoverable diagnostic for the omitted name, + so a report never claims `complete` while silently omitting that form. + +### Neutral + +- A future schema version 2 could introduce a dedicated `exports` array and an + explicit un-export representation. This decision does not preclude it; it + defers it until a schema break is warranted on its own merits. + +## Acceptance criteria + +1. A Makefile that assigns variables and then exports one of them by name + reports `complete` with exit code 0, and every rule and assignment in the + file is present. +2. No form of `export` or `unexport` produces a `parse-internal` message or + exit code 2. +3. Bare export directives are distinguishable from assignments by + `operator == "" && define_block == false`. +4. `schemas/makeutil.parse.v1.schema.json` is unchanged and every new fixture's + report validates against it. +5. An operator-less definition that is neither a `define` nor an `export` still + fails loudly, so genuinely broken trees are not masked. diff --git a/docs/contents.md b/docs/contents.md index 33316e0..e00c96f 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -27,10 +27,14 @@ set. architecture, security boundaries, and verification strategy. - [ADR-0001: Parse one GNU Makefile into versioned JSON facts](adrs/0001-single-file-gnu-make-parse.md) records the first-slice boundary, accepted on 2026-07-13. +- [ADR-0002: Represent bare `export` directives as valueless variable facts](adrs/0002-bare-export-directive-representation.md) + records the schema-preserving representation, accepted on 2026-08-13. - [Execution plans](execplans/) describe approved, milestone-oriented delivery work: - [Implement ADR-0001](execplans/adr-0001-single-file-gnu-make-parse.md) plans the single-file GNU Make parser and its verification. + - [Parse bare export directives](execplans/bare-export-directives.md) plans + the removal of the fatal abort on `export` and `unexport` directives. ## Rust reference material diff --git a/docs/design.md b/docs/design.md index 14a7a95..99ae518 100644 --- a/docs/design.md +++ b/docs/design.md @@ -77,10 +77,13 @@ rewriting, and bindings remain later decisions. The implementation uses [`makefile-lossless`](https://github.com/jelmer/makefile-lossless), initially pinned to `=0.3.40`. A temporary `[patch.crates-io]` override selects commit -`8dd35801b75b332c2ac2f995ae398ef8238559fa` from a project-maintained fork -because release 0.3.40 does not lex the documented GNU Make `!=` assignment -operator. Remove the override when an upstream release containing the fix is -adopted; do not replace the immutable commit with a branch name. +`2ae7134beb04416851ab18c8a5d5893348fbe26c` from a project-maintained fork, +which carries two fixes absent from release 0.3.40: lexing the documented GNU +Make `!=` assignment operator, and retaining every name of a multi-name +`export A B C` directive within the definition node. Remove the override when +an upstream release containing both is adopted; do not replace the immutable +commit with a branch name. The published version string stays `0.3.40`, so +`parser_version` is unaffected by a revision bump. The crate supplies: @@ -314,9 +317,79 @@ for the first bounded rules. ``` The schema-v1 operator set is closed: `""`, `"="`, `":="`, `"::="`, `":::="`, -`"+="`, `"?="`, and `"!="`. The empty string means a `define` block without an -assignment token. The operator remains source-faithful; the first slice does -not calculate the effective value or precedence. +`"+="`, `"?="`, and `"!="`. The empty string means a definition without an +assignment token: either a `define` block or a bare `export` directive. The +operator remains source-faithful; the first slice does not calculate the +effective value or precedence. + +#### 6.6.1. Bare `export` directives + +A bare `export NAME` names a variable assigned elsewhere and carries no +operator and no value. It is represented as an entry in `variables` with the +empty operator, an empty `raw_value`, `exported` true, and `define_block` +false. The discriminating predicate for consumers is +`operator == "" && define_block == false`. + +A directive naming several variables yields one entry per name, sharing the +span of the whole directive. This needed a parser change: the previously pinned +revision trapped the second name in an error node and pushed the rest out of +the definition node entirely, so `makeutil` could not recover them. A form the +parser cannot name at all — a bare `export`, `export define NAME`, or a name +whose text the parser treats as one of its own keywords — yields no entry +rather than an invented one, together with a diagnostic that keeps the report +`recovered` rather than falsely `complete`. That diagnostic is `makeutil`'s own +rather than the parser's, because the parser does not always emit one: +`override export override` is dropped upstream silently. + +The names on a directive line are read from the definition node's identifier +tokens, anchored on the name the parser itself reports. Keyword text cannot be +used to skip the directive prefix, because a variable may legitimately be called +`unexport`. Names the parser itself treats as keywords — `export`, `override` +and `define` — remain unrepresentable, because it reports no name for them. For +a line such as `export export FOO`, the adapter retains the nameable `FOO` fact +and reports `recovered` with a diagnostic for the omitted name. That keeps the +report honest without redefining what the parser considers a name. + +The alternative — a new top-level `exports` array — is more honest but the +schema sets `"additionalProperties": false` at every level, so it would be a +breaking change requiring schema version 2 and a coordinated re-pin by every +consumer. Recording the directive only as a diagnostic was also rejected, +because consumers fail closed on any status other than `complete` and every +Makefile with a bare export would remain effectively unparsable. The accepted +cost is conflation: a consumer treating every entry in `variables` as an +assignment sees extra entries for export directives. See +[ADR-0002](adrs/0002-bare-export-directive-representation.md) for the full +decision and +[the bare export execution plan](execplans/bare-export-directives.md) for the +delivery record. + +An `unexport` directive remains unrepresented: it parses as a rule whose first +target is `unexport` and forces a `recovered` status. Expressing an explicit +un-export needs a field that schema version 1 does not have, so support is +deferred to a future schema version. + +#### 6.6.2. Note for consumers pinning `makeutil` + +Downstream repositories pin `makeutil` by commit SHA, so this change reaches +them only when they move their pin. Consumers moving to a commit on or after +the merge of the `bare-export-directives` work should know that: + +- A bare `export NAME` directive no longer aborts the parse. Before the change + a single such line produced no JSON at all and exit code 2, discarding every + fact in the file; it now appears in `variables` with an empty `operator`. +- Reports may therefore contain more `variables` entries than before, and a + name may appear twice — once for its assignment and once for the directive + that exports it. Identify bare-export facts with + `operator == "" && define_block == false`; do not use a non-empty `operator` + filter, because `define` blocks also serialize with an empty operator. +- `unexport` remains unsupported and still reports a misleading rule with a + `recovered` status. +- A multi-name `export A B C` reports `complete` with one entry per name. This + required a parser revision bump, so the `[patch.crates-io]` commit differs + from earlier builds; the published `parser_version` is unchanged at `0.3.40`. +- `schema_version` is unchanged at `1`, and + `schemas/makeutil.parse.v1.schema.json` is byte-identical, so no schema + re-validation work is required. ### 6.7. Include facts diff --git a/docs/developers-guide.md b/docs/developers-guide.md index fd0d2d2..c3ea667 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -56,9 +56,25 @@ not pass upstream strings beyond the adapter. `AssignmentOperator` is the shared, closed domain and parser-port representation for schema-v1 variable operators. The parser adapter is its only producer; `SyntaxObservation` and report types are its permitted consumers. Its -`Define` variant serializes as an empty string and means a `define` block -without an assignment token. Extend the enum only through a schema-versioned -contract decision, and do not pass upstream operator strings beyond the adapter. +`Define` variant serializes as an empty string and means a definition without +an assignment token: either a `define` block or a bare `export` directive, told +apart by the `define_block` flag. Extend the enum only through a +schema-versioned contract decision, and do not pass upstream operator strings +beyond the adapter. + +The private `makefile_export` helpers own translation of operator-less export +definitions into variable observations. Before extraction, a repository sweep +found no equivalent directive-expansion helper: `variable_observation` was the +sole variable translator and produced one observation. In production, +`variable_observation` is the only permitted caller of `assignment_operator` and +`export_directive_observations`; `export_directive_observations` alone may call +`directive_names`. Focused unit tests may exercise each helper directly. +Compose the helpers only while translating one upstream `VariableDefinition`: +use `assignment_operator` for ordinary variable facts, and use +`export_directive_observations` only for an operator-less export so it can emit +zero or more directive facts with the shared directive span. They are +adapter-private mechanics, not domain ports, general directive parsers, or +reusable CST walkers. The makefile adapter privately scans leading recipe modifiers. This scanner exists because the upstream API has no always-execute accessor and its silent @@ -90,10 +106,12 @@ cannot forge another physical line; it is not a general path normalizer or JSON encoder. The exact 0.3.40 parser requirement is temporarily patched to immutable fork -commit `8dd35801b75b332c2ac2f995ae398ef8238559fa`, which adds `!=` lexer -support. Keep the commit pin reproducible. When upgrading to an upstream -release that contains the fix, remove the `[patch.crates-io]` entry and rerun -the complete assignment-operator contract matrix before updating the lockfile. +commit `2ae7134beb04416851ab18c8a5d5893348fbe26c`, which adds `!=` lexer +support and retains every name of a multi-name `export A B C` directive inside +the definition node. Keep the commit pin reproducible. When upgrading to an +upstream release that contains both fixes, remove the `[patch.crates-io]` entry +and rerun the complete assignment-operator contract matrix and the +export-directive suite before updating the lockfile. Tests keep raw Makefile text under `tests/fixtures/makefiles/`. Unit and property tests exercise the domain, `rstest-bdd` scenarios exercise observable diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md new file mode 100644 index 0000000..805be31 --- /dev/null +++ b/docs/execplans/bare-export-directives.md @@ -0,0 +1,1424 @@ +# Parse bare `export` and `unexport` directives without aborting + +This ExecPlan (execution plan) is a living document. The sections `Constraints`, +`Tolerances`, `Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, +and `Outcomes & Retrospective` must be kept up to date as work proceeds. + +Status: COMPLETE — every stage, A to G, has been delivered on this branch. The +Stage E parser change lives in the pinned fork on branch +`bare-export-directive-lists` and is under review as a draft pull request +there; this branch pins the resulting revision. + +## Purpose / big picture + +`makeutil parse` reads one GNU Makefile and prints a versioned JSON document of +syntax facts. Today it aborts outright on a construct that appears in ordinary +real-world Makefiles: a bare `export` directive that exports variables which +were assigned elsewhere, with no assignment on the `export` line itself. + +A one-line Makefile containing only `export FOO` produces no JSON at all. The +tool prints a fatal message on standard error and exits 2: + +```plaintext +makeutil: parse-internal: required variable-assignment-operator accessor was absent +``` + +Because `makeutil parse` handles a whole file at once, a single such line +destroys the report for the entire Makefile. Every fact in the file — every +rule, every variable, every include — is lost, not just the export line. +Downstream tools that consume the JSON therefore record an operational error +for the whole repository rather than a set of facts with one gap. + +After this change, a novice can run `makeutil parse` over a Makefile containing +`export MOLD_VERSION_FILE MOLD_SHA256SUMS_FILE RUST_TOOLCHAIN_FILE` and get a +complete JSON report on standard output with exit code 0, in which each +exported name appears in the `variables` array with an empty operator. No +construct in this plan may cause a `parse-internal` abort ever again. + +Observable success, in one command: + +```console +$ printf 'FOO := 1\nBAR := 2\nexport FOO BAR\n' > /tmp/demo.mk +$ makeutil parse /tmp/demo.mk | python3 -m json.tool | head -20 +$ echo "exit=$?" +exit=0 +``` + +The report must show `"status": "complete"` with an empty `diagnostics` array, +and `variables` must contain four entries: the two assignments and the two +directive facts described below. + +## Constraints + +These are hard invariants. If satisfying the objective requires violating one, +stop, record the conflict in `Decision Log`, and escalate rather than working +around it. + +- The JSON contract is versioned and consumers pin `makeutil` by commit SHA. + `schema_version` must remain `1` and the file + `schemas/makeutil.parse.v1.schema.json` must keep + `"additionalProperties": false` at every level. No new top-level key and no + new property on the `variable` object may be introduced by this plan, because + under `additionalProperties: false` such an addition is a breaking change for + any consumer validating against version 1. See the `Decision Log` for the + representation chosen to respect this. +- `parse.status` must remain honest. `complete` means the parser emitted no + diagnostics and the facts are trustworthy; `recovered` means the tool could + not fully understand the input. Never report `complete` for a construct the + tool cannot faithfully represent, and never silently discard a construct + while claiming `complete`. Consumers treat any status other than `complete` + as indeterminate and fail closed, so a wrong `complete` is worse than a + `recovered`. +- The byte-for-byte round-trip invariant in + `ensure_round_trip` (`src/adapters/makefile.rs`, lines 46-52) must continue + to hold for every fixture added by this plan. +- Process exit codes are part of the contract and must not change: 0 for a + `complete` parse, 1 for a `recovered` parse, 2 for a fatal internal or + input-handling failure. This mapping lives in `src/adapters/cli.rs` at line + 211 (`exit_code: u8::from(report.parse.status != ParseStatus::Complete)`) and + line 197 (the `parse-internal` fatal path). +- Do not modify anything in the consumer repositories that pin `makeutil`. + Re-pinning is their responsibility and is out of scope here. +- `make provenance` is a commit gate that rejects certain operator-specific and + owner-qualified strings appearing anywhere outside the `Makefile` itself. Do + not paste absolute filesystem paths from the working environment, nor + fully-qualified forge URLs for the parser fork, into any Markdown or Rust + file. Use repository-relative paths and bare revision hashes. If the gate + fails, read the `provenance` target in `Makefile` to see exactly what it + matched. +- Clippy lints in this repository are unusually strict (see `[lints.clippy]` in + `Cargo.toml`): `unwrap_used`, `expect_used`, `indexing_slicing`, + `option_if_let_else`, `must_use_candidate` and others are `deny`. Lints must + not be silenced. In tests, `.expect(...)` is permitted; in production code + and in non-`#[cfg(test)]` helpers it is not. +- No source file may exceed 400 lines. `src/adapters/makefile.rs` is currently + 325 lines, so there is limited headroom; extract helpers into a new module if + the budget would be exceeded. + +## Tolerances (exception triggers) + +- Scope: if the makeutil-side change (Stages B to D) touches more than six files + or more than 250 net lines, stop and escalate. +- Interface: if any public signature in `src/ports.rs`, `src/domain/mod.rs`, or + `src/application.rs` must change shape, stop and escalate. Adding a variant + to the private-facing `SyntaxObservation` enum is expected and is not an + escalation; changing `ParseReport` is. +- Schema: if any change to `schemas/makeutil.parse.v1.schema.json` appears + necessary, stop and escalate. That is a version-2 conversation, not this plan. +- Dependencies: if a new crate dependency is required, stop and escalate. The + revision bump of the already-patched `makefile-lossless` in Stage E is not a + new dependency and is expected. +- Upstream: Stage E changes a separate repository. If the upstream change + requires touching the lexer (`src/lex.rs` upstream) rather than the parser + (`src/lossless.rs` upstream), stop and escalate — that is a much larger blast + radius than this plan assumes. +- Iterations: if a red test still fails after five green attempts, stop and + escalate with the failing output. +- Time: if any single stage exceeds three hours, stop and escalate. + +## Risks + +- Risk: the upstream parser change in Stage E alters the concrete syntax tree + shape for inputs unrelated to `export`, silently changing existing reports. + Severity: high. Likelihood: low. Mitigation: the upstream change is gated + behind "a directive prefix keyword was consumed on this line", which cannot + be true for a plain assignment or a rule. Before bumping the pinned revision, + run the full `make test` suite and compare the two `insta` snapshots in + `tests/snapshots/` — an unexpected snapshot diff is the tripwire. +- Risk: representing a bare `export FOO` as an entry in the `variables` array + misleads a consumer that reads `variables` as "the set of variables this + Makefile assigns". Severity: medium. Likelihood: medium. Mitigation: the + discriminating predicate is documented in `docs/users-guide.md` and in the + schema-adjacent documentation as part of Stage D, and is asserted by a test. + See the `Decision Log` entry on representation for the full trade-off. +- Risk: the pinned upstream revision cannot be rebuilt or the fork is + unavailable when Stage E runs. Severity: medium. Likelihood: low. Mitigation: + Stages B to D deliver standalone value (no more aborts) without touching + upstream. Stage E is a separate, independently revertible commit. If upstream + is unavailable, stop after Stage D and record the multi-name limitation as a + known gap. +- Risk: the revision pinned after Stage E lives on an unmerged fork branch, so + a rebase, force-push, or branch deletion during review could make it + unreachable and break every build of this branch. Severity: high. Likelihood: + medium. Mitigation: the fork carries an annotated tag, + `makeutil-pin-2ae7134`, pointing at the pinned commit, which keeps it + reachable independently of the branch. The pin itself stays an immutable + commit hash rather than the tag name, as the repository requires. +- Risk: `make lint` runs `cargo doc` with `-D warnings` and a third-party lint + driver (`whitaker`) that may not be installed in every environment. Severity: + low. Likelihood: medium. Mitigation: run + `cargo clippy --all-targets --all-features -- -D warnings` directly as a + fallback and record in `Surprises & Discoveries` that the full `make lint` + could not be executed, rather than declaring the gate passed. + +## Progress + +- [x] Stage A: orientation and reproduction confirmed on the current working + tree (no code changes). Every row of the behaviour table reproduced + exactly; no divergence from the pin. +- [x] Stage B: red tests and fixtures added; each fails for the expected reason. + Evidence in `Artefacts and notes`. +- [x] Stage C: minimal makeutil change so no `export` or `unexport` form aborts. +- [x] Stage D: refactor, documentation, snapshots, and full commit gates. +- [x] Stage E: parser change landed on the fork branch + `bare-export-directive-lists`, revision bumped here, and both deferred + pins flipped. `export A B C` now reports `complete` with one entry per + name. Initially blocked on authority; see the `Decision Log`. +- [x] Stage F: `unexport` behaviour pinned by regression test + (`unexport_directive_degrades_honestly` in `tests/corpus.rs`) and + documented as a known limitation in `docs/users-guide.md`. +- [x] Stage G: consumer-facing note added as §6.6.2 of `docs/design.md`, + recording both the behaviour change and the new parser revision to pin. + One deviation: it names the merge of this branch rather than the merge + commit SHA the plan asks for, which is unknowable before the merge + happens. Whoever merges should substitute the SHA, or accept the branch + reference as sufficient. + +## Surprises & discoveries + +Recorded during investigation, before implementation began. + +- Observation: the upstream parser already models a single-name bare export + correctly. `export FOO\n` produces a clean tree with no errors at all. + Evidence: the concrete syntax tree for `export FOO\n` is + `ROOT@0..11 > VARIABLE@0..11` containing `IDENTIFIER "export"`, `WHITESPACE`, + `IDENTIFIER "FOO"`, `NEWLINE`, and `Parse::errors()` is empty. The accessor + `VariableDefinition::name()` returns `Some("FOO")`, `is_export()` returns + `true`, and `assignment_operator()` returns `None`. Impact: the single-name + defect is entirely inside makeutil. It is fixed by Stage C alone, with no + upstream work and no schema work. +- Observation: the multi-name form, which is the one that actually occurs in the + wild, is *not* modelled upstream. Only the first name is captured; the second + lands in an error node and the third escapes the variable node entirely. + Evidence: for `export FOO BAR BAZ\n` the tree is `ROOT@0..19` containing + `VARIABLE@0..14` (with `ERROR@11..14` wrapping `IDENTIFIER "BAR"`), then a + loose `WHITESPACE`, a loose `IDENTIFIER "BAZ"`, and a `NEWLINE` as direct + children of `ROOT`. One error is reported: `expected assignment operator` at + range `10..11`. Impact: Stage C alone makes this input parse without + aborting, but it reports `recovered` and loses two of the three names. + Consumers that fail closed on `status != "complete"` are still blocked. Stage + E is therefore required to actually resolve the real-world case, not optional + polish. +- Observation: `unexport` is not recognized as a directive at all; it is parsed + as a rule whose first target is the word `unexport`. Evidence: + `makeutil parse` on a file containing `unexport FOO BAR\n` exits 1 and emits + `"rules": [{"targets": ["unexport", "FOO", "BAR"], ...}]` with + `"status": "recovered"` and the diagnostic `expected ':'`. The same holds for + `unexport` alone. Impact: `unexport` never aborts, so it is not the blocking + defect, but the facts it produces are actively misleading — a consumer sees a + rule that does not exist. Faithfully modelling `unexport` needs a way to say + "this name was explicitly un-exported", which schema version 1 cannot + express. Stage F pins the current behaviour and documents the gap; full + support is deferred. +- Observation (Stage C): adding the export handling inline took + `src/adapters/makefile.rs` to 398 lines, two lines below the 400-line limit. + Impact: the extraction the plan permits was taken rather than deferred. The + operator mapping, the directive-name walk, and the directive expansion now + live in `src/adapters/makefile_export.rs` (107 lines) with their unit tests in + `src/adapters/makefile_export_tests.rs`, leaving `makefile.rs` at 318 lines. + The unit tests named in Stage B therefore live in the new test file rather + than in `src/adapters/makefile_tests.rs`. +- Observation (Stage C): upstream reports the `expected assignment operator` + positioned diagnostic at a byte range relative to the offending line rather + than to the file, so for a multi-line fixture the diagnostic location points + at the wrong line. Evidence: for `FOO := 1\nBAR := 2\nexport FOO BAR\n` the + positioned diagnostic is reported at bytes 3..4, which is on line 1, while the + `export` is on line 3. Impact: pre-existing and outside this plan's scope. + Stage E removes the diagnostic for this input entirely, so the mislocation + stops being visible for exports, but it presumably remains for other + recovered constructs. Worth its own investigation. +- Observation (stage review): `export unexport` is a legitimate, cleanly parsed + Makefile line exporting a variable named `unexport`. Evidence: upstream + reports `name = Some("unexport")`, `is_export() = true`, and no errors at + all. Impact: any implementation that identifies directive keywords by token + text discards it. The first implementation did exactly that and reported + `complete` with the fact missing. See the `Decision Log`. +- Observation (stage review): `export define FOO ... endef` is modelled by + upstream with `name() = None`, `is_export() = true` and `is_define() = true`, + together with three errors. Evidence: the same holds for `export define` with + no name. Impact: gating the no-facts path on "export and not define" left + this form aborting with exit code 2. The plan's Stage C wording did not + anticipate the two modifiers co-occurring. +- Observation (second stage review): upstream does not diagnose every export + line it fails to name. Evidence: `override export override` parses with no + errors at all and `name() = None`, because upstream's own `name()` accessor + refuses any identifier whose text is `export`, `override` or `define`. + Impact: the first fix for the name-less case returned no observations and + trusted upstream to supply the diagnostic, so this input produced a + `complete` report with the whole line silently missing — the same honesty + breach the previous review had found in a different shape. The adapter now + emits its own diagnostic, so the guarantee no longer depends on upstream's + behaviour. This is the second time trusting an upstream invariant produced a + false `complete`; prefer guarantees the adapter can enforce itself. +- Observation (second stage review): `override define FOO ... endef` still + aborts with exit code 2 and the `variable-name` message, losing the whole + file exactly as bare exports used to. Evidence: reproduced against the built + binary. It is a documented GNU Make construct and is not an `export` form, so + it is outside this plan's scope and was left alone rather than fixed + opportunistically. Impact: undiscovered work. Like the target-specific export + defect below, it deserves its own plan, and the two would sensibly be planned + together as "definition forms that still abort". +- Observation: a target-specific export is silently modelled wrongly and, unlike + the cases above, reports `complete`. Evidence: `foo: export BAR := baz\n` + parses with `"status": "complete"` and a single rule whose prerequisites are + `["export", "BAR", ":=", "baz"]`. Impact: this is a separate honesty defect + outside the scope of this plan. Do not fix it here. Record it so it is not + lost; it deserves its own plan. + +## Decision log + +- Decision: represent a bare `export NAME` as an entry in the existing + `variables` array, using the schema's existing empty-string operator, rather + than adding a new top-level `exports` array to the report. Rationale: three + options were weighed. (1) A new top-level `exports` array is the most honest + representation, but the schema sets `"additionalProperties": false` on the + root object, so any consumer validating a report against + `schemas/makeutil.parse.v1.schema.json` would reject reports containing it. + That is a breaking change requiring a version-2 schema and a coordinated + re-pin by every consumer — disproportionate to a crash fix. (2) Recording the + directive only as a recoverable diagnostic would stop the abort but force + `status: "recovered"`, and consumers fail closed on any status other than + `complete`. Every Makefile with a bare export would remain effectively + unparsable downstream. This fails the purpose of the work. (3) Reusing + `variables` with `operator: ""` costs nothing in the schema: the `operator` + enum already contains `""`, introduced for `define` blocks, and the existing + `exported` and `define_block` booleans are enough to discriminate. The + predicate a consumer applies is: `operator == "" && define_block == false` + means "this is a bare export directive, not an assignment"; `raw_value` is + the empty string for such a fact. This keeps `status: "complete"`, unblocks + consumers immediately, and changes no schema file. The accepted cost is + conflation: a naive consumer treating every entry in `variables` as an + assignment will now see three extra "variables" for `export A B C`. This is + mitigated by documentation in `docs/users-guide.md` and by a test that pins + the discriminating predicate. It is recorded as a medium risk above. + Date/Author: 2026-08-13, plan author. + +- Decision: do not bump `schema_version`, and do not edit + `schemas/makeutil.parse.v1.schema.json` at all. Rationale: follows directly + from the representation decision. Existing reports keep validating; new + reports validate against the unchanged version-1 schema because no new key or + enum member is introduced. This is verified rather than assumed by extending + the `reports_validate_against_schema` case list in `tests/report_schema.rs`. + Date/Author: 2026-08-13, plan author. + +- Decision: split the work into a makeutil-only fix (Stages B to D) and an + upstream parser fix (Stage E), delivered as separate commits. Rationale: the + single-name case and the never-abort guarantee are entirely within makeutil's + control and deliver value immediately. The multi-name case requires the + upstream tree to carry all the names, which makeutil cannot synthesize from a + tree that has already discarded them. Splitting keeps the upstream revision + bump independently revertible if it causes an unexpected snapshot change. + Date/Author: 2026-08-13, plan author. + +- Decision: makeutil, not the upstream crate, owns extraction of the exported + name list from the syntax tree. Rationale: the upstream change should be + confined to the parser's tree shape, keeping the upstream diff small and its + existing accessors (`VariableDefinition::name()`, which returns the first + name) backwards compatible. makeutil already imports `SyntaxKind` and + `rowan::ast::AstNode` in `src/adapters/makefile.rs`, so walking the variable + node's identifier tokens is a local, well-scoped helper rather than a new + upstream API surface that must then be supported forever. Date/Author: + 2026-08-13, plan author. + +- Decision: defer faithful `unexport` support and defer the target-specific + export defect; both are documented rather than fixed. Rationale: `unexport` + needs a representation for "explicitly un-exported", which schema version 1 + cannot express without a new field, and the constraint above forbids that. + Neither construct causes an abort, so neither blocks the purpose of this + plan. Pinning the current behaviour in the corpus suite makes the gap visible + and makes any future upstream improvement fail loudly rather than change + reports silently. Date/Author: 2026-08-13, plan author. + +- Decision: `variable_observation` returns `Vec` rather than + `Option`, as the plan's Stage C invited. Rationale: one + `export A B C` node must yield one fact per name, which an `Option` cannot + express, and an empty vector covers the name-less `export` without a second + concept. The call site in `collect_items` becomes `observations.extend(...)`, + which reads better than a conditional push. Date/Author: 2026-08-13, + implementer. + +- Decision: directive names are read by anchoring on the name the parser + itself reports, not by skipping identifier tokens whose text matches a + directive keyword. Rationale: the first implementation followed the plan's + Stage C wording and filtered out any identifier whose text was `export`, + `unexport`, `override` or `define`. Review found this silently discarded + `export unexport`, which upstream parses cleanly as exporting a variable + legitimately named `unexport`, while the report still claimed `complete` — a + direct breach of the honesty constraint. Upstream's + `VariableDefinition::name()` is the authority for the first name and returns + `None` precisely when the line names nothing it could parse, so the + identifier tokens preceding that name are exactly the prefix keywords + consumed. This is also correct for `export export FOO`, where upstream + consumes both leading `export` tokens. The plan's Stage C wording is + superseded on this point. Date/Author: 2026-08-13, implementer, after stage + review. + +- Decision: an `export` line whose name upstream cannot determine yields no + facts regardless of whether it is also a `define`. Rationale: the first + implementation gated this on `is_directive_only`, which excludes `define`, so + `export define FOO` and `export define` still aborted with exit code 2 — + failing the plan's own acceptance criterion 3. Upstream models both with no + name at all and diagnoses both, so dropping the facts leaves the report + honestly `recovered`. A name-less definition that is not an export still + fails loudly, so genuinely broken trees are not masked. Date/Author: + 2026-08-13, implementer, after stage review. + +- Decision: an export line the parser cannot name yields a diagnostic of + `makeutil`'s own rather than relying on upstream to have emitted one. + Rationale: the second stage review showed upstream drops + `override export override` without any error, so trusting it produced a + `complete` report with the line missing. Emitting the diagnostic in the + adapter makes the honesty guarantee independent of upstream. The cost is a + second diagnostic on inputs upstream does diagnose, such as a bare `export`, + which is noise rather than inaccuracy: both diagnostics are true and the + status is `recovered` either way. Date/Author: 2026-08-13, implementer, after + second stage review. + +- Decision (review finding retained rather than fixed): the + `|| context.is_export` arm of `assignment_operator` stays, although review + showed it is unreachable by construction — reaching it with an absent + operator requires `is_define`, which the first disjunct already covers. + Rationale: the plan's `Interfaces and dependencies` section specifies this + arm, and it keeps the function correct in isolation rather than only correct + given its single caller. The doc comment previously claimed it was a live + guard against upstream change, which was untrue; it now states plainly that + it is unreachable as the caller is written. Date/Author: 2026-08-13, + implementer, after second stage review. + +- Decision (review nit declined): the extracted module keeps the name + `makefile_export.rs` rather than being renamed to `makefile_variable.rs`. + Rationale: review observed correctly that the module also owns the generic + operator mapping for all eight operators, which is not export-specific. The + name is however the one the plan's Stage C names as the extraction target, + and the module comment states the wider scope. Renaming would diverge from + the plan for a cosmetic gain. Date/Author: 2026-08-13, implementer, after + stage review. + +- TOLERANCE BREACH (recorded, not worked around): the scope tolerance for + Stages B to D — "more than six files or more than 250 net lines" — is + exceeded. The delivered change touches twelve code files and four + documentation files, with 417 insertions against 69 deletions, so 348 net + lines. Analysis: the tolerance contradicts the plan's own Stage B and Stage D + instructions, which by themselves enumerate three new fixtures, a new + integration test file holding five named tests, an extended case list in + `tests/report_schema.rs`, a new Gherkin scenario in + `tests/features/parse.feature`, its step and registration in + `tests/parse_bdd.rs`, the adapter change, the adapter unit tests, and updates + to `docs/users-guide.md` and `docs/design.md` — already more than six files + before a single line is written. The overage therefore reflects a tolerance + set too tightly, not scope creep: no file was touched that the plan did not + name, except the module extraction the plan explicitly permits and its test + file. The breach is recorded here and reported rather than engineered around, + because compressing the work into six files would mean discarding + deliverables the plan requires. Date/Author: 2026-08-13, implementer. + +- UNBLOCKED (2026-08-13): the Stage E blockage below is resolved. The fork the + plan directs Stage E at is the same estate-owned repository that + `[patch.crates-io]` already points `makefile-lossless` to, so authority to + change it was granted. Stage E proceeded as planned: the parser change is on + the fork branch `bare-export-directive-lists`, open as a draft pull request + there, and this branch pins the resulting revision. Date/Author: 2026-08-13, + implementer. + +- Decision: the upstream name-list path is gated on the line *leading* with + `export`, not on any directive keyword having been consumed as the plan's + Stage E wording proposed. Rationale: review checked the first implementation + against GNU Make 4.4.1 and found the looser gate accepted + `override export FOO BAR`, which make rejects with "missing separator". Make + accepts a name list only when the line starts with `export`. The plan's + wording is superseded on this point, and the rejection is pinned by tests on + both sides. The converse spelling `export override FOO` is a separate matter: + make accepts it, reading `override` as one of the exported names, and the + parser accepts the line too — but its `name()` accessor refuses `override` as + a name, so makeutil reports only `FOO` while still saying `complete`. That + gap predates this stage and is recorded below rather than claimed as fixed. + Date/Author: 2026-08-13, implementer, after upstream review. + +- Decision: `export define FOO ... endef` is deliberately left reporting its + existing diagnostic rather than being drawn into the name-list path. + Rationale: it is valid GNU Make, but the parser's name slot swallows the + `define` keyword, so the following identifier is the block's name and not a + further export name. The first implementation consumed it as a name, which + erased the only signal that the line had not been understood — the node + looked clean while the body was mangled into bogus rules elsewhere. Modelling + exported define blocks properly is a separate piece of work. Date/Author: + 2026-08-13, implementer, after upstream review. + +- Decision (review findings declined upstream): `$(BAR)` as a name in an + export list stays unsupported, and no `exported_names()` accessor was added + to the parser crate. Rationale: accepting `$(BAR)` would put the name inside + a nested EXPR node rather than a direct identifier token, so this consumer's + walk would skip it silently and report `complete` with a name missing — + trading a loud error for a quiet omission. It remains an error, which is + honest. The accessor was declined because the plan's `Decision Log` already + records that makeutil, not the crate, owns name extraction, so that the + upstream diff stays small and no new API has to be supported forever. The + reviewer's contrary argument — that a maintainer will want the contract + inside the crate — is reasonable and is noted for whoever proposes this + upstream. Date/Author: 2026-08-13, implementer, after upstream review. + +- BLOCKED (superseded by the entry above): Stage E cannot be executed under + the delegated authority for this work, which forbids pushes, pull requests, + or issues against any repository other than the makeutil repository itself. + Stage E's substance is a parser change in the separate `makefile-lossless` + fork, followed by a revision bump here that can only point at a commit + published in that fork. Consequence: the multi-name form `export A B C` + continues to report `recovered` and to capture only the first name, exactly + as the plan's Stage C go/no-go anticipates. Stages B to D, F, and G stand on + their own: no form of `export` or `unexport` aborts any more, and the + single-name case is `complete`. `multi_name_export_never_aborts` and + `bare_export_names_are_all_collected` remain in their Stage C form, so + whoever resumes Stage E has the pins already written and need only flip them. + Date/Author: 2026-08-13, implementer. + +- Decision: Stages B and C are delivered as a single commit rather than one + commit each. Rationale: `AGENTS.md` forbids committing anything that fails a + quality gate, and a Stage B commit is red by construction. Test-first order + is preserved in the work itself and the red evidence is recorded in + `Artefacts and notes`, so the discipline is auditable without committing a + broken tree. Date/Author: 2026-08-13, implementer. + +## Outcomes & retrospective + +Recorded at the end of Stage G, after Stage E landed. + +Does `export A B C` parse to `complete` with all three names? Yes, after Stage +E. `tests/fixtures/makefiles/export-directive-list.mk` reports `complete` with +no diagnostics and six variable entries — three assignments and three directive +facts — plus its `check` rule. + +Did any snapshot change unexpectedly? No snapshot changed at all, before or +after the revision bump. Both `insta` snapshots in `tests/snapshots/` and +`schemas/makeutil.parse.v1.schema.json` are byte-identical to their pre-change +state. The bump's tripwire therefore never fired, which is the evidence that +the upstream change is confined to multi-name export lines. + +Did the `variables`-reuse representation cause confusion in review? Not the +representation itself, which review accepted twice. What review caught were +three defects in how the directive was recognized, all recorded in the +`Decision Log`. Identifying directive keywords by token text silently discarded +`export unexport`. Gating the no-facts path on "export and not define" left +`export define FOO` aborting with exit code 2. Trusting upstream to diagnose +every line it could not name produced a `complete` report with +`override export override` missing entirely. + +Each breached the plan's own honesty or never-abort constraint while every gate +passed, and each was found by an adversarial reading of the *implementation* +rather than of the plan: the fixtures were drawn from the forms the plan +enumerated, so they could not catch inputs the plan had not imagined. The +recurring root cause is worth carrying forward — twice the implementation +leaned on an assumption about upstream behaviour, and both times the assumption +was false for some input. A guarantee the adapter enforces itself is worth more +than one inherited from a dependency. + +The corrected implementation anchors on the name upstream reports rather than +on a keyword list, and emits its own diagnostic whenever an export line yields +no facts. Sixteen export and unexport forms are pinned by a parameterized test +asserting status and variable names together, so a form that stopped producing +its fact could not keep passing. + +Stage E repeated the pattern in a third setting, which is worth recording +because it is now a clear trend rather than a coincidence. Its first +implementation was reviewed against GNU Make itself as an oracle rather than +against the plan, and that immediately found two lines whose parse changed in +ways make does not sanction — `override export FOO BAR`, which make rejects, and +`export define FOO`, where the change erased a diagnostic. Both had passed the +whole gate set. Checking a parser change against the thing it parses, rather +than against the tests written alongside it, was the single most valuable +technique used in this work. + +Several defects were observed and deliberately left alone, all outside this +plan's scope and all deserving their own plan, ideally a shared one: +`foo: export BAR := baz` reports `complete` with prerequisites +`["export", "BAR", ":=", "baz"]`; `override define FOO ... endef` still aborts +with exit code 2, losing the whole file; `override export FOO` in its +single-name form parses `complete` although GNU Make rejects it; +`export define FOO ... endef` is valid GNU Make that the parser still does not +model, though it now degrades honestly rather than aborting; and +`export export FOO` or `export override FOO` retains `FOO` but reports +`recovered` with a diagnostic, while GNU Make exports both names. + +Those are the two honesty-rule breaches identified during this work, and they +are worth stating plainly rather than burying. The target-specific form remains +a surviving breach: it is reported as `complete` while being modelled as a +misleading rule. The keyword-prefixed forms were the second breach: the +baseline implementation silently omitted the keyword-named export while +reporting `complete`. The current adapter retains the later `FOO` fact and +reports `recovered` with a diagnostic, but the keyword-named fact remains +unrepresentable; this recovery path is pinned by `no_export_form_aborts`. +Closing that remaining omission means redefining what the parser's `name()` +accessor treats as a name, which is a breaking change for every consumer of +that crate and needs its own decision, not a quiet fix appended to this plan. +The target-specific form likewise requires a separate plan. + +## Context and orientation + +Read this section in full before touching anything. It assumes no prior +knowledge of the repository. + +### What the tool does + +`makeutil` is a Rust command-line tool. Its only subcommand today is +`makeutil parse `, which reads a single GNU Makefile and writes one JSON +document to standard output describing what it found: rules, variable +definitions, include directives, and any parser diagnostics. It never executes +`make` and never follows `include` directives; it only reports syntax. + +The design is a hexagonal one — the domain owns the report types and defines a +port, and an adapter wraps the third-party parser so that no upstream type +leaks into the report. The layers are: + +- `src/domain/mod.rs` — the report types that are serialized to JSON: + `ParseReport`, `RuleFact`, `VariableFact`, `IncludeFact`, `ParseDiagnostic`, + `ParseStatus`, and the `AssignmentOperator` enum. These derive `Serialize` + and their field names are the JSON key names. `SCHEMA_VERSION` is `1`. +- `src/domain/location.rs` — `SourceSpan` (byte offsets) and `LocationIndex`, + which converts byte offsets into one-based line and column positions. +- `src/ports.rs` — the `MakefileParser` trait, the `SyntaxObservation` enum that + the adapter produces, and `ParserPortError`. `SyntaxObservation` is the + internal, pre-location vocabulary; it has variants `Rule`, `Variable`, + `Include`, and `Diagnostic`. +- `src/adapters/makefile.rs` — the adapter over the third-party parser crate. It + walks the concrete syntax tree and produces `SyntaxObservation` values. +- `src/application.rs` — `parse_source`, which hashes the input, drives the + parser, resolves every span into a location, and assembles a `ParseReport`. + The `ReportAssembly::status` method (near the end of the file) is what decides + `complete` versus `recovered`: any diagnostic at all means `recovered`. +- `src/adapters/cli.rs` — argument handling and the process exit policy. + +"Concrete syntax tree" (CST) here means a lossless tree that retains every byte +of the input, including whitespace and comments, so the original text can be +reproduced exactly. The crate providing it is `makefile-lossless`, built on the +`rowan` library. `rowan` trees are made of *nodes* (which have a `SyntaxKind` +such as `VARIABLE`, `RULE`, `EXPR`, `ERROR`) and *tokens* (leaves such as +`IDENTIFIER`, `WHITESPACE`, `OPERATOR`, `NEWLINE`). + +`Cargo.toml` pins `makefile-lossless = "=0.3.40"` and then redirects it with a +`[patch.crates-io]` entry to a specific git revision of a fork, +`8dd35801b75b332c2ac2f995ae398ef8238559fa`. This matters: the established way +to land an upstream parser change in this project is to make the change on that +fork and bump the revision in `[patch.crates-io]`. The published version number +`0.3.40` is unchanged by such a bump, so neither `ToolIdentity::default()` in +`src/domain/mod.rs` nor the `parser_version` constant in the JSON schema needs +to change. + +### Where the defect lives + +The abort originates in `assignment_operator` in `src/adapters/makefile.rs`, +lines 196-216. It maps the upstream operator string to the domain enum: + +```rust +fn assignment_operator( + operator: Option<&str>, + is_define: bool, +) -> Result { + match operator { + None if is_define => Ok(AssignmentOperator::Define), + Some("=") => Ok(AssignmentOperator::Recursive), + // ... other operators ... + None => Err(ParserPortError::MissingField { + field: "variable-assignment-operator", + }), + } +} +``` + +The final arm is the crash. A bare `export FOO` has no operator token and is +not a `define` block, so `operator` is `None` and `is_define` is `false`, and +the adapter returns `MissingField`. That error propagates out of +`variable_observation` (lines 174-194), out of `collect_items`, out of +`MakefileLosslessParser::parse`, through `ParseApplicationError::Parser`, and +into `src/adapters/cli.rs` line 197, which prints +`makeutil: parse-internal: ` and exits 2. + +Note the ordering in `MakefileLosslessParser::parse` (lines 34-43): +`collect_items` runs *before* `collect_diagnostics`. That is why a file whose +only problem is one export line yields no diagnostics and no partial report — +the traversal dies before diagnostics are ever gathered. + +There is a second, distinct abort on the same path. A bare `export` alone on a +line makes `VariableDefinition::name()` return `None`, so +`variable_observation` (line 180) returns +`MissingField { field: "variable-name" }` and the tool exits 2 with +`makeutil: parse-internal: required variable-name accessor was absent`. + +### Verified current behaviour of every export form + +The following were measured against the current default-branch build. Reproduce +them in Stage A. "Exit 2" means the fatal `parse-internal` path; exit 1 means a +`recovered` report was still printed; exit 0 means `complete`. + +- `export FOO` — exit 2, + `required variable-assignment-operator accessor was absent`. Upstream tree is + clean with no errors; this is purely a makeutil defect. +- `export FOO BAR BAZ` — exit 2, same message. Upstream additionally reports + `expected assignment operator`, and has already lost `BAR` into an error node + and `BAZ` out of the variable node altogether. +- `export` alone — exit 2, `required variable-name accessor was absent`. + Upstream reports `expected variable name`. +- `unexport FOO` — exit 1. Report is produced but is wrong: a rule with targets + `["unexport", "FOO"]`, status `recovered`, diagnostic `expected ':'`. +- `unexport FOO BAR` — exit 1, same shape with targets + `["unexport", "FOO", "BAR"]`. +- `unexport` alone — exit 1, a rule with targets `["unexport"]`. +- `export FOO := bar` — exit 0, `complete`. Correctly reported as a variable + with `operator: ":="` and `exported: true`. Already works; must not regress. +- `foo: export BAR := baz` — exit 0, `complete`, but wrong: a rule with + prerequisites `["export", "BAR", ":=", "baz"]`. Out of scope; see + `Surprises & Discoveries`. +- `FOO := bar` followed by `export FOO` and then ordinary rules — exit 2. This + is the real-world shape and demonstrates the blast radius: the assignment and + every rule in the file are lost because of one directive line. + +### How the tests are organized + +- `src/adapters/makefile_tests.rs` — unit tests for the adapter, included into + `src/adapters/makefile.rs` at the bottom via + `#[cfg(test)] #[path = "makefile_tests.rs"] mod tests;`. These call private + helpers such as `assignment_operator` and `condition_kind` directly. Note the + existing test `ordinary_variable_requires_an_operator`, which asserts the + very behaviour this plan changes — it must be updated, not deleted. +- `tests/fixtures/makefiles/` — Makefile fixtures: `all-facts.mk` (the complete + happy path), `recovered.mk`, `multiline-define.mk`, and + `conditional-error-directive.mk`. +- `tests/corpus.rs` — the "unsupported syntax corpus". Its module comment states + the policy directly: these tests pin honest degradation for constructs the + tool cannot represent, and are designed to fail on purpose if an upstream + release learns the construct, so the pin and the expectations are revisited + together. Stage F adds to this file. +- `tests/report_schema.rs` — validates serialized reports against + `schemas/makeutil.parse.v1.schema.json` using the `jsonschema` crate, checks + that a deliberately malformed document is rejected, and holds two `insta` + snapshot tests whose stored output lives in `tests/snapshots/`. +- `tests/parse_bdd.rs` and `tests/features/parse.feature` — behaviour-driven + tests using `rstest-bdd`. The feature file holds `Scenario` blocks in Gherkin; + `tests/parse_bdd.rs` holds the `#[given]`, `#[when]`, `#[then]` step + functions and a `World` struct carrying arguments, stdin, stdout, stderr, and + exit code. +- `tests/cli_e2e.rs` — end-to-end tests that run the built binary. +- `tests/domain_contract.rs`, `tests/output_failures.rs`, + `tests/source_adapter.rs` — not affected by this plan. + +### Commit gates + +Run these from the repository root before every commit. Run them sequentially, +never in parallel — the build cache is shared and parallel invocations fight +over it. + +```sh +make check-fmt +make lint +make typecheck +make test +make markdownlint +make provenance +``` + +`make fmt` applies formatting fixes (`cargo +nightly fmt --all` plus Markdown +formatting) and must be run after editing any Markdown. `make test` prefers +`cargo nextest run` when available and falls back to `cargo test`, and also +runs doctests. `make markdownlint` additionally runs the `spelling` and +`provenance` targets. Prose must use en-GB Oxford spelling (`organize`, +`standardize`, `behaviour`, `colour`) and Markdown paragraphs must wrap at 80 +columns. + +## Plan of work + +### Stage A: reproduce and orient (no code changes) + +Confirm the defect in the working tree before changing anything, so the +baseline matches this plan. Build the binary and run it over each form listed +under "Verified current behaviour of every export form" above, checking the +exit code and message of each. Record any divergence from the table in +`Surprises & Discoveries` before proceeding — a divergence means the upstream +pin has moved and the rest of this plan needs re-checking. + +Go/no-go: proceed only if `export FOO` exits 2 with the +`variable-assignment-operator` message. + +### Stage B: red tests and fixtures + +Add the failing tests first. Every test added here must fail before Stage C, +and each must fail for the reason stated, not for an unrelated reason such as a +missing fixture file. + +Create two fixtures. + +`tests/fixtures/makefiles/bare-export.mk` — the single-name and already-working +forms, which Stage C alone must make `complete`: + +```makefile +MOLD_VERSION_FILE := .mold-version +RUST_TOOLCHAIN_FILE := rust-toolchain.toml + +export MOLD_VERSION_FILE +export RUST_TOOLCHAIN_FILE +export CARGO_TERM_COLOR := always + +build: + @echo building +``` + +`tests/fixtures/makefiles/export-directive-list.mk` — the real-world multi-name +shape, plus the name-less and `unexport` forms. This fixture stays `recovered` +after Stage C and becomes partially better after Stage E: + +```makefile +MOLD_VERSION_FILE := .mold-version +MOLD_SHA256SUMS_FILE := .mold-sha256sums +RUST_TOOLCHAIN_FILE := rust-toolchain.toml + +export MOLD_VERSION_FILE MOLD_SHA256SUMS_FILE RUST_TOOLCHAIN_FILE + +check: + @echo checking +``` + +Create a third fixture for the forms that remain unrepresentable, +`tests/fixtures/makefiles/export-directive-limits.mk`: + +```makefile +FOO := 1 +export +unexport FOO +``` + +Then add these tests. + +In `src/adapters/makefile_tests.rs`, replace the existing test +`ordinary_variable_requires_an_operator` with a test asserting the new +behaviour, and add a companion for the directive form. The name-less signature +of `assignment_operator` will change in Stage C; write the test against the +intended new signature so that it fails to compile first, which is a valid red +state here. Add: + +- `bare_export_directive_uses_empty_schema_variant`, asserting that an + operator-less, non-`define`, exported variable maps to + `AssignmentOperator::Define` (the empty-string schema variant) rather than + producing `ParserPortError::MissingField`. +- `plain_variable_without_operator_is_still_rejected`, asserting that an + operator-less variable with neither `define` nor `export` still yields + `MissingField { field: "variable-assignment-operator" }`. This guards against + over-relaxing the check. +- `bare_export_names_are_all_collected`, calling the new helper described in + Stage C over a parsed `export FOO BAR BAZ` tree. Before Stage E this asserts + the single name `["FOO"]`; Stage E flips it to `["FOO", "BAR", "BAZ"]`. + +In `tests/report_schema.rs`, extend the `reports_validate_against_schema` case +list with the three new fixtures, so the schema contract is checked for each. +These will fail at Stage B because `parse_source` returns an error for the +first two fixtures rather than a report. + +Add a new integration test file, `tests/export_directives.rs`, with a module +comment explaining that it pins the report shape for GNU Make's export family. +It must contain: + +- `single_name_bare_export_is_complete` — parses `bare-export.mk` and asserts + `status == ParseStatus::Complete`, `diagnostics.is_empty()`, and that + `variables` contains entries named `MOLD_VERSION_FILE` and + `RUST_TOOLCHAIN_FILE` with `operator == AssignmentOperator::Define`, + `exported == true`, `define_block == false`, and `raw_value.is_empty()`. This + is the test that pins the consumer-facing discriminating predicate; say so in + its doc comment. +- `assignments_and_directives_are_distinguishable` — on the same report, asserts + that the entry for the assignment `CARGO_TERM_COLOR` has + `operator == AssignmentOperator::Simple`, proving directives and assignments + are told apart by the operator alone. +- `facts_after_a_bare_export_survive` — asserts the `build` rule is present, + proving the blast radius is gone. +- `multi_name_export_never_aborts` — parses `export-directive-list.mk` and + asserts only that `parse_source` returns `Ok`. Before Stage E it additionally + asserts `status == ParseStatus::Recovered`; Stage E changes this to + `Complete` with all three names present. +- `name_less_export_degrades_to_a_diagnostic` — parses + `export-directive-limits.mk`, asserts `Ok`, asserts + `status == ParseStatus::Recovered`, asserts at least one diagnostic, and + asserts no variable fact was invented for the name-less `export`. + +Add one behavioural scenario. In `tests/features/parse.feature`, append: + +```gherkin + Scenario: Parse a Makefile that exports already-defined variables + Given a Makefile fixture that exports already-defined variables + When makeutil parses the fixture by path + Then stdout contains one schema version 1 JSON document + And the process exits with code 0 + And stderr is empty +``` + +In `tests/parse_bdd.rs`, add the matching `#[given]` step, following the shape +of the existing `complete_fixture` step, pointing at +`tests/fixtures/makefiles/bare-export.mk`, and register the scenario with the +`#[scenario]` attribute in the same style as the existing scenarios. + +Red validation for this stage is described under `Concrete steps`. + +Go/no-go: proceed only when every new test fails and the failures are the +expected ones. If any new test passes at this point, the fixture is wrong. + +### Stage C: minimal implementation + +Make the smallest change that turns every red test green except the ones +explicitly deferred to Stage E. + +Change `assignment_operator` in `src/adapters/makefile.rs` (lines 196-216) so +that an absent operator is acceptable for a bare export directive as well as +for a `define` block. The function currently takes `(Option<&str>, bool)`; two +booleans in a row would be an unreadable signature, so introduce a small +parameter struct in the same file rather than adding a positional `bool`: + +```rust +/// Modifiers that make an absent assignment operator legitimate. +#[derive(Debug, Clone, Copy)] +struct OperatorContext { + /// Whether the definition is a `define` ... `endef` block. + is_define: bool, + /// Whether the `export` directive keyword is present. + is_export: bool, +} +``` + +and change the two `None` arms to: + +```rust +None if context.is_define || context.is_export => Ok(AssignmentOperator::Define), +None => Err(ParserPortError::MissingField { + field: "variable-assignment-operator", +}), +``` + +Update the single call site in `variable_observation` (line 183) to pass an +`OperatorContext` built from `variable.is_define()` and `variable.is_export()`. + +Then handle the name-less `export`. In `variable_observation` (lines 174-194), +`variable.name()` currently produces `MissingField` when absent. A bare +`export` with no names is a real GNU Make construct meaning "export every +variable", which schema version 1 cannot represent, and upstream already emits +an `expected variable name` diagnostic for it. So the correct behaviour is to +produce no fact at all and let the upstream diagnostic drive +`status: "recovered"`. Change `variable_observation` to return +`Result, ParserPortError>`, returning `Ok(None)` when +`variable.is_export()` is true and `variable.name()` is `None`, and update the +`TraversalEvent::Item(MakefileItem::Variable(...))` arm of `collect_items` +(lines 72-74) to push only when a fact was produced. Keep the `MissingField` +error for a name-less variable that is *not* an export, so genuinely broken +trees still fail loudly. + +Add the name-collection helper that Stage E will rely on. In +`src/adapters/makefile.rs`, add: + +```rust +/// Collect every identifier a directive-only `export` line names. +/// +/// A bare `export A B C` names three variables. Upstream's +/// `VariableDefinition::name()` returns only the first, so walk the node's own +/// identifier tokens, skipping the directive keywords themselves. +fn directive_names(variable: &VariableDefinition) -> Vec; +``` + +It walks `variable.syntax().children_with_tokens()`, keeps tokens whose kind is +`SyntaxKind::IDENTIFIER` and whose text is not `export`, `unexport`, +`override`, or `define`, and returns their texts in source order. Before Stage +E this returns one name for the multi-name input, because upstream has already +moved the rest out of the node; that is expected and is what +`bare_export_names_are_all_collected` asserts at this stage. + +Use `directive_names` in `variable_observation` when the operator is absent and +`is_export` is true: emit one `SyntaxObservation::Variable` per name, each with +`raw_value` empty, `exported: true`, `define_block: false`, and the span of the +whole directive node. Emitting several observations from one node means +`variable_observation` should return a `Vec` rather than an +`Option`; prefer that shape over the `Option` above if it reads more cleanly, +and record the choice in `Decision Log`. An empty vector then naturally covers +the name-less case. + +If `src/adapters/makefile.rs` approaches the 400-line limit, extract the export +handling into `src/adapters/makefile_export.rs` and declare it from +`src/adapters/mod.rs`, keeping the module comment convention. + +Go/no-go: every Stage B test passes except `multi_name_export_never_aborts`'s +eventual `Complete` assertion and `bare_export_names_are_all_collected`'s +eventual three-name assertion, both of which remain in their Stage C form. + +### Stage D: refactor, documentation, and gates + +Clean up without changing behaviour, then document. + +Update `docs/users-guide.md`, in the "Interpret results" section, with a short +prose paragraph explaining that a bare `export NAME` directive appears in the +`variables` array with an empty `operator` and an empty `raw_value`, that +`exported` is `true` and `define_block` is `false` for such an entry, and that +a consumer wanting only genuine assignments should filter on a non-empty +`operator`. Give the exact predicate. This paragraph is the mitigation for the +conflation risk; do not skip it. + +Update `docs/design.md` to record the representation decision and reference +this plan. If the decision is judged substantive enough to warrant its own +Architectural Decision Record, add one under `docs/adrs/` following the +numbering and style of `docs/adrs/0001-single-file-gnu-make-parse.md` and +reference it from the design document, as `AGENTS.md` requires. + +Refresh the `insta` snapshots if and only if a snapshot legitimately changed. +Do not accept a snapshot change without an explanation — an unexplained diff in +`tests/snapshots/report_schema__all_fact_variants_have_stable_json.snap` means +Stage C altered behaviour for inputs it should not have touched. + +Run every commit gate listed under `Commit gates`. Commit. + +### Stage E: upstream parser fix for multi-name exports + +This stage changes the pinned parser fork, not this repository's `src/`. + +Clone the fork at the currently pinned revision +`8dd35801b75b332c2ac2f995ae398ef8238559fa` — the URL is in the +`[patch.crates-io]` section of `Cargo.toml`; take it from there rather than +transcribing it into any document. The relevant function is `parse_assignment` +in the upstream file `src/lossless.rs`, roughly lines 733 to 816. Its current +structure is: skip whitespace; consume up to two directive keywords (`export`, +`override`) if present; consume the variable name; skip whitespace; then match +on the next token. That final match has an arm for a valid operator, an arm for +`NEWLINE` (already commented "Bare `export VARNAME` without assignment operator +is valid GNU Make"), an arm for end of input, and a catch-all +`_ => self.error("expected assignment operator".to_string())`. + +The catch-all is what breaks `export A B C`: after `FOO`, the next token is the +identifier `BAR`, which falls into the catch-all. + +The change: record whether a directive keyword was consumed in the prefix loop, +and in the catch-all arm, when a directive keyword *was* consumed and the next +token is an `IDENTIFIER`, consume alternating whitespace and identifier tokens +into the same `VARIABLE` node until `NEWLINE` or end of input, without emitting +an error. When no directive keyword was consumed, keep the existing error — a +plain `FOO BAR` line is still malformed. + +Write upstream tests first, in the upstream repository, following its existing +test style in `src/lossless.rs` (see `test_parse_export_assign` for the pattern +of asserting a rendered tree). At minimum: `export FOO BAR BAZ\n` produces one +`VARIABLE` node spanning the whole line with three identifier tokens after the +`export` keyword, no `ERROR` node, and no reported errors; and `FOO BAR\n` +without a directive still errors as before. + +Then, in this repository: + +1. Bump the `rev` in `[patch.crates-io]` in `Cargo.toml` to the new upstream + commit and run `cargo update -p makefile-lossless` so `Cargo.lock` follows. +2. Flip `bare_export_names_are_all_collected` in + `src/adapters/makefile_tests.rs` to expect `["FOO", "BAR", "BAZ"]`. +3. Flip `multi_name_export_never_aborts` in `tests/export_directives.rs` to + assert `status == ParseStatus::Complete`, no diagnostics, and three variable + facts named `MOLD_VERSION_FILE`, `MOLD_SHA256SUMS_FILE`, and + `RUST_TOOLCHAIN_FILE`, each with an empty operator and `exported == true`. + Rename the test to `multi_name_export_is_complete` at this point. +4. Re-run the full gate set and inspect both snapshots for unexpected diffs. + +Do not change `parser_version` anywhere: the published version string remains +`0.3.40`, so `ToolIdentity::default()` in `src/domain/mod.rs` and the +`parser_version` const in `schemas/makeutil.parse.v1.schema.json` are untouched. + +Go/no-go: if the upstream change causes any snapshot diff outside the export +fixtures, stop and escalate rather than accepting the snapshot. + +### Stage F: pin the `unexport` limitation + +Add a test to `tests/corpus.rs`, whose module comment already describes exactly +this policy, named `unexport_directive_degrades_honestly`. It parses +`tests/fixtures/makefiles/export-directive-limits.mk` and asserts that the +parse succeeds, that `status` is `Recovered`, that at least one diagnostic is +present, and that the misleading rule fact whose first target is `unexport` is +present — pinning the current behaviour explicitly so that if a future upstream +release learns `unexport`, this test fails and forces the expectations to be +revisited alongside the pin, exactly as the file's existing tests do. + +Document the limitation in `docs/users-guide.md` in one short paragraph: today +an `unexport` directive is reported as a rule and forces `recovered`, and +faithful support awaits a schema version able to express an explicit un-export. +Do not attempt to fix it in this plan. + +### Stage G: consumer re-pin note + +Downstream repositories consume `makeutil` by pinning a specific commit SHA, so +the fix reaches them only when they move their pin. Add a short entry to the +repository's release or changelog documentation (follow whatever convention +`docs/contents.md` indexes; if none exists, add the note to `docs/design.md` +under the export representation section) recording: the merge commit SHA that +carries this fix, the fact that bare `export` directives now appear in +`variables` with an empty `operator`, and the fact that `unexport` remains +unsupported. Changes inside consumer repositories are explicitly out of scope +for this plan. + +## Concrete steps + +All commands run from the repository root. + +Stage A, reproduce: + +```console +$ cargo build --bin makeutil +$ printf 'export FOO\n' > /tmp/mk-a.mk +$ ./target/debug/makeutil parse /tmp/mk-a.mk; echo "exit=$?" +makeutil: parse-internal: required variable-assignment-operator accessor was absent +exit=2 +$ printf 'export\n' > /tmp/mk-b.mk +$ ./target/debug/makeutil parse /tmp/mk-b.mk; echo "exit=$?" +makeutil: parse-internal: required variable-name accessor was absent +exit=2 +$ printf 'unexport FOO BAR\n' > /tmp/mk-c.mk +$ ./target/debug/makeutil parse /tmp/mk-c.mk > /dev/null; echo "exit=$?" +exit=1 +``` + +Stage B, red. Run the new tests and confirm they fail: + +```sh +cargo test --test export_directives +``` + +Expect failures of the form +`Parser(MissingField { field: "variable-assignment-operator" })` reported +through `parse_source` returning `Err`, not assertion failures about values. +Also run: + +```sh +cargo test --lib adapters::makefile::tests +cargo test --test report_schema +cargo test --test parse_bdd +``` + +The adapter unit tests may fail to compile at this point because +`assignment_operator` does not yet take an `OperatorContext`. A compile failure +naming that function is an acceptable red state; a compile failure naming +anything else is not — fix the test instead. + +Stage C, green: + +```sh +cargo test --test export_directives +cargo test --lib +cargo test --test report_schema +cargo test --test parse_bdd +./target/debug/makeutil parse tests/fixtures/makefiles/bare-export.mk; echo "exit=$?" +``` + +The last command must print a JSON document containing `"status":"complete"` +and exit 0. + +Stage D, gates, run sequentially: + +```sh +make fmt +make check-fmt +make lint +make typecheck +make test +make markdownlint +make provenance +``` + +Stage E, after the upstream revision bump: + +```sh +cargo update -p makefile-lossless +cargo test --test export_directives +cargo build --bin makeutil +./target/debug/makeutil parse tests/fixtures/makefiles/export-directive-list.mk; echo "exit=$?" +``` + +Expect `"status":"complete"`, exit 0, and three variable entries with +`"operator":""` and `"exported":true`. Then rerun the full gate set. + +Stage F: + +```sh +cargo test --test corpus +``` + +## Validation and acceptance + +Acceptance is behavioural, not structural. + +1. A Makefile that assigns variables and then exports one of them by name parses + to `"status": "complete"` with exit code 0, and every rule and assignment in + the file is present in the report. Verified by + `single_name_bare_export_is_complete` and + `facts_after_a_bare_export_survive` in `tests/export_directives.rs`, and by + the new BDD scenario "Parse a Makefile that exports already-defined + variables". +2. A Makefile containing `export A B C` on one line parses to + `"status": "complete"` with exit code 0 and all three names present in + `variables`. Verified by `multi_name_export_is_complete` after Stage E. +3. No form of `export` or `unexport` produces a `parse-internal` message or exit + code 2. Verified by `no_export_form_aborts`, which covers sixteen forms, + together with `name_less_export_degrades_to_a_diagnostic` and + `unexport_directive_degrades_honestly`. +4. Bare export directives are distinguishable from assignments by the + `operator` field alone. Verified by + `assignments_and_directives_are_distinguishable`. +5. Every new fixture's report validates against the unchanged version-1 schema. + Verified by the extended `reports_validate_against_schema` case list in + `tests/report_schema.rs`. +6. Nothing that worked before regresses: `export FOO := bar` still reports + `operator: ":="` with `exported: true`, and a plain operator-less variable + still fails loudly. Verified by + `plain_variable_without_operator_is_still_rejected` and the unchanged + existing suites. + +Red-Green-Refactor evidence to record in `Artefacts and notes` as the work +proceeds: + +- Red: `cargo test --test export_directives` before Stage C, showing failures + caused by `parse_source` returning `Err(Parser(MissingField { .. }))`. +- Green: the same command after Stage C, showing all tests passing except the + two explicitly deferred to Stage E. +- Refactor: `make check-fmt && make lint && make typecheck && make test`, all + passing, after the Stage D cleanup. + +Quality criteria for "done": + +- Tests: `make test` passes with no ignored or skipped new tests, and both + `insta` snapshots are either unchanged or changed for an explained reason. +- Lint and typecheck: `make lint` and `make typecheck` pass with no warnings and + no new lint suppressions anywhere in the diff. +- Documentation: `make markdownlint` passes, which also runs the spelling and + provenance gates. +- Schema: `schemas/makeutil.parse.v1.schema.json` is byte-identical to its state + before this work. + +## Idempotence and recovery + +Every step is re-runnable. The test commands and the `make` gates are pure +checks. `make fmt` is idempotent. `cargo update -p makefile-lossless` is +idempotent once the `rev` is set. + +The only step with a wider blast radius is the Stage E revision bump. To roll +it back, restore the previous `rev` in `[patch.crates-io]` in `Cargo.toml`, run +`cargo update -p makefile-lossless`, and revert the Stage E test flips. Because +Stage E is a separate commit from Stages B to D, `git revert` of that single +commit restores the Stage D state, in which bare single-name exports already +work. + +Do not accept an `insta` snapshot with `cargo insta accept` without first +reading the diff. If a snapshot was accepted in error, `git checkout` the file +under `tests/snapshots/` and rerun. + +Temporary Makefiles written under `/tmp` during Stage A can be deleted freely; +nothing in the repository depends on them. + +## Artefacts and notes + +### Red-Green-Refactor evidence + +Stage A reproduction, run against the pre-change build: every row of the +behaviour table above reproduced exactly. `export FOO`, `export FOO BAR BAZ` and +`export` all exited 2; the three `unexport` forms exited 1; both +`export FOO := bar` and `foo: export BAR := baz` exited 0. + +Stage B red, `cargo test --test export_directives`: + +```plaintext +running 5 tests +test facts_after_a_bare_export_survive ... FAILED +test assignments_and_directives_are_distinguishable ... FAILED +test name_less_export_degrades_to_a_diagnostic ... FAILED +test multi_name_export_never_aborts ... FAILED +test single_name_bare_export_is_complete ... FAILED + +---- name_less_export_degrades_to_a_diagnostic stdout ---- +Error: Parser(MissingField { field: "variable-name" }) +---- single_name_bare_export_is_complete stdout ---- +Error: Parser(MissingField { field: "variable-assignment-operator" }) +``` + +The other three failed with the same `variable-assignment-operator` error, so +every failure was the abort itself rather than an assertion about values. +`cargo test --test report_schema` failed the three new fixture cases with the +same two errors, `cargo test --lib` failed to compile with +`unresolved imports super::OperatorContext, super::directive_names` as +anticipated, and `cargo test --test parse_bdd` failed only the new scenario, +with exit code 2 where 0 was expected. + +Stage C green, `cargo test --test export_directives`: 5 passed, 0 failed. +`cargo test --lib`: 20 passed, 0 failed. + +Stage review red, after the reviewer identified the keyword-text defect. Both +were reproduced against the built binary before being fixed: + +```console +$ printf 'A := 1\nexport unexport\nb:\n\techo\n' > t.mk +$ makeutil parse t.mk # status=complete, variables=['A'], exit=0 +$ printf 'export define FOO\nbody\nendef\n' > t.mk +$ makeutil parse t.mk +makeutil: parse-internal: required variable-name accessor was absent +exit=2 +``` + +Stage review green: `export unexport` now reports `complete` with the +`unexport` fact present, and both `export define` forms report `recovered` with +exit code 1 and no invented facts. + +Stage E, upstream: 483 tests and 98 doctests pass on the fork branch, with +`cargo fmt --check` and `cargo clippy --all-targets` clean. Review there ran a +2016-case differential sweep against the previous revision, confirming that +every changed case is an `export`-led line and that the lossless round-trip +holds throughout. Parity with GNU Make 4.4.1 was checked directly for the +decisive lines: `export FOO BAR` and `export override FOO` are accepted by both; +`override export FOO BAR` and `override FOO BAR` are rejected by both. + +Stage E, the one-command success criterion: + +```console +$ printf 'FOO := 1\nBAR := 2\nexport FOO BAR\n' > demo.mk +$ makeutil parse demo.mk | python3 -m json.tool | head -20 + "parse": { "status": "complete", "diagnostics": [] } +$ echo "exit=$?" +exit=0 +``` + +`variables` holds four entries for that input: the two assignments and one +directive fact per exported name. The three-name fixture produces six entries — +three assignments and three directives — with `complete` status and no +diagnostics. + +Final gate run, all sequential and all passing: `make check-fmt`, `make lint` +(including the `whitaker` driver), `make typecheck`, `make test`, and +`make markdownlint` (which also runs `spelling` and `provenance`), 0 errors. + +Both `insta` snapshots and `schemas/makeutil.parse.v1.schema.json` are +unchanged from their pre-change state, before and after the revision bump. + +### Captured concrete syntax trees + +The concrete syntax trees below were captured against the pinned parser +revision and justify the staging. Keep them; they are the evidence that the +single-name case is a makeutil defect and the multi-name case is an upstream +one. + +Single-name bare export — clean tree, no errors, purely a makeutil defect: + +```plaintext +ROOT@0..11 + VARIABLE@0..11 + IDENTIFIER@0..6 "export" + WHITESPACE@6..7 " " + IDENTIFIER@7..10 "FOO" + NEWLINE@10..11 "\n" +``` + +Multi-name export — the second name is trapped in an error node and the third +escapes the variable node entirely, so makeutil cannot recover them without an +upstream change: + +```plaintext +ROOT@0..19 + VARIABLE@0..14 + IDENTIFIER@0..6 "export" + WHITESPACE@6..7 " " + IDENTIFIER@7..10 "FOO" + WHITESPACE@10..11 " " + ERROR@11..14 + IDENTIFIER@11..14 "BAR" + WHITESPACE@14..15 " " + IDENTIFIER@15..18 "BAZ" + NEWLINE@18..19 "\n" +``` + +Reported error for the above: `expected assignment operator` at range `10..11`. + +Name-less export — upstream already diagnoses it, so makeutil only needs to +stop aborting: + +```plaintext +ROOT@0..7 + VARIABLE@0..7 + IDENTIFIER@0..6 "export" + ERROR@6..7 + NEWLINE@6..7 "\n" +``` + +`unexport` — parsed as a rule, which is why it never aborts and why it is +nonetheless wrong: + +```plaintext +ROOT@0..17 + RULE@0..17 + TARGETS@0..16 + IDENTIFIER@0..8 "unexport" + WHITESPACE@8..9 " " + IDENTIFIER@9..12 "FOO" + WHITESPACE@12..13 " " + IDENTIFIER@13..16 "BAR" + ERROR@16..17 + NEWLINE@16..17 "\n" +``` + +Expected JSON shape for a bare export fact after Stage C, abbreviated: + +```json +{ + "ordinal": 2, + "name": "MOLD_VERSION_FILE", + "operator": "", + "raw_value": "", + "exported": true, + "overridden": false, + "define_block": false, + "conditions": [], + "location": { + "start_byte": 0, + "end_byte": 0, + "start_line": 1, + "start_column": 1, + "end_line": 1, + "end_column": 1 + } +} +``` + +The `location` values above are placeholders; the real ones come from the +directive's span. + +## Interfaces and dependencies + +No new crate dependencies. The parser remains `makefile-lossless`, redirected by +`[patch.crates-io]` in `Cargo.toml` to a git revision; Stage E bumps that +revision and nothing else about the dependency graph. + +At the end of Stage C, the following must exist in `src/adapters/makefile.rs` +(or in `src/adapters/makefile_export.rs` if the line budget forces an +extraction), all private to the crate: + +```rust +/// Modifiers that make an absent assignment operator legitimate. +#[derive(Debug, Clone, Copy)] +struct OperatorContext { + is_define: bool, + is_export: bool, +} + +fn assignment_operator( + operator: Option<&str>, + context: OperatorContext, +) -> Result; + +/// Collect every identifier a directive-only `export` line names. +fn directive_names(variable: &VariableDefinition) -> Vec; +``` + +and `variable_observation` must return a collection of observations rather than +exactly one, so that a single `export A B C` node yields one +`SyntaxObservation::Variable` per exported name and a name-less `export` yields +none: + +```rust +fn variable_observation( + variable: &VariableDefinition, + conditions: &[ConditionObservation], + source_length: usize, +) -> Result, ParserPortError>; +``` + +Everything in `src/domain/mod.rs`, `src/ports.rs`, `src/application.rs`, and +`src/adapters/cli.rs` keeps its current public shape. `AssignmentOperator` +gains no new variant; the existing `Define` variant, which serializes to the +empty string, carries the bare-export case. If a new variant seems necessary, +that is a tolerance breach — stop and escalate, because it would change the +schema's `operator` enum. diff --git a/docs/users-guide.md b/docs/users-guide.md index ce9ccec..dcbef9a 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -56,3 +56,48 @@ standard error and do not intentionally emit JSON. Control characters in caller-supplied paths are escaped in this diagnostic, so its first line cannot be forged. The JSON report preserves the exact caller-supplied logical path. Recovered reports are insufficient proof that a Makefile is compliant. + +### Bare `export` directives + +A bare `export NAME` directive names a variable without assigning a value on +that line. Such a directive appears in the `variables` array with `operator` +set to the empty string, `raw_value` set to the empty string, `exported` set to +`true`, and `define_block` set to `false`. A consumer that wants only genuine +assignments should therefore filter on a non-empty `operator`; the predicate +`operator == "" && define_block == false` identifies a bare export directive +rather than an assignment. A name may appear twice, once for its assignment and +once for the directive that exports it, so the operator rather than the name +distinguishes the two. + +A directive naming several variables, such as `export A B C`, yields one entry +per name and reports `complete`. The names may be spread across a line +continuation. + +The empty operator is shared with `define` blocks, which is why `define_block` +is part of the predicate. An `export NAME := value` line is an ordinary +assignment: it reports its real operator and its value, with `exported` set to +`true`. + +A bare `export` with no names at all means "export every variable", which +schema version 1 cannot express. Such a line produces no entry rather than an +invented one, a diagnostic explains the omission, and the report is +`recovered`. The same holds for `export define NAME`, and for a line exporting +a variable whose name is itself `export`, `override` or `define`, none of which +the parser names. + +`override export NAME` is reported as an ordinary export directive, although +GNU Make itself rejects that combination. Its multi-name form is `recovered`. + +A directive whose exported names include `export`, `override` or `define`, such +as `export export FOO`, retains every nameable fact and reports `recovered` +with a diagnostic. This prevents the report from claiming `complete` while +omitting a name the parser cannot represent. + +### `unexport` is not yet supported + +An `unexport` directive is currently reported as a rule whose first target is +the word `unexport`, and it forces a `recovered` status with an `expected ':'` +diagnostic. Schema version 1 has no way to express "this name was explicitly +un-exported", so faithful support awaits a schema version that can. Treat any +`unexport` line in a report as an unrepresented construct rather than as a real +rule. diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index 2244830..6800f73 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -38,3 +38,33 @@ not intentionally emit JSON. See the [user guide](users-guide.md) for the complete command, stream, and source-location contracts. + +## Handle export directives + +Bare `export NAME` directives now appear as valueless entries in `variables`. +They use an empty `operator` and `raw_value`, with `exported` set to `true` and +`define_block` set to `false`. Consumers should identify bare-export facts with +`operator == "" && define_block == false` and preserve those entries when +directive facts matter. + +A multi-name directive such as `export A B C` produces one valueless entry per +name and returns `complete` when every name is representable. For example, the +`variables` entries for that directive include separate facts like these: + +```json +[ + {"name": "A", "operator": "", "raw_value": "", "exported": true, + "define_block": false}, + {"name": "B", "operator": "", "raw_value": "", "exported": true, + "define_block": false}, + {"name": "C", "operator": "", "raw_value": "", "exported": true, + "define_block": false} +] +``` + +Consumers must not assume that one `variables` entry corresponds to one +directive. + +`unexport` remains unrepresented in schema version 1. It produces a recovered +report with diagnostics and must not be treated as a rule. Consumers that +previously rejected fatal parses should also handle this recovered outcome. diff --git a/src/adapters/makefile.rs b/src/adapters/makefile.rs index 744d1c8..7b7511d 100644 --- a/src/adapters/makefile.rs +++ b/src/adapters/makefile.rs @@ -14,8 +14,9 @@ use makefile_lossless::{ }; use rowan::ast::AstNode as _; +use super::makefile_export::{OperatorContext, assignment_operator, export_directive_observations}; use crate::{ - domain::{AssignmentOperator, ConditionBranch, ConditionKind, SourceSpan}, + domain::{ConditionBranch, ConditionKind, SourceSpan}, ports::{ ConditionObservation, MakefileParser, @@ -70,7 +71,7 @@ fn collect_items( observations.push(rule_observation(&rule, &conditions, source_length)?); } TraversalEvent::Item(MakefileItem::Variable(variable)) => { - observations.push(variable_observation(&variable, &conditions, source_length)?); + observations.extend(variable_observation(&variable, &conditions, source_length)?); } TraversalEvent::Item(MakefileItem::Include(include)) => { observations.push(include_observation(&include, &conditions, source_length)?); @@ -171,48 +172,59 @@ fn rule_observation( }) } +/// One `export A B C` line names several variables, and a name-less `export` +/// names none, so a single definition node can yield any number of facts. fn variable_observation( variable: &VariableDefinition, conditions: &[ConditionObservation], source_length: usize, -) -> Result { - Ok(SyntaxObservation::Variable { +) -> Result, ParserPortError> { + let operator = variable.assignment_operator(); + let context = OperatorContext { + is_define: variable.is_define(), + is_export: variable.is_export(), + }; + let definition_span = span(variable.syntax().text_range(), source_length)?; + + // An export line upstream could not name is unrepresentable rather than + // broken: a bare `export` exports everything, `export define FOO` is + // modelled with no name at all, and upstream refuses any name whose text + // is one of its own directive keywords. Aborting the whole file over it + // would be disproportionate, but dropping it silently would let the report + // claim `complete` while a construct went missing. Emitting a diagnostic + // here rather than relying on upstream to emit one keeps the report + // `recovered` whatever upstream does — `override export override` is + // dropped by upstream without any error of its own. A name-less definition + // that is not an export is still a broken tree and keeps failing loudly + // below. + if context.is_export && variable.name().is_none() { + return Ok(vec![SyntaxObservation::Diagnostic { + message: "export directive names could not be determined".to_owned(), + code: None, + span: definition_span, + }]); + } + + if operator.is_none() && context.is_directive_only() { + return Ok(export_directive_observations( + variable, + conditions, + definition_span, + )); + } + + Ok(vec![SyntaxObservation::Variable { name: variable.name().ok_or(ParserPortError::MissingField { field: "variable-name", })?, - operator: assignment_operator( - variable.assignment_operator().as_deref(), - variable.is_define(), - )?, + operator: assignment_operator(operator.as_deref(), context)?, raw_value: variable.raw_value().unwrap_or_default(), - exported: variable.is_export(), + exported: context.is_export, overridden: variable.is_override(), - define_block: variable.is_define(), + define_block: context.is_define, conditions: conditions.to_vec(), - span: span(variable.syntax().text_range(), source_length)?, - }) -} - -fn assignment_operator( - operator: Option<&str>, - is_define: bool, -) -> Result { - match operator { - None if is_define => Ok(AssignmentOperator::Define), - Some("=") => Ok(AssignmentOperator::Recursive), - Some(":=") => Ok(AssignmentOperator::Simple), - Some("::=") => Ok(AssignmentOperator::PosixSimple), - Some(":::=") => Ok(AssignmentOperator::ImmediateRecursive), - Some("+=") => Ok(AssignmentOperator::Append), - Some("?=") => Ok(AssignmentOperator::Conditional), - Some("!=") => Ok(AssignmentOperator::Shell), - Some(raw_operator) => Err(ParserPortError::UnsupportedAssignmentOperator { - operator: raw_operator.to_owned(), - }), - None => Err(ParserPortError::MissingField { - field: "variable-assignment-operator", - }), - } + span: definition_span, + }]) } fn include_observation( diff --git a/src/adapters/makefile_export.rs b/src/adapters/makefile_export.rs new file mode 100644 index 0000000..55e50b3 --- /dev/null +++ b/src/adapters/makefile_export.rs @@ -0,0 +1,166 @@ +//! Assignment-operator and `export` directive handling for the parser adapter. +//! +//! GNU Make's `export` keyword serves two roles that look alike in the tree: +//! it can modify a definition (`export FOO := bar`) or stand alone as a +//! directive naming variables assigned elsewhere (`export FOO BAR`). The +//! directive form carries no assignment operator and no value, so it needs +//! separate treatment from an ordinary definition. This module owns that +//! distinction and the mapping of upstream operator tokens onto the schema's +//! operator set. + +use makefile_lossless::{SyntaxKind, VariableDefinition}; +use rowan::ast::AstNode as _; + +use crate::{ + domain::{AssignmentOperator, SourceSpan}, + ports::{ConditionObservation, ParserPortError, SyntaxObservation}, +}; + +/// Modifiers that make an absent assignment operator legitimate. +#[derive(Debug, Clone, Copy)] +pub(super) struct OperatorContext { + /// Whether the definition is a `define` … `endef` block. + pub(super) is_define: bool, + /// Whether the `export` directive keyword is present. + pub(super) is_export: bool, +} + +impl OperatorContext { + /// Whether the line only names variables assigned elsewhere, rather than + /// carrying a definition of its own. + pub(super) const fn is_directive_only(self) -> bool { self.is_export && !self.is_define } +} + +/// Map an upstream operator token onto the schema version 1 operator set. +/// +/// An absent operator is legitimate for a `define` block and for a bare +/// `export` directive; both use the schema's empty operator. Any other +/// operator-less definition is a broken tree and must fail loudly. +/// +/// The `is_export` half of that allowance is unreachable as the caller is +/// written: an operator-less export reaching here must also be a `define`, +/// which the first disjunct already covers. It is retained only so the +/// function is correct in isolation — an operator-less `export` has a +/// well-defined answer whatever routes to it — and never as a live guard. +pub(super) fn assignment_operator( + operator: Option<&str>, + context: OperatorContext, +) -> Result { + match operator { + None if context.is_define || context.is_export => Ok(AssignmentOperator::Define), + Some("=") => Ok(AssignmentOperator::Recursive), + Some(":=") => Ok(AssignmentOperator::Simple), + Some("::=") => Ok(AssignmentOperator::PosixSimple), + Some(":::=") => Ok(AssignmentOperator::ImmediateRecursive), + Some("+=") => Ok(AssignmentOperator::Append), + Some("?=") => Ok(AssignmentOperator::Conditional), + Some("!=") => Ok(AssignmentOperator::Shell), + Some(raw_operator) => Err(ParserPortError::UnsupportedAssignmentOperator { + operator: raw_operator.to_owned(), + }), + None => Err(ParserPortError::MissingField { + field: "variable-assignment-operator", + }), + } +} + +/// Expand a directive-only `export` line into one valueless fact per name. +/// +/// A name-less `export` exports every variable, which schema version 1 cannot +/// express. `variable_observation` emits the recovery diagnostic before this +/// helper runs, so this helper only expands directives with a nameable fact. +pub(super) fn export_directive_observations( + variable: &VariableDefinition, + conditions: &[ConditionObservation], + definition_span: SourceSpan, +) -> Vec { + let mut observations = directive_names(variable) + .into_iter() + .map(|name| SyntaxObservation::Variable { + name, + operator: AssignmentOperator::Define, + raw_value: String::new(), + exported: true, + overridden: variable.is_override(), + define_block: false, + conditions: conditions.to_vec(), + span: definition_span, + }) + .collect::>(); + if has_unrepresentable_name_before_anchor(variable) { + observations.push(SyntaxObservation::Diagnostic { + message: "some export directive names could not be represented".to_owned(), + code: None, + span: definition_span, + }); + } + observations +} + +/// Whether a bare export starts with a name that upstream omits before its +/// first usable name. +/// +/// Upstream treats `export`, `override`, and `define` as directive keywords +/// when choosing `VariableDefinition::name()`. On `export export FOO` and +/// `export override FOO`, the second identifier is actually an exported name +/// but is omitted before the `FOO` anchor. The fact cannot be represented in +/// schema version 1, so the caller must mark the otherwise usable facts as +/// recovered rather than silently reporting a complete parse. +fn has_unrepresentable_name_before_anchor(variable: &VariableDefinition) -> bool { + let Some(first_name) = variable.name() else { + return false; + }; + let mut identifiers = variable + .syntax() + .children_with_tokens() + .filter_map(rowan::NodeOrToken::into_token) + .filter(|token| token.kind() == SyntaxKind::IDENTIFIER) + .map(|token| token.text().to_owned()); + if identifiers.next().as_deref() != Some("export") { + return false; + } + identifiers + .take_while(|name| name != &first_name) + .next() + .is_some() +} + +/// Collect every identifier a directive-only `export` line names. +/// +/// A bare `export A B C` names three variables but upstream's +/// `VariableDefinition::name()` returns only the first, so the remaining names +/// have to be read from the node's own identifier tokens. +/// +/// That first name anchors the walk rather than a list of keywords to skip: +/// the directive keywords upstream consumed are exactly the identifier tokens +/// preceding it, and a variable may legitimately be called `unexport`. +/// Filtering by keyword text instead would silently discard `export unexport`, +/// which upstream parses cleanly. Names upstream itself treats as keywords — +/// `export`, `override` and `define` — remain unrepresentable, because it +/// reports no name for them at all; the caller turns that into a diagnostic. +/// +/// The walk assumes the anchor is a direct identifier token of the definition +/// node, which is how upstream finds it too. Should that ever cease to hold, +/// the anchor is still reported on its own rather than the line being dropped. +pub(super) fn directive_names(variable: &VariableDefinition) -> Vec { + let Some(first_name) = variable.name() else { + return Vec::new(); + }; + let names: Vec = variable + .syntax() + .children_with_tokens() + .filter_map(rowan::NodeOrToken::into_token) + .filter(|token| token.kind() == SyntaxKind::IDENTIFIER) + .map(|token| token.text().to_owned()) + .skip_while(|text| *text != first_name) + .collect(); + if names.is_empty() { + vec![first_name] + } else { + names + } +} + +#[cfg(test)] +#[path = "makefile_export_tests.rs"] +mod tests; diff --git a/src/adapters/makefile_export_tests.rs b/src/adapters/makefile_export_tests.rs new file mode 100644 index 0000000..5e6d322 --- /dev/null +++ b/src/adapters/makefile_export_tests.rs @@ -0,0 +1,164 @@ +//! Unit tests for operator mapping and `export` directive name collection. + +use makefile_lossless::{Makefile, MakefileItem, Parse, VariableDefinition}; +use pretty_assertions::assert_eq; +use rstest::rstest; + +use super::{OperatorContext, assignment_operator, directive_names, export_directive_observations}; +use crate::{ + domain::{AssignmentOperator, ConditionBranch, ConditionKind, SourceSpan}, + ports::{ConditionObservation, ParserPortError, SyntaxObservation}, +}; + +/// Span standing in for the directive's own range, which these tests do not +/// exercise. +const SPAN: SourceSpan = SourceSpan { start: 0, end: 0 }; + +/// Build a context from the modifiers a definition line carries. +fn context(is_define: bool, is_export: bool) -> OperatorContext { + OperatorContext { + is_define, + is_export, + } +} + +/// Parse one source line and return its variable definition node, if upstream +/// modelled the line as one. +fn first_variable(source: &str) -> Option { + Parse::::parse_makefile(source) + .tree() + .items() + .find_map(|item| match item { + MakefileItem::Variable(variable) => Some(variable), + _ => None, + }) +} + +#[rstest] +fn define_without_operator_uses_empty_schema_variant() { + assert_eq!( + assignment_operator(None, context(true, false)), + Ok(AssignmentOperator::Define) + ); +} + +/// The directive path supplies the operator itself rather than routing through +/// [`assignment_operator`], so the empty-operator guarantee is pinned on the +/// facts a real directive produces. +#[rstest] +fn bare_export_directive_uses_empty_schema_variant() { + let variable = first_variable("export MOLD_VERSION_FILE\n") + .expect("a bare export should be modelled as a variable definition"); + let observations = export_directive_observations(&variable, &[], SPAN); + + assert_eq!( + observations, + vec![SyntaxObservation::Variable { + name: "MOLD_VERSION_FILE".to_owned(), + operator: AssignmentOperator::Define, + raw_value: String::new(), + exported: true, + overridden: false, + define_block: false, + conditions: Vec::new(), + span: SPAN, + }] + ); +} + +/// `override export FOO` is a directive too, and the override modifier must +/// survive the expansion into per-name facts. +#[rstest] +fn overridden_export_directive_retains_its_modifier() { + let variable = first_variable("override export FOO\n") + .expect("an overridden export should be modelled as a variable definition"); + let observations = export_directive_observations(&variable, &[], SPAN); + let overridden = observations.iter().map(|observation| match observation { + SyntaxObservation::Variable { + name, overridden, .. + } => (name.as_str(), *overridden), + _ => ("", false), + }); + + assert_eq!(overridden.collect::>(), vec![("FOO", true)]); +} + +/// The conditional ancestry of the directive line is carried onto every fact +/// it produces. +#[rstest] +fn export_directive_facts_carry_their_conditions() { + let variable = first_variable("export FOO\n") + .expect("a bare export should be modelled as a variable definition"); + let ancestry = vec![ConditionObservation { + kind: ConditionKind::Ifdef, + expression: "COND".to_owned(), + branch: ConditionBranch::If, + span: SPAN, + }]; + let observations = export_directive_observations(&variable, &ancestry, SPAN); + + assert_eq!( + observations + .first() + .and_then(|observation| match observation { + SyntaxObservation::Variable { conditions, .. } => Some(conditions.clone()), + _ => None, + }), + Some(ancestry) + ); +} + +#[rstest] +fn plain_variable_without_operator_is_still_rejected() { + assert_eq!( + assignment_operator(None, context(false, false)), + Err(ParserPortError::MissingField { + field: "variable-assignment-operator", + }) + ); +} + +#[rstest] +fn unsupported_assignment_operator_is_rejected() { + assert_eq!( + assignment_operator(Some("unknown"), context(false, false)), + Err(ParserPortError::UnsupportedAssignmentOperator { + operator: "unknown".to_owned(), + }) + ); +} + +#[rstest] +#[case(true, false, false)] +#[case(false, true, true)] +#[case(true, true, false)] +#[case(false, false, false)] +fn directive_only_lines_are_recognized( + #[case] is_define: bool, + #[case] is_export: bool, + #[case] expected: bool, +) { + assert_eq!(context(is_define, is_export).is_directive_only(), expected); +} + +/// Every name a directive line carries is collected, now that the pinned +/// parser revision keeps the whole list inside the variable node. +/// +/// The keyword-named cases guard the anchoring rule: a variable may be called +/// `unexport`, and `export export FOO` has upstream consume both leading +/// keywords, so neither may be decided by matching keyword text. +#[rstest] +#[case::single("export FOO\n", vec!["FOO"])] +#[case::multiple("export FOO BAR BAZ\n", vec!["FOO", "BAR", "BAZ"])] +#[case::continued("export FOO \\\n\tBAR\n", vec!["FOO", "BAR"])] +#[case::name_less("export\n", Vec::new())] +#[case::keyword_named_variable("export unexport\n", vec!["unexport"])] +#[case::repeated_keyword_prefix("export export FOO\n", vec!["FOO"])] +#[case::overridden("override export FOO\n", vec!["FOO"])] +#[case::unnameable("export export\n", Vec::new())] +fn bare_export_names_are_all_collected(#[case] source: &str, #[case] expected: Vec<&str>) { + let variable = first_variable(source) + .expect("an export directive should be modelled as a variable definition"); + + assert_eq!(directive_names(&variable), expected); +} diff --git a/src/adapters/makefile_tests.rs b/src/adapters/makefile_tests.rs index 45150df..41505e9 100644 --- a/src/adapters/makefile_tests.rs +++ b/src/adapters/makefile_tests.rs @@ -6,13 +6,7 @@ use makefile_lossless::{Makefile, Parse}; use pretty_assertions::assert_eq; use rstest::rstest; -use super::{ - MakefileLosslessParser, - assignment_operator, - collect_diagnostics, - condition_kind, - ensure_round_trip, -}; +use super::{MakefileLosslessParser, collect_diagnostics, condition_kind, ensure_round_trip}; use crate::{ domain::{AssignmentOperator, ConditionBranch, ConditionKind, SourceSpan}, ports::{ConditionObservation, MakefileParser as _, ParserPortError, SyntaxObservation}, @@ -38,34 +32,6 @@ fn round_trip_mismatch_is_rejected() { ); } -#[rstest] -fn define_without_operator_uses_empty_schema_variant() { - assert_eq!( - assignment_operator(None, true), - Ok(AssignmentOperator::Define) - ); -} - -#[rstest] -fn ordinary_variable_requires_an_operator() { - assert_eq!( - assignment_operator(None, false), - Err(ParserPortError::MissingField { - field: "variable-assignment-operator", - }) - ); -} - -#[rstest] -fn unsupported_assignment_operator_is_rejected() { - assert_eq!( - assignment_operator(Some("unknown"), false), - Err(ParserPortError::UnsupportedAssignmentOperator { - operator: "unknown".to_owned(), - }) - ); -} - #[rstest] fn multiline_define_preserves_exact_body() { let source = include_str!("../../tests/fixtures/makefiles/multiline-define.mk"); diff --git a/src/adapters/mod.rs b/src/adapters/mod.rs index 414ed49..bf864be 100644 --- a/src/adapters/mod.rs +++ b/src/adapters/mod.rs @@ -2,6 +2,7 @@ pub mod cli; mod makefile; +mod makefile_export; pub mod source; pub use makefile::MakefileLosslessParser; diff --git a/src/domain/mod.rs b/src/domain/mod.rs index c64b4ef..c586528 100644 --- a/src/domain/mod.rs +++ b/src/domain/mod.rs @@ -114,8 +114,12 @@ pub enum ConditionKind { /// GNU Make variable assignment operator represented by schema version 1. /// -/// `Define` represents a define block without an assignment token and serializes -/// as the schema's empty operator. +/// `Define` represents a definition carrying no assignment token and +/// serializes as the schema's empty operator. Two constructs use it: a +/// `define` block, and a bare `export NAME` directive naming a variable +/// assigned elsewhere. The `define_block` flag on [`VariableFact`] tells them +/// apart, so `operator == Define && !define_block` identifies an export +/// directive rather than an assignment. /// /// # Examples /// @@ -131,7 +135,8 @@ pub enum ConditionKind { /// ``` #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize)] pub enum AssignmentOperator { - /// Define block without an assignment token. + /// Definition without an assignment token: a `define` block or a bare + /// `export` directive. #[default] #[serde(rename = "")] Define, diff --git a/tests/cli_e2e.rs b/tests/cli_e2e.rs index 6608b22..c410411 100644 --- a/tests/cli_e2e.rs +++ b/tests/cli_e2e.rs @@ -39,6 +39,44 @@ fn complete_path_emits_one_json_document(mut makeutil_command: Command) { ); } +#[rstest] +fn bare_export_path_emits_export_facts(mut makeutil_command: Command) { + let output = makeutil_command + .args(["parse", "tests/fixtures/makefiles/bare-export-cli.mk"]) + .output() + .expect("binary should run"); + + assert_eq!(output.status.code(), Some(0)); + assert!(output.stderr.is_empty()); + let document: serde_json::Value = + serde_json::from_slice(&output.stdout).expect("stdout should be JSON"); + assert_eq!( + document + .get("schema_version") + .and_then(serde_json::Value::as_u64), + Some(1), + ); + let variables = document + .get("variables") + .and_then(serde_json::Value::as_array) + .expect("report should contain a variables array"); + assert_eq!(variables.len(), 4); + let export_names = variables + .iter() + .filter(|variable| { + variable.get("operator") == Some(&serde_json::Value::String(String::new())) + && variable.get("exported") == Some(&serde_json::Value::Bool(true)) + && variable.get("define_block") == Some(&serde_json::Value::Bool(false)) + && variable + .get("raw_value") + .and_then(serde_json::Value::as_str) + == Some("") + }) + .filter_map(|variable| variable.get("name").and_then(serde_json::Value::as_str)) + .collect::>(); + assert_eq!(export_names, ["FOO", "BAR"]); +} + #[rstest] fn recovered_path_exits_one_with_json(mut makeutil_command: Command) { let output = makeutil_command diff --git a/tests/corpus.rs b/tests/corpus.rs index 61573a2..6011e98 100644 --- a/tests/corpus.rs +++ b/tests/corpus.rs @@ -11,6 +11,40 @@ use makeutil::{adapters::MakefileLosslessParser, domain::ParseStatus, parse_sour use pretty_assertions::assert_eq; use rstest::rstest; +/// An `unexport` directive is not recognized as a directive at all: upstream +/// parses it as a rule whose first target is the word `unexport`. Schema +/// version 1 cannot express "this name was explicitly un-exported", so the +/// misleading rule is pinned here rather than corrected. If a future upstream +/// release learns the directive, this test fails and forces the pin and the +/// representation to be revisited together. +#[rstest] +fn unexport_directive_degrades_honestly() { + let report = parse_source( + include_bytes!("fixtures/makefiles/export-directive-limits.mk"), + "export-directive-limits.mk", + &MakefileLosslessParser, + ) + .expect("the export-directive-limits corpus fixture must parse into a report"); + + assert_eq!(report.parse.status, ParseStatus::Recovered); + assert!( + !report.parse.diagnostics.is_empty(), + "a recovered parse must carry at least one diagnostic", + ); + assert!( + report + .rules + .iter() + .any(|rule| rule.targets.first().map(String::as_str) == Some("unexport")), + "the misleading unexport rule must remain pinned: {:?}", + report + .rules + .iter() + .map(|rule| rule.targets.clone()) + .collect::>(), + ); +} + /// A bare `$(error ...)` directive inside a conditional must parse as /// recovered, with the /// surrounding facts retained. diff --git a/tests/export_directives.rs b/tests/export_directives.rs new file mode 100644 index 0000000..4325ad6 --- /dev/null +++ b/tests/export_directives.rs @@ -0,0 +1,361 @@ +//! Report-shape regressions for GNU Make's `export` directive family. +//! +//! A bare `export NAME` names a variable without assigning a value on that +//! line, so it carries no assignment operator. Schema version 1 has no +//! dedicated representation for such a directive, so these tests pin the +//! chosen one — an entry in `variables` with the empty operator — and pin the +//! never-abort guarantee for every form the parser cannot represent faithfully. + +use makefile_lossless::{Makefile, Parse}; +use makeutil::{ + ParseApplicationError, + adapters::MakefileLosslessParser, + domain::{AssignmentOperator, ParseReport, ParseStatus, SourceLocation, VariableFact}, + parse_source, +}; +use pretty_assertions::assert_eq; +use proptest::{prelude::*, test_runner::TestCaseError}; +use rstest::{fixture, rstest}; + +#[fixture] +fn bare_export_report() -> Result { + parse_source( + include_bytes!("fixtures/makefiles/bare-export.mk"), + "bare-export.mk", + &MakefileLosslessParser, + ) +} + +fn variable<'report>(report: &'report ParseReport, name: &str) -> Option<&'report VariableFact> { + report + .variables + .iter() + .find(|variable| variable.name == name) +} + +/// Find the directive fact for a name. +/// +/// A name may appear twice — once for its assignment and once for the `export` +/// directive that names it — so the empty operator, not the name, selects the +/// directive. +fn directive<'report>(report: &'report ParseReport, name: &str) -> Option<&'report VariableFact> { + report + .variables + .iter() + .find(|variable| variable.name == name && variable.operator == AssignmentOperator::Define) +} + +/// No `export` or `unexport` form may abort the parse, and none may go missing +/// from a report that still claims `complete`. +/// +/// The variable names are asserted alongside the status so that a form which +/// silently stopped producing its fact could not keep passing. A form the +/// parser cannot name yields no fact, and the adapter's own diagnostic — not +/// upstream's, which is not always present — keeps such a report `recovered`. +#[rstest] +#[case::single("export FOO\n", ParseStatus::Complete, vec!["FOO"], None)] +#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Complete, vec!["FOO", "BAR", "BAZ"], None)] +#[case::continued("export FOO \\\n\tBAR\n", ParseStatus::Complete, vec!["FOO", "BAR"], None)] +#[case::name_less("export\n", ParseStatus::Recovered, Vec::new(), Some((0, 7)))] +#[case::keyword_named_variable("export unexport\n", ParseStatus::Complete, vec!["unexport"], None)] +// Upstream cannot represent the first exported name when it matches one of +// its directive keywords. Retain every later name it can identify, but mark +// the omitted name with a located recovery diagnostic. +#[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Recovered, vec!["FOO"], Some((0, 18)))] +#[case::override_named_variable("export override FOO\n", ParseStatus::Recovered, vec!["FOO"], Some((0, 20)))] +#[case::unnameable("export export\n", ParseStatus::Recovered, Vec::new(), Some((0, 14)))] +// GNU Make actually rejects `override export FOO` with "missing separator". +// The parser accepts the single-name form, which predates this work and is +// left alone; only the multi-name form was in scope. Pinned as it behaves. +#[case::overridden("override export FOO\n", ParseStatus::Complete, vec!["FOO"], None)] +#[case::overridden_list("override export FOO BAR\n", ParseStatus::Recovered, vec!["FOO"], None)] +#[case::undiagnosed_upstream( + "override export override\n", + ParseStatus::Recovered, + Vec::new(), + None +)] +#[case::exported_define( + "export define FOO\nbody\nendef\n", + ParseStatus::Recovered, + Vec::new(), + None +)] +#[case::name_less_exported_define( + "export define\nbody\nendef\n", + ParseStatus::Recovered, + Vec::new(), + None +)] +#[case::assignment("export FOO := bar\n", ParseStatus::Complete, vec!["FOO"], None)] +#[case::unexport_single("unexport FOO\n", ParseStatus::Recovered, Vec::new(), None)] +#[case::unexport_multiple("unexport FOO BAR\n", ParseStatus::Recovered, Vec::new(), None)] +#[case::unexport_name_less("unexport\n", ParseStatus::Recovered, Vec::new(), None)] +fn no_export_form_aborts( + #[case] source: &str, + #[case] expected: ParseStatus, + #[case] expected_names: Vec<&str>, + #[case] expected_diagnostic_span: Option<(usize, usize)>, +) -> Result<(), Box> { + let report = parse_source(source.as_bytes(), "export.mk", &MakefileLosslessParser)?; + let names: Vec<&str> = report + .variables + .iter() + .map(|variable| variable.name.as_str()) + .collect(); + + assert_eq!((report.parse.status, names), (expected, expected_names)); + if expected == ParseStatus::Recovered && report.parse.diagnostics.is_empty() { + return Err("a recovered report must explain itself with a diagnostic".into()); + } + if let Some((start_byte, end_byte)) = expected_diagnostic_span { + let expected_location = SourceLocation { + start_byte, + end_byte, + start_line: 1, + start_column: 1, + end_line: 2, + end_column: 1, + }; + if !report + .parse + .diagnostics + .iter() + .any(|diagnostic| diagnostic.location == expected_location) + { + return Err("the recovered diagnostic must cover the complete directive line".into()); + } + } + Ok(()) +} + +/// A definition the parser cannot name and that is not an `export` is a broken +/// tree, not an unrepresentable construct, so it must still fail loudly rather +/// than being quietly dropped along with the export forms. +#[rstest] +fn a_name_less_definition_that_is_not_an_export_still_fails() { + let outcome = parse_source(b"define\n", "define.mk", &MakefileLosslessParser); + + assert_eq!( + outcome.err().map(|error| error.to_string()), + Some("required variable-name accessor was absent".to_owned()) + ); +} + +/// A variable may legitimately be called `unexport`. Deciding directive names +/// by keyword text would discard it while still reporting `complete`. +#[rstest] +fn a_variable_named_like_a_keyword_is_not_discarded() -> Result<(), Box> { + let report = parse_source(b"export unexport\n", "export.mk", &MakefileLosslessParser)?; + let directive = + variable(&report, "unexport").ok_or("the exported name must not be discarded")?; + + assert_eq!( + ( + directive.operator, + directive.exported, + directive.define_block + ), + (AssignmentOperator::Define, true, false), + ); + Ok(()) +} + +/// An `export define` block is dropped rather than invented, so no fact may +/// claim the keyword itself was exported. +#[rstest] +fn exported_define_block_invents_no_fact() -> Result<(), Box> { + let report = parse_source( + b"export define FOO\nbody\nendef\n", + "export.mk", + &MakefileLosslessParser, + )?; + let names: Vec<&str> = report + .variables + .iter() + .map(|variable| variable.name.as_str()) + .collect(); + + assert_eq!( + (report.parse.status, names), + (ParseStatus::Recovered, Vec::new()) + ); + if report.parse.diagnostics.is_empty() { + return Err("dropping the block must be explained by a diagnostic".into()); + } + Ok(()) +} + +/// Pins the consumer-facing discriminating predicate for bare export +/// directives: an empty `operator` with `define_block` false identifies a +/// directive rather than an assignment, and `raw_value` is empty. +#[rstest] +fn single_name_bare_export_is_complete( + bare_export_report: Result, +) -> Result<(), Box> { + let report = bare_export_report?; + + assert_eq!(report.parse.status, ParseStatus::Complete); + assert_eq!(report.parse.diagnostics, Vec::new()); + + for name in ["MOLD_VERSION_FILE", "RUST_TOOLCHAIN_FILE"] { + let directive = directive(&report, name) + .ok_or_else(|| format!("the report must contain a directive fact for {name}"))?; + assert_eq!( + ( + directive.exported, + directive.define_block, + directive.raw_value.as_str() + ), + (true, false, ""), + "{name} must be an exported, valueless directive rather than a define block", + ); + } + Ok(()) +} + +#[rstest] +fn assignments_and_directives_are_distinguishable( + bare_export_report: Result, +) -> Result<(), Box> { + let report = bare_export_report?; + let assignment = variable(&report, "CARGO_TERM_COLOR") + .ok_or("the report must contain the CARGO_TERM_COLOR assignment")?; + + assert_eq!( + ( + assignment.operator, + assignment.exported, + assignment.raw_value.as_str() + ), + (AssignmentOperator::Simple, true, "always"), + ); + Ok(()) +} + +#[rstest] +fn facts_after_a_bare_export_survive( + bare_export_report: Result, +) -> Result<(), Box> { + let report = bare_export_report?; + let targets: Vec<&str> = report + .rules + .iter() + .flat_map(|rule| rule.targets.iter().map(String::as_str)) + .collect(); + + if targets.contains(&"build") { + Ok(()) + } else { + Err(format!("facts after a bare export must survive: {targets:?}").into()) + } +} + +/// The real-world shape: one directive exporting several already-assigned +/// variables, which must now report every name and a complete parse. +#[rstest] +fn multi_name_export_is_complete() -> Result<(), Box> { + let report = parse_source( + include_bytes!("fixtures/makefiles/export-directive-list.mk"), + "export-directive-list.mk", + &MakefileLosslessParser, + )?; + + assert_eq!(report.parse.status, ParseStatus::Complete); + assert_eq!(report.parse.diagnostics, Vec::new()); + + let expected_span = (120, 186); + for name in [ + "MOLD_VERSION_FILE", + "MOLD_SHA256SUMS_FILE", + "RUST_TOOLCHAIN_FILE", + ] { + let fact = directive(&report, name) + .ok_or_else(|| format!("the report must contain a directive fact for {name}"))?; + assert_eq!( + ( + fact.exported, + fact.overridden, + fact.define_block, + fact.raw_value.as_str() + ), + (true, false, false, ""), + "{name} must be an exported, valueless directive", + ); + assert_eq!( + (fact.location.start_byte, fact.location.end_byte), + expected_span, + "{name} must use the directive line's full span", + ); + } + Ok(()) +} + +proptest! { + /// Every valid name on a bare export becomes one ordered directive fact. + #[test] + fn multi_name_bare_export_preserves_facts_and_source( + names in proptest::collection::vec("[A-Z][A-Z0-9_]{0,12}", 1..12), + ) { + let source = format!("export {}\n", names.join(" ")); + let parsed = Parse::::parse_makefile(&source); + prop_assert_eq!(parsed.tree().to_string(), source.as_str()); + + let report = parse_source( + source.as_bytes(), + "generated-export.mk", + &MakefileLosslessParser, + ) + .map_err(|error| TestCaseError::fail(error.to_string()))?; + let facts = report + .variables + .iter() + .filter(|variable| variable.operator == AssignmentOperator::Define) + .collect::>(); + let observed_names = facts + .iter() + .map(|variable| variable.name.clone()) + .collect::>(); + let expected_span = (0, source.len()); + + prop_assert_eq!(report.parse.status, ParseStatus::Complete); + prop_assert!(report.parse.diagnostics.is_empty()); + prop_assert_eq!(facts.len(), names.len()); + prop_assert_eq!(observed_names.as_slice(), names.as_slice()); + for fact in facts { + prop_assert_eq!(fact.raw_value.as_str(), ""); + prop_assert!(fact.exported); + prop_assert!(!fact.overridden); + prop_assert!(!fact.define_block); + prop_assert_eq!( + (fact.location.start_byte, fact.location.end_byte), + expected_span, + ); + } + } +} + +#[rstest] +fn name_less_export_degrades_to_a_diagnostic() -> Result<(), Box> { + let report = parse_source( + include_bytes!("fixtures/makefiles/export-directive-limits.mk"), + "export-directive-limits.mk", + &MakefileLosslessParser, + )?; + + assert_eq!(report.parse.status, ParseStatus::Recovered); + if report.parse.diagnostics.is_empty() { + return Err("a recovered parse must carry at least one diagnostic".into()); + } + let names: Vec<&str> = report + .variables + .iter() + .map(|variable| variable.name.as_str()) + .collect(); + assert_eq!( + names, + vec!["FOO"], + "no variable fact may be invented for a name-less export", + ); + Ok(()) +} diff --git a/tests/features/parse.feature b/tests/features/parse.feature index b3f16fc..2cc8f9e 100644 --- a/tests/features/parse.feature +++ b/tests/features/parse.feature @@ -7,6 +7,13 @@ Feature: Parse one GNU Makefile into JSON facts And the process exits with code 0 And stderr is empty + Scenario: Parse a Makefile that exports already-defined variables + Given a Makefile fixture that exports already-defined variables + When makeutil parses the fixture by path + Then stdout contains one schema version 1 JSON document + And the process exits with code 0 + And stderr is empty + Scenario: Parse complete source from standard input Given complete GNU Makefile source on standard input When makeutil parses dash with stdin filename Makefile diff --git a/tests/fixtures/makefiles/bare-export-cli.mk b/tests/fixtures/makefiles/bare-export-cli.mk new file mode 100644 index 0000000..c5a7b7b --- /dev/null +++ b/tests/fixtures/makefiles/bare-export-cli.mk @@ -0,0 +1,3 @@ +FOO := one +BAR := two +export FOO BAR diff --git a/tests/fixtures/makefiles/bare-export.mk b/tests/fixtures/makefiles/bare-export.mk new file mode 100644 index 0000000..466bc6d --- /dev/null +++ b/tests/fixtures/makefiles/bare-export.mk @@ -0,0 +1,9 @@ +MOLD_VERSION_FILE := .mold-version +RUST_TOOLCHAIN_FILE := rust-toolchain.toml + +export MOLD_VERSION_FILE +export RUST_TOOLCHAIN_FILE +export CARGO_TERM_COLOR := always + +build: + @echo building diff --git a/tests/fixtures/makefiles/export-directive-limits.mk b/tests/fixtures/makefiles/export-directive-limits.mk new file mode 100644 index 0000000..851464e --- /dev/null +++ b/tests/fixtures/makefiles/export-directive-limits.mk @@ -0,0 +1,3 @@ +FOO := 1 +export +unexport FOO diff --git a/tests/fixtures/makefiles/export-directive-list.mk b/tests/fixtures/makefiles/export-directive-list.mk new file mode 100644 index 0000000..9ee6916 --- /dev/null +++ b/tests/fixtures/makefiles/export-directive-list.mk @@ -0,0 +1,8 @@ +MOLD_VERSION_FILE := .mold-version +MOLD_SHA256SUMS_FILE := .mold-sha256sums +RUST_TOOLCHAIN_FILE := rust-toolchain.toml + +export MOLD_VERSION_FILE MOLD_SHA256SUMS_FILE RUST_TOOLCHAIN_FILE + +check: + @echo checking diff --git a/tests/parse_bdd.rs b/tests/parse_bdd.rs index cf9d78b..c97e9df 100644 --- a/tests/parse_bdd.rs +++ b/tests/parse_bdd.rs @@ -39,6 +39,15 @@ fn complete_fixture(world: &mut World) { ]; } +#[given("a Makefile fixture that exports already-defined variables")] +fn bare_export_fixture(world: &mut World) { + world.arguments = vec![ + "makeutil".to_owned(), + "parse".to_owned(), + "tests/fixtures/makefiles/bare-export.mk".to_owned(), + ]; +} + #[given("complete GNU Makefile source on standard input")] fn complete_stdin(world: &mut World) { world.stdin = b"all:\n\techo ok\n".to_vec(); } @@ -150,6 +159,11 @@ fn run_world(world: &mut World) { "fixtures/makefiles/all-facts.mk" )))); } + if path == Utf8Path::new("tests/fixtures/makefiles/bare-export.mk") { + return Ok(Box::new(Cursor::new(include_bytes!( + "fixtures/makefiles/bare-export.mk" + )))); + } Err(std::io::Error::new( std::io::ErrorKind::NotFound, "fixture is absent", @@ -172,6 +186,12 @@ fn run_world(world: &mut World) { )] fn parse_path(_world: World) {} +#[scenario( + path = "tests/features/parse.feature", + name = "Parse a Makefile that exports already-defined variables" +)] +fn parse_bare_export(_world: World) {} + #[scenario( path = "tests/features/parse.feature", name = "Parse complete source from standard input" diff --git a/tests/report_schema.rs b/tests/report_schema.rs index 7725540..bc1cab7 100644 --- a/tests/report_schema.rs +++ b/tests/report_schema.rs @@ -46,6 +46,15 @@ fn all_facts_report() -> Result { include_bytes!("fixtures/makefiles/conditional-error-directive.mk"), "conditional-error-directive.mk" )] +#[case(include_bytes!("fixtures/makefiles/bare-export.mk"), "bare-export.mk")] +#[case( + include_bytes!("fixtures/makefiles/export-directive-list.mk"), + "export-directive-list.mk" +)] +#[case( + include_bytes!("fixtures/makefiles/export-directive-limits.mk"), + "export-directive-limits.mk" +)] fn reports_validate_against_schema( #[case] source: &[u8], #[case] path: &str,