From f33ddacc564ea0fc80f7795c70bc76167fe9f7d7 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 11:31:06 +0100 Subject: [PATCH 1/9] Draft an ExecPlan for bare export directive parsing `makeutil parse` aborts with `parse-internal: required variable-assignment-operator accessor was absent` on any Makefile containing a bare `export` directive (`export FOO BAR`), destroying the report for the whole file. Real-world trigger: netsuke's `Makefile:35`, which blinds every downstream concordat rule package for that repository. The plan represents each exported name as a `variables` entry with the empty operator already in the version 1 schema (discriminated by `operator == "" && !define_block`), keeping `schema_version: 1` and `status: "complete"`; guarantees `parse-internal` can never abort a parse again (recovered diagnostics instead); and splits the makeutil-only fix from the upstream makefile-lossless change needed to carry multi-name export lists. `unexport` fidelity and target-specific exports are documented deferrals. Plan only; no implementation accompanies it. --- docs/execplans/bare-export-directives.md | 1059 ++++++++++++++++++++++ 1 file changed, 1059 insertions(+) create mode 100644 docs/execplans/bare-export-directives.md diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md new file mode 100644 index 0000000..0cf0ee8 --- /dev/null +++ b/docs/execplans/bare-export-directives.md @@ -0,0 +1,1059 @@ +# 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: DRAFT + +## 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 five entries: the two assignments and the three +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 your 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: `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 + +- [ ] Stage A: orientation and reproduction confirmed on the current working + tree (no code changes). +- [ ] Stage B: red tests and fixtures added; each fails for the expected reason. +- [ ] Stage C: minimal makeutil change so no `export` or `unexport` form aborts. +- [ ] Stage D: refactor, documentation, snapshots, and full commit gates. +- [ ] Stage E: upstream parser fix for multi-name `export`; revision bump; tests + promoted from `recovered` to `complete`. +- [ ] Stage F: `unexport` behaviour pinned by regression test and documented as + a known limitation. +- [ ] Stage G: consumer-facing note recording the new revision to pin. + +## 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: 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. + +## Outcomes & retrospective + +To be completed at the end of Stage G. At minimum, record: whether a Makefile +containing `export A B C` now parses to `complete` with all three names present; +whether any snapshot changed unexpectedly when the upstream revision was bumped; +and whether the `variables`-reuse representation caused confusion in review. + +## 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 yourself 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 on your own working tree before changing anything, so you +know the baseline is what this plan describes. 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 you cannot explain — 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 `multi_name_export_never_aborts`, + `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 + +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. From a66cbf5f8def1bb37b210c93785769ed92d4f0bd Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 12:20:11 +0100 Subject: [PATCH 2/9] Parse bare export directives without aborting A bare `export FOO` line names a variable assigned elsewhere, so it carries no assignment operator. The adapter treated an absent operator as a broken tree and returned MissingField, which propagated out as a fatal `parse-internal` message and exit code 2. Because the traversal collects facts before diagnostics, a single such line destroyed the report for the whole file: every rule, variable, and include was lost, and downstream consumers recorded an operational error instead of facts with one gap. Accept an absent operator when the line carries `export` as well as when it carries `define`, and represent each exported name as a variable fact with the schema's existing empty operator, an empty value, `exported` true, and `define_block` false. A name-less `export` exports everything, which schema version 1 cannot express, so it now yields no fact at all and upstream's own diagnostic drives the recovered status rather than an invented variable. An operator-less line that is neither a define nor an export still fails loudly, so genuinely broken trees are not masked. One `export A B C` node must yield one fact per name, so `variable_observation` returns a collection rather than a single observation. The operator mapping, directive-name walk, and directive expansion move into `adapters::makefile_export`, keeping both files well inside the 400-line limit. The schema is untouched and `schema_version` remains 1: the empty operator was already in the enum for define blocks, so no consumer validating against version 1 sees a new key or a new enum member. The multi-name form still reports `recovered` and captures only the first name, because the pinned parser drops the remaining names before the adapter sees them. Fixing that needs a change in the parser fork. --- docs/contents.md | 2 + docs/design.md | 31 +- docs/execplans/bare-export-directives.md | 477 +++++++++++------- docs/users-guide.md | 25 + src/adapters/makefile.rs | 61 +-- src/adapters/makefile_export.rs | 107 ++++ src/adapters/makefile_export_tests.rs | 86 ++++ src/adapters/makefile_tests.rs | 36 +- src/adapters/mod.rs | 1 + tests/export_directives.rs | 139 +++++ tests/features/parse.feature | 7 + tests/fixtures/makefiles/bare-export.mk | 9 + .../makefiles/export-directive-limits.mk | 3 + .../makefiles/export-directive-list.mk | 8 + tests/parse_bdd.rs | 20 + tests/report_schema.rs | 9 + 16 files changed, 758 insertions(+), 263 deletions(-) create mode 100644 src/adapters/makefile_export.rs create mode 100644 src/adapters/makefile_export_tests.rs create mode 100644 tests/export_directives.rs create mode 100644 tests/fixtures/makefiles/bare-export.mk create mode 100644 tests/fixtures/makefiles/export-directive-limits.mk create mode 100644 tests/fixtures/makefiles/export-directive-list.mk diff --git a/docs/contents.md b/docs/contents.md index 33316e0..5f9ac33 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -31,6 +31,8 @@ set. 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..cb8a6e0 100644 --- a/docs/design.md +++ b/docs/design.md @@ -314,9 +314,34 @@ 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; a directive naming several variables yields one entry per name. The +discriminating predicate for consumers is +`operator == "" && define_block == false`. + +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 +[the bare export execution plan](execplans/bare-export-directives.md). + +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.7. Include facts diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index 0cf0ee8..1b3595c 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -1,11 +1,10 @@ # 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. +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: DRAFT +Status: IN PROGRESS ## Purpose / big picture @@ -63,13 +62,13 @@ around it. - `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 + 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. + `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 @@ -87,8 +86,8 @@ around it. - 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. + 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. @@ -98,8 +97,8 @@ around it. - 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 + `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. @@ -118,43 +117,41 @@ around it. - 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. + 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. + 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. + 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: `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. + 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 -- [ ] Stage A: orientation and reproduction confirmed on the current working - tree (no code changes). -- [ ] Stage B: red tests and fixtures added; each fails for the expected reason. -- [ ] Stage C: minimal makeutil change so no `export` or `unexport` form aborts. -- [ ] Stage D: refactor, documentation, snapshots, and full commit gates. -- [ ] Stage E: upstream parser fix for multi-name `export`; revision bump; tests - promoted from `recovered` to `complete`. +- [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. +- [ ] Stage E: BLOCKED. The upstream parser fix lives in a separate repository + that this work is not authorized to push to. See the `Decision Log`. - [ ] Stage F: `unexport` behaviour pinned by regression test and documented as a known limitation. - [ ] Stage G: consumer-facing note recording the new revision to pin. @@ -169,115 +166,173 @@ Recorded during investigation, before implementation began. `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. + `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. + 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 + 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. + `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: 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. + 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 + 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. + 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`. + `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. + 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. + 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. + 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. + +- 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. + +- BLOCKED: 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 To be completed at the end of Stage G. At minimum, record: whether a Makefile -containing `export A B C` now parses to `complete` with all three names present; -whether any snapshot changed unexpectedly when the upstream revision was bumped; -and whether the `variables`-reuse representation caused confusion in review. +containing `export A B C` now parses to `complete` with all three names +present; whether any snapshot changed unexpectedly when the upstream revision +was bumped; and whether the `variables`-reuse representation caused confusion +in review. ## Context and orientation @@ -323,8 +378,8 @@ such as `VARIABLE`, `RULE`, `EXPR`, `ERROR`) and *tokens* (leaves such as `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 +`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 @@ -351,9 +406,9 @@ fn 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 +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 @@ -373,12 +428,12 @@ line makes `VariableDefinition::name()` return `None`, so ### Verified current behaviour of every export form The following were measured against the current default-branch build. Reproduce -them yourself in Stage A. "Exit 2" means the fatal `parse-internal` path; exit 1 -means a `recovered` report was still printed; exit 0 means `complete`. +them yourself 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` — 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. @@ -404,8 +459,8 @@ means a `recovered` report was still printed; exit 0 means `complete`. `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. + 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`. @@ -420,8 +475,9 @@ means a `recovered` report was still printed; exit 0 means `complete`. 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/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. @@ -443,10 +499,11 @@ 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. +`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 @@ -455,23 +512,23 @@ targets. Prose must use en-GB Oxford spelling (`organize`, `standardize`, Confirm the defect on your own working tree before changing anything, so you know the baseline is what this plan describes. 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. +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 +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`: +`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 @@ -485,9 +542,9 @@ 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: +`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 @@ -532,8 +589,8 @@ state here. Add: 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. +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. @@ -588,8 +645,8 @@ 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 +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`: @@ -617,10 +674,10 @@ 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 +`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 @@ -643,10 +700,10 @@ 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 +`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 @@ -673,19 +730,19 @@ 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 +`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 +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 you cannot explain — an unexplained diff in +Refresh the `insta` snapshots if and only if a snapshot legitimately changed. +Do not accept a snapshot change you cannot explain — 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. @@ -748,17 +805,17 @@ fixtures, stop and escalate rather than accepting the snapshot. 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. +`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. +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 @@ -825,8 +882,8 @@ 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. +The last command must print a JSON document containing `"status":"complete"` +and exit 0. Stage D, gates, run sequentially: @@ -865,9 +922,10 @@ 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". + `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. @@ -914,8 +972,8 @@ 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 +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 @@ -930,9 +988,46 @@ nothing in the repository depends on them. ## Artefacts and notes -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. +### 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. + +### 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: @@ -965,8 +1060,8 @@ ROOT@0..19 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: +Name-less export — upstream already diagnoses it, so makeutil only needs to +stop aborting: ```plaintext ROOT@0..7 diff --git a/docs/users-guide.md b/docs/users-guide.md index ce9ccec..cc4700a 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -56,3 +56,28 @@ 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 assigned elsewhere, so it +carries no assignment operator and no value. 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 directive +naming several variables yields one entry per name. + +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`. + +### `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/src/adapters/makefile.rs b/src/adapters/makefile.rs index 744d1c8..f40d62d 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,40 @@ 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)?; + + 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..4d24a7b --- /dev/null +++ b/src/adapters/makefile_export.rs @@ -0,0 +1,107 @@ +//! 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. +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, so it yields no facts and upstream's own diagnostic drives the +/// recovered status. +pub(super) fn export_directive_observations( + variable: &VariableDefinition, + conditions: &[ConditionObservation], + definition_span: SourceSpan, +) -> Vec { + 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() +} + +/// Keywords that may precede the names on a directive line. +const DIRECTIVE_KEYWORDS: [&str; 4] = ["export", "unexport", "override", "define"]; + +/// 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. +pub(super) fn directive_names(variable: &VariableDefinition) -> Vec { + variable + .syntax() + .children_with_tokens() + .filter_map(rowan::NodeOrToken::into_token) + .filter(|token| token.kind() == SyntaxKind::IDENTIFIER) + .map(|token| token.text().to_owned()) + .filter(|text| !DIRECTIVE_KEYWORDS.contains(&text.as_str())) + .collect() +} + +#[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..dc2bc45 --- /dev/null +++ b/src/adapters/makefile_export_tests.rs @@ -0,0 +1,86 @@ +//! Unit tests for operator mapping and `export` directive name collection. + +use makefile_lossless::{Makefile, MakefileItem, Parse}; +use pretty_assertions::assert_eq; +use rstest::rstest; + +use super::{OperatorContext, assignment_operator, directive_names}; +use crate::{domain::AssignmentOperator, ports::ParserPortError}; + +/// Build a context from the modifiers a definition line carries. +fn context(is_define: bool, is_export: bool) -> OperatorContext { + OperatorContext { + is_define, + is_export, + } +} + +#[rstest] +fn define_without_operator_uses_empty_schema_variant() { + assert_eq!( + assignment_operator(None, context(true, false)), + Ok(AssignmentOperator::Define) + ); +} + +#[rstest] +fn bare_export_directive_uses_empty_schema_variant() { + assert_eq!( + assignment_operator(None, context(false, true)), + Ok(AssignmentOperator::Define) + ); +} + +#[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); +} + +/// Upstream keeps only the first name of a multi-name `export` inside the +/// variable node, so the helper reports one name until the pinned parser +/// revision learns the directive list form. +#[rstest] +#[case("export FOO\n", vec!["FOO"])] +#[case("export FOO BAR BAZ\n", vec!["FOO"])] +#[case("export\n", Vec::new())] +fn bare_export_names_are_all_collected(#[case] source: &str, #[case] expected: Vec<&str>) { + let parsed = Parse::::parse_makefile(source); + let variable = parsed + .tree() + .items() + .find_map(|item| match item { + MakefileItem::Variable(variable) => Some(variable), + _ => None, + }) + .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/tests/export_directives.rs b/tests/export_directives.rs new file mode 100644 index 0000000..8b19a71 --- /dev/null +++ b/tests/export_directives.rs @@ -0,0 +1,139 @@ +//! Report-shape regressions for GNU Make's `export` directive family. +//! +//! A bare `export NAME` names a variable assigned elsewhere, 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 makeutil::{ + ParseApplicationError, + adapters::MakefileLosslessParser, + domain::{AssignmentOperator, ParseReport, ParseStatus, VariableFact}, + parse_source, +}; +use pretty_assertions::assert_eq; +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) +} + +/// 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 = report + .variables + .iter() + .find(|variable| { + variable.name == name && variable.operator == AssignmentOperator::Define + }) + .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()) + } +} + +#[rstest] +fn multi_name_export_never_aborts() -> 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::Recovered); + Ok(()) +} + +#[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.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, From f6feae1cee5c2120fd4495cb793c1c2cf5981238 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 12:33:41 +0100 Subject: [PATCH 3/9] Stop discarding export names that look like keywords MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage review of the bare-export change found two ways it still broke the honesty and never-abort guarantees it was written to provide. `directive_names` decided which identifiers were directive keywords by matching token text against `export`, `unexport`, `override` and `define`. A variable may legitimately be called `unexport`, and upstream parses `export unexport` cleanly, so the name was dropped while the report still claimed `complete` — silently discarding a construct, which the JSON contract forbids. Anchor the walk on the name upstream itself reports instead: the identifiers preceding it are exactly the prefix keywords the parser consumed, which is also correct for `export export FOO`, where upstream consumes both leading keywords. An absent name means upstream found none and said so with a diagnostic, so the line names nothing. `export define FOO ... endef` is valid GNU Make, and upstream models it with no name at all. The no-facts path was gated on "export and not define", so this form still aborted with exit code 2. Gate it on the absent name alone: upstream diagnoses both `export define` forms, so dropping the facts leaves the report honestly recovered. A name-less definition that is not an export still fails loudly. Both behaviours are now pinned across every export and unexport form, including the keyword-named and repeated-prefix cases that motivated the change. Record the representation in ADR-0002 rather than leaving the rationale only in the execution plan, and correct the users' guide and design document, which claimed multi-name directives yield one entry per name. They do not yet: the pinned parser keeps only the first name, so such a line reports `recovered` with one entry until the parser learns the directive list form. --- ...02-bare-export-directive-representation.md | 123 ++++++++++++++++++ docs/contents.md | 2 + docs/design.md | 21 ++- docs/execplans/bare-export-directives.md | 47 +++++++ docs/users-guide.md | 17 ++- src/adapters/makefile.rs | 10 ++ src/adapters/makefile_export.rs | 28 +++- src/adapters/makefile_export_tests.rs | 110 +++++++++++++--- src/domain/mod.rs | 11 +- tests/export_directives.rs | 86 +++++++++++- 10 files changed, 418 insertions(+), 37 deletions(-) create mode 100644 docs/adrs/0002-bare-export-directive-representation.md 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..3022a40 --- /dev/null +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -0,0 +1,123 @@ +# ADR-0002: Represent bare `export` directives as valueless variable facts + +## Status + +Accepted on 2026-08-13 + +## Context + +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. + +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 form the parser cannot name — a bare `export` with no names, which means +"export every variable", or `export define NAME` — yields no entry rather than +an invented one. The parser already diagnoses these, so the report degrades to +`recovered` rather than falsely claiming `complete`. + +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 `export` or `unexport`, so keyword-text filtering would silently +discard real facts. + +`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. + +### 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 the + `operator` field alone. +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 5f9ac33..e00c96f 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -27,6 +27,8 @@ 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) diff --git a/docs/design.md b/docs/design.md index cb8a6e0..2534418 100644 --- a/docs/design.md +++ b/docs/design.md @@ -324,10 +324,22 @@ effective value or precedence. 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; a directive naming several variables yields one entry per name. The -discriminating predicate for consumers is +false. The discriminating predicate for consumers is `operator == "" && define_block == false`. +A directive naming several variables is intended to yield one entry per name, +but the pinned parser revision keeps only the first name inside the definition +node and discards the rest, so such a line currently yields one entry and a +`recovered` status. Completing it needs a parser change, not a `makeutil` one. +A form the parser cannot name at all — a bare `export`, or +`export define NAME` — yields no entry rather than an invented one, and the +upstream diagnostic keeps the report `recovered` rather than falsely `complete`. + +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 +`export` or `unexport`. + 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 @@ -336,7 +348,10 @@ 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 -[the bare export execution plan](execplans/bare-export-directives.md). +[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 diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index 1b3595c..529af4f 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -209,6 +209,18 @@ Recorded during investigation, before implementation began. 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: 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 @@ -288,6 +300,41 @@ Recorded during investigation, before implementation began. 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 (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 diff --git a/docs/users-guide.md b/docs/users-guide.md index cc4700a..b83d1e5 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -65,14 +65,27 @@ carries no assignment operator and no value. Such a directive appears in the 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 directive -naming several variables yields one entry per name. +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`, is not yet fully +modelled: the pinned parser captures only the first name and drops the rest +before `makeutil` sees them, so the report contains one entry and its status is +`recovered` rather than `complete`. Full support awaits a parser revision that +keeps every name. Until then, treat a `recovered` status on such a line as a +gap in the facts rather than as evidence about the Makefile. 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, and the report is `recovered`. The same holds for +`export define NAME`, which the pinned parser does not name. + ### `unexport` is not yet supported An `unexport` directive is currently reported as a rule whose first target is diff --git a/src/adapters/makefile.rs b/src/adapters/makefile.rs index f40d62d..c1af4e1 100644 --- a/src/adapters/makefile.rs +++ b/src/adapters/makefile.rs @@ -186,6 +186,16 @@ fn variable_observation( }; 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, and `export define FOO` is + // modelled with no name at all. Upstream diagnoses both, so yielding no + // facts leaves the report honestly recovered instead of aborting the whole + // file. 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::new()); + } + if operator.is_none() && context.is_directive_only() { return Ok(export_directive_observations( variable, diff --git a/src/adapters/makefile_export.rs b/src/adapters/makefile_export.rs index 4d24a7b..150d2e9 100644 --- a/src/adapters/makefile_export.rs +++ b/src/adapters/makefile_export.rs @@ -36,6 +36,12 @@ impl OperatorContext { /// 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 defensive: a directive-only +/// export never reaches here because [`export_directive_observations`] handles +/// it and supplies the operator itself. It is retained so that an upstream +/// change routing some other operator-less export down this path cannot +/// resurrect the fatal abort this module exists to remove. pub(super) fn assignment_operator( operator: Option<&str>, context: OperatorContext, @@ -83,22 +89,30 @@ pub(super) fn export_directive_observations( .collect() } -/// Keywords that may precede the names on a directive line. -const DIRECTIVE_KEYWORDS: [&str; 4] = ["export", "unexport", "override", "define"]; - /// 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. +/// 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` or +/// `export`. Filtering by keyword text instead would silently discard +/// `export unexport`, which upstream parses cleanly as exporting a variable +/// named `unexport`. An absent name means upstream could not find one and has +/// said so with a diagnostic, so the line names nothing. pub(super) fn directive_names(variable: &VariableDefinition) -> Vec { + let Some(first_name) = variable.name() else { + return Vec::new(); + }; variable .syntax() .children_with_tokens() .filter_map(rowan::NodeOrToken::into_token) .filter(|token| token.kind() == SyntaxKind::IDENTIFIER) .map(|token| token.text().to_owned()) - .filter(|text| !DIRECTIVE_KEYWORDS.contains(&text.as_str())) + .skip_while(|text| *text != first_name) .collect() } diff --git a/src/adapters/makefile_export_tests.rs b/src/adapters/makefile_export_tests.rs index dc2bc45..0963472 100644 --- a/src/adapters/makefile_export_tests.rs +++ b/src/adapters/makefile_export_tests.rs @@ -1,11 +1,18 @@ //! Unit tests for operator mapping and `export` directive name collection. -use makefile_lossless::{Makefile, MakefileItem, Parse}; +use makefile_lossless::{Makefile, MakefileItem, Parse, VariableDefinition}; use pretty_assertions::assert_eq; use rstest::rstest; -use super::{OperatorContext, assignment_operator, directive_names}; -use crate::{domain::AssignmentOperator, ports::ParserPortError}; +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 { @@ -15,6 +22,18 @@ fn context(is_define: bool, is_export: bool) -> OperatorContext { } } +/// 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!( @@ -23,11 +42,69 @@ fn define_without_operator_uses_empty_schema_variant() { ); } +/// 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!( - assignment_operator(None, context(false, true)), - Ok(AssignmentOperator::Define) + 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) ); } @@ -67,19 +144,20 @@ fn directive_only_lines_are_recognized( /// Upstream keeps only the first name of a multi-name `export` inside the /// variable node, so the helper reports one name until the pinned parser /// revision learns the directive list form. +/// +/// 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("export FOO\n", vec!["FOO"])] -#[case("export FOO BAR BAZ\n", vec!["FOO"])] -#[case("export\n", Vec::new())] +#[case::single("export FOO\n", vec!["FOO"])] +#[case::multiple_names_lost_upstream("export FOO BAR BAZ\n", vec!["FOO"])] +#[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 parsed = Parse::::parse_makefile(source); - let variable = parsed - .tree() - .items() - .find_map(|item| match item { - MakefileItem::Variable(variable) => Some(variable), - _ => None, - }) + 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/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/export_directives.rs b/tests/export_directives.rs index 8b19a71..da61858 100644 --- a/tests/export_directives.rs +++ b/tests/export_directives.rs @@ -31,6 +31,85 @@ fn variable<'report>(report: &'report ParseReport, name: &str) -> Option<&'repor .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. +/// +/// A form upstream cannot name is dropped rather than invented, so the status +/// must be `recovered`: reporting `complete` while discarding a construct +/// would tell a consumer the facts are trustworthy when they are incomplete. +#[rstest] +#[case::single("export FOO\n", ParseStatus::Complete)] +#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Recovered)] +#[case::name_less("export\n", ParseStatus::Recovered)] +#[case::keyword_named_variable("export unexport\n", ParseStatus::Complete)] +#[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Complete)] +#[case::unnameable("export export\n", ParseStatus::Recovered)] +#[case::overridden("override export FOO\n", ParseStatus::Complete)] +#[case::exported_define("export define FOO\nbody\nendef\n", ParseStatus::Recovered)] +#[case::name_less_exported_define("export define\nbody\nendef\n", ParseStatus::Recovered)] +#[case::assignment("export FOO := bar\n", ParseStatus::Complete)] +#[case::unexport_single("unexport FOO\n", ParseStatus::Recovered)] +#[case::unexport_multiple("unexport FOO BAR\n", ParseStatus::Recovered)] +#[case::unexport_name_less("unexport\n", ParseStatus::Recovered)] +fn no_export_form_aborts( + #[case] source: &str, + #[case] expected: ParseStatus, +) -> Result<(), Box> { + let report = parse_source(source.as_bytes(), "export.mk", &MakefileLosslessParser)?; + + assert_eq!(report.parse.status, expected); + Ok(()) +} + +/// 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!(names, Vec::<&str>::new()); + 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. @@ -44,12 +123,7 @@ fn single_name_bare_export_is_complete( assert_eq!(report.parse.diagnostics, Vec::new()); for name in ["MOLD_VERSION_FILE", "RUST_TOOLCHAIN_FILE"] { - let directive = report - .variables - .iter() - .find(|variable| { - variable.name == name && variable.operator == AssignmentOperator::Define - }) + let directive = directive(&report, name) .ok_or_else(|| format!("the report must contain a directive fact for {name}"))?; assert_eq!( ( From e07a4dd8968afd91ec04dcc6b84409d60200b57a Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 12:35:22 +0100 Subject: [PATCH 4/9] Pin the unexport limitation and note the consumer impact MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unexport` is not recognized as a directive at all: upstream parses it as a rule whose first target is the word `unexport`. That never aborts, so it did not block the export fix, but the facts it produces are actively misleading — a consumer sees a rule that does not exist. Schema version 1 has no way to say "this name was explicitly un-exported", so the current behaviour is pinned in the unsupported-syntax corpus rather than corrected. If a future upstream release learns the directive, the test fails on purpose and forces the pin and the representation to be revisited together, which is exactly the policy that file documents. Add a note for downstream repositories, which consume `makeutil` by pinning a commit SHA and so only see this work when they move their pin. It records that bare exports no longer abort, that reports may now carry more `variables` entries and may name a variable twice, that `unexport` remains unsupported, that a multi-name export still reports recovered, and that the schema is unchanged so no re-validation is needed. Mark the execution plan blocked rather than complete. Every stage that does not require changing the parser fork is delivered; the multi-name case still reports recovered with the first name only, and the tests that would prove it fixed are written and need only their expectations flipped. --- docs/design.md | 21 +++++++ docs/execplans/bare-export-directives.md | 74 +++++++++++++++++++++--- tests/corpus.rs | 34 +++++++++++ 3 files changed, 120 insertions(+), 9 deletions(-) diff --git a/docs/design.md b/docs/design.md index 2534418..733d06b 100644 --- a/docs/design.md +++ b/docs/design.md @@ -358,6 +358,27 @@ 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. Filter on a non-empty `operator` to recover the previous + "assignments only" view. +- `unexport` remains unsupported and still reports a misleading rule with a + `recovered` status. +- A multi-name `export A B C` no longer aborts but still reports `recovered` + with only the first name, pending a parser change. +- `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 ```json diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index 529af4f..a4618fc 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -4,7 +4,10 @@ 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: IN PROGRESS +Status: BLOCKED — Stages A to D, F and G are complete and merged into this +branch. Stage E cannot proceed under the delegated authority for this work +because it changes a separate repository; see the `Decision Log`. Everything +achievable without that change has been delivered. ## Purpose / big picture @@ -152,9 +155,12 @@ around it. - [x] Stage D: refactor, documentation, snapshots, and full commit gates. - [ ] Stage E: BLOCKED. The upstream parser fix lives in a separate repository that this work is not authorized to push to. See the `Decision Log`. -- [ ] Stage F: `unexport` behaviour pinned by regression test and documented as - a known limitation. -- [ ] Stage G: consumer-facing note recording the new revision to pin. +- [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`. It + records the behaviour change rather than a new parser revision to pin, + because Stage E did not run. ## Surprises & discoveries @@ -375,11 +381,37 @@ Recorded during investigation, before implementation began. ## Outcomes & retrospective -To be completed at the end of Stage G. At minimum, record: whether a Makefile -containing `export A B C` now parses to `complete` with all three names -present; whether any snapshot changed unexpectedly when the upstream revision -was bumped; and whether the `variables`-reuse representation caused confusion -in review. +Recorded at the end of Stage G, with Stage E blocked. + +Does `export A B C` parse to `complete` with all three names? No. It parses +without aborting and reports `recovered` with the first name only. The pinned +parser traps the second name in an error node and pushes the third out of the +variable node entirely, so `makeutil` cannot recover them. Closing this needs +Stage E, which is blocked on authority to change a separate repository. The +tests that would prove it are already written and merely need their +expectations flipped. + +Did any snapshot change unexpectedly? No snapshot changed at all. The upstream +revision was never bumped, and both `insta` snapshots in `tests/snapshots/` are +byte-identical to their pre-change state, as is +`schemas/makeutil.parse.v1.schema.json`. + +Did the `variables`-reuse representation cause confusion in review? Not the +representation itself, which review accepted. What review did catch were two +defects in how the directive was recognized, both recorded in the +`Decision Log`: identifying directive keywords by token text silently discarded +`export unexport`, and gating the no-facts path on "export and not define" left +`export define FOO` aborting with exit code 2. Both breached the plan's own +constraints while every gate passed, which is the lesson worth keeping: the +fixture set was drawn from the plan's enumerated forms, and it took an +adversarial reading of the *implementation* — not of the plan — to find inputs +the plan had not imagined. The corrected implementation anchors on the name +upstream reports rather than on a keyword list, and every export and unexport +form is now pinned by a parameterized status test. + +A further honesty defect was observed and deliberately left alone: +`foo: export BAR := baz` reports `complete` with prerequisites +`["export", "BAR", ":=", "baz"]`. It deserves its own plan. ## Context and orientation @@ -1069,6 +1101,30 @@ 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. + +Final gate run, all sequential and all passing: `make check-fmt`, `make lint` +(including the `whitaker` driver), `make typecheck`, `make test` (129 tests +run, 129 passed, 0 skipped, plus 3 doctests), 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. + ### Captured concrete syntax trees The concrete syntax trees below were captured against the pinned parser 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. From 05fc9cf55ac5dc40386652e196ab037592b0adcd Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 12:46:38 +0100 Subject: [PATCH 5/9] Diagnose export lines the parser cannot name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping an unnameable export line and trusting the parser to have diagnosed it left a silent hole in the report. `override export override` parses upstream with no errors at all and no name, because the parser refuses any identifier whose text is one of its own directive keywords. The line was therefore discarded while the report still claimed `complete` with an empty diagnostics array — the same honesty breach that keyword-text filtering caused, arriving by a different route. Emit a diagnostic from the adapter instead of inheriting one. The guarantee that a dropped construct forces `recovered` now holds whatever the parser does, rather than for the inputs it happens to diagnose. The cost is a second diagnostic on inputs the parser does diagnose, such as a bare `export`; both are true and the status is unchanged. Guard the same class in `directive_names`: when the anchored walk yields nothing despite the parser reporting a name, report that name rather than silently returning an empty list. Strengthen the export form matrix to assert variable names alongside status, so a form that stopped producing its fact can no longer keep passing, and pin that a name-less definition which is not an export still fails loudly — the guard rail that keeps genuinely broken trees from being swept up by the drop path. Correct the documentation that rested on the false premise, in the users' guide, the design document and ADR-0002, and record in the plan that `override define FOO ... endef` still aborts. That is a documented GNU Make construct outside this plan's scope, and it loses the whole file exactly as bare exports used to. --- ...02-bare-export-directive-representation.md | 23 +++-- docs/design.md | 12 ++- docs/execplans/bare-export-directives.md | 91 +++++++++++++++---- docs/users-guide.md | 6 +- src/adapters/makefile.rs | 21 +++-- src/adapters/makefile_export.rs | 33 ++++--- tests/export_directives.rs | 73 +++++++++++---- 7 files changed, 193 insertions(+), 66 deletions(-) diff --git a/docs/adrs/0002-bare-export-directive-representation.md b/docs/adrs/0002-bare-export-directive-representation.md index 3022a40..fa7dcba 100644 --- a/docs/adrs/0002-bare-export-directive-representation.md +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -34,7 +34,10 @@ did. 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. +directive naming several variables is intended to yield one entry per name, but +the parser revision pinned when this decision was taken keeps only the first +name inside the definition node, so such a line yields one entry and a +`recovered` status until the parser learns the directive list form. The discriminating predicate for consumers is `operator == "" && define_block == false`, which identifies an export directive @@ -46,15 +49,18 @@ because the empty operator was already in the enum for `define` blocks and no new key or enum member is introduced. A form the parser cannot name — a bare `export` with no names, which means -"export every variable", or `export define NAME` — yields no entry rather than -an invented one. The parser already diagnoses these, so the report degrades to -`recovered` rather than falsely claiming `complete`. +"export every variable", `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 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 `export` or `unexport`, so keyword-text filtering would silently -discard real facts. +be called `unexport`, and keyword-text filtering would silently discard it. `unexport` remains unrepresented and is documented as a known gap. @@ -101,6 +107,11 @@ member is as breaking as a new key. test. - The representation cannot express "export every variable", so that form is reported as an absence plus a diagnostic rather than as a fact. +- A multi-name `export A B C` is not yet fully modelled: it reports `recovered` + with only the first name until the pinned parser keeps the rest. +- A variable whose name is `export`, `override` or `define` is unrepresentable, + because the parser reports no name for such a line. It degrades to a + diagnostic rather than a fact. ### Neutral diff --git a/docs/design.md b/docs/design.md index 733d06b..fa6a759 100644 --- a/docs/design.md +++ b/docs/design.md @@ -331,14 +331,18 @@ A directive naming several variables is intended to yield one entry per name, but the pinned parser revision keeps only the first name inside the definition node and discards the rest, so such a line currently yields one entry and a `recovered` status. Completing it needs a parser change, not a `makeutil` one. -A form the parser cannot name at all — a bare `export`, or -`export define NAME` — yields no entry rather than an invented one, and the -upstream diagnostic keeps the report `recovered` rather than falsely `complete`. +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 -`export` or `unexport`. +`unexport`. Names the parser itself treats as keywords — `export`, `override` +and `define` — remain unrepresentable, because it reports no name for them. 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 diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index a4618fc..c968d59 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -158,9 +158,12 @@ around it. - [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`. It - records the behaviour change rather than a new parser revision to pin, - because Stage E did not run. +- [x] Stage G: consumer-facing note added as §6.6.2 of `docs/design.md`. Two + deviations: it records the behaviour change rather than a new parser + revision to pin, because Stage E did not run; and 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 @@ -227,6 +230,25 @@ Recorded during investigation, before implementation began. 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 @@ -332,6 +354,28 @@ Recorded during investigation, before implementation began. 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 @@ -397,21 +441,34 @@ byte-identical to their pre-change state, as is `schemas/makeutil.parse.v1.schema.json`. Did the `variables`-reuse representation cause confusion in review? Not the -representation itself, which review accepted. What review did catch were two -defects in how the directive was recognized, both recorded in the -`Decision Log`: identifying directive keywords by token text silently discarded -`export unexport`, and gating the no-facts path on "export and not define" left -`export define FOO` aborting with exit code 2. Both breached the plan's own -constraints while every gate passed, which is the lesson worth keeping: the -fixture set was drawn from the plan's enumerated forms, and it took an -adversarial reading of the *implementation* — not of the plan — to find inputs -the plan had not imagined. The corrected implementation anchors on the name -upstream reports rather than on a keyword list, and every export and unexport -form is now pinned by a parameterized status test. - -A further honesty defect was observed and deliberately left alone: +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. Fourteen 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. + +Two further defects were observed and deliberately left alone, both outside +this plan's scope and both deserving their own plan, ideally a shared one: `foo: export BAR := baz` reports `complete` with prerequisites -`["export", "BAR", ":=", "baz"]`. It deserves its own plan. +`["export", "BAR", ":=", "baz"]`, and `override define FOO ... endef` still +aborts with exit code 2, losing the whole file. ## Context and orientation diff --git a/docs/users-guide.md b/docs/users-guide.md index b83d1e5..431584a 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -83,8 +83,10 @@ assignment: it reports its real operator and its value, with `exported` set to 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, and the report is `recovered`. The same holds for -`export define NAME`, which the pinned parser does not name. +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 pinned parser names. ### `unexport` is not yet supported diff --git a/src/adapters/makefile.rs b/src/adapters/makefile.rs index c1af4e1..7b7511d 100644 --- a/src/adapters/makefile.rs +++ b/src/adapters/makefile.rs @@ -187,13 +187,22 @@ fn variable_observation( 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, and `export define FOO` is - // modelled with no name at all. Upstream diagnoses both, so yielding no - // facts leaves the report honestly recovered instead of aborting the whole - // file. A name-less definition that is not an export is still a broken - // tree and keeps failing loudly below. + // 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::new()); + 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() { diff --git a/src/adapters/makefile_export.rs b/src/adapters/makefile_export.rs index 150d2e9..c9fc4a5 100644 --- a/src/adapters/makefile_export.rs +++ b/src/adapters/makefile_export.rs @@ -37,11 +37,11 @@ impl OperatorContext { /// `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 defensive: a directive-only -/// export never reaches here because [`export_directive_observations`] handles -/// it and supplies the operator itself. It is retained so that an upstream -/// change routing some other operator-less export down this path cannot -/// resurrect the fatal abort this module exists to remove. +/// 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, @@ -97,23 +97,32 @@ pub(super) fn export_directive_observations( /// /// 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` or -/// `export`. Filtering by keyword text instead would silently discard -/// `export unexport`, which upstream parses cleanly as exporting a variable -/// named `unexport`. An absent name means upstream could not find one and has -/// said so with a diagnostic, so the line names nothing. +/// 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(); }; - variable + 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() + .collect(); + if names.is_empty() { + vec![first_name] + } else { + names + } } #[cfg(test)] diff --git a/tests/export_directives.rs b/tests/export_directives.rs index da61858..da848a4 100644 --- a/tests/export_directives.rs +++ b/tests/export_directives.rs @@ -43,35 +43,64 @@ fn directive<'report>(report: &'report ParseReport, name: &str) -> Option<&'repo .find(|variable| variable.name == name && variable.operator == AssignmentOperator::Define) } -/// No `export` or `unexport` form may abort the parse. +/// No `export` or `unexport` form may abort the parse, and none may go missing +/// from a report that still claims `complete`. /// -/// A form upstream cannot name is dropped rather than invented, so the status -/// must be `recovered`: reporting `complete` while discarding a construct -/// would tell a consumer the facts are trustworthy when they are incomplete. +/// 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)] -#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Recovered)] -#[case::name_less("export\n", ParseStatus::Recovered)] -#[case::keyword_named_variable("export unexport\n", ParseStatus::Complete)] -#[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Complete)] -#[case::unnameable("export export\n", ParseStatus::Recovered)] -#[case::overridden("override export FOO\n", ParseStatus::Complete)] -#[case::exported_define("export define FOO\nbody\nendef\n", ParseStatus::Recovered)] -#[case::name_less_exported_define("export define\nbody\nendef\n", ParseStatus::Recovered)] -#[case::assignment("export FOO := bar\n", ParseStatus::Complete)] -#[case::unexport_single("unexport FOO\n", ParseStatus::Recovered)] -#[case::unexport_multiple("unexport FOO BAR\n", ParseStatus::Recovered)] -#[case::unexport_name_less("unexport\n", ParseStatus::Recovered)] +#[case::single("export FOO\n", ParseStatus::Complete, vec!["FOO"])] +#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Recovered, vec!["FOO"])] +#[case::name_less("export\n", ParseStatus::Recovered, Vec::new())] +#[case::keyword_named_variable("export unexport\n", ParseStatus::Complete, vec!["unexport"])] +#[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Complete, vec!["FOO"])] +#[case::unnameable("export export\n", ParseStatus::Recovered, Vec::new())] +#[case::overridden("override export FOO\n", ParseStatus::Complete, vec!["FOO"])] +#[case::undiagnosed_upstream("override export override\n", ParseStatus::Recovered, Vec::new())] +#[case::exported_define("export define FOO\nbody\nendef\n", ParseStatus::Recovered, Vec::new())] +#[case::name_less_exported_define( + "export define\nbody\nendef\n", + ParseStatus::Recovered, + Vec::new() +)] +#[case::assignment("export FOO := bar\n", ParseStatus::Complete, vec!["FOO"])] +#[case::unexport_single("unexport FOO\n", ParseStatus::Recovered, Vec::new())] +#[case::unexport_multiple("unexport FOO BAR\n", ParseStatus::Recovered, Vec::new())] +#[case::unexport_name_less("unexport\n", ParseStatus::Recovered, Vec::new())] fn no_export_form_aborts( #[case] source: &str, #[case] expected: ParseStatus, + #[case] expected_names: Vec<&str>, ) -> 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, expected); + 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()); + } 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] @@ -106,7 +135,13 @@ fn exported_define_block_invents_no_fact() -> Result<(), Box::new()); + 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(()) } From a31fc3b5d77af7a963aa0fd1ee5822ad3f7c4c49 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 13:13:58 +0100 Subject: [PATCH 6/9] Report every name of a multi-name export directive The real-world shape of the construct that motivated this work is `export A B C` on one line, and until now it was the one form still degraded: no abort, but `recovered` status with only the first name. The pinned parser trapped the second name in an error node and pushed the third outside the definition node, so no amount of tree walking here could recover them. Bump the parser fork to a revision that keeps every name of an `export`-led list inside the definition node, and flip the two expectations that were written against the old behaviour. The line now reports `complete` with one valueless entry per exported name, which is what the plan set out to achieve. `VariableDefinition::name()` still reports the first name upstream, so the extraction here is unchanged: it anchors on that name and takes the identifier tokens that follow. Names spread over a line continuation are collected too. Two expectations move in the other direction, matching GNU Make rather than convenience. `override export FOO BAR` is rejected by make with "missing separator", so it reports `recovered` rather than being read as a name list. Its single-name form `override export FOO` is also rejected by make but has always parsed cleanly here; that predates this work and is pinned as it behaves rather than quietly changed. Neither snapshot moved and the schema is untouched, which is the evidence that the revision bump is confined to multi-name export lines. --- Cargo.lock | 2 +- Cargo.toml | 2 +- ...02-bare-export-directive-representation.md | 12 +- docs/design.md | 36 ++-- docs/execplans/bare-export-directives.md | 154 +++++++++++++----- docs/users-guide.md | 14 +- src/adapters/makefile_export_tests.rs | 8 +- tests/export_directives.rs | 28 +++- 8 files changed, 181 insertions(+), 75 deletions(-) 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 index fa7dcba..611eff1 100644 --- a/docs/adrs/0002-bare-export-directive-representation.md +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -34,10 +34,8 @@ did. 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 is intended to yield one entry per name, but -the parser revision pinned when this decision was taken keeps only the first -name inside the definition node, so such a line yields one entry and a -`recovered` status until the parser learns the directive list form. +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 @@ -107,8 +105,10 @@ member is as breaking as a new key. test. - The representation cannot express "export every variable", so that form is reported as an absence plus a diagnostic rather than as a fact. -- A multi-name `export A B C` is not yet fully modelled: it reports `recovered` - with only the first name until the pinned parser keeps the rest. +- 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 reports no name for such a line. It degrades to a diagnostic rather than a fact. diff --git a/docs/design.md b/docs/design.md index fa6a759..fd75976 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: @@ -327,16 +330,16 @@ 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 is intended to yield one entry per name, -but the pinned parser revision keeps only the first name inside the definition -node and discards the rest, so such a line currently yields one entry and a -`recovered` status. Completing it needs a parser change, not a `makeutil` one. -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. +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 @@ -377,8 +380,9 @@ the merge of the `bare-export-directives` work should know that: "assignments only" view. - `unexport` remains unsupported and still reports a misleading rule with a `recovered` status. -- A multi-name `export A B C` no longer aborts but still reports `recovered` - with only the first name, pending a parser change. +- 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. diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index c968d59..3c9dca5 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -4,10 +4,10 @@ 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: BLOCKED — Stages A to D, F and G are complete and merged into this -branch. Stage E cannot proceed under the delegated authority for this work -because it changes a separate repository; see the `Decision Log`. Everything -achievable without that change has been delivered. +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 @@ -153,8 +153,10 @@ around it. 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. -- [ ] Stage E: BLOCKED. The upstream parser fix lives in a separate repository - that this work is not authorized to push to. See the `Decision Log`. +- [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`. @@ -403,18 +405,61 @@ Recorded during investigation, before implementation began. because compressing the work into six files would mean discarding deliverables the plan requires. Date/Author: 2026-08-13, implementer. -- BLOCKED: 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. +- 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 converse + spelling `export override FOO` is accepted, reading `override` as one of the + names. The plan's wording is superseded on this point, and both behaviours + are pinned by tests. 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 @@ -427,18 +472,16 @@ Recorded during investigation, before implementation began. Recorded at the end of Stage G, with Stage E blocked. -Does `export A B C` parse to `complete` with all three names? No. It parses -without aborting and reports `recovered` with the first name only. The pinned -parser traps the second name in an error node and pushes the third out of the -variable node entirely, so `makeutil` cannot recover them. Closing this needs -Stage E, which is blocked on authority to change a separate repository. The -tests that would prove it are already written and merely need their -expectations flipped. +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. The upstream -revision was never bumped, and both `insta` snapshots in `tests/snapshots/` are -byte-identical to their pre-change state, as is -`schemas/makeutil.parse.v1.schema.json`. +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 @@ -464,11 +507,24 @@ no facts. Fourteen 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. -Two further defects were observed and deliberately left alone, both outside -this plan's scope and both deserving their own plan, ideally a shared one: +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"]`, and `override define FOO ... endef` still -aborts with exit code 2, losing the whole file. +`["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; and +`export define FOO ... endef` is valid GNU Make that the parser still does not +model, though it now degrades honestly rather than aborting. ## Context and orientation @@ -1174,13 +1230,37 @@ 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 plan's prose asks for five entries and +three directive facts, which belongs to the three-name example in +`Purpose / big picture` rather than to this two-name command. The three-name +fixture does produce 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` (129 tests -run, 129 passed, 0 skipped, plus 3 doctests), and `make markdownlint` (which -also runs `spelling` and `provenance`), 0 errors. +(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. +unchanged from their pre-change state, before and after the revision bump. ### Captured concrete syntax trees diff --git a/docs/users-guide.md b/docs/users-guide.md index 431584a..bc46a5b 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -69,12 +69,9 @@ 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`, is not yet fully -modelled: the pinned parser captures only the first name and drops the rest -before `makeutil` sees them, so the report contains one entry and its status is -`recovered` rather than `complete`. Full support awaits a parser revision that -keeps every name. Until then, treat a `recovered` status on such a line as a -gap in the facts rather than as evidence about the Makefile. +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 @@ -86,7 +83,10 @@ 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 pinned parser names. +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`. ### `unexport` is not yet supported diff --git a/src/adapters/makefile_export_tests.rs b/src/adapters/makefile_export_tests.rs index 0963472..5e6d322 100644 --- a/src/adapters/makefile_export_tests.rs +++ b/src/adapters/makefile_export_tests.rs @@ -141,16 +141,16 @@ fn directive_only_lines_are_recognized( assert_eq!(context(is_define, is_export).is_directive_only(), expected); } -/// Upstream keeps only the first name of a multi-name `export` inside the -/// variable node, so the helper reports one name until the pinned parser -/// revision learns the directive list form. +/// 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_names_lost_upstream("export FOO BAR BAZ\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"])] diff --git a/tests/export_directives.rs b/tests/export_directives.rs index da848a4..3398d0d 100644 --- a/tests/export_directives.rs +++ b/tests/export_directives.rs @@ -52,12 +52,17 @@ fn directive<'report>(report: &'report ParseReport, name: &str) -> Option<&'repo /// upstream's, which is not always present — keeps such a report `recovered`. #[rstest] #[case::single("export FOO\n", ParseStatus::Complete, vec!["FOO"])] -#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Recovered, vec!["FOO"])] +#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Complete, vec!["FOO", "BAR", "BAZ"])] +#[case::continued("export FOO \\\n\tBAR\n", ParseStatus::Complete, vec!["FOO", "BAR"])] #[case::name_less("export\n", ParseStatus::Recovered, Vec::new())] #[case::keyword_named_variable("export unexport\n", ParseStatus::Complete, vec!["unexport"])] #[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Complete, vec!["FOO"])] #[case::unnameable("export export\n", ParseStatus::Recovered, Vec::new())] +// 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"])] +#[case::overridden_list("override export FOO BAR\n", ParseStatus::Recovered, vec!["FOO"])] #[case::undiagnosed_upstream("override export override\n", ParseStatus::Recovered, Vec::new())] #[case::exported_define("export define FOO\nbody\nendef\n", ParseStatus::Recovered, Vec::new())] #[case::name_less_exported_define( @@ -210,15 +215,32 @@ fn facts_after_a_bare_export_survive( } } +/// 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_never_aborts() -> Result<(), Box> { +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::Recovered); + assert_eq!(report.parse.status, ParseStatus::Complete); + assert_eq!(report.parse.diagnostics, Vec::new()); + + 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.define_block, fact.raw_value.as_str()), + (true, false, ""), + "{name} must be an exported, valueless directive", + ); + } Ok(()) } From 6e068bf2e3370b111ad9b4bf6e04230343ba96b2 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 13 Aug 2026 13:24:25 +0100 Subject: [PATCH 7/9] Describe the keyword-named export gap accurately Stage review checked every changed expectation against GNU Make 4.4.1 and found that the documentation claimed more than the code delivers. ADR-0002 said a variable whose name is `export`, `override` or `define` "degrades to a diagnostic rather than a fact". That holds only when such a name is alone on the line. Mixed with a nameable one, as in `export export FOO`, GNU Make exports both while the report names only `FOO` and still says `complete`. The omission is silent, which is the one surviving breach of the honesty rule in this work. It is pre-existing and unchanged here, but describing it wrongly is worse than leaving it open, so the ADR, the design document and the users' guide now say what actually happens, and `no_export_form_aborts` pins both spellings so the gap stays visible. Closing it means redefining what the parser's `name()` accessor treats as a name, which breaks every consumer of that crate and needs its own decision. The developers' guide still recorded the superseded parser pin and described the empty operator as meaning a define block only; both now match the code. The plan's Decision Log claimed `export override FOO` reads `override` as a name and that both behaviours are pinned. True of GNU Make, false of what the tests assert. Split the claim so make's behaviour and this tool's divergence are stated separately. Pin the multi-name facts more deeply: `overridden` and the shared whole-directive span were asserted by no test, although the ADR and the design document make positive claims about both. The pinned parser revision sits on an unmerged fork branch, so a rebase or deletion during review could make it unreachable. It now carries an annotated tag on the fork, recorded as the mitigation in the plan's Risks. The pin itself stays an immutable commit hash. --- ...02-bare-export-directive-representation.md | 17 ++++-- docs/design.md | 5 ++ docs/developers-guide.md | 18 ++++--- docs/execplans/bare-export-directives.md | 53 +++++++++++++------ docs/users-guide.md | 5 ++ tests/export_directives.rs | 20 ++++++- 6 files changed, 87 insertions(+), 31 deletions(-) diff --git a/docs/adrs/0002-bare-export-directive-representation.md b/docs/adrs/0002-bare-export-directive-representation.md index 611eff1..e0f0378 100644 --- a/docs/adrs/0002-bare-export-directive-representation.md +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -46,9 +46,9 @@ 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 form the parser cannot name — a bare `export` with no names, which means -"export every variable", `export define NAME`, or a name whose text the parser -treats as one of its own keywords — yields no entry rather than an invented +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. @@ -110,8 +110,15 @@ member is as breaking as a new key. 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 reports no name for such a line. It degrades to a - diagnostic rather than a fact. + because the parser's name accessor skips those texts. When such a name is the + only one on the line, as in `export export`, no fact is produced and the + report is `recovered`. When it is mixed with a nameable one, as in + `export export FOO` or `export override FOO`, GNU Make exports both but the + report names only `FOO` and still says `complete` — a silent omission, and a + known departure from the honesty rule above. Correcting it means changing + what the parser's `name()` accessor considers a name, which is a breaking + change for every consumer of that crate and is deliberately not attempted + here. ### Neutral diff --git a/docs/design.md b/docs/design.md index fd75976..e3a7c66 100644 --- a/docs/design.md +++ b/docs/design.md @@ -346,6 +346,11 @@ 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. +Where such a name is the only one on the line the report is `recovered`, but +where it accompanies a nameable one, as in `export export FOO`, GNU Make +exports both while the report names only `FOO` and still says `complete`. That +omission is silent and is a known gap; closing it means redefining what the +parser considers a name, which would break every consumer of that crate. 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 diff --git a/docs/developers-guide.md b/docs/developers-guide.md index fd0d2d2..7faf7dd 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -56,9 +56,11 @@ 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 makefile adapter privately scans leading recipe modifiers. This scanner exists because the upstream API has no always-execute accessor and its silent @@ -90,10 +92,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 index 3c9dca5..20be416 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -137,6 +137,13 @@ around it. 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 @@ -160,12 +167,12 @@ around it. - [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`. Two - deviations: it records the behaviour change rather than a new parser - revision to pin, because Stage E did not run; and 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. +- [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 @@ -418,11 +425,14 @@ Recorded during investigation, before implementation began. 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 converse - spelling `export override FOO` is accepted, reading `override` as one of the - names. The plan's wording is superseded on this point, and both behaviours - are pinned by tests. Date/Author: 2026-08-13, implementer, after upstream - review. + 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. @@ -470,7 +480,7 @@ Recorded during investigation, before implementation began. ## Outcomes & retrospective -Recorded at the end of Stage G, with Stage E blocked. +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 @@ -503,7 +513,7 @@ 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. Fourteen export and unexport forms are pinned by a parameterized test +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. @@ -522,9 +532,18 @@ 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; and +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. +model, though it now degrades honestly rather than aborting; and +`export export FOO` or `export override FOO` reports only `FOO` with a +`complete` status where GNU Make exports both names. + +That last one is the only known surviving breach of the honesty rule, and it is +worth stating plainly rather than burying: the omission is silent. It is +pre-existing, unchanged by this work, and pinned by `no_export_form_aborts` so +it stays visible. Closing it 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. ## Context and orientation @@ -1122,8 +1141,8 @@ Acceptance is behavioural, not structural. `"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 `multi_name_export_never_aborts`, - `name_less_export_degrades_to_a_diagnostic`, and + 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 diff --git a/docs/users-guide.md b/docs/users-guide.md index bc46a5b..1a0e7ab 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -88,6 +88,11 @@ 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 that exports a variable named `export`, `override` or `define` +alongside an ordinary one, such as `export export FOO`, reports only the +ordinary name and still says `complete`. GNU Make exports both. Treat such a +line as under-reported; it is a known gap. + ### `unexport` is not yet supported An `unexport` directive is currently reported as a rule whose first target is diff --git a/tests/export_directives.rs b/tests/export_directives.rs index 3398d0d..6fe011d 100644 --- a/tests/export_directives.rs +++ b/tests/export_directives.rs @@ -56,7 +56,12 @@ fn directive<'report>(report: &'report ParseReport, name: &str) -> Option<&'repo #[case::continued("export FOO \\\n\tBAR\n", ParseStatus::Complete, vec!["FOO", "BAR"])] #[case::name_less("export\n", ParseStatus::Recovered, Vec::new())] #[case::keyword_named_variable("export unexport\n", ParseStatus::Complete, vec!["unexport"])] +// GNU Make exports both names on these two lines. The parser's name accessor +// refuses `export` and `override` as names, so only the ordinary one is +// reported and the status stays `complete` — a known silent omission, +// documented in ADR-0002 and pinned here so it stays visible. #[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Complete, vec!["FOO"])] +#[case::override_named_variable("export override FOO\n", ParseStatus::Complete, vec!["FOO"])] #[case::unnameable("export export\n", ParseStatus::Recovered, Vec::new())] // GNU Make actually rejects `override export FOO` with "missing separator". // The parser accepts the single-name form, which predates this work and is @@ -228,6 +233,7 @@ fn multi_name_export_is_complete() -> Result<(), Box> { assert_eq!(report.parse.status, ParseStatus::Complete); assert_eq!(report.parse.diagnostics, Vec::new()); + let mut spans = Vec::new(); for name in [ "MOLD_VERSION_FILE", "MOLD_SHA256SUMS_FILE", @@ -236,11 +242,21 @@ fn multi_name_export_is_complete() -> Result<(), Box> { let fact = directive(&report, name) .ok_or_else(|| format!("the report must contain a directive fact for {name}"))?; assert_eq!( - (fact.exported, fact.define_block, fact.raw_value.as_str()), - (true, false, ""), + ( + fact.exported, + fact.overridden, + fact.define_block, + fact.raw_value.as_str() + ), + (true, false, false, ""), "{name} must be an exported, valueless directive", ); + spans.push((fact.location.start_byte, fact.location.end_byte)); } + + // Every name on one directive line shares that line's span, because the + // directive is the only source range any of them has. + assert_eq!(spans.first(), spans.last()); Ok(()) } From dddcc6be75ff52abd5566c1d705abc4941102a7c Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 15 Aug 2026 13:39:15 +0200 Subject: [PATCH 8/9] Recover repeated bare export directives Preserve nameable facts while marking keyword-shaped names that the upstream parser cannot represent as recovered with a source location. Expand parser, property, and black-box coverage, and document the schema, migration, and helper-reuse contracts. --- ...02-bare-export-directive-representation.md | 20 ++- docs/design.md | 10 +- docs/developers-guide.md | 14 ++ docs/execplans/bare-export-directives.md | 39 ++--- docs/users-guide.md | 26 ++-- docs/v0-1-0-migration-guide.md | 12 ++ src/adapters/makefile_export.rs | 40 ++++- tests/cli_e2e.rs | 34 +++++ tests/export_directives.rs | 141 ++++++++++++++---- tests/fixtures/makefiles/bare-export-cli.mk | 3 + 10 files changed, 257 insertions(+), 82 deletions(-) create mode 100644 tests/fixtures/makefiles/bare-export-cli.mk diff --git a/docs/adrs/0002-bare-export-directive-representation.md b/docs/adrs/0002-bare-export-directive-representation.md index e0f0378..7a06c51 100644 --- a/docs/adrs/0002-bare-export-directive-representation.md +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -2,9 +2,13 @@ ## Status -Accepted on 2026-08-13 +Accepted -## Context +## 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 @@ -110,15 +114,9 @@ member is as breaking as a new key. 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. When such a name is the - only one on the line, as in `export export`, no fact is produced and the - report is `recovered`. When it is mixed with a nameable one, as in - `export export FOO` or `export override FOO`, GNU Make exports both but the - report names only `FOO` and still says `complete` — a silent omission, and a - known departure from the honesty rule above. Correcting it means changing - what the parser's `name()` accessor considers a name, which is a breaking - change for every consumer of that crate and is deliberately not attempted - here. + 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 diff --git a/docs/design.md b/docs/design.md index e3a7c66..f00f0f8 100644 --- a/docs/design.md +++ b/docs/design.md @@ -345,12 +345,10 @@ 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. -Where such a name is the only one on the line the report is `recovered`, but -where it accompanies a nameable one, as in `export export FOO`, GNU Make -exports both while the report names only `FOO` and still says `complete`. That -omission is silent and is a known gap; closing it means redefining what the -parser considers a name, which would break every consumer of that crate. +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 diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 7faf7dd..c3ea667 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -62,6 +62,20 @@ 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 and ignore-error accessors are sensitive to modifier order. It may be called diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index 20be416..3ac75f3 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -45,7 +45,7 @@ exit=0 ``` The report must show `"status": "complete"` with an empty `diagnostics` array, -and `variables` must contain five entries: the two assignments and the three +and `variables` must contain four entries: the two assignments and the two directive facts described below. ## Constraints @@ -81,7 +81,7 @@ around it. 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 your working environment, nor + 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 @@ -639,8 +639,8 @@ line makes `VariableDefinition::name()` return `None`, so ### Verified current behaviour of every export form The following were measured against the current default-branch build. Reproduce -them yourself in Stage A. "Exit 2" means the fatal `parse-internal` path; exit -1 means a `recovered` report was still printed; exit 0 means `complete`. +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 @@ -720,12 +720,12 @@ columns. ### Stage A: reproduce and orient (no code changes) -Confirm the defect on your own working tree before changing anything, so you -know the baseline is what this plan describes. 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. +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. @@ -953,7 +953,7 @@ 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 you cannot explain — an unexplained diff in +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. @@ -1268,11 +1268,9 @@ exit=0 ``` `variables` holds four entries for that input: the two assignments and one -directive fact per exported name. The plan's prose asks for five entries and -three directive facts, which belongs to the three-name example in -`Purpose / big picture` rather than to this two-name command. The three-name -fixture does produce six entries — three assignments and three directives — with -`complete` status and no diagnostics. +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 @@ -1358,7 +1356,14 @@ Expected JSON shape for a bare export fact after Stage C, abbreviated: "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 } + "location": { + "start_byte": 0, + "end_byte": 0, + "start_line": 1, + "start_column": 1, + "end_line": 1, + "end_column": 1 + } } ``` diff --git a/docs/users-guide.md b/docs/users-guide.md index 1a0e7ab..dcbef9a 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -59,15 +59,15 @@ Recovered reports are insufficient proof that a Makefile is compliant. ### Bare `export` directives -A bare `export NAME` directive names a variable assigned elsewhere, so it -carries no assignment operator and no value. 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 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 @@ -88,10 +88,10 @@ 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 that exports a variable named `export`, `override` or `define` -alongside an ordinary one, such as `export export FOO`, reports only the -ordinary name and still says `complete`. GNU Make exports both. Treat such a -line as under-reported; it is a known gap. +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 diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index 2244830..bd62421 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -38,3 +38,15 @@ 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 that need assignments only should +filter for a non-empty `operator`; preserve the export entries when directive +facts matter. + +`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_export.rs b/src/adapters/makefile_export.rs index c9fc4a5..56ab96c 100644 --- a/src/adapters/makefile_export.rs +++ b/src/adapters/makefile_export.rs @@ -74,7 +74,7 @@ pub(super) fn export_directive_observations( conditions: &[ConditionObservation], definition_span: SourceSpan, ) -> Vec { - directive_names(variable) + let mut observations = directive_names(variable) .into_iter() .map(|name| SyntaxObservation::Variable { name, @@ -86,7 +86,43 @@ pub(super) fn export_directive_observations( conditions: conditions.to_vec(), span: definition_span, }) - .collect() + .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. diff --git a/tests/cli_e2e.rs b/tests/cli_e2e.rs index 6608b22..c4afccf 100644 --- a/tests/cli_e2e.rs +++ b/tests/cli_e2e.rs @@ -39,6 +39,40 @@ 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)) + }) + .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/export_directives.rs b/tests/export_directives.rs index 6fe011d..4325ad6 100644 --- a/tests/export_directives.rs +++ b/tests/export_directives.rs @@ -1,18 +1,20 @@ //! Report-shape regressions for GNU Make's `export` directive family. //! -//! A bare `export NAME` names a variable assigned elsewhere, 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. +//! 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, VariableFact}, + domain::{AssignmentOperator, ParseReport, ParseStatus, SourceLocation, VariableFact}, parse_source, }; use pretty_assertions::assert_eq; +use proptest::{prelude::*, test_runner::TestCaseError}; use rstest::{fixture, rstest}; #[fixture] @@ -51,38 +53,49 @@ fn directive<'report>(report: &'report ParseReport, name: &str) -> Option<&'repo /// 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"])] -#[case::multiple("export FOO BAR BAZ\n", ParseStatus::Complete, vec!["FOO", "BAR", "BAZ"])] -#[case::continued("export FOO \\\n\tBAR\n", ParseStatus::Complete, vec!["FOO", "BAR"])] -#[case::name_less("export\n", ParseStatus::Recovered, Vec::new())] -#[case::keyword_named_variable("export unexport\n", ParseStatus::Complete, vec!["unexport"])] -// GNU Make exports both names on these two lines. The parser's name accessor -// refuses `export` and `override` as names, so only the ordinary one is -// reported and the status stays `complete` — a known silent omission, -// documented in ADR-0002 and pinned here so it stays visible. -#[case::repeated_keyword_prefix("export export FOO\n", ParseStatus::Complete, vec!["FOO"])] -#[case::override_named_variable("export override FOO\n", ParseStatus::Complete, vec!["FOO"])] -#[case::unnameable("export export\n", ParseStatus::Recovered, Vec::new())] +#[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"])] -#[case::overridden_list("override export FOO BAR\n", ParseStatus::Recovered, vec!["FOO"])] -#[case::undiagnosed_upstream("override export override\n", ParseStatus::Recovered, Vec::new())] -#[case::exported_define("export define FOO\nbody\nendef\n", ParseStatus::Recovered, Vec::new())] +#[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() + Vec::new(), + None )] -#[case::assignment("export FOO := bar\n", ParseStatus::Complete, vec!["FOO"])] -#[case::unexport_single("unexport FOO\n", ParseStatus::Recovered, Vec::new())] -#[case::unexport_multiple("unexport FOO BAR\n", ParseStatus::Recovered, Vec::new())] -#[case::unexport_name_less("unexport\n", ParseStatus::Recovered, Vec::new())] +#[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 @@ -95,6 +108,24 @@ fn no_export_form_aborts( 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(()) } @@ -233,7 +264,7 @@ fn multi_name_export_is_complete() -> Result<(), Box> { assert_eq!(report.parse.status, ParseStatus::Complete); assert_eq!(report.parse.diagnostics, Vec::new()); - let mut spans = Vec::new(); + let expected_span = (120, 186); for name in [ "MOLD_VERSION_FILE", "MOLD_SHA256SUMS_FILE", @@ -251,15 +282,59 @@ fn multi_name_export_is_complete() -> Result<(), Box> { (true, false, false, ""), "{name} must be an exported, valueless directive", ); - spans.push((fact.location.start_byte, fact.location.end_byte)); + assert_eq!( + (fact.location.start_byte, fact.location.end_byte), + expected_span, + "{name} must use the directive line's full span", + ); } - - // Every name on one directive line shares that line's span, because the - // directive is the only source range any of them has. - assert_eq!(spans.first(), spans.last()); 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( 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 From b70842089306293a0a393a7b0a4286c5bea7ac6c Mon Sep 17 00:00:00 2001 From: leynos Date: Sat, 22 Aug 2026 05:21:07 +0200 Subject: [PATCH 9/9] Clarify bare export recovery contract Align the schema predicate, migration guidance, and historical plan record with the adapter behaviour, and require valueless export facts in the CLI contract. --- ...02-bare-export-directive-representation.md | 4 ++-- docs/design.md | 5 ++-- docs/execplans/bare-export-directives.md | 20 ++++++++++------ docs/v0-1-0-migration-guide.md | 24 ++++++++++++++++--- src/adapters/makefile_export.rs | 4 ++-- tests/cli_e2e.rs | 4 ++++ 6 files changed, 45 insertions(+), 16 deletions(-) diff --git a/docs/adrs/0002-bare-export-directive-representation.md b/docs/adrs/0002-bare-export-directive-representation.md index 7a06c51..4c25036 100644 --- a/docs/adrs/0002-bare-export-directive-representation.md +++ b/docs/adrs/0002-bare-export-directive-representation.md @@ -131,8 +131,8 @@ member is as breaking as a new key. 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 the - `operator` field alone. +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 diff --git a/docs/design.md b/docs/design.md index f00f0f8..99ae518 100644 --- a/docs/design.md +++ b/docs/design.md @@ -379,8 +379,9 @@ the merge of the `bare-export-directives` work should know that: 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. Filter on a non-empty `operator` to recover the previous - "assignments only" view. + 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 diff --git a/docs/execplans/bare-export-directives.md b/docs/execplans/bare-export-directives.md index 3ac75f3..805be31 100644 --- a/docs/execplans/bare-export-directives.md +++ b/docs/execplans/bare-export-directives.md @@ -535,15 +535,21 @@ 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` reports only `FOO` with a -`complete` status where GNU Make exports both names. - -That last one is the only known surviving breach of the honesty rule, and it is -worth stating plainly rather than burying: the omission is silent. It is -pre-existing, unchanged by this work, and pinned by `no_export_form_aborts` so -it stays visible. Closing it means redefining what the parser's `name()` +`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 diff --git a/docs/v0-1-0-migration-guide.md b/docs/v0-1-0-migration-guide.md index bd62421..6800f73 100644 --- a/docs/v0-1-0-migration-guide.md +++ b/docs/v0-1-0-migration-guide.md @@ -43,9 +43,27 @@ source-location contracts. 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 that need assignments only should -filter for a non-empty `operator`; preserve the export entries when directive -facts matter. +`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 diff --git a/src/adapters/makefile_export.rs b/src/adapters/makefile_export.rs index 56ab96c..55e50b3 100644 --- a/src/adapters/makefile_export.rs +++ b/src/adapters/makefile_export.rs @@ -67,8 +67,8 @@ pub(super) fn 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, so it yields no facts and upstream's own diagnostic drives the -/// recovered status. +/// 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], diff --git a/tests/cli_e2e.rs b/tests/cli_e2e.rs index c4afccf..c410411 100644 --- a/tests/cli_e2e.rs +++ b/tests/cli_e2e.rs @@ -67,6 +67,10 @@ fn bare_export_path_emits_export_facts(mut makeutil_command: Command) { 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::>();