From 620f9c1a21e6454b84d0054b6433beaa5b9e7da7 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:34:31 +0800 Subject: [PATCH 1/8] fix(scanner, manifest, build): one answer per question, and no key that reaches no decision (2026.9.9.1) Four defects with two shapes behind them: a question answered twice whose answers are read by different sites, and a key that is parsed and then reaches no decision. Design record: .agents/docs/2026-09-09-two-answers-and-two-silences.md `module : private;` was read as an implementation partition. The name tokeniser admits `:` so that `M:part` scans as one token, and the partition test is "the name contains a colon" -- but the colon in the private module fragment comes first and belongs to no name. A valid file was refused at scan time with a sentence that is false about it. A regression from #433, first released in v2026.8.18.1: on clang the same source built and ran under 2026.8.17.1. The scanner also no longer records an identity no source could have declared; a BOM combined with a fragment previously produced a BMI named after a colon. A UTF-8 byte-order mark is consumed where bytes become lines, and by every TOML document mcpp reads. MSVC writes one by default, so this is ordinary input. The failure it caused appeared at the consumer (`module 'x' not found`) rather than at the file that was wrong. UTF-16/32 is refused by name. `module_lang` and `module_output` become per-edge, bound from what the scan found rather than from the file's extension. A module-extension file need not provide a module, and where the classifier and the scanner disagreed the edge carried an interface's flags with no BMI: clang accepted `-fmodule-output=` with no value and wrote no BMI at all, and compiled implementation units as interfaces. GCC's interface spelling is the plain language, so the identical project built there and not on clang. `ConditionalConfig::empty()` replaces two hand-written gates that omitted `libraries` and `linkLibraryDirs`, so a predicate carrying only a `[target..runtime]` table was parsed and discarded. It composes `BuildInputs::empty()` and the existing `XlingsConfig::empty()`, closing a fourth instance in the xpkg gate. All 228 descriptors in mcpp-index parse identically before and after. `[runtime]` and `[target..runtime]` now report an unsupported key, the rule `[build]` and `[target.]` have followed since #418 and #249. Deliberately not done: no detector for GNU-spelled `ldflags` reaching MSVC. `ldflags` is a documented raw pass-through and `[runtime] libraries` is the supported neutral form; inspecting the former would be a second answer to a question the author already answered. --- ...2026-09-09-two-answers-and-two-silences.md | 499 ++++++++++++++++++ .agents/docs/README.md | 7 +- CHANGELOG.md | 69 +++ docs/04-mcpp-toml.md | 43 ++ docs/22-target-side.md | 21 +- docs/zh/04-mcpp-toml.md | 36 ++ docs/zh/22-target-side.md | 18 +- mcpp.toml | 2 +- modules/libs/src/toml.cppm | 14 + modules/manifest/src/toml.cppm | 56 +- modules/manifest/src/types.cppm | 38 ++ modules/manifest/src/xpkg.cppm | 10 +- modules/toolchain-model/src/model.cppm | 27 + modules/versioning/src/version.cppm | 2 +- src/build/ninja_backend.cppm | 77 ++- src/modgraph/scanner.cppm | 134 +++++ ...vate_module_fragment_is_not_a_partition.sh | 114 ++++ ..._byte_order_mark_does_not_hide_a_module.sh | 80 +++ ...xtension_file_need_not_provide_a_module.sh | 83 +++ ...ditional_runtime_table_alone_is_applied.sh | 129 +++++ tests/unit/test_manifest.cpp | 139 +++++ tests/unit/test_modgraph.cpp | 103 ++++ tests/unit/test_ninja_backend.cpp | 131 +++++ 23 files changed, 1802 insertions(+), 30 deletions(-) create mode 100644 .agents/docs/2026-09-09-two-answers-and-two-silences.md create mode 100755 tests/e2e/634_a_private_module_fragment_is_not_a_partition.sh create mode 100755 tests/e2e/635_a_byte_order_mark_does_not_hide_a_module.sh create mode 100755 tests/e2e/636_a_module_extension_file_need_not_provide_a_module.sh create mode 100755 tests/e2e/637_a_conditional_runtime_table_alone_is_applied.sh diff --git a/.agents/docs/2026-09-09-two-answers-and-two-silences.md b/.agents/docs/2026-09-09-two-answers-and-two-silences.md new file mode 100644 index 00000000..789e1e35 --- /dev/null +++ b/.agents/docs/2026-09-09-two-answers-and-two-silences.md @@ -0,0 +1,499 @@ +--- +subject: modules +status: active +--- + +# Two answers and two silences: the scanner's second grammar, and the manifest keys nothing reads + +A consumer project pinned at mcpp 2026.8.17.1 reported five obstacles to +upgrading. This record judges each against two questions, and only those two: + +1. **Does it violate a contract mcpp already holds?** A defect is a place where + mcpp contradicts something it has already decided — in code, in a comment + that states a rule, or in `docs/`. A gap that mcpp never promised to fill is + a feature request, and belongs in a different record. +2. **Does the repair fit the semantics already in the tree?** A repair that + introduces a second answer to a question mcpp already answers is not a + repair, however green it measures. + +Three reports survive both questions as defects. Two are refuted in part: one +described correct behaviour as a bug, and one asks mcpp to police a key it +deliberately does not interpret. The defects that survive are not five +independent bugs — they are **four mechanisms**, and two of them are instances +of failure modes this codebase has already named in its own comments. + +## 1. What was measured + +Host: Linux 6.8, x86_64. `gcc 16.1.0` and `clang 22.1.8`, both from the xlings +store. mcpp `2026.9.8.1` (released binary) and `2026.8.17.1` (the consumer's +pin). The working tree is `origin/main`, whose `MCPP_VERSION` is `2026.9.8.1` — +so the source read here and the binary measured here are the same version. + +| # | Construct | 2026.8.17.1 | 2026.9.8.1 | Compiler alone | +|---|---|---|---|---| +| 1 | `module : private;`, clang | builds, links, runs (exit 0) | `error: scanner errors` (exit 2) | clang: compiles, emits `.pcm` and `.o` | +| 1b | `module : private;`, gcc | `error: module already declared` | same scanner error | gcc: `sorry, unimplemented: private module fragment` | +| 2 | UTF-8 BOM on a `.cppm` | — | `module 'hello' not found` at the *consumer* | clang: compiles the BOM'd file without complaint | +| 3 | implementation unit in a `.cppm` | — | **gcc: builds. clang: hard error.** | — | +| 4 | `[target.linux.runtime] libraries` alone | — | silently dropped | — | +| 4b | the same, plus one unrelated `defines` entry | — | applied (`-ldl` present) | — | +| 5 | unknown key in `[runtime]` | — | silent | — | + +Rows 1 and 4b are the load-bearing ones. Row 1 is a clean A/B across the +regression boundary: same source, same clang, same machine, opposite outcomes. +Row 4b differs from row 4 in exactly one dimension that has nothing to do with +libraries, which is what makes it evidence about the gate rather than about the +predicate. + +## 2. Mechanism A — a second grammar that disagrees with the normative one + +`scan_file` (`src/modgraph/scanner.cppm:577`) is a hand-written parser for a +fragment of the C++ grammar. It exists because the compiler's own P1689 scan is +slower and, for clang, needs an external tool; `MCPP_SCANNER=p1689` selects the +normative path and is **opt-in**, so the hand-written scanner is what every +default build uses. Where the two disagree, the compiler is right by +construction. Two disagreements were measured. + +### 2.1 The private module fragment (regression, `df7a443d`, #433) + +`is_module_name_char` (`scanner.cppm:194`) admits `:` so that `M:part` scans as +one token. The `module` branch therefore reads `module : private;` as a +declaration whose "name" is `:`, and `scanner.cppm:732` tests +`name.find(':') != npos` to decide "this is an implementation partition": + +``` +module M; implementation unit → requires M +module M:part; implementation partition → provides M:part +module : private; private module fragment → provides and requires nothing +``` + +The third production is not a name with a colon in it. The predicate "contains +a colon" collapses it into the second, A private module fragment may only appear in +a primary interface unit, so wherever that unit's `export module` line was seen +`u.provides` is set and the branch reports `file already provides module 'X'; +cannot also provide ':'`. That sentence is false about the source it names. + +Where the interface line was *not* seen — §2.2 gives one way that happens — the +same branch takes its other arm, raises nothing at all, and records the file as +providing a module literally named `:`. Measured on the compound input (a BOM'd +interface that also has a private fragment), the generated graph acquires an +edge for `pcm.cache/-.pcm`: a BMI for a module whose name is punctuation. +Neither arm of that branch is a name. + +`git log -L` attributes the branch to `df7a443d` (#433), first released in +`v2026.8.18.1`. Before it, the `else` read `if (!u.provides)`, and since +`u.provides` was set, the line fell through and was ignored — correct behaviour +by accident. The commit fixed a real defect (implementation partitions recorded +as requiring themselves) and took this one with it. + +**Verdict: defect.** `module : private;` is standard C++20, clang implements it, +and mcpp refuses a file the toolchain mcpp itself resolved would accept. On gcc +the code cannot build either way, but that is the compiler's sentence to pass, +and it passes it clearly. + +**Repair.** Recognise the production before the partition test and record +nothing from it. Then, so the class is closed rather than the instance: +**never record a module identity that is not a well-formed module name.** `":"` +and `":private"` are not. Today any mis-parse of this line silently becomes a +module named after punctuation; with the check, the next one is a diagnostic. + +mcpp must not additionally warn that gcc lacks the feature. The compiler says +so, and a second answer is what this record exists to avoid. + +### 2.2 The UTF-8 BOM + +`scan_file` opens an `ifstream` (`scanner.cppm:581`) and reads lines +(`:656`). Nothing consumes a byte-order mark, and `trim` uses `std::isspace`, +which is false for `0xEF 0xBB 0xBF`. The first line of a BOM'd file is therefore +`\xEF\xBB\xBFexport module hello;`, which matches neither `export` nor `module`, +so the declaration is not seen. Every normative parser skips the mark; clang +compiled the same file without comment. + +MSVC writes UTF-8 with BOM by default, so this is ordinary input, not a corner. + +**Verdict: defect**, and of a specific kind — mcpp's second parser diverging +from the parser whose answer is definitive. + +**Repair.** Consume the mark once, where bytes become lines, not at call sites. +Refuse a UTF-16/32 mark by name rather than misparsing it, which is the +"no silent acceptance" rule this codebase applies elsewhere. + +*Listed separately so it can be dropped without dropping the above:* a BOM on +`mcpp.toml` produces `1:1: error: expected key`. That is honest — it fails, and +it fails at the right place — but the message does not name the cause, and the +file is one MSVC users commonly author. This is diagnostic quality, not +correctness, and it carries its own criterion below precisely so that shipping +the scanner fix cannot quietly retire it. + +## 3. Mechanism B — two answers to "is this a module interface" + +This is the mechanism the other three symptoms share, and mcpp has already +written the rule down. `docs/04-mcpp-toml.md` §2.3: + +> the scanner reads `export module` and gives the edge a BMI while the +> classifier says the file has no role, and what the author sees is +> `undefined reference` to a module-mangled symbol. + +`scanner.cppm:616` enforces that rule in one direction: classifier says `Other`, +scanner found a module → refused, with a diagnostic that names the file, the +extension and the key to add. Its comment explains the choice: mcpp guessing +what an unknown extension "must have meant" would be *a second answer to the +same question — the very thing that produced this defect.* + +The opposite direction is unguarded: + +| | scanner: provides | scanner: provides nothing | +|---|---|---| +| **classifier: ModuleInterface** | normal | **unguarded** | +| **classifier: Other** | refused at `scanner.cppm:616` | normal | + +The unguarded cell is reachable, and the two readers disagree by construction: + +- `pick_rule` (`ninja_backend.cppm:1541`) switches on `cu.kind` — the + **classifier's** answer, derived from the extension — and returns + `cxx_module` for every `ModuleInterface`. +- `bmi_out` is bound only `if (cu.providesModule)` — the **scanner's** answer, + derived from the content (`ninja_backend.cppm:1837`, `:1934`, `:2009`). + +Three symptoms follow. + +**Symptom 1 — an empty flag.** `ninja_backend.cppm:811` builds the flag +unconditionally for any toolchain with `needsExplicitModuleOutput` (clang and +MSVC; not gcc): + +``` +rule cxx_module + command = if [ -n "$bmi_out" ] && [ -f "$bmi_out" ]; then cp -p ...; fi + && $cxx ... -fmodule-output=$bmi_out -x c++-module -c $in -o $out + && if [ -n "$bmi_out" ] && ...; fi +``` + +Both guards wrap the backup and the restore. The flag between them has none. +The comment three lines above the code says "If `$bmi_out` is empty (no module +provided), we just compile normally" — a protection that is not there. +Measured: an edge with no `bmi_out` binding, `clang++ -fmodule-output=` exits 0, +writes the object, and writes **no BMI**. Zero `.pcm` existed anywhere under +`target/` after the BOM build; the failure then surfaced at `main.cpp` as +`module 'hello' not found`. + +**Symptom 2 — a project that builds on gcc and fails on clang.** A `.cppm` +holding an implementation unit (`module foo;`) is legal C++ and the scanner +parses it correctly — it records `requires foo` and provides nothing. The +classifier still says `ModuleInterface`, so the edge takes `cxx_module`, whose +clang spelling is `-x c++-module`. Clang is thereby told the file is an +interface and reports `missing 'export' specifier in module declaration while +building module interface`. The gcc rule spells the same edge `-x c++`, so gcc +reads the file's own declaration and the identical project builds. This is not +the BOM case; the input is well-formed, and the split is mcpp's. + +**Symptom 3 — the BOM's downstream half**, which §2.2 already covers. + +**Verdict: defect**, against a rule stated in mcpp's own documentation and +already enforced in the mirror direction. + +**Repair, and the decision it requires.** What does a module-interface +extension declare? Two readings are available and mcpp has never chosen between +them in the backend: + +- **A. The extension says how to *scan*; the content says what the file *is*.** + Then the edge must be built from the scan result: a unit that provides a + module gets `cxx_module` with a bound `bmi_out`; a unit that provides nothing + does not. The empty flag becomes unreachable, and symptom 2 disappears because + both compilers then receive the same instruction. + + **Routing to `cxx_object` is not sufficient, and this was measured.** Clang's + driver infers `c++-module` from the `.cppm` extension on its own, and + `cxx_object` carries no `-x` flag at all — so an implementation unit sent + there fails with the identical `missing 'export' specifier` error. What the + edge must carry is an **explicit `-x` spelling chosen from the scan result**: + `-x c++-module` where the unit provides a module, `-x c++` where it does not. + Measured: `clang++ -x c++ -c foo_impl.cppm` compiles and writes the object; + the same command without `-x` does not. The gcc rule already spells `-x c++` + unconditionally, which is the whole reason gcc was never affected — the + portability split in symptom 2 is one flag wide. +- **B. The extension says the file *is* an interface.** Then a `.cppm` whose + scan finds no module declaration is refused at scan time — the mirror of + `scanner.cppm:616`. + +**A is recommended**, because it is what `scanner.cppm:616`'s own reasoning +implies: an extension cannot know whether a given file is an interface or an +implementation unit, only the content can, and the scanner's answer is *read* +rather than guessed. B is not excluded by A and may be added as hardening, but +it is a separate decision with a separate criterion, because after A the only +input that reaches it is a genuinely malformed file. + +**What the repair must not be.** Wrapping the flag in `[ -n "$bmi_out" ]` makes +the command line well-formed and leaves the graph wrong: the file is still +compiled as a module that provides nothing, and the consumer still fails at a +distance. It would also hide the next instance of the mismatch. The invariant +worth adding instead is the assertion that a `cxx_module` edge always binds +`bmi_out` — a branch that should not happen, given a line that prints. + +## 4. Mechanism C — a declared key that reaches no decision + +`[target..runtime]` **is implemented**, contrary to the report: parsed at +`toml.cppm:2598` into `cc.libraries` / `cc.linkLibraryDirs`, applied at +`prepare.cppm:418`. It works — but only when something *else* under the same +predicate is also present. + +`toml.cppm:2865` decides whether to record the `ConditionalConfig` at all: + +```cpp +if (!cc.inputs.cflags.empty() || !cc.inputs.cxxflags.empty() + || !cc.inputs.ldflags.empty() || !cc.inputs.sources.empty() + ... + || !cc.xlings.deps.empty() || !cc.xlings.featureDeps.empty()) + m.conditionalConfigs.push_back(std::move(cc)); +``` + +Fourteen fields are named. `cc.libraries` and `cc.linkLibraryDirs` are not among +them. A predicate carrying only a `runtime` table therefore produces a config +that is parsed, populated, and discarded. Adding one unrelated `defines` entry +under the same predicate makes `-ldl` appear. + +**This is the third instance of a failure mode the struct documents on itself.** +`ConditionalConfig` in `modules/manifest/src/types.cppm` carries two comments: + +- #258 — "the conditional reader maintained its own subset of `[build]`'s keys + and nobody noticed it had fallen behind." Repaired **structurally**, by + carrying the whole `BuildInputs` type: "the set cannot drift." +- #359 — "the conditional channel carried three of the four dependency maps and + silently lacked the fourth, which is the exact failure this struct's + `BuildInputs` comment above describes for #258." Repaired **locally**, by + adding the fourth. + +The emptiness gate is a third hand-maintained list over the same struct, and +`xpkg.cppm:1530` is a fourth. Reading the xpkg loop, it fills nine `cc.inputs` +fields and its gate names eight: `privateIncludeDirs` is filled and unlisted, so +an xpkg conditional block carrying only private include directories is dropped +the same way. *(Found by reading; not measured end-to-end — it is stated here so +it is checked, not so it is believed.)* + +**Why no test caught it.** The only e2e for the neutral link intent, +`tests/e2e/262_pack_consumed_by_native_cl.sh`, is `# requires: msvc` and does +not run on the Linux shards. It would not have caught it if it did: its fixture +is a *generated distribution package*, which by design carries both `ldflags` +and `libraries` — so `cc.inputs.ldflags` is non-empty, the gate passes, and the +bug is unreachable in the one shape the test builds. + +**Verdict: defect.** Not a missing feature — a shipped feature that a fourth +reader discards. + +**Repair.** Take #258's medicine rather than #359's: give `ConditionalConfig` an +`empty()` member defined **beside the fields**, and have both gates call it. +Adding a field then forces the question where the field is written, instead of +in two files that do not mention each other. Pushing unconditionally is simpler +and probably harmless, but it changes what `merge_conditional_config`'s two +disjoint passes iterate over, so it is the larger change and is not recommended +without its own measurement. + +## 5. Mechanism D — a key that is never read + +`toml.cppm:2542` states the contract: + +> Unsupported scalar and array keys are REPORTED, not dropped. `[targets.]` +> has done this since #249; this table did not, so a key that looks plausible — +> `cxx_runtime_tests` was the real one — was accepted in silence and had no +> effect (#418). + +Measured coverage of that contract: + +| Table | Unknown key | +|---|---| +| `[build]` | reported | +| `[target.]` | reported | +| `[target..build]` | reported | +| `[runtime]` | **silent** | +| `[target..runtime]` | **silent** | +| `[package]` | **silent** | + +`[runtime]` reads ten keys individually (`toml.cppm:2001`–`2026`) with no sweep; +the `[target.]` sweep skips tables by design (`if (value.is_table()) +continue;`), which is correct for the conditional channel but leaves the keys +*inside* `[target..runtime]` swept by nobody. + +**Verdict: defect** for the two runtime tables — a stated contract enforced on +three tables and not on two others that are read the same way. The key sets are +small and closed, so the existing sweep pattern applies directly. + +`[package]` is listed but **not** bundled: whether package metadata stays open +for forward compatibility is a policy question, not an oversight, and deciding +it inside a scanner fix is how a requirement disappears. + +## 6. What is not a defect + +Two reports do not survive, and saying so is part of the plan. + +**`[target.windows.runtime]` ignored on a Linux host is correct.** The predicate +is false; the table does not apply. The report read a correctly-evaluated +predicate as a silent drop. The genuine defect nearby is §4, which the report +reached by the wrong route — its conclusion "the neutral form cannot be written +per platform" is false, and the true statement is narrower and stranger: it can, +unless it is the only thing you write. + +**mcpp has no detector for GNU-spelled `ldflags` reaching MSVC, and should not +grow one. DECIDED, and recorded here so the question is not reopened by the +next reader who meets an `LNK4044`.** `ldflags` is a documented raw escape +hatch; its contents are the author's spelling, passed through. `[runtime] libraries` is the neutral form +that exists precisely so the spelling need not be committed, and it renders +`user32.lib` or `-luser32` per dialect. A linter that inspected `ldflags` for +GNU syntax would be mcpp giving a second answer to a question the author already +answered — the exact move `scanner.cppm:616` refuses. The remaining work belongs to +`mcpp-index`, not here, and both halves of that sentence were checked before it +was left standing. + +`mcpplibs/mcpp-index` `pkgs/c/compat.glfw.lua:125` does ship +`ldflags = { "-lgdi32" }` under its `windows` section, so the report is accurate. + +**And the package can fix itself without any change to mcpp**, which is the half +worth recording because the opposite conclusion is easy to reach. An xpkg +descriptor has TWO conditional channels, and only one of them lacks the neutral +form. `target_cfg["cfg(...)"]` accepts `cflags`/`cxxflags`/`ldflags`/`sources`/ +`defines`/`flags`/the include-dir keys and nothing else — an unknown sub-key +there is a hard error — so it cannot express a per-target `libraries`. But the +`mcpp.` sections can: they already parse a nested `runtime` table with +`libraries` and `link_library_dirs`, and mcpp splices only the matching +platform's body before parsing, on the TARGET axis rather than the host's +(`synthesize_from_xpkg_lua`, axis-typed by #254). So + +```lua +windows = { runtime = { libraries = { "gdi32" } } } +``` + +is available today and renders as `gdi32.lib` for a native `cl.exe` consumer and +`-lgdi32` for a GNU one. + +Adding `runtime` to `target_cfg` as well would therefore be a change with no +demonstrated need behind it, and it is deliberately NOT made here. The package +change is left for `mcpp-index` to make on its own schedule, because dropping +the `ldflags` line has a compatibility question attached — how old a client may +still read the descriptor — that belongs with the index and not with this +record. + +**`[scan_overrides]` and `[xlings.workspace]` are capabilities, not defects.** +They are out of scope for this record. + +## 7. Repair order and criteria + +Ordering. §2.1 and §2.2 are independent of everything else and are what unblock +the consumer's upgrade. §3 is the structural repair and removes the mechanism +behind §2.2's downstream half; it should land after §2.2 so that the BOM case +is fixed at its cause rather than absorbed by the classifier change. §4 and §5 +are manifest-side and independent of all of the above. + +Criteria. Each must fail before the change and pass after; several past +regressions here passed because the assertion could not distinguish the two +worlds. + +1. **Private module fragment.** A fixture with `module : private;` in a primary + interface, built with clang, links and **runs**, asserting the program's exit + status — not merely that the scanner is quiet. Both spellings (`module : + private;` and `module :private;`). Denominator: the fixture must be listed by + the e2e index, and the test must not carry a `# requires:` that the default + shards do not satisfy — the lesson of `262`. +2. **Malformed identity.** A unit test asserting that the scanner records no + module whose name is not a well-formed module name, driven by the token, so + that it fails if the §2.1 fix is reverted while the fixture still passes. +3. **BOM.** A `.cppm` whose bytes begin `EF BB BF`, built and run. The mark is + WRITTEN BY THE TEST rather than committed, which is the opposite of what this + record first proposed: a committed fixture carrying a BOM is exactly the kind + of file an editor, a linter or a checkout filter normalises without saying so, + and the test would then pass while asserting nothing. Writing the bytes in the + script cannot be disarmed that way — and the script still verifies that the + three bytes are there before relying on them. +4. **BOM on `mcpp.toml`** — its own assertion on the message text, so it cannot + be retired by 3 shipping. +5. **Classifier/scanner.** Two assertions, because one does not imply the other: + (a) no `cxx_module` edge in any generated `build.ninja` lacks a `bmi_out` + binding — asserted over the edges, with the count of edges examined printed, + so an empty denominator is visible; (b) a project with an implementation unit + in a `.cppm` builds and runs **on gcc and on clang**, since one compiler alone + has no information about this defect. (b) must assert the program's exit + status: a criterion that stops at "the compile succeeded" passes today on gcc + and would have reported this defect as absent. +6. **Conditional runtime gate.** The minimal pair from row 4/4b: a predicate + carrying *only* a `runtime` table applies, and the control differing by one + unrelated field still applies. The negative leg is the load-bearing one. +7. **Unknown runtime keys.** Assert on `unsupported key` and the offending key + name. Not on the substring `unknown` — `x86_64-unknown-linux-gnu` contains it, + which cost one false reading while this record was being written. + +## 8. What was implemented + +Landed in mcpp 2026.9.9.1, one change per mechanism. + +| Mechanism | Change | Site | +|---|---|---| +| A.1 private fragment | the production is recognised before the partition test | `scanner.cppm` | +| A.1 identity | an identity that is not a well-formed module name is refused | `scanner.cppm` | +| A.2 BOM | a UTF-8 mark is consumed where bytes become lines; UTF-16/32 refused by name | `scanner.cppm` | +| A.2 BOM | the same rule for every TOML document mcpp reads | `libs/toml.cppm` | +| B | `module_lang` and `module_output` become PER-EDGE, bound from `providesModule` | `ninja_backend.cppm` | +| B | `moduleImplLangFlag` — the spelling for a module-extension file that is not an interface | `toolchain-model/model.cppm` | +| C | `BuildInputs::empty()` and `ConditionalConfig::empty()`, replacing two hand-written gates | `manifest/types.cppm`, `toml.cppm`, `xpkg.cppm` | +| D | unknown-key sweeps for `[runtime]` and `[target..runtime]` | `manifest/toml.cppm` | + +Reading B's row against §3: the recommendation there was A, and the emitter now +implements it in the form the measurement forced — the rule states neither flag +and every module edge states its own. `pick_rule` is unchanged, and deliberately: +an extension can answer "which rule shape", because every module-extension file +needs that rule's depfile and BMI-preservation machinery. It cannot answer "is +this an interface". Splitting the question that way is what makes the empty-flag +state unreachable rather than merely unlikely. + +`ConditionalConfig::empty()` composes rather than enumerating: it calls +`BuildInputs::empty()` and `XlingsConfig::empty()`, the latter of which already +existed. That is #258's medicine applied one level further out, and it closed a +fourth instance found while writing it — `xpkg.cppm`'s gate omitted +`privateIncludeDirs`, which its own loop fills. + +### Measurements after + +Each row was measured before the change and after, on the same machine, with +gcc 16.1.0 and clang 22.1.8. + +| Construct | 2026.9.8.1 | 2026.9.9.1 | +|---|---|---| +| `module : private;`, clang | scanner error, exit 2 | builds, runs, exit 0 | +| `module :private;`, clang | scanner error, exit 2 | builds, runs, exit 0 | +| BOM on a `.cppm` | `module 'hello' not found` at the consumer | builds, runs, exit 0 | +| BOM + private fragment | graph gains `pcm.cache/-.pcm` | builds, runs; no such edge | +| implementation unit in a `.cppm`, clang | `missing 'export' specifier` | builds, runs, exit 0 | +| the same, gcc | builds | builds | +| `[target.linux.runtime]` alone | dropped | applied | +| unknown key in `[runtime]` | silent | reported | +| BOM on `mcpp.toml` | `1:1: expected key` | builds | + +The criteria were then run against the implementation REVERTED, because a test +that passes in both worlds measures nothing. Ten of the twelve new unit tests +fail on the reverted tree and all four new e2e tests fail against the released +2026.9.8.1 binary. The two that stay green are the negative controls — +`WellFormedNamesSurviveTheIdentityGuard` and +`CorrectlySpelledRuntimeKeysAreSilent` — which exist to fail if the new refusals +are too broad, and are supposed to pass in both. + +`ConditionalConfig::empty()` replaced a gate in the descriptor reader as well +as in the manifest reader, and a descriptor is read by every consumer of the +index rather than by one project. All 228 descriptors in `mcpplibs/mcpp-index` +were parsed with both binaries and compared: 228 identical, 0 differing, 0 +errors on either side. That is the denominator the change is safe against, and +it is stated because "no published package changed behaviour" is otherwise a +claim with nothing behind it. + +One denominator is worth recording because it is empty where a reader would +expect it not to be: mcpp's own build emits **zero** `cxx_module` edges, since +every module unit in it takes the two-phase split path (`cxx_precompile` / +`cxx_module_object`, both of which are reached only after `providesModule` has +been tested). Self-hosting therefore does not exercise mechanism B at all, and +the e2e asserts a minimum edge count for exactly that reason. + +## 9. What would refute this + +The §3 recommendation rests on the claim that an extension cannot decide what a +file is. If mcpp intends `module_extensions` to mean "these files are +interfaces, and an implementation unit among them is an authoring error", then +reading B is right, symptom 2 is user error, and the repair is a diagnostic +rather than a rule change. That decision is the maintainer's; the measurement +that forces it is row 3, where the same input builds on one compiler and not the +other. Whichever reading is chosen, mcpp must hold **one** of them — today it +holds neither, and which one a build gets is decided by the compiler. diff --git a/.agents/docs/README.md b/.agents/docs/README.md index 03047ff8..13da79ea 100644 --- a/.agents/docs/README.md +++ b/.agents/docs/README.md @@ -18,7 +18,7 @@ superseded_by: 2026-09-07-....md # when status is superseded --- ``` -272 records. +273 records. ## By subject @@ -35,10 +35,15 @@ Records that declare one. Everything else is listed by date below. - [The island boundary's names: one rule for both lanes, and the check that makes it true](2026-09-08-island-boundary-names.md) — active - [Implementation plan: the island boundary's names](2026-09-08-island-boundary-names-implementation-plan.md) — active +### modules + +- [Two answers and two silences: the scanner's second grammar, and the manifest keys nothing reads](2026-09-09-two-answers-and-two-silences.md) — active + ## By date ### 2026-09 +- [Two answers and two silences: the scanner's second grammar, and the manifest keys nothing reads](2026-09-09-two-answers-and-two-silences.md) — active - [The documentation as a book: a chapter-by-chapter design](2026-09-08-the-documentation-as-a-book.md) — active - [The island boundary's names: one rule for both lanes, and the check that makes it true](2026-09-08-island-boundary-names.md) — active - [Implementation plan: the island boundary's names](2026-09-08-island-boundary-names-implementation-plan.md) — active diff --git a/CHANGELOG.md b/CHANGELOG.md index cf50f479..017056e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,75 @@ ## [Unreleased] +## [2026.9.9.1] - 2026-09-09 + +本次修复的四条缺陷,来源是同一类问题:一个问题被回答了两次,而读答案的地方各读各的; +或者一个键被解析了,却没有接到任何决定上。设计记录见 +`.agents/docs/2026-09-09-two-answers-and-two-silences.md`。 + +### `module : private;` 不再被读成实现分区 + +私有模块片段([module.private.frag])是第三种产生式,不是「名字以冒号开头的分区」: + +```cpp +module M; // 实现单元: 需要 M +module M:part; // 实现分区: 提供 M:part +module : private; // 私有模块片段:两者都不声明 +``` + +扫描器的名字分词器为了让 `M:part` 成为一个 token 而接受 `:`,分区判据又是「名字里 +含冒号」,于是这条合法的声明被拒绝在扫描期,消息是 `file already provides module 'M'; +cannot also provide ':'` —— 一句关于用户源码的假话。这是 #433 引入的回归,首发于 +v2026.8.18.1;在 clang 上,同一份源码在 2026.8.17.1 能构建并运行,在 2026.9.8.1 被拒绝。 + +编译器是否实现该特性由编译器回答:GCC 16.1 报 `sorry, unimplemented: private module +fragment`,mcpp 不在其上追加任何说法。 + +同时,扫描器不再记录任何源码不可能声明出来的模块身份。此前 BOM 与私有片段同时出现时, +构建图里会长出 `pcm.cache/-.pcm` —— 一个名字是标点的模块的 BMI,而没有任何东西报告它。 + +### 源码与 mcpp.toml 都按 UTF-8 读取,BOM 被消耗 + +MSVC 默认写出带 BOM 的 UTF-8。`trim` 用的 `std::isspace` 对那三个字节为假,于是标记 +留在首个 token 上,模块声明不再被看见。失败因此不出现在出错的那个文件上,而出现在 +消费者那里:`module 'foo' not found`。UTF-16/32 标记改为具名拒绝而不是被误读。 + +同一规则适用于 mcpp.toml:带 BOM 的清单此前报 `1:1: error: expected key`,一句关于 +token 为真、关于文件无用的话。 + +### 模块扩展名的文件不必提供模块 + +实现单元(`module M;`)是 `.cppm` 的合法居民。`module_extensions` 说的是扫描哪些文件, +不是每个文件是什么 —— 后者只有内容能回答。规则由扩展名选出,而 BMI 绑定来自扫描结果, +两者在这类文件上不一致: + +- clang:`-fmodule-output=` 空值被接受、退出 0、不写任何 BMI,失败转移到无关的消费者; +- clang:`-x c++-module` 把实现单元当接口编译,报 `missing 'export' specifier`; +- MSVC:`/ifcOutput ` 后无路径,会吃掉下一个 token。 + +GCC 的接口拼法本就是纯语言,因此同一个工程在 GCC 上能构建、在 clang 上不能 —— 这道 +可移植性裂口只有一个 flag 宽。现在这两个 flag 按边绑定,取自扫描结果,空值状态因此 +在构造上不可达。 + +### 只写 `[target.<谓词>.runtime]` 的谓词块不再被丢弃 + +`[target..runtime]` 是链接行中与方言无关的那一半,自 2026.8.29.1 起可用。但决定 +是否记录该条件块的闸门是一份手写的字段析取式,`libraries` 与 `link_library_dirs` 加入 +结构体时没有同时加入它。于是该块被解析、被填充,然后丢弃;在同一谓词下随便再写一条 +无关的 `defines` 就能让它生效。 + +闸门改为 `ConditionalConfig::empty()`,定义在字段旁边,并由 mcpp.toml 与 xpkg 两个读者 +共用;它进一步组合 `BuildInputs::empty()` 与既有的 `XlingsConfig::empty()`。同一形状的 +第四处随之关闭:xpkg 的闸门漏掉了它自己会填的 `privateIncludeDirs`。索引里 228 份 +descriptor 用新旧两个二进制解析,结果逐字节相同。 + +### `[runtime]` 与 `[target.<谓词>.runtime]` 的未知键会被报出 + +不受支持的键被报出而不是丢弃,是 `[build]`、`[target.]` 与 +`[target..build]` 自 #418 / #249 / #544 起各自遵守的规则;这两张表按同样的方式 +被读取,却没有任何东西清扫它们。消息里列出它比对用的那份键表。 +`[runtime.]` 子表是 provider 覆盖而不是键,不在清扫范围内。 + ## [2026.9.8.1] - 2026-09-08 ### 一个包的 host module,按它们互相 import 的顺序编译 diff --git a/docs/04-mcpp-toml.md b/docs/04-mcpp-toml.md index 9f6d5685..6f3945e0 100644 --- a/docs/04-mcpp-toml.md +++ b/docs/04-mcpp-toml.md @@ -854,6 +854,43 @@ mcpp = { To extend the plan-vs-ddi audit to *every* module unit (not just overrides), set `MCPP_VERIFY_MODGRAPH=1` when generating the build. +### 2.8.4 What The Default Scanner Reads + +The scanner answers two questions per file — what does this unit provide, and +what does it require — and nothing else decides them. + +**Three module declarations, and they are distinct productions.** + +```cpp +module M; // implementation unit: requires M, provides nothing +module M:part; // implementation partition: provides M:part +module : private; // private module fragment: declares neither +``` + +The third is not a partition whose name begins with a colon. It contributes no +edge in either direction, and what follows it is still part of the same unit. +Whether the compiler implements it is the compiler's answer: GCC 16.1 reports +`sorry, unimplemented: private module fragment`, and mcpp adds nothing to that. + +**A module-extension file need not provide a module.** An implementation unit +is a legal inhabitant of a `.cppm`, and `module_extensions` says which files to +*scan*, not what each one *is*. The compile mode follows the scan: a unit that +provides a module is compiled as an interface and given somewhere to write its +BMI; one that does not is compiled as an ordinary translation unit. Both +compilers therefore receive the same instruction for the same file, which they +did not before mcpp 2026.9.9.1 — Clang infers `c++-module` from the extension +and rejected the file, while GCC built it. + +**Source is read as UTF-8.** A UTF-8 byte-order mark is consumed and is not part +of the text, which is what every compiler does with one and what MSVC writes by +default. A UTF-16 or UTF-32 mark is refused by name rather than misread. The +same rule applies to `mcpp.toml`. + +**A name that no source could have declared is refused.** A module identity is a +dot-separated sequence of identifiers, optionally followed by `:` and one more +such sequence. Anything else fails the scan rather than entering the build graph, +where it would become a BMI path that nothing reports. + ### 2.9 `[profile.]` — Build Profiles ```toml @@ -965,6 +1002,12 @@ deploy_files = ["bin/widget.dll"] provider = "acme.widget-runtime@2.0.0" ``` +An unsupported key in this table is **reported and ignored**, and the message +lists the keys it checked against. A `[runtime.]` sub-table is a +provider override rather than a key, so it is not swept. The same rule applies +to `[target..runtime]`, whose vocabulary is `libraries` and +`link_library_dirs` only ([22 — The Target Side](22-target-side.md)). + `requirements` records a non-empty `kind`/`value`, a `link` or `run` phase, and whether the requirement is mandatory (`required` defaults to `true`). diff --git a/docs/22-target-side.md b/docs/22-target-side.md index a323e0d7..388b9ed9 100644 --- a/docs/22-target-side.md +++ b/docs/22-target-side.md @@ -381,7 +381,26 @@ for arch/env conditions and combinators. conditional source globs, e.g. gating `src/x86/**/*.asm` behind `cfg(arch = "x86_64")`; `!`-exclusion globs work here too), plus `flags` and `include_dirs` / `include_dirs_after` (mcpp 0.0.102+), plus - `private_include_dirs` and `std-module-flags` (mcpp 2026.9.1.1+). + `private_include_dirs` and `std-module-flags` (mcpp 2026.9.1.1+), and + `runtime` with `libraries` / `link_library_dirs` (mcpp 2026.8.29.1+). +- **`runtime` is the dialect-neutral half of a link line.** `build.ldflags` is + spelled the GNU way, and a native `cl.exe` rejects `-L`. These two keys say + the same thing without committing to a spelling: mcpp renders them as + `-L` + `-l` or `/LIBPATH:` + `.lib` according to the + target. They are the same two keys `[runtime]` (§2.11 of + [04 — mcpp.toml](04-mcpp-toml.md)) already has at the top level; this makes + them per-target and invents no vocabulary. Any other `[runtime]` key is + reported here and ignored, because the rest are not per-target. + + ```toml + # Linked only on Windows, and spelled correctly for whichever compiler builds it. + [target.windows.runtime] + libraries = ["user32", "gdi32"] + ``` + + A predicate whose only content is this table is applied like any other. Until + mcpp 2026.9.9.1 it was not: the block was parsed and then discarded unless + something else appeared under the same predicate. - **What `build` accepts is exactly the set of *additive build inputs*** — the things that combine by appending and are consumed after the predicate is evaluated, which is the member list of `BuildInputs`. `linkage`, `target`, diff --git a/docs/zh/04-mcpp-toml.md b/docs/zh/04-mcpp-toml.md index 78ee1835..2854bda0 100644 --- a/docs/zh/04-mcpp-toml.md +++ b/docs/zh/04-mcpp-toml.md @@ -744,6 +744,37 @@ mcpp = { 要把 plan 与 ddi 的比对审计扩展到**每一个**模块单元(而不只是 override), 在生成构建时设置 `MCPP_VERIFY_MODGRAPH=1`。 +### 2.8.4 默认扫描器读到的东西 + +扫描器对每个文件只回答两个问题 —— 这个单元提供什么、需要什么 —— 并且没有别的 +东西决定它们。 + +**三种模块声明,它们是不同的产生式。** + +```cpp +module M; // 实现单元: 需要 M,不提供任何东西 +module M:part; // 实现分区: 提供 M:part +module : private; // 私有模块片段:两者都不声明 +``` + +第三种不是一个名字以冒号开头的分区。它在两个方向上都不产生边,其后的内容仍属于 +同一个单元。编译器是否实现它由编译器回答:GCC 16.1 报 +`sorry, unimplemented: private module fragment`,mcpp 不在其上追加任何说法。 + +**模块扩展名的文件不必提供模块。** 实现单元是 `.cppm` 的合法居民,而 +`module_extensions` 说的是**扫描**哪些文件,不是每个文件**是**什么。编译方式跟随 +扫描结果:提供模块的单元按接口编译并获得写 BMI 的位置;不提供的按普通翻译单元 +编译。因此两个编译器对同一个文件收到相同的指令 —— 在 mcpp 2026.9.9.1 之前并非 +如此:Clang 从扩展名推断出 `c++-module` 并拒绝该文件,而 GCC 能构建它。 + +**源码按 UTF-8 读取。** UTF-8 字节序标记会被消耗,不属于正文 —— 这正是每个编译器 +对它的处理,也是 MSVC 默认写出的东西。UTF-16 或 UTF-32 标记会被具名拒绝,而不是 +被误读。同一规则适用于 `mcpp.toml`。 + +**任何源码都不可能声明出来的名字会被拒绝。** 模块身份是以点分隔的标识符序列, +其后可选地跟一个 `:` 和另一个这样的序列。此外的形式在扫描阶段失败,而不是进入 +构建图 —— 在那里它会变成一条没有任何东西会报告的 BMI 路径。 + ### 2.9 `[profile.]` — 构建档案 ```toml @@ -843,6 +874,11 @@ deploy_files = ["bin/widget.dll"] provider = "acme.widget-runtime@2.0.0" ``` +本表中不受支持的键会被**报出并忽略**,消息里列出它比对用的那份键表。 +`[runtime.]` 子表是 provider 覆盖而不是键,因此不在清扫范围内。 +同一规则适用于 `[target..runtime]`,其词汇表只有 `libraries` 与 +`link_library_dirs`(见[22 —— 目标侧](22-target-side.md))。 + `requirements` 记录非空 `kind`/`value`、`link` 或 `run` 阶段,以及是否强制 (`required` 默认 `true`)。`artifacts` 必须含 `role`、`path`、`provenance`; 可选 requirement 仍保留为 provenance,但不会进入硬 ABI/doctor 输入。 diff --git a/docs/zh/22-target-side.md b/docs/zh/22-target-side.md index 0bf0a674..d797269b 100644 --- a/docs/zh/22-target-side.md +++ b/docs/zh/22-target-side.md @@ -326,7 +326,23 @@ cxxflags = ["-march=x86-64-v2"] 的 `build`(mcpp 0.0.95+ —— 条件源码 glob,例如把 `src/x86/**/*.asm` 收在 `cfg(arch = "x86_64")` 之后;`!` 排除 glob 在此同样有效),再加 `flags` 与 `include_dirs` / `include_dirs_after`(mcpp 0.0.102+), - 以及 `private_include_dirs` 与 `std-module-flags`(mcpp 2026.9.1.1+)。 + 以及 `private_include_dirs` 与 `std-module-flags`(mcpp 2026.9.1.1+), + 还有带 `libraries` / `link_library_dirs` 的 `runtime`(mcpp 2026.8.29.1+)。 +- **`runtime` 是链接行中与方言无关的那一半。** `build.ldflags` 按 GNU 拼法书写, + 而原生 `cl.exe` 不接受 `-L`。这两个键表达同一件事而不承诺拼法:mcpp 按目标 + 渲染成 `-L` + `-l` 或 `/LIBPATH:` + `.lib`。它们就是 + 顶层 `[runtime]`(见 [04 —— mcpp.toml](04-mcpp-toml.md) §2.11)已有的同两个键, + 此处只是让它们按目标生效,并未引入新词汇。`[runtime]` 的其余键在这里会被报出 + 并忽略,因为它们不是按目标区分的。 + + ```toml + # 只在 Windows 上链接,并按实际编译器的拼法书写。 + [target.windows.runtime] + libraries = ["user32", "gdi32"] + ``` + + 一个谓词下只写这一张表时,它与其他情形一样生效。在 mcpp 2026.9.9.1 之前并非 + 如此:除非同一谓词下还写了别的东西,该块会被解析后丢弃。 - **`build` 接受的恰好是*可叠加的构建输入*集合** —— 那些以追加方式合并、 并在谓词求值之后被消费的东西,也就是 `BuildInputs` 的成员表。`linkage`、`target` 与档案开关刻意不在其中:它们是**目标选择的输入**(用一个针对 `target` 求值的谓词 diff --git a/mcpp.toml b/mcpp.toml index fa4f69c8..ded70a15 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,6 +1,6 @@ [package] name = "mcpp" -version = "2026.9.8.1" +version = "2026.9.9.1" description = "Modern C++ build & package management tool" license = "Apache-2.0" authors = ["mcpp-community"] diff --git a/modules/libs/src/toml.cppm b/modules/libs/src/toml.cppm index 0a1a80d6..a5384c84 100644 --- a/modules/libs/src/toml.cppm +++ b/modules/libs/src/toml.cppm @@ -496,6 +496,20 @@ const Table* Document::get_table(std::string_view path) const { std::expected parse(std::string_view src) { using namespace detail; + // A UTF-8 byte-order mark is not part of the document. + // + // MSVC writes one by default, so an mcpp.toml authored on Windows commonly + // begins `EF BB BF [ p a c k a g e ]`. The lexer saw those bytes as the + // start of a bare key and reported `1:1: error: expected key`, which is a + // true statement about the token and tells the author nothing about the + // file. The same mark is skipped by the source scanner for the same reason + // and with the same sentence: the mark is an encoding annotation, not text, + // and every other reader of these files skips it. + // + // Consumed here rather than in the manifest reader so that the rule holds + // for every document this parser is given — mcpp.toml, mcpp.lock, and the + // configuration files — instead of for the one that happened to report it. + if (src.starts_with("\xEF\xBB\xBF")) src.remove_prefix(3); Lexer L { src }; Table root; std::set> explicitTables; diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index f8a66c31..f650fbdd 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -2137,9 +2137,38 @@ std::expected parse_string(std::string_view content, } } // [runtime.] provider = "" — explicit provider override. + // + // The same pass reports an unsupported FLAT key. `[build]`, + // `[target.]` and `[target..build]` have each reported one + // since #418/#249/#544, under a rule this file states plainly: an + // unsupported key is REPORTED, not dropped, because a key that looks + // plausible and does nothing is the worst thing a configuration file can + // contain. `[runtime]` was read the same way and swept by nobody. + // + // SUB-TABLES ARE SKIPPED, for the reason the `[target.]` sweep + // gives: they are a channel, not a typo. Here every `[runtime.]` + // names a capability whose spelling this file cannot know. + static constexpr std::string_view kKnownRuntimeKeys[] = { + "artifacts", "capabilities", "deploy_files", "dlopen_libs", "frameworks", + "libraries", "library_dirs", "link_library_dirs", "provides", + "requirements", "runtime_search_dirs", "transitive_needed_dirs", + }; if (auto* rt = doc->get_table("runtime"); rt && !rt->empty()) { for (auto& [rk, rv] : *rt) { - if (!rv.is_table()) continue; // flat keys handled above + if (!rv.is_table()) { + if (std::ranges::find(kKnownRuntimeKeys, rk) == std::ranges::end(kKnownRuntimeKeys)) { + std::string supported; + for (auto k : kKnownRuntimeKeys) { + if (!supported.empty()) supported += ", "; + supported += k; + } + m.schemaWarnings.push_back(std::format( + "[runtime] has unsupported key '{}' (ignored). Supported " + "keys: {}. A [runtime.] table declares a " + "provider override and is not a key.", rk, supported)); + } + continue; // flat keys handled above + } auto& tt = rv.as_table(); if (auto it = tt.find("provider"); it != tt.end() && it->second.is_string()) m.runtimeConfig.providerOverrides[rk] = it->second.as_string(); @@ -2603,6 +2632,18 @@ std::expected parse_string(std::string_view content, if (auto f = rt.find("libraries"); f != rt.end() && f->second.is_array()) for (auto& v : f->second.as_array()) if (v.is_string()) cc.libraries.push_back(v.as_string()); + // Two keys, and therefore a third key is a typo. The sweep over + // `[target.]` above cannot reach here: it skips tables, + // because tables are its conditional channel — so this table's + // own keys were swept by nothing. + for (auto& [rk, _] : rt) { + if (rk == "link_library_dirs" || rk == "libraries") continue; + m.schemaWarnings.push_back(std::format( + "[target.{}.runtime] has unsupported key '{}' (ignored). " + "Supported keys: libraries, link_library_dirs. This table " + "is the dialect-neutral link intent; other [runtime] keys " + "are not per-target.", triple, rk)); + } } if (auto bit = body.find("build"); bit != body.end() && bit->second.is_table()) { auto& bt = bit->second.as_table(); @@ -2862,14 +2903,11 @@ std::expected parse_string(std::string_view content, std::vector{}); } } - if (!cc.inputs.cflags.empty() || !cc.inputs.cxxflags.empty() - || !cc.inputs.ldflags.empty() || !cc.inputs.sources.empty() - || !cc.inputs.defines.empty() - || !cc.inputs.globFlags.empty() || !cc.inputs.includeDirs.empty() - || !cc.inputs.includeDirsAfter.empty() - || !cc.dependencies.empty() || !cc.devDependencies.empty() - || !cc.buildDependencies.empty() || !cc.featureDeps.empty() - || !cc.xlings.deps.empty() || !cc.xlings.featureDeps.empty()) + // `ConditionalConfig::empty()` and not a disjunction written here: + // this list omitted `libraries` and `linkLibraryDirs`, so a + // predicate carrying only a `[target..runtime]` table was + // parsed and then dropped. See the note on that member. + if (!cc.empty()) m.conditionalConfigs.push_back(std::move(cc)); } } diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index fdf1efb0..524f0527 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -285,6 +285,17 @@ struct BuildInputs { // glibc and wrong for picolibc — and while this lived beside the package's // identity there was no spelling for that difference. std::vector stdModuleFlags; + + // Does this carry anything? The sibling of `append` below: that function + // is the one place a contribution is combined, and this is the one place + // it is weighed. Both enumerate every field, and both are here so a field + // added above is a field the compiler puts in front of a reader twice. + bool empty() const { + return sources.empty() && cflags.empty() && cxxflags.empty() + && ldflags.empty() && defines.empty() && globFlags.empty() + && includeDirs.empty() && includeDirsAfter.empty() + && privateIncludeDirs.empty() && stdModuleFlags.empty(); + } }; // The single additive merge. Every conditional axis folds through this, so @@ -1124,6 +1135,33 @@ struct ConditionalConfig { // names the project's environment, which is one per project rather than // one per target. XlingsConfig xlings; + + // Does this block say anything at all? + // + // DEFINED HERE, BESIDE THE FIELDS, AND THAT IS THE POINT. Two parsers — + // mcpp.toml and the xpkg descriptor — decide whether to record a block by + // asking this question, and both used to answer it with a hand-written + // disjunction over the fields they happened to know. `libraries` and + // `linkLibraryDirs` were added to this struct without being added to either + // list, so a predicate carrying ONLY a `[target..runtime]` table was + // parsed, populated and then dropped. Adding one unrelated `defines` entry + // under the same predicate made it work, which is what a reader would have + // to discover to explain the behaviour. + // + // That is the third time this struct has been read by a list that fell + // behind it: see the `BuildInputs` note above for #258 and the + // `featureDeps` note for #359, both of which end with the same sentence. + // #258 was repaired structurally, by carrying a whole type so the set could + // not drift; #359 was repaired by adding the missing member to the list. + // This is #258's medicine: a field added below and forgotten here is a + // question asked at the point where the field is written, instead of in two + // files that do not mention each other. + bool empty() const { + return inputs.empty() && linkLibraryDirs.empty() && libraries.empty() + && dependencies.empty() && devDependencies.empty() + && buildDependencies.empty() && featureDeps.empty() + && xlings.empty(); + } }; // `[lib]` — library "root" interface convention. diff --git a/modules/manifest/src/xpkg.cppm b/modules/manifest/src/xpkg.cppm index ef3d5f7d..285be4f0 100644 --- a/modules/manifest/src/xpkg.cppm +++ b/modules/manifest/src/xpkg.cppm @@ -1527,12 +1527,10 @@ synthesize_from_xpkg_lua(std::string_view luaContent, cur.skip_ws_and_comments(); } cur.consume('}'); - if (!cc.inputs.cflags.empty() || !cc.inputs.cxxflags.empty() - || !cc.inputs.ldflags.empty() || !cc.inputs.sources.empty() - || !cc.inputs.defines.empty() - || !cc.inputs.globFlags.empty() - || !cc.inputs.includeDirs.empty() - || !cc.inputs.includeDirsAfter.empty()) + // The same question the mcpp.toml reader asks, asked the same + // way. This list omitted `privateIncludeDirs`, which this loop + // fills. + if (!cc.empty()) m.conditionalConfigs.push_back(std::move(cc)); cur.skip_ws_and_comments(); } diff --git a/modules/toolchain-model/src/model.cppm b/modules/toolchain-model/src/model.cppm index a583c560..8229fa7d 100644 --- a/modules/toolchain-model/src/model.cppm +++ b/modules/toolchain-model/src/model.cppm @@ -383,6 +383,25 @@ struct BmiTraits { // Positional on GNU, so the emitter must place it before `-c $in`. std::string_view moduleInterfaceLangFlag; // " -x c++" | " -x c++-module" | " /interface /TP" + // How this compiler is told that a translation unit came from a + // module-interface EXTENSION and is nevertheless NOT an interface. + // + // A module-extension file need not provide a module: an implementation + // unit (`module M;`) is a legal inhabitant of a `.cppm`, and it provides + // nothing importable. The extension cannot answer that question — only the + // content can — so the emitter chooses between this flag and + // `moduleInterfaceLangFlag` from what the SCAN found, and states one of + // them on every module-extension edge. + // + // SAYING NOTHING IS NOT AN OPTION, AND THAT IS THE MEASUREMENT THIS FIELD + // EXISTS FOR. Clang's driver maps `.cppm` to `c++-module` on its own, so + // an implementation unit compiled with no `-x` at all fails with + // `missing 'export' specifier in module declaration while building module + // interface` — while GCC, whose interface flag is already the plain + // language, builds the same file. The identical project therefore built on + // one compiler and not the other, and the split was one flag wide. + std::string_view moduleImplLangFlag; // " -x c++" | " /TP" + // Non-empty ⇔ the driver can emit the BMI *and stop*, producing the SAME // BMI an ordinary compile of that TU would have produced. Both halves // matter, and the second one is the trap. @@ -533,6 +552,10 @@ BmiTraits bmi_traits(const Toolchain& tc) { // explicitly, because mcpp's interfaces are `.cppm` and cl does // not know that suffix. The other two families now match it. .moduleInterfaceLangFlag = " /interface /TP", + // `/interface` is the half that says "this is an interface"; `/TP` + // is the half that says "this is C++". A non-interface unit keeps + // the second and drops the first. + .moduleImplLangFlag = " /TP", }; } if (is_clang(tc)) { @@ -549,6 +572,7 @@ BmiTraits bmi_traits(const Toolchain& tc) { .moduleOutputPrefix = " -fmodule-output=", .bmiSearchPrefix = " -fprebuilt-module-path=", .moduleInterfaceLangFlag = " -x c++-module", + .moduleImplLangFlag = " -x c++", .bmiOnlyFlags = " --precompile -Xclang -emit-reduced-module-interface", }; } @@ -570,6 +594,9 @@ BmiTraits bmi_traits(const Toolchain& tc) { // it only needs to be told the LANGUAGE. `-x c++-module` is not a // value GCC accepts. .moduleInterfaceLangFlag = " -x c++", + // The same flag, and for GCC that is not a coincidence: since GCC reads + // interface-ness from the content, both answers are "this is C++". + .moduleImplLangFlag = " -x c++", }; } diff --git a/modules/versioning/src/version.cppm b/modules/versioning/src/version.cppm index 8444cd22..e30a29e8 100644 --- a/modules/versioning/src/version.cppm +++ b/modules/versioning/src/version.cppm @@ -31,6 +31,6 @@ import std; export namespace mcpp { -inline constexpr std::string_view MCPP_VERSION = "2026.9.8.1"; +inline constexpr std::string_view MCPP_VERSION = "2026.9.9.1"; } // namespace mcpp diff --git a/src/build/ninja_backend.cppm b/src/build/ninja_backend.cppm index 128ea6b4..d5df6b3a 100644 --- a/src/build/ninja_backend.cppm +++ b/src/build/ninja_backend.cppm @@ -801,15 +801,42 @@ std::string emit_ninja_string(const BuildPlan& plan) { // dyndep file, this prevents cascading rebuilds when only the module // implementation changed (not the interface). // - // $bmi_out is set per build edge to the BMI path (gcm.cache/.gcm). - // If $bmi_out is empty (no module provided), we just compile normally. + // $bmi_out is set per build edge to the BMI path (gcm.cache/.gcm), + // and ONLY on an edge whose unit actually provides a module. // Runtime library paths for private toolchain executables are scoped onto // the ninja subprocess instead of being emitted into each visible rule. + // + // `$module_output` AND `$module_lang` ARE PER-EDGE, AND THAT IS THE FIX + // FOR A DEFECT THIS COMMENT USED TO DESCRIBE INSTEAD OF PREVENT. + // + // Both flags used to be rule-level constants built from the toolchain, so + // every edge taking this rule got them whether or not it had a BMI. The + // rule is chosen from the file's EXTENSION (`pick_rule`) and `$bmi_out` is + // bound from what the SCAN found, so the two disagree whenever a + // module-extension file provides no module — a `.cppm` holding an + // implementation unit, or one whose declaration the scanner missed. + // + // What that produced, measured on clang 22.1.8: + // -fmodule-output= empty value, accepted, exit 0, NO BMI WRITTEN; + // the failure then surfaced at an unrelated + // consumer as `module 'x' not found`. + // -x c++-module an implementation unit compiled as an interface: + // `missing 'export' specifier in module + // declaration`. GCC's spelling is the plain + // language, so the identical project built there + // and not here. + // The old comment on this spot read "If $bmi_out is empty (no module + // provided), we just compile normally", which is what the two `[ -n ]` + // guards below do for the BACKUP and the RESTORE. The flag between them + // had no guard, and the sentence described a protection that was not + // there. + // + // Binding both from the scan result puts the empty-flag state out of + // reach by construction rather than by a shell test, and a shell test + // could not have covered the Windows branch, which has no shell. // Command spellings come from the toolchain's CommandDialect (gnu vs // msvc); the rule *structure* is shared across compilers. - std::string module_output_flag = traits.needsExplicitModuleOutput - ? std::string(traits.moduleOutputPrefix) + "$bmi_out" : ""; // msvc: /showIncludes feeds ninja's deps=msvc header tracking; the // stable-English prefix is guaranteed by VSLANG=1033 in envOverrides. const bool msvcDeps = dial.ninjaDepsMode == std::string_view("msvc"); @@ -960,13 +987,20 @@ std::string emit_ninja_string(const BuildPlan& plan) { // getting it wrong is SILENT (Clang hands an unrecognized suffix to the // linker, warns, and exits 0 having produced no BMI at all). const std::string module_src_flags{traits.moduleInterfaceLangFlag}; + // Rule-level, and correct here: every rule that reads this — cxx_module_bmi, + // cxx_precompile, cxx_module_object — is reached only through a branch that + // has already tested `cu.providesModule`, so the BMI path is never the empty + // string on those edges. `cxx_module` is the one rule with no such guard, + // and it takes `$module_output` per edge instead. + const std::string module_output_flag = traits.needsExplicitModuleOutput + ? std::string(traits.moduleOutputPrefix) + "$bmi_out" : ""; append("rule cxx_module\n"); if constexpr (mcpp::platform::is_windows) { // Windows: skip BMI restat optimization (requires POSIX shell). const std::string payload = " $local_includes"; - append(std::format(" command = $cxx{} $cxxflags $unit_cxxflags{}{} {}\n", - rsp_ref(payload), module_output_flag, - module_src_flags, compile_tail)); + append(std::format(" command = $cxx{} $cxxflags $unit_cxxflags" + " $module_output $module_lang {}\n", + rsp_ref(payload), compile_tail)); append_rspfile(payload); append_cxx_deps(); } else { @@ -977,7 +1011,8 @@ std::string emit_ninja_string(const BuildPlan& plan) { // `-x c++` / `-x c++-module` is POSITIONAL on GNU drivers: it // must precede `-c $in` (which compile_tail carries) or it // applies to nothing. - "$cxx $local_includes $cxxflags $unit_cxxflags{}{} {}{}{} && " + "$cxx $local_includes $cxxflags $unit_cxxflags" + " $module_output $module_lang {}{}{} && " // `$mcpp bmi-equal`, not `cmp -s`: GCC stamps a wall clock into // the BMI content, so a byte compare NEVER reports "unchanged" // and this whole fast path was dead. Measured: touching a module @@ -988,8 +1023,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { "mv \"$bmi_out.bak\" \"$bmi_out\"; " "else " "rm -f \"$bmi_out.bak\"; " - "fi\n", module_output_flag, module_src_flags, mmd_flag, - compile_tail, mmd_filter)); + "fi\n", mmd_flag, compile_tail, mmd_filter)); append_cxx_deps(); } append(" description = MOD $out\n"); @@ -1538,6 +1572,27 @@ std::string emit_ninja_string(const BuildPlan& plan) { // `.S` vs `.s` is the one extension test that survives, and deliberately: // it selects between two compile modes of ONE role (preprocessed or not), // which is not a role distinction. + // The two flags a `cxx_module` edge takes from WHAT THE SCAN FOUND rather + // than from the file's extension. + // + // `pick_rule` below answers "which rule shape", and an extension can answer + // that: every module-extension file needs the module rule's depfile + // handling and BMI-preservation machinery. It cannot answer "is this an + // interface", because an implementation unit (`module M;`) is a legal + // inhabitant of a `.cppm` and provides nothing. That second question is + // answered here, once, from `cu.providesModule` — the same field `bmi_out` + // is bound from, so the flag and the binding can no longer disagree. + auto module_edge_vars = [&](const mcpp::build::CompileUnit& cu) -> std::string { + if (!cu.providesModule) + return std::format(" module_lang ={}\n", traits.moduleImplLangFlag); + std::string v = std::format(" module_lang ={}\n", + traits.moduleInterfaceLangFlag); + if (traits.needsExplicitModuleOutput) + v += std::format(" module_output ={}{}\n", traits.moduleOutputPrefix, + bmi_path(*cu.providesModule)); + return v; + }; + auto pick_rule = [](const mcpp::build::CompileUnit& cu) -> std::string { switch (cu.kind) { case mcpp::SourceKind::ModuleInterface: return "cxx_module"; @@ -1934,6 +1989,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { out_line += "\n bmi_out = " + bmi_path(*cu.providesModule); } out_line += "\n"; + if (rule == "cxx_module") out_line += module_edge_vars(cu); } else { out_line += order_only_for(cu) + "\n"; } @@ -2008,6 +2064,7 @@ std::string emit_ninja_string(const BuildPlan& plan) { if (cu.providesModule) { out_line += " bmi_out = " + bmi_path(*cu.providesModule) + "\n"; } + if (rule == "cxx_module") out_line += module_edge_vars(cu); append(std::move(out_line)); } append("\n"); diff --git a/src/modgraph/scanner.cppm b/src/modgraph/scanner.cppm index d96b4c30..21b41e53 100644 --- a/src/modgraph/scanner.cppm +++ b/src/modgraph/scanner.cppm @@ -137,6 +137,92 @@ std::string_view trim(std::string_view s) { return s.substr(i, j - i); } +// A byte-order mark is not part of the source text, and every compiler that +// reads these files skips one. This scanner did not. +// +// MSVC writes UTF-8 with a BOM by default, so a `.cppm` whose first line is +// `export module foo;` commonly arrives as `EF BB BF e x p o r t …`. `trim` +// uses std::isspace, which is false for all three bytes, so the mark stayed +// attached to the first token, `starts_with("export")` failed, and the module +// declaration was not seen. What the author got was not a message about the +// file: the unit provided nothing, so a consumer failed later with +// `module 'foo' not found` — measured on clang 22.1.8, which compiles the same +// BOM'd file without comment. +// +// This is the shape of defect this scanner is most exposed to. It is a SECOND +// parser of a language whose first parser is the compiler, and where the two +// disagree the compiler is right by construction. The mark is therefore +// consumed once, here, where bytes become lines — not at call sites. +// +// A UTF-16/32 mark is refused rather than skipped. Those files are not UTF-8 +// at all, so every subsequent line would be misread; a named refusal costs one +// branch and is the difference between a message and a mystery. +std::optional consume_byte_order_mark(std::istream& is) { + // Four bytes, because UTF-32's mark is four and its first two are UTF-16's. + // Read in text mode like the rest of the scan: no mark contains 0x0D, so no + // newline translation can occur inside one, and seeking back to an absolute + // offset of 0 or 3 lands where it reads. + std::array b{}; + int got = 0; + for (; got < 4; ++got) { + const int c = is.get(); + if (c == std::char_traits::eof()) break; + b[static_cast(got)] = static_cast(c); + } + const auto rewind_to = [&](int offset) { + is.clear(); + is.seekg(offset); + }; + if (got >= 3 && b[0] == 0xEF && b[1] == 0xBB && b[2] == 0xBF) { + rewind_to(3); // the mark is consumed; the text starts here + return std::nullopt; + } + if (got >= 4 && ((b[0] == 0xFF && b[1] == 0xFE && b[2] == 0x00 && b[3] == 0x00) || + (b[0] == 0x00 && b[1] == 0x00 && b[2] == 0xFE && b[3] == 0xFF))) + return "file begins with a UTF-32 byte-order mark; mcpp reads source as " + "UTF-8. Re-save the file as UTF-8 (with or without a BOM)."; + if (got >= 2 && ((b[0] == 0xFF && b[1] == 0xFE) || (b[0] == 0xFE && b[1] == 0xFF))) + return "file begins with a UTF-16 byte-order mark; mcpp reads source as " + "UTF-8. Re-save the file as UTF-8 (with or without a BOM)."; + rewind_to(0); + return std::nullopt; +} + +// Is `name` something that can legally be a module's identity? +// +// A module-name is a dot-separated sequence of identifiers, optionally followed +// by `:` and one more such sequence. This is not a general validator — the +// compiler owns that — it exists to stop the scanner RECORDING an identity that +// no source could have declared. +// +// The need is not hypothetical. `is_module_name_char` admits `:` so that +// `M:part` scans as one token, which means a mis-parse of the `module` line can +// produce a "name" that is punctuation. Measured before this check existed: a +// file with a UTF-8 BOM and a private module fragment recorded a module called +// `:`, and the generated graph grew an edge for `pcm.cache/-.pcm` — a BMI for a +// module whose name is a colon. Nothing reported it, because every step after +// the mis-parse was working correctly on the answer it was given. +bool is_well_formed_module_name(std::string_view name) { + const auto is_identifier = [](std::string_view s) { + if (s.empty()) return false; + if (!(std::isalpha(static_cast(s[0])) || s[0] == '_')) + return false; + return std::ranges::all_of(s, [](char c) { + return std::isalnum(static_cast(c)) || c == '_'; + }); + }; + const auto is_dotted = [&](std::string_view s) { + return !s.empty() + && std::ranges::all_of(std::views::split(s, '.'), [&](auto part) { + return is_identifier(std::string_view(part)); + }); + }; + const auto colon = name.find(':'); + if (colon == std::string_view::npos) return is_dotted(name); + if (name.find(':', colon + 1) != std::string_view::npos) return false; + return is_dotted(name.substr(0, colon)) && is_dotted(name.substr(colon + 1)); +} + // Strip a trailing line comment ("//..."). std::string_view strip_line_comment(std::string_view s) { auto p = s.find("//"); @@ -648,6 +734,9 @@ std::expected scan_file(const std::filesystem::path& file return u; } + if (auto bom = consume_byte_order_mark(is)) + return std::unexpected(ScanError{file, 1, *bom}); + int if_depth = 0; // #if/#ifdef nesting std::size_t lineno = 0; bool in_raw = false; // inside a multi-line raw string @@ -693,12 +782,57 @@ std::expected scan_file(const std::filesystem::path& file if (r.empty() || r == ";") { continue; // global module fragment marker (`module;`) } + // `module : private;` — the private module fragment + // ([module.private.frag]), and a THIRD production, not a spelling + // of the two below. + // + // module M; implementation unit -> requires M + // module M:part; implementation partition -> provides M:part + // module : private; private module fragment -> neither + // + // It was read as the second, because `is_module_name_char` admits + // `:` (so that `M:part` scans as one token) and the partition test + // is `name.find(':') != npos`. The colon in this production comes + // FIRST and belongs to no name, so the test collapsed a distinct + // production into the partition case and reported + // `file already provides module 'M'; cannot also provide ':'` — a + // sentence that is false about the source it names. + // + // A regression, and a measured one: `df7a443d` (#433, first + // released in v2026.8.18.1) added the partition test, and before it + // the `!u.provides` guard on the arm below happened to let this + // line fall through. On clang, which implements the feature, the + // same file built and ran under 2026.8.17.1 and was refused at scan + // time by 2026.9.8.1. + // + // A module-declaration requires a module-name, so `module :` has + // exactly one legal continuation; anything else is malformed and is + // left to the compiler, which owns that judgement. GCC 16.1 reports + // `sorry, unimplemented: private module fragment` and mcpp adds + // nothing to that — a second answer to a question the compiler has + // already answered is what this scanner exists to avoid. + if (r.starts_with(":")) { + const auto rest = trim(r.substr(1)); + if (rest.starts_with("private") && + (rest.size() == 7 || !is_module_name_char(rest[7]))) + continue; + } std::string name; std::size_t i = 0; while (i < r.size() && is_module_name_char(r[i])) { name.push_back(r[i]); ++i; } + // Both arms below record an IDENTITY. Neither may record one that + // no source could have declared: a recorded non-name propagates + // into the build graph as a BMI path and is reported by nothing. + if (!is_well_formed_module_name(name)) { + return std::unexpected(ScanError{file, lineno, + std::format("'{}' is not a module name. A module " + "declaration is `module ;`, " + "`module :;` or " + "`module : private;`.", name)}); + } if (is_export) { if (u.provides) { return std::unexpected(ScanError{file, lineno, diff --git a/tests/e2e/634_a_private_module_fragment_is_not_a_partition.sh b/tests/e2e/634_a_private_module_fragment_is_not_a_partition.sh new file mode 100755 index 00000000..07d92d1a --- /dev/null +++ b/tests/e2e/634_a_private_module_fragment_is_not_a_partition.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# requires: gcc +# A PRIVATE MODULE FRAGMENT IS NOT AN IMPLEMENTATION PARTITION. +# +# `module : private;` ([module.private.frag]) is a third production, not a +# spelling of the two the scanner already handled: +# +# module M; implementation unit -> requires M +# module M:part; implementation partition -> provides M:part +# module : private; private module fragment -> neither +# +# It was read as the second, because the scanner's name tokeniser admits `:` +# so that `M:part` scans as one token, and the partition test is "the name +# contains a colon". The colon in this production comes FIRST and belongs to no +# name, so a valid file was refused at scan time with `file already provides +# module 'M'; cannot also provide ':'` -- a sentence that is false about the +# source it names. A regression from #433, first released in v2026.8.18.1. +# +# THE CRITERION IS THE GRAPH, NOT THE BUILD'S EXIT STATUS, and that is forced by +# the compiler rather than chosen. GCC 16.1 answers `sorry, unimplemented: +# private module fragment`, so on the toolchain this shard has, a correct mcpp +# still cannot produce a binary. What a correct mcpp does is get out of the way: +# it scans the file, emits the graph, and lets the compiler answer for its own +# feature set. Before the fix mcpp exited before writing any graph at all. +# +# Both spellings are exercised because the tokeniser treats them differently -- +# `module : private;` yields the "name" `:` and `module :private;` yields +# `:private` -- so one passing says nothing about the other. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT +cd "$TMP" + +for spelling in "module : private;" "module :private;"; do + rm -rf proj + mkdir -p proj/src + cat > proj/src/pf.cppm < proj/src/main.cpp <<'EOF' +import pf; +int main() { return pf() == 42 ? 0 : 1; } +EOF + cat > proj/mcpp.toml <<'EOF' +[package] +name = "pf" +version = "0.1.0" +EOF + + cd proj + set +e + "${MCPP:-mcpp}" build > build.log 2>&1 + build_status=$? + set -e + + # The discriminating leg. This string is what the defect emitted, and it is + # a claim about the user's source. + if grep -q "cannot also provide" build.log; then + echo "FAIL [$spelling]: the private module fragment was read as a partition" + cat build.log + exit 1 + fi + # The identity guard must not fire on it either. + if grep -q "is not a module name" build.log; then + echo "FAIL [$spelling]: the fragment was rejected as a malformed identity" + cat build.log + exit 1 + fi + + # THE POSITIVE LEG. An absence alone would also be satisfied by an mcpp that + # failed earlier for an unrelated reason, so assert the artifact a correct + # scan produces: a graph, with a module edge for this file. + graph=$(find target -name build.ninja | head -1) + [ -n "$graph" ] || { echo "FAIL [$spelling]: no build graph was written" + cat build.log; exit 1; } + grep -q "cxx_module .*pf\.cppm" "$graph" || { + echo "FAIL [$spelling]: no module edge for pf.cppm" + grep 'pf\.cppm' "$graph" || true + exit 1; } + + # Where the toolchain implements the feature (clang today, GCC when it + # lands) the program must also RUN. Where it does not, the compiler's own + # sentence is the right answer and mcpp adds nothing to it. + if [ $build_status -eq 0 ]; then + bin=$(find target -type f -name pf -perm -u+x | head -1) + [ -n "$bin" ] || { echo "FAIL [$spelling]: build succeeded with no binary"; exit 1; } + "$bin" || { echo "FAIL [$spelling]: the program did not return 42"; exit 1; } + echo "ok [$spelling]: built and ran" + else + # The failure must be the COMPILER's, at the fragment's own line. + # GCC 16.1 declines the feature twice and with two different sentences: + # `module already declared` from the p1689 scan, and `sorry, + # unimplemented: private module fragment` from codegen. Matching either + # sentence would bind this test to a wording; matching the LINE binds it + # to the claim that actually matters -- mcpp got out of the way, and + # whatever declined did so at the declaration itself. + grep -q "pf\.cppm:4" build.log || { + echo "FAIL [$spelling]: build failed, but not at the fragment" + cat build.log + exit 1; } + grep -q "scanner errors" build.log && { + echo "FAIL [$spelling]: mcpp's own scan refused the file" + cat build.log + exit 1; } + echo "ok [$spelling]: scanned; the compiler declined the feature" + fi + cd .. +done diff --git a/tests/e2e/635_a_byte_order_mark_does_not_hide_a_module.sh b/tests/e2e/635_a_byte_order_mark_does_not_hide_a_module.sh new file mode 100755 index 00000000..93f36efb --- /dev/null +++ b/tests/e2e/635_a_byte_order_mark_does_not_hide_a_module.sh @@ -0,0 +1,80 @@ +#!/usr/bin/env bash +# requires: gcc +# A UTF-8 BYTE-ORDER MARK DOES NOT HIDE A MODULE DECLARATION. +# +# MSVC writes UTF-8 with a BOM by default, so a `.cppm` whose first line is +# `export module foo;` commonly arrives as `EF BB BF e x p o r t ...`. Every +# compiler skips the mark. mcpp's scanner did not: `trim` uses isspace, which is +# false for all three bytes, so the mark stayed attached to the first token and +# the declaration was never seen. +# +# WHAT THAT COST IS THE POINT. The unit provided nothing, so nothing failed at +# the file that was wrong -- the consumer failed instead, with `module 'bom' not +# found`, which sends a reader to the import rather than to the encoding. +# +# THE MARK IS WRITTEN HERE RATHER THAN COMMITTED. A fixture file carrying a BOM +# is exactly the kind of thing an editor, a linter or a checkout filter quietly +# normalises; writing the bytes in the script means the test cannot be disarmed +# by a tool that never mentions it. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT +cd "$TMP" +mkdir -p src + +printf '\xEF\xBB\xBF' > src/bom.cppm +cat >> src/bom.cppm <<'EOF' +export module bom; +export int bom() { return 42; } +EOF + +cat > src/main.cpp <<'EOF' +import bom; +int main() { return bom() == 42 ? 0 : 1; } +EOF + +cat > mcpp.toml <<'EOF' +[package] +name = "bomtest" +version = "0.1.0" +EOF + +# The fixture must actually carry the mark, or everything below asserts nothing. +head -c 3 src/bom.cppm | od -An -tx1 | tr -d ' \n' | grep -qx 'efbbbf' || { + echo "FAIL: the fixture lost its byte-order mark; this test would pass vacuously" + exit 1; } + +"${MCPP:-mcpp}" build > build.log 2>&1 || { + echo "FAIL: a BOM'd module interface did not build" + cat build.log + exit 1; } + +# The symptom was reported at the CONSUMER, so it is named here explicitly: a +# future regression that reintroduces it must not read as a generic build error. +grep -q "imported but not provided" build.log && { + echo "FAIL: the module declaration behind the BOM was not seen" + cat build.log + exit 1; } + +bin=$(find target -type f -name bomtest -perm -u+x | head -1) +[ -n "$bin" ] || { echo "FAIL: no binary"; exit 1; } +"$bin" || { echo "FAIL: the program did not return 42"; exit 1; } + +# A manifest authored on the same editor carries the same mark. It used to +# produce `1:1: error: expected key`, a true statement about the token that +# says nothing about the file. +printf '\xEF\xBB\xBF' > mcpp2.toml +cat >> mcpp2.toml <<'EOF' +[package] +name = "bomtest" +version = "0.1.0" +EOF +mv mcpp2.toml mcpp.toml +rm -rf target +"${MCPP:-mcpp}" build > build2.log 2>&1 || { + echo "FAIL: a BOM'd mcpp.toml was not read" + cat build2.log + exit 1; } + +echo "ok: the mark is consumed by both readers" diff --git a/tests/e2e/636_a_module_extension_file_need_not_provide_a_module.sh b/tests/e2e/636_a_module_extension_file_need_not_provide_a_module.sh new file mode 100755 index 00000000..abe0b489 --- /dev/null +++ b/tests/e2e/636_a_module_extension_file_need_not_provide_a_module.sh @@ -0,0 +1,83 @@ +#!/usr/bin/env bash +# requires: gcc +# A MODULE-EXTENSION FILE NEED NOT PROVIDE A MODULE. +# +# An implementation unit (`module M;`) is a legal inhabitant of a `.cppm`, and +# it provides nothing importable. mcpp answers "is this a module interface" +# twice: the CLASSIFIER answers from the extension, and the SCANNER answers from +# the content. `pick_rule` read the first and the BMI binding read the second, +# so on a file where they disagree the edge carried an interface's flags with no +# BMI to put anywhere. +# +# THE CRITERION IS THE GRAPH, AND IT HAS TO BE. On GCC this project built before +# the fix and builds after it, because GCC's interface spelling is the plain +# language and GCC needs no explicit BMI path -- so a build-outcome assertion +# would pass in both worlds and measure nothing. What the two worlds disagree +# about is what mcpp EMITS: every module edge now states which of the two it is. +# The compiler that made the defect visible is clang, whose driver infers +# `c++-module` from the extension and rejects the implementation unit outright; +# the assertion below holds on both, which is the reason it is written this way. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT +cd "$TMP" +mkdir -p src + +cat > src/foo.cppm <<'EOF' +export module foo; +export int foo(); +EOF + +# The same extension, and not an interface. +cat > src/foo_impl.cppm <<'EOF' +module foo; +int foo() { return 5; } +EOF + +cat > src/main.cpp <<'EOF' +import foo; +int main() { return foo() == 5 ? 0 : 1; } +EOF + +cat > mcpp.toml <<'EOF' +[package] +name = "implunit" +version = "0.1.0" +EOF + +"${MCPP:-mcpp}" build > build.log 2>&1 || { + echo "FAIL: an implementation unit in a .cppm did not build" + cat build.log + exit 1; } + +bin=$(find target -type f -name implunit -perm -u+x | head -1) +[ -n "$bin" ] || { echo "FAIL: no binary"; exit 1; } +"$bin" || { echo "FAIL: the program did not return 5"; exit 1; } + +graph=$(find target -name build.ninja | head -1) +[ -n "$graph" ] || { echo "FAIL: no build graph"; exit 1; } + +# EVERY module edge states its language, AND THE DENOMINATOR IS ASSERTED. +# A ratio check whose denominator is zero reads exactly like a pass. +edges=$(grep -c ' : cxx_module ' "$graph" || true) +langs=$(grep -c '^ module_lang =' "$graph" || true) +[ "$edges" -ge 2 ] || { + echo "FAIL: expected at least 2 module edges, found $edges" + echo " (this fixture no longer exercises the question)" + grep -n 'cxx_module' "$graph" || true + exit 1; } +[ "$edges" = "$langs" ] || { + echo "FAIL: $edges module edges but $langs stated a language" + echo " an edge with no module_lang takes whatever the driver infers" + echo " from the extension, which is the defect this test exists for" + exit 1; } + +# The empty-value spelling must not appear. On clang this was accepted, exited +# 0, and wrote no BMI at all. +grep -qE -- '-fmodule-output=(\s|$)' "$graph" && { + echo "FAIL: an empty -fmodule-output= reached the graph" + grep -n -- '-fmodule-output=' "$graph" + exit 1; } + +echo "ok: $edges module edges, all $langs of them stated" diff --git a/tests/e2e/637_a_conditional_runtime_table_alone_is_applied.sh b/tests/e2e/637_a_conditional_runtime_table_alone_is_applied.sh new file mode 100755 index 00000000..d822db16 --- /dev/null +++ b/tests/e2e/637_a_conditional_runtime_table_alone_is_applied.sh @@ -0,0 +1,129 @@ +#!/usr/bin/env bash +# requires: gcc +# A PREDICATE CARRYING ONLY A RUNTIME TABLE IS STILL APPLIED. +# +# `[target..runtime]` is the dialect-neutral half of a link line: mcpp +# renders `libraries` as `-lm` or `m.lib` depending on the target, which is what +# lets one manifest name a system library for a `cl.exe` consumer and a GNU one +# alike. It has been implemented since 2026.8.29.1, and it worked -- unless it +# was the only thing written under its predicate. +# +# The gate deciding whether to keep a parsed conditional block was a hand-written +# disjunction over the fields its author knew, and `libraries` / +# `link_library_dirs` were added to the struct without being added to it. So the +# block was parsed, populated and dropped. +# +# THE TWO LEGS DIFFER IN ONE DIMENSION THAT HAS NOTHING TO DO WITH LIBRARIES, +# and that is what makes this evidence about the gate rather than about the +# predicate. The second project adds a `defines` entry -- a key the old gate did +# list -- and before the fix that single unrelated line decided whether the +# libraries were applied. +# +# It is also why no existing test caught it: the only fixture exercising the +# neutral form is a GENERATED distribution package, and those always carry +# `ldflags` as well, so the gate always passed. +set -e + +TMP=$(mktemp -d) +trap "rm -rf $TMP" EXIT +cd "$TMP" + +make_project() { # $1 = dir, $2 = extra manifest text + mkdir -p "$1/src" + cat > "$1/src/main.cpp" <<'EOF' +int main() { return 0; } +EOF + { cat <<'EOF' +[package] +name = "condrt" +version = "0.1.0" + +[target.linux.runtime] +libraries = ["m"] +EOF + printf '%s\n' "$2"; } > "$1/mcpp.toml" +} + +# Leg A: the runtime table is the ONLY thing under the predicate. +make_project alone "" +# Leg B: the control -- identical, plus one unrelated key the old gate listed. +make_project sibling '[target.linux.build] +defines = ["UNRELATED_TO_LIBRARIES"]' + +for leg in alone sibling; do + ( cd "$leg" && "${MCPP:-mcpp}" build > build.log 2>&1 ) || { + echo "FAIL [$leg]: build" + cat "$leg/build.log" + exit 1; } + graph=$(find "$leg/target" -name build.ninja | head -1) + [ -n "$graph" ] || { echo "FAIL [$leg]: no build graph"; exit 1; } + if ! grep -q -- '-lm\b' "$graph"; then + echo "FAIL [$leg]: [target.linux.runtime] libraries did not reach the link line" + if [ "$leg" = alone ]; then + echo " the block was parsed and then discarded because nothing" + echo " else was written under the same predicate" + fi + grep -n 'ldflags' "$graph" | head -3 + exit 1 + fi + echo "ok [$leg]: -lm present" +done + +# The reverse leg. A predicate that does NOT match this host must contribute +# nothing, or the two assertions above would also be satisfied by an mcpp that +# stopped evaluating predicates at all. +mkdir -p offhost/src +cat > offhost/src/main.cpp <<'EOF' +int main() { return 0; } +EOF +cat > offhost/mcpp.toml <<'EOF' +[package] +name = "condrt" +version = "0.1.0" + +[target.windows.runtime] +libraries = ["user32"] +EOF +( cd offhost && "${MCPP:-mcpp}" build > build.log 2>&1 ) || { + echo "FAIL [offhost]: build"; cat offhost/build.log; exit 1; } +graph=$(find offhost/target -name build.ninja | head -1) +grep -q -- '-luser32' "$graph" && { + echo "FAIL [offhost]: a windows predicate contributed to a linux link line" + exit 1; } +echo "ok [offhost]: a non-matching predicate contributes nothing" + +# An unsupported key in either runtime table is REPORTED, not dropped -- the rule +# [build] and [target.] have each followed since #418 and #249. +mkdir -p typo/src +cat > typo/src/main.cpp <<'EOF' +int main() { return 0; } +EOF +cat > typo/mcpp.toml <<'EOF' +[package] +name = "condrt" +version = "0.1.0" + +[runtime] +dlopen_lib = ["one"] + +[target.linux.runtime] +libraries = ["m"] +framework = ["Cocoa"] +EOF +( cd typo && "${MCPP:-mcpp}" build > build.log 2>&1 ) || { + echo "FAIL [typo]: build"; cat typo/build.log; exit 1; } +grep -q "unsupported key 'dlopen_lib'" typo/build.log || { + echo "FAIL [typo]: a plausible typo in [runtime] was accepted in silence" + cat typo/build.log + exit 1; } +grep -q "unsupported key 'framework'" typo/build.log || { + echo "FAIL [typo]: a plausible typo in [target..runtime] was accepted in silence" + cat typo/build.log + exit 1; } +# The correctly spelled sibling in the same table still took effect, so the +# sweep is not simply reporting everything. +graph=$(find typo/target -name build.ninja | head -1) +grep -q -- '-lm\b' "$graph" || { + echo "FAIL [typo]: the correctly spelled key stopped working" + exit 1; } +echo "ok [typo]: reported, and the neighbouring key still applies" diff --git a/tests/unit/test_manifest.cpp b/tests/unit/test_manifest.cpp index 6ebba0a0..1c59175c 100644 --- a/tests/unit/test_manifest.cpp +++ b/tests/unit/test_manifest.cpp @@ -5013,3 +5013,142 @@ cuda = { provides = ["gpu-blas"] } EXPECT_EQ(all.find("gpu-blas"), std::string::npos) << "a feature-provided capability is supplied; no warning is due:\n" << all; } + +// A predicate that carries ONLY a `[target..runtime]` table is recorded. +// +// It was not. The gate deciding whether to keep a ConditionalConfig was a +// hand-written disjunction over the fields the writer knew about, and +// `libraries` / `link_library_dirs` were added to the struct without being +// added to it — so the block was parsed, populated and dropped. Adding any +// unrelated key under the same predicate made it work, which is the control +// below: the two manifests differ in one dimension that has nothing to do with +// libraries, and before the fix that dimension decided the outcome. +TEST(Manifest, AConditionalRuntimeTableAloneIsRecorded) { + constexpr auto only_runtime = R"( +[package] +name = "x" +version = "0.1.0" +[target.linux.runtime] +libraries = ["dl"] +link_library_dirs = ["lib"] +)"; + auto m = mcpp::manifest::parse_string(only_runtime); + ASSERT_TRUE(m.has_value()) << m.error().format(); + ASSERT_EQ(m->conditionalConfigs.size(), 1u) + << "a runtime-only predicate was parsed and then discarded"; + ASSERT_EQ(m->conditionalConfigs[0].libraries.size(), 1u); + EXPECT_EQ(m->conditionalConfigs[0].libraries[0], "dl"); + EXPECT_EQ(m->conditionalConfigs[0].linkLibraryDirs.size(), 1u); +} + +TEST(Manifest, AConditionalRuntimeTableWithAnUnrelatedSiblingIsAlsoRecorded) { + // The control for the test above. This shape ALWAYS worked, which is why + // the defect survived: every fixture that exercised the neutral link intent + // was a generated distribution package, and those carry `ldflags` too. + constexpr auto with_sibling = R"( +[package] +name = "x" +version = "0.1.0" +[target.linux.runtime] +libraries = ["dl"] +[target.linux.build] +defines = ["UNRELATED_TO_LIBRARIES"] +)"; + auto m = mcpp::manifest::parse_string(with_sibling); + ASSERT_TRUE(m.has_value()) << m.error().format(); + ASSERT_EQ(m->conditionalConfigs.size(), 1u); + ASSERT_EQ(m->conditionalConfigs[0].libraries.size(), 1u); + EXPECT_EQ(m->conditionalConfigs[0].libraries[0], "dl"); +} + +// An unsupported key in `[runtime]` is REPORTED, not dropped — the rule +// `[build]`, `[target.]` and `[target..build]` have each followed +// since #418/#249/#544, and which these two tables did not. +TEST(Manifest, AnUnknownRuntimeKeyIsReported) { + constexpr auto src = R"( +[package] +name = "x" +version = "0.1.0" +[runtime] +libraries = ["m"] +dlopen_lib = ["one"] +)"; + auto m = mcpp::manifest::parse_string(src); + ASSERT_TRUE(m.has_value()) << m.error().format(); + ASSERT_EQ(m->schemaWarnings.size(), 1u); + EXPECT_NE(m->schemaWarnings[0].find("dlopen_lib"), std::string::npos) + << m->schemaWarnings[0]; + // The message names the list the check uses, so a reader can find the key + // they meant without leaving the message. + EXPECT_NE(m->schemaWarnings[0].find("dlopen_libs"), std::string::npos) + << m->schemaWarnings[0]; + // The correctly spelled sibling still took effect. + ASSERT_EQ(m->runtimeConfig.linkIntent.libraries.size(), 1u); +} + +TEST(Manifest, AnUnknownConditionalRuntimeKeyIsReported) { + constexpr auto src = R"( +[package] +name = "x" +version = "0.1.0" +[target.linux.runtime] +libraries = ["dl"] +frameworks = ["Cocoa"] +)"; + auto m = mcpp::manifest::parse_string(src); + ASSERT_TRUE(m.has_value()) << m.error().format(); + ASSERT_EQ(m->schemaWarnings.size(), 1u); + // `frameworks` is a real `[runtime]` key and not a per-target one, so the + // plausible-looking case is the one held here. + EXPECT_NE(m->schemaWarnings[0].find("frameworks"), std::string::npos) + << m->schemaWarnings[0]; + EXPECT_NE(m->schemaWarnings[0].find("[target.linux.runtime]"), std::string::npos) + << m->schemaWarnings[0]; +} + +// The negative control for both sweeps. Without it they pass against a parser +// that reports every key, which reads identically in a green suite. +TEST(Manifest, CorrectlySpelledRuntimeKeysAreSilent) { + constexpr auto src = R"( +[package] +name = "x" +version = "0.1.0" +[runtime] +libraries = ["m"] +library_dirs = ["lib"] +dlopen_libs = ["one"] +capabilities = ["cap"] +provides = ["thing"] +frameworks = ["Cocoa"] +link_library_dirs = ["lib"] +transitive_needed_dirs = ["lib"] +runtime_search_dirs = ["lib"] +deploy_files = ["a.txt"] +[runtime.somecapability] +provider = "pkg" +[target.linux.runtime] +libraries = ["dl"] +link_library_dirs = ["lib"] +)"; + auto m = mcpp::manifest::parse_string(src); + ASSERT_TRUE(m.has_value()) << m.error().format(); + std::string all; + for (auto const& w : m->schemaWarnings) { all += w; all += '\n'; } + EXPECT_TRUE(m->schemaWarnings.empty()) << all; + // A `[runtime.]` sub-table is a provider override, not a typo. + EXPECT_EQ(m->runtimeConfig.providerOverrides.at("somecapability"), "pkg"); +} + +// A manifest authored on Windows commonly begins with a UTF-8 byte-order mark. +// It used to reach the lexer as a bare key and produce `1:1: expected key`, a +// true statement about the token that says nothing about the file. +TEST(Manifest, AByteOrderMarkOnTheManifestIsNotAnError) { + const std::string src = + "\xEF\xBB\xBF" + "[package]\n" + "name = \"x\"\n" + "version = \"0.1.0\"\n"; + auto m = mcpp::manifest::parse_string(src); + ASSERT_TRUE(m.has_value()) << m.error().format(); + EXPECT_EQ(m->package.name, "x"); +} diff --git a/tests/unit/test_modgraph.cpp b/tests/unit/test_modgraph.cpp index 23bae543..b0ea91a5 100644 --- a/tests/unit/test_modgraph.cpp +++ b/tests/unit/test_modgraph.cpp @@ -1017,3 +1017,106 @@ TEST(Scanner, GlobWalkSurvivesNamesTheCodePageCannotSpell) { std::filesystem::remove_all(dir); #endif } + +// `module : private;` is a THIRD production, not a spelling of the two the +// scanner already knew ([module.private.frag]). It declares nothing: the unit +// still provides what its `export module` line said, and requires nothing new. +// +// It was read as an implementation partition, because `is_module_name_char` +// admits `:` and the partition test is `name.find(':') != npos`. Both spellings +// are checked because the scanner tokenises them differently — `module : +// private;` yields the "name" `:` and `module :private;` yields `:private` — +// and one of the two passing proves nothing about the other. +TEST(Scanner, PrivateModuleFragmentDeclaresNothing) { + for (std::string_view spelling : {"module : private;", "module :private;"}) { + auto dir = make_tempdir("mcpp-privfrag"); + write(dir / "src" / "pf.cppm", + std::format("export module pf;\n" + "export int pf();\n" + "{}\n" + "int pf() {{ return 1; }}\n", spelling)); + + auto u = scan_file(dir / "src" / "pf.cppm", "pkg", + mcpp::builtin_extension_table()); + ASSERT_TRUE(u.has_value()) << spelling << ": " << u.error().format(); + ASSERT_TRUE(u->provides.has_value()) << spelling; + EXPECT_EQ(u->provides->logicalName, "pf") << spelling; + EXPECT_TRUE(u->providesInterface) << spelling; + // The fragment contributes no edge in either direction. + EXPECT_TRUE(u->requires_.empty()) << spelling; + } +} + +// A UTF-8 byte-order mark is not part of the source text. MSVC writes one by +// default, so this is ordinary input; every compiler skips it, and this scanner +// did not — the mark stayed attached to the first token, the declaration was +// not seen, and the unit silently provided nothing. +TEST(Scanner, ByteOrderMarkDoesNotHideTheModuleDeclaration) { + auto dir = make_tempdir("mcpp-bom"); + write(dir / "src" / "bom.cppm", + "\xEF\xBB\xBF" + "export module bom;\n" + "import std;\n" + "export int bom();\n"); + + auto u = scan_file(dir / "src" / "bom.cppm", "pkg", + mcpp::builtin_extension_table()); + ASSERT_TRUE(u.has_value()) << u.error().format(); + ASSERT_TRUE(u->provides.has_value()) + << "the BOM hid the module declaration"; + EXPECT_EQ(u->provides->logicalName, "bom"); + ASSERT_EQ(u->requires_.size(), 1u); + EXPECT_EQ(u->requires_[0].logicalName, "std"); +} + +// UTF-16/32 is refused by name rather than misread. Every line after the mark +// would be garbage, so a named refusal is the difference between a message and +// a mystery. +TEST(Scanner, Utf16ByteOrderMarkIsRefusedByName) { + auto dir = make_tempdir("mcpp-bom16"); + write(dir / "src" / "u16.cppm", std::string_view("\xFF\xFE" "e\0x\0p\0", 8)); + + auto u = scan_file(dir / "src" / "u16.cppm", "pkg", + mcpp::builtin_extension_table()); + ASSERT_FALSE(u.has_value()); + EXPECT_NE(u.error().message.find("UTF-16"), std::string::npos) + << u.error().message; +} + +// The scanner must never RECORD an identity no source could have declared. +// +// `is_module_name_char` admits `:` so `M:part` scans as one token, so a +// mis-parse of the `module` line can produce a "name" that is punctuation. +// Measured before this guard existed: a file combining a BOM with a private +// module fragment recorded a module called `:`, and the build graph grew an +// edge for a BMI named after a colon. Nothing reported it — every step after +// the mis-parse worked correctly on the answer it was given. +TEST(Scanner, AModuleIdentityThatIsNotANameIsRefused) { + auto dir = make_tempdir("mcpp-badname"); + write(dir / "src" / "bad.cppm", "module :not_private;\n"); + + auto u = scan_file(dir / "src" / "bad.cppm", "pkg", + mcpp::builtin_extension_table()); + ASSERT_FALSE(u.has_value()); + EXPECT_NE(u.error().message.find("is not a module name"), std::string::npos) + << u.error().message; +} + +// The guard above must not refuse what the language allows. A partition, a +// dotted name and a dotted partition are all names. +TEST(Scanner, WellFormedNamesSurviveTheIdentityGuard) { + struct Case { std::string_view decl; std::string_view provides; }; + for (auto [decl, provides] : { + Case{"export module a.b.c;", "a.b.c"}, + Case{"export module a.b:part;", "a.b:part"}, + Case{"export module m:p.q;", "m:p.q"}, + Case{"export module _u9;", "_u9"}}) { + auto dir = make_tempdir("mcpp-goodname"); + write(dir / "src" / "g.cppm", std::format("{}\n", decl)); + auto u = scan_file(dir / "src" / "g.cppm", "pkg", + mcpp::builtin_extension_table()); + ASSERT_TRUE(u.has_value()) << decl << ": " << u.error().format(); + ASSERT_TRUE(u->provides.has_value()) << decl; + EXPECT_EQ(u->provides->logicalName, provides) << decl; + } +} diff --git a/tests/unit/test_ninja_backend.cpp b/tests/unit/test_ninja_backend.cpp index dd4921c8..e9362f72 100644 --- a/tests/unit/test_ninja_backend.cpp +++ b/tests/unit/test_ninja_backend.cpp @@ -1919,3 +1919,134 @@ TEST(ActionDepfile, NoDepfileEmitsNeitherLine) { EXPECT_EQ(block.find("depfile ="), std::string::npos) << block; EXPECT_EQ(block.find("deps = gcc"), std::string::npos) << block; } + +// A module-extension file need not provide a module: an implementation unit +// (`module M;`) is a legal inhabitant of a `.cppm` and provides nothing +// importable. The rule is chosen from the EXTENSION and the BMI binding comes +// from the SCAN, so the two disagree exactly there — and both flags that depend +// on the answer used to be rule-level constants applied to every edge alike. +// +// Measured on clang 22.1.8 before this changed: +// -fmodule-output= with no value: accepted, exit 0, no BMI written, and the +// failure surfaced at an unrelated consumer. +// -x c++-module on an implementation unit: `missing 'export' specifier +// in module declaration`. GCC's interface spelling is the +// plain language, so the identical project built on GCC +// and not on Clang. +TEST(NinjaBackend, ModuleEdgeFlagsComeFromTheScanNotTheExtension) { + auto plan = minimal_plan(); + plan.toolchain.compiler = mcpp::toolchain::CompilerId::Clang; + plan.toolchain.binaryPath = "/usr/bin/clang++"; + plan.compileUnits.push_back({ + .source = "src/iface.cppm", + .kind = mcpp::SourceKind::ModuleInterface, + .object = "obj/iface.o", + .packageName = "objc_rule_test", + }); + plan.compileUnits.back().providesModule = "iface"; + plan.compileUnits.push_back({ + .source = "src/impl.cppm", + .kind = mcpp::SourceKind::ModuleInterface, + .object = "obj/impl.o", + .packageName = "objc_rule_test", + }); // provides nothing: an implementation unit in a module extension + + auto ninja = emit_ninja_string(plan); + + // THE RULE STATES NEITHER FLAG. Holding this is what stops the fix from + // being undone by someone restoring a rule-level constant that "works" + // for every edge that happens to provide a module. + auto rule_start = ninja.find("rule cxx_module\n"); + ASSERT_NE(rule_start, std::string::npos) << ninja; + auto rule = ninja.substr(rule_start, ninja.find("\n\n", rule_start) - rule_start); + EXPECT_NE(rule.find("$module_output $module_lang"), std::string::npos) << rule; + EXPECT_EQ(rule.find("-fmodule-output=$bmi_out"), std::string::npos) + << "the BMI flag is per-edge; a rule-level one reaches edges with no BMI"; + EXPECT_EQ(rule.find("-x c++-module"), std::string::npos) + << "the interface spelling is per-edge, not a property of the rule"; + + // EVERY cxx_module EDGE BINDS module_lang, AND THE DENOMINATOR IS ASSERTED. + // A count-based check whose denominator is zero reads exactly like a pass. + std::size_t edges = 0, with_lang = 0; + for (std::size_t pos = 0; (pos = ninja.find(" : cxx_module ", pos)) != std::string::npos; ) { + ++edges; + auto end = ninja.find("\nbuild ", pos); + if (end == std::string::npos) end = ninja.size(); + if (ninja.substr(pos, end - pos).find("\n module_lang =") != std::string::npos) + ++with_lang; + pos = end; + } + ASSERT_EQ(edges, 2u) << ninja; + EXPECT_EQ(with_lang, edges) << ninja; + + // The interface: told it is an interface, and given somewhere to put the BMI. + EXPECT_NE(ninja.find(" module_lang = -x c++-module\n"), std::string::npos) << ninja; + EXPECT_NE(ninja.find(" module_output = -fmodule-output=pcm.cache/iface.pcm\n"), + std::string::npos) << ninja; + // The implementation unit: told it is C++, and given no BMI path at all. + EXPECT_NE(ninja.find(" module_lang = -x c++\n"), std::string::npos) << ninja; + EXPECT_EQ(count_occurrences(ninja, " module_output ="), 1u) + << "only the unit that provides a module names a BMI"; + // The empty-value spelling must not appear anywhere in the graph. + EXPECT_EQ(ninja.find("-fmodule-output= "), std::string::npos) << ninja; + EXPECT_EQ(ninja.find("-fmodule-output=\n"), std::string::npos) << ninja; +} + +// GCC needs no explicit module output at all, so the same two units must emit +// `module_lang` twice and `module_output` never. Without this the test above +// would leave "the GCC branch still emits a flag it does not want" unmeasured. +TEST(NinjaBackend, GccModuleEdgesCarryTheLanguageAndNoBmiFlag) { + auto plan = minimal_plan(); // GCC + plan.compileUnits.push_back({ + .source = "src/iface.cppm", + .kind = mcpp::SourceKind::ModuleInterface, + .object = "obj/iface.o", + .packageName = "objc_rule_test", + }); + plan.compileUnits.back().providesModule = "iface"; + plan.compileUnits.push_back({ + .source = "src/impl.cppm", + .kind = mcpp::SourceKind::ModuleInterface, + .object = "obj/impl.o", + .packageName = "objc_rule_test", + }); + + auto ninja = emit_ninja_string(plan); + EXPECT_EQ(count_occurrences(ninja, " module_lang = -x c++\n"), 2u) << ninja; + EXPECT_EQ(count_occurrences(ninja, " module_output ="), 0u) << ninja; +} + +// The third dialect, and the one where the empty value was worst. `/ifcOutput` +// takes its path as a SEPARATE token, so `/ifcOutput ` followed by nothing +// consumed whatever came next on the command line -- the observed failure was +// `could not open output file '/interface'`, naming a flag as a filename. +TEST(NinjaBackend, MsvcModuleEdgesSplitTheInterfaceFlagFromTheLanguageFlag) { + auto plan = minimal_plan(); + plan.toolchain.compiler = mcpp::toolchain::CompilerId::MSVC; + plan.toolchain.binaryPath = "cl.exe"; + plan.compileUnits.push_back({ + .source = "src/iface.cppm", + .kind = mcpp::SourceKind::ModuleInterface, + .object = "obj/iface.o", + .packageName = "objc_rule_test", + }); + plan.compileUnits.back().providesModule = "iface"; + plan.compileUnits.push_back({ + .source = "src/impl.cppm", + .kind = mcpp::SourceKind::ModuleInterface, + .object = "obj/impl.o", + .packageName = "objc_rule_test", + }); + + auto ninja = emit_ninja_string(plan); + + // `/interface /TP` says both "this is an interface" and "this is C++"; a + // unit that is not an interface keeps only the second half. + EXPECT_NE(ninja.find(" module_lang = /interface /TP\n"), std::string::npos) << ninja; + EXPECT_NE(ninja.find(" module_lang = /TP\n"), std::string::npos) << ninja; + EXPECT_EQ(count_occurrences(ninja, " module_lang = /interface /TP\n"), 1u) << ninja; + // Exactly one edge names an .ifc, and it is the one that provides. + EXPECT_EQ(count_occurrences(ninja, " module_output = /ifcOutput "), 1u) << ninja; + // The trailing-space spelling with nothing after it must not exist. + EXPECT_EQ(ninja.find("/ifcOutput \n"), std::string::npos) << ninja; +} From 0b283ac933cb3856acd70d70e10e268dc126e0c3 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:49:00 +0800 Subject: [PATCH 2/8] test(manifest): the runtime sweep must not warn on what mcpp pack emits `[[runtime.artifacts]]` and `[[runtime.requirements]]` are arrays rather than tables, so they reach the unknown-key sweep instead of being skipped with the `[runtime.]` provider channel. They are what `mcpp pack` writes into every packed library, so a false positive there would warn on all of them. Measured as zero before this was added; the assertion is what keeps it zero. --- tests/unit/test_manifest.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/unit/test_manifest.cpp b/tests/unit/test_manifest.cpp index 1c59175c..e4b4b314 100644 --- a/tests/unit/test_manifest.cpp +++ b/tests/unit/test_manifest.cpp @@ -5126,6 +5126,14 @@ runtime_search_dirs = ["lib"] deploy_files = ["a.txt"] [runtime.somecapability] provider = "pkg" +[[runtime.artifacts]] +role = "interface" +path = "include" +provenance = "source" +[[runtime.requirements]] +kind = "capability" +value = "display.present" +phase = "run" [target.linux.runtime] libraries = ["dl"] link_library_dirs = ["lib"] @@ -5137,6 +5145,12 @@ link_library_dirs = ["lib"] EXPECT_TRUE(m->schemaWarnings.empty()) << all; // A `[runtime.]` sub-table is a provider override, not a typo. EXPECT_EQ(m->runtimeConfig.providerOverrides.at("somecapability"), "pkg"); + // `[[runtime.artifacts]]` and `[[runtime.requirements]]` are arrays, not + // tables, so they reach the sweep rather than being skipped with the + // provider channel. They are exactly what `mcpp pack` emits into every + // packed library, so a false positive here would warn on all of them. + EXPECT_EQ(m->runtimeConfig.artifacts.size(), 1u); + EXPECT_EQ(m->runtimeConfig.requirements.size(), 1u); } // A manifest authored on Windows commonly begins with a UTF-8 byte-order mark. From 92b1d9c6b0c0990572178df7a465af51aa9388cb Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:53:52 +0800 Subject: [PATCH 3/8] fix(scanner): the identity refusal quotes what was read, not what was recorded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tokeniser stops at the first character it does not accept, so a declaration it cannot read at all yields an EMPTY name -- and `'' is not a module name` names nothing the author can find. Measured on a non-ASCII name, where `is_module_name_char` tests bytes with std::isalnum and every byte of a UTF-8 sequence is false: the message read `''` where the source said `模块`. GCC 16.1 refuses that declaration too (`unrecognized 'MODULE-EXPORT ...'`), so this is a clearer sentence for a refusal that already existed rather than a new restriction. The message now also states what a name is. --- src/modgraph/scanner.cppm | 15 ++++++++++++++- tests/unit/test_modgraph.cpp | 24 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/modgraph/scanner.cppm b/src/modgraph/scanner.cppm index 21b41e53..d885e3a7 100644 --- a/src/modgraph/scanner.cppm +++ b/src/modgraph/scanner.cppm @@ -827,11 +827,24 @@ std::expected scan_file(const std::filesystem::path& file // no source could have declared: a recorded non-name propagates // into the build graph as a BMI path and is reported by nothing. if (!is_well_formed_module_name(name)) { + // QUOTE WHAT WAS READ, NOT WHAT WAS RECORDED. The tokeniser + // stops at the first character it does not accept, so a + // declaration it cannot read at all yields an EMPTY name — and + // `'' is not a module name` names nothing the author can find. + // The measured case is a non-ASCII name: `is_module_name_char` + // tests bytes with std::isalnum, which is false for every byte + // of a UTF-8 sequence, so `export module ;` produced + // the empty string. (GCC 16.1 refuses that declaration too, + // with `unrecognized 'MODULE-EXPORT ...'`, so this is a clearer + // sentence for the same refusal rather than a new restriction.) + const auto decl = trim(r.substr(0, r.find(';'))); return std::unexpected(ScanError{file, lineno, std::format("'{}' is not a module name. A module " "declaration is `module ;`, " "`module :;` or " - "`module : private;`.", name)}); + "`module : private;`, and a name is a " + "dot-separated sequence of ASCII identifiers.", + name.empty() ? std::string(decl) : name)}); } if (is_export) { if (u.provides) { diff --git a/tests/unit/test_modgraph.cpp b/tests/unit/test_modgraph.cpp index b0ea91a5..5301ade7 100644 --- a/tests/unit/test_modgraph.cpp +++ b/tests/unit/test_modgraph.cpp @@ -1102,6 +1102,30 @@ TEST(Scanner, AModuleIdentityThatIsNotANameIsRefused) { << u.error().message; } +// The message quotes WHAT WAS READ, not what was recorded. +// +// The tokeniser stops at the first character it does not accept, so a +// declaration it cannot read at all yields an EMPTY name, and `'' is not a +// module name` names nothing the author can find. A non-ASCII name is the +// measured case: `is_module_name_char` tests bytes with std::isalnum, false for +// every byte of a UTF-8 sequence. GCC 16.1 refuses the same declaration with +// `unrecognized 'MODULE-EXPORT ...'`, so this is a clearer sentence for a +// refusal that already existed, not a new restriction. +TEST(Scanner, AnUnreadableModuleNameIsQuotedAsWritten) { + auto dir = make_tempdir("mcpp-nonascii"); + write(dir / "src" / "u.cppm", "export module \u6a21\u5757;\n"); + + auto u = scan_file(dir / "src" / "u.cppm", "pkg", + mcpp::builtin_extension_table()); + ASSERT_FALSE(u.has_value()); + EXPECT_NE(u.error().message.find("is not a module name"), std::string::npos) + << u.error().message; + EXPECT_EQ(u.error().message.find("'' is not"), std::string::npos) + << "the message quoted an empty name: " << u.error().message; + EXPECT_NE(u.error().message.find("\u6a21\u5757"), std::string::npos) + << u.error().message; +} + // The guard above must not refuse what the language allows. A partition, a // dotted name and a dotted partition are all names. TEST(Scanner, WellFormedNamesSurviveTheIdentityGuard) { From 0e670df1bf2a655af7e1c542b437c13299163779 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:54:39 +0800 Subject: [PATCH 4/8] docs(record): the identity refusal's message was refined during implementation --- .../docs/2026-09-09-two-answers-and-two-silences.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.agents/docs/2026-09-09-two-answers-and-two-silences.md b/.agents/docs/2026-09-09-two-answers-and-two-silences.md index 789e1e35..f0c544e1 100644 --- a/.agents/docs/2026-09-09-two-answers-and-two-silences.md +++ b/.agents/docs/2026-09-09-two-answers-and-two-silences.md @@ -441,6 +441,16 @@ needs that rule's depfile and BMI-preservation machinery. It cannot answer "is this an interface". Splitting the question that way is what makes the empty-flag state unreachable rather than merely unlikely. +One refinement came out of implementing A.1 rather than out of designing it. +The identity refusal first quoted the name it had RECORDED, which is empty +whenever the tokeniser could not read the declaration at all — so a non-ASCII +module name produced `'' is not a module name`, a sentence naming nothing the +author can search for. `is_module_name_char` tests bytes with `std::isalnum`, +and every byte of a UTF-8 sequence is false. The message now quotes what was +READ. This is not a new restriction: GCC 16.1 refuses the same declaration with +`unrecognized 'MODULE-EXPORT ...'`, so the two versions of mcpp differ in which +sentence the author gets, not in whether the file builds. + `ConditionalConfig::empty()` composes rather than enumerating: it calls `BuildInputs::empty()` and `XlingsConfig::empty()`, the latter of which already existed. That is #258's medicine applied one level further out, and it closed a From 21134839a8791333d1c949609b4e3224a2b20d91 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:56:04 +0800 Subject: [PATCH 5/8] docs(record): the 228-descriptor comparison is not evidence about the gate `mcpp xpkg parse` does not print conditional configs, so it could not have shown a difference there. The statement with a denominator is that no descriptor in the index uses `target_cfg` at all (0 of 228), so the xpkg half of the change cannot alter a published package today. --- .../2026-09-09-two-answers-and-two-silences.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/.agents/docs/2026-09-09-two-answers-and-two-silences.md b/.agents/docs/2026-09-09-two-answers-and-two-silences.md index f0c544e1..9a36ef9f 100644 --- a/.agents/docs/2026-09-09-two-answers-and-two-silences.md +++ b/.agents/docs/2026-09-09-two-answers-and-two-silences.md @@ -486,9 +486,18 @@ are too broad, and are supposed to pass in both. as in the manifest reader, and a descriptor is read by every consumer of the index rather than by one project. All 228 descriptors in `mcpplibs/mcpp-index` were parsed with both binaries and compared: 228 identical, 0 differing, 0 -errors on either side. That is the denominator the change is safe against, and -it is stated because "no published package changed behaviour" is otherwise a -claim with nothing behind it. +errors on either side. + +**That number is weaker than it looks, and the real denominator is zero.** +`mcpp xpkg parse` prints the package, its versions, its standard and its source +and target counts; it does not print conditional configs, so it could not have +shown a difference in the gate even if there were one. The statement worth +making is the other one: **no descriptor in the index uses `target_cfg` at all** +(0 of 228), so the xpkg half of this change cannot alter any published package +today. It is a correctness fix for a channel nothing currently exercises. The +228-descriptor comparison still says something — the change broke no parse — +but it is not evidence about the gate, and recording it as though it were is +the failure this record is otherwise about. One denominator is worth recording because it is empty where a reader would expect it not to be: mcpp's own build emits **zero** `cxx_module` edges, since From c595e5ce68f69c5d5188bc5785f22798171faa30 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 16:59:29 +0800 Subject: [PATCH 6/8] refactor(manifest): the conditional runtime sweep prints the list it checks The message spelled its two keys as prose while the check compared two separate literals. The `[build]` sweep a few hundred lines above carries the note saying why that matters: its message was once a third hand-written copy that had drifted from both others, so the only spelling that turned the feature on was the one being reported as unsupported. --- modules/manifest/src/toml.cppm | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index f650fbdd..03e85406 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -2636,13 +2636,28 @@ std::expected parse_string(std::string_view content, // `[target.]` above cannot reach here: it skips tables, // because tables are its conditional channel — so this table's // own keys were swept by nothing. + // + // ONE LIST, USED BY THE CHECK AND PRINTED BY THE MESSAGE. The + // `[build]` sweep a few hundred lines above carries the note + // explaining why: its message was once a third hand-written copy + // and had drifted from both others, so the only spelling that + // turned the feature on was the one reported as unsupported. + static constexpr std::string_view kKnownCondRuntimeKeys[] = { + "libraries", "link_library_dirs", + }; for (auto& [rk, _] : rt) { - if (rk == "link_library_dirs" || rk == "libraries") continue; + if (std::ranges::find(kKnownCondRuntimeKeys, rk) + != std::ranges::end(kKnownCondRuntimeKeys)) continue; + std::string supported; + for (auto k : kKnownCondRuntimeKeys) { + if (!supported.empty()) supported += ", "; + supported += k; + } m.schemaWarnings.push_back(std::format( "[target.{}.runtime] has unsupported key '{}' (ignored). " - "Supported keys: libraries, link_library_dirs. This table " - "is the dialect-neutral link intent; other [runtime] keys " - "are not per-target.", triple, rk)); + "Supported keys: {}. This table is the dialect-neutral " + "link intent; other [runtime] keys are not per-target.", + triple, rk, supported)); } } if (auto bit = body.find("build"); bit != body.end() && bit->second.is_table()) { From 9aa80d739e02b5765a805875b792b01d9a00fbb9 Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:17:12 +0800 Subject: [PATCH 7/8] fix(manifest): the emptiness predicate is a free function, as its sibling is Windows CI reported `test_modgraph.cpp` failing to compile with optional:262: error: no matching constructor for initialization of '_SMF_control<_Optional_construct_base>, ...>' against `types.cppm`'s `Profile`, a struct this change never touched. The chain is `Manifest` -> `std::map` -> `Profile`'s `std::optional`, and that member's own comment already records the same breakage under clang with the MSVC standard library on an earlier occasion. The cause was making the new emptiness predicate an inline MEMBER of two structs this module exports: that changes what importers materialise from its BMI, and what it materialised here was a copy constructor that does not compile. `append`, the operation this one is the sibling of, has been a free function since it was written. `is_empty` now matches it, and BuildInputs and ConditionalConfig have exactly the member sets they had before. Also records where the four new e2e tests run. All declare `# requires: gcc`, which `run_all.sh` grants only on Linux, so the end-to-end leg is Linux-only and the cross-dialect coverage -- including the MSVC `/ifcOutput` spelling that no Linux runner can reach -- is in the unit tests, which run everywhere. --- ...2026-09-09-two-answers-and-two-silences.md | 32 +++++++++++-- CHANGELOG.md | 8 +++- modules/manifest/src/toml.cppm | 4 +- modules/manifest/src/types.cppm | 47 ++++++++++++------- modules/manifest/src/xpkg.cppm | 2 +- 5 files changed, 67 insertions(+), 26 deletions(-) diff --git a/.agents/docs/2026-09-09-two-answers-and-two-silences.md b/.agents/docs/2026-09-09-two-answers-and-two-silences.md index 9a36ef9f..29c81746 100644 --- a/.agents/docs/2026-09-09-two-answers-and-two-silences.md +++ b/.agents/docs/2026-09-09-two-answers-and-two-silences.md @@ -430,7 +430,7 @@ Landed in mcpp 2026.9.9.1, one change per mechanism. | A.2 BOM | the same rule for every TOML document mcpp reads | `libs/toml.cppm` | | B | `module_lang` and `module_output` become PER-EDGE, bound from `providesModule` | `ninja_backend.cppm` | | B | `moduleImplLangFlag` — the spelling for a module-extension file that is not an interface | `toolchain-model/model.cppm` | -| C | `BuildInputs::empty()` and `ConditionalConfig::empty()`, replacing two hand-written gates | `manifest/types.cppm`, `toml.cppm`, `xpkg.cppm` | +| C | `is_empty(BuildInputs)` and `is_empty(ConditionalConfig)`, replacing two hand-written gates | `manifest/types.cppm`, `toml.cppm`, `xpkg.cppm` | | D | unknown-key sweeps for `[runtime]` and `[target..runtime]` | `manifest/toml.cppm` | Reading B's row against §3: the recommendation there was A, and the emitter now @@ -451,12 +451,25 @@ READ. This is not a new restriction: GCC 16.1 refuses the same declaration with `unrecognized 'MODULE-EXPORT ...'`, so the two versions of mcpp differ in which sentence the author gets, not in whether the file builds. -`ConditionalConfig::empty()` composes rather than enumerating: it calls -`BuildInputs::empty()` and `XlingsConfig::empty()`, the latter of which already +`is_empty(ConditionalConfig)` composes rather than enumerating: it calls +`is_empty(BuildInputs)` and `XlingsConfig::empty()`, the latter of which already existed. That is #258's medicine applied one level further out, and it closed a fourth instance found while writing it — `xpkg.cppm`'s gate omitted `privateIncludeDirs`, which its own loop fills. +**They are free functions, and that was forced by a measurement rather than +chosen.** The first version made them members, which is the obvious shape and +the wrong one: adding an inline member to a struct this module exports changes +what importers materialise from its BMI, and `Profile` carries a +`std::optional` that is already known to break under clang with the +MSVC standard library — the note on that member records the earlier occasion. +Windows CI failed on `test_modgraph.cpp` with +`no matching constructor for _SMF_control<_Optional_construct_base<...>>`, +reported against a struct this change never touched, reached through +`Manifest` -> `std::map` -> `Profile`. `append`, the +sibling operation, has been a free function since it was written; these now +match it, and the structs' member sets are exactly what they were. + ### Measurements after Each row was measured before the change and after, on the same machine, with @@ -482,7 +495,7 @@ fail on the reverted tree and all four new e2e tests fail against the released `CorrectlySpelledRuntimeKeysAreSilent` — which exist to fail if the new refusals are too broad, and are supposed to pass in both. -`ConditionalConfig::empty()` replaced a gate in the descriptor reader as well +`is_empty(ConditionalConfig)` replaced a gate in the descriptor reader as well as in the manifest reader, and a descriptor is read by every consumer of the index rather than by one project. All 228 descriptors in `mcpplibs/mcpp-index` were parsed with both binaries and compared: 228 identical, 0 differing, 0 @@ -499,6 +512,17 @@ today. It is a correctness fix for a channel nothing currently exercises. The but it is not evidence about the gate, and recording it as though it were is the failure this record is otherwise about. +WHERE THE NEW e2e TESTS RUN, STATED RATHER THAN ASSUMED. All four declare +`# requires: gcc`, and `run_all.sh` grants that capability only in its `Linux` +branch — so they run on the Linux shards and are skipped on macOS and Windows. +That is the right boundary for what they assert (three of the four defects are +invisible on gcc, so their criteria are assertions on the emitted graph rather +than on a build outcome, and the graph is the same everywhere), but it means the +end-to-end leg exists on one platform only. The cross-dialect coverage is in the +unit tests, which construct Clang, GCC and MSVC plans explicitly and run in +every platform's unit job — including the MSVC spelling, whose empty-value +symptom (`/ifcOutput` consuming the next token) no Linux runner could reach. + One denominator is worth recording because it is empty where a reader would expect it not to be: mcpp's own build emits **zero** `cxx_module` edges, since every module unit in it takes the two-phase split path (`cxx_precompile` / diff --git a/CHANGELOG.md b/CHANGELOG.md index 017056e4..74b2bf15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,8 +62,12 @@ GCC 的接口拼法本就是纯语言,因此同一个工程在 GCC 上能构建 结构体时没有同时加入它。于是该块被解析、被填充,然后丢弃;在同一谓词下随便再写一条 无关的 `defines` 就能让它生效。 -闸门改为 `ConditionalConfig::empty()`,定义在字段旁边,并由 mcpp.toml 与 xpkg 两个读者 -共用;它进一步组合 `BuildInputs::empty()` 与既有的 `XlingsConfig::empty()`。同一形状的 +闸门改为 `is_empty(ConditionalConfig)`,与既有的 `append` 并列为自由函数,并由 mcpp.toml +与 xpkg 两个读者共用;它进一步组合 `is_empty(BuildInputs)` 与既有的 `XlingsConfig::empty()`。 +自由函数而非成员是被实测逼出来的:给这个模块导出的结构体加内联成员会改变导入方从它的 BMI +里实例化的东西,而 `Profile` 的 `std::optional` 成员在 clang + MSVC 标准库下 +本就会崩——初版写成成员,Windows CI 上 `test_modgraph.cpp` 因此编译失败,报错指向一个本次 +改动从未碰过的结构体。同一形状的 第四处随之关闭:xpkg 的闸门漏掉了它自己会填的 `privateIncludeDirs`。索引里 228 份 descriptor 用新旧两个二进制解析,结果逐字节相同。 diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index 03e85406..857aa6a9 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -2918,11 +2918,11 @@ std::expected parse_string(std::string_view content, std::vector{}); } } - // `ConditionalConfig::empty()` and not a disjunction written here: + // `is_empty(ConditionalConfig)` and not a disjunction written here: // this list omitted `libraries` and `linkLibraryDirs`, so a // predicate carrying only a `[target..runtime]` table was // parsed and then dropped. See the note on that member. - if (!cc.empty()) + if (!mcpp::manifest::is_empty(cc)) m.conditionalConfigs.push_back(std::move(cc)); } } diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index 524f0527..bc132576 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -285,17 +285,6 @@ struct BuildInputs { // glibc and wrong for picolibc — and while this lived beside the package's // identity there was no spelling for that difference. std::vector stdModuleFlags; - - // Does this carry anything? The sibling of `append` below: that function - // is the one place a contribution is combined, and this is the one place - // it is weighed. Both enumerate every field, and both are here so a field - // added above is a field the compiler puts in front of a reader twice. - bool empty() const { - return sources.empty() && cflags.empty() && cxxflags.empty() - && ldflags.empty() && defines.empty() && globFlags.empty() - && includeDirs.empty() && includeDirsAfter.empty() - && privateIncludeDirs.empty() && stdModuleFlags.empty(); - } }; // The single additive merge. Every conditional axis folds through this, so @@ -322,6 +311,27 @@ inline void append(BuildInputs& dst, const BuildInputs& src) { src.stdModuleFlags.end()); } +// Does this carry anything? The sibling of `append` above: that function is the +// one place a contribution is combined, and this is the one place it is weighed. +// Both enumerate every field, and they sit together so that a field added to +// BuildInputs is a field the reader meets twice. +// +// A FREE FUNCTION, NOT A MEMBER, AND THAT IS NOT STYLE. Adding an inline member +// to a struct exported from this module changes what importers materialise from +// its BMI, and `Profile`'s `std::optional` member is already known +// to break under clang with the MSVC standard library — see the note on that +// member for the measurement. The first version of this was a member, and it +// failed exactly there: `test_modgraph.cpp` stopped compiling with +// `no matching constructor for _SMF_control<_Optional_construct_base<...>>`, +// reported against a struct the change never touched. `append` has been a free +// function since it was written; this one matches it. +inline bool is_empty(const BuildInputs& b) { + return b.sources.empty() && b.cflags.empty() && b.cxxflags.empty() + && b.ldflags.empty() && b.defines.empty() && b.globFlags.empty() + && b.includeDirs.empty() && b.includeDirsAfter.empty() + && b.privateIncludeDirs.empty() && b.stdModuleFlags.empty(); +} + // A build-graph node declared by a build program (`mcpp:action=`). // // The architectural point (see @@ -1156,14 +1166,17 @@ struct ConditionalConfig { // This is #258's medicine: a field added below and forgotten here is a // question asked at the point where the field is written, instead of in two // files that do not mention each other. - bool empty() const { - return inputs.empty() && linkLibraryDirs.empty() && libraries.empty() - && dependencies.empty() && devDependencies.empty() - && buildDependencies.empty() && featureDeps.empty() - && xlings.empty(); - } }; +// The same question for a whole conditional block, and a free function for the +// same reason `is_empty(BuildInputs)` is one. +inline bool is_empty(const ConditionalConfig& c) { + return is_empty(c.inputs) && c.linkLibraryDirs.empty() && c.libraries.empty() + && c.dependencies.empty() && c.devDependencies.empty() + && c.buildDependencies.empty() && c.featureDeps.empty() + && c.xlings.empty(); +} + // `[lib]` — library "root" interface convention. // // Convention-over-configuration: a library package's primary module diff --git a/modules/manifest/src/xpkg.cppm b/modules/manifest/src/xpkg.cppm index 285be4f0..9f5d382d 100644 --- a/modules/manifest/src/xpkg.cppm +++ b/modules/manifest/src/xpkg.cppm @@ -1530,7 +1530,7 @@ synthesize_from_xpkg_lua(std::string_view luaContent, // The same question the mcpp.toml reader asks, asked the same // way. This list omitted `privateIncludeDirs`, which this loop // fills. - if (!cc.empty()) + if (!mcpp::manifest::is_empty(cc)) m.conditionalConfigs.push_back(std::move(cc)); cur.skip_ws_and_comments(); } From 0435c1732e1794f979218029466b290b3c576fee Mon Sep 17 00:00:00 2001 From: speak-agent <248744407+speak-agent@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:52:17 +0800 Subject: [PATCH 8/8] fix(manifest): Profile's dependency_linkage becomes two plain members Windows CI could not compile `tests/unit/test_modgraph.cpp`: optional:262: error: no matching constructor for initialization of '_SMF_control<_Optional_construct_base>, ...>' reached through `Manifest` -> `std::map` -> `Profile::dependencyLinkage`, an `std::optional` DATA MEMBER of an exported struct. `TargetEntry::sysroot`'s comment in the same file already forbids that shape and prescribes the remedy -- two plain members -- after the same error on an earlier occasion. `Profile` was the last member in the module still shaped that way. Semantics are unchanged: not declared still means "whatever [build] said", which is what the optional carried and what `dependencyLinkageDeclared` carries now. WHICH EDIT IN THIS BRANCH TIPS IT IS NOT ESTABLISHED. Two diagnoses were refuted: that the new emptiness predicate's being an inline member was the cause (made it a free function; CI failed identically), and that the runner image had changed (re-ran main's own Windows job unchanged on today's image; it passed). `test_modgraph.cpp` copies a Manifest by value on main too, so the landmine was already armed; this removes the type that cannot be copied rather than the perturbation that reached it. --- ...2026-09-09-two-answers-and-two-silences.md | 29 +++++++++++++++++-- CHANGELOG.md | 16 ++++++++++ modules/manifest/src/toml.cppm | 4 ++- modules/manifest/src/types.cppm | 29 +++++++++++++++---- src/build/prepare.cppm | 4 +-- tests/unit/test_manifest.cpp | 6 ++-- 6 files changed, 74 insertions(+), 14 deletions(-) diff --git a/.agents/docs/2026-09-09-two-answers-and-two-silences.md b/.agents/docs/2026-09-09-two-answers-and-two-silences.md index 29c81746..8ff10e5c 100644 --- a/.agents/docs/2026-09-09-two-answers-and-two-silences.md +++ b/.agents/docs/2026-09-09-two-answers-and-two-silences.md @@ -457,8 +457,33 @@ existed. That is #258's medicine applied one level further out, and it closed a fourth instance found while writing it — `xpkg.cppm`'s gate omitted `privateIncludeDirs`, which its own loop fills. -**They are free functions, and that was forced by a measurement rather than -chosen.** The first version made them members, which is the obvious shape and +**A latent defect in this module had to be removed before any of it could +land, and it is worth recording because the diagnosis went wrong twice.** +Windows CI failed to compile `tests/unit/test_modgraph.cpp` with + + optional:262: error: no matching constructor for initialization of + '_SMF_control<_Optional_construct_base>, ...>' + +reached through `Manifest` -> `std::map` -> +`Profile::dependencyLinkage`, an `std::optional` DATA MEMBER of an +exported struct. `TargetEntry::sysroot`'s comment, a few hundred lines above in +the same file, already forbids that shape and gives the remedy — two plain +members — after the same error on an earlier occasion. `Profile` was the last +member in the module still shaped that way. + +The first attempt blamed the new emptiness predicate for being an inline member +of an exported struct, and made it a free function. CI failed identically, which +refuted that. The second attempt blamed the runner image, and was refuted by +re-running main's own Windows job unchanged on today's image: it passed. So the +trigger is somewhere in this change and the cause is the member, and those are +different questions. **Which edit tips it is not established here** — the honest +statement is that any perturbation of this module's interface can, and that +removing the type which cannot be copied removes the class rather than the +instance. `test_modgraph.cpp` copies a `Manifest` by value on main too +(`scan_packages({PackageRoot{dir, m}})`), so the landmine was always armed. + +**The emptiness predicates are free functions, which was the first repair and is +kept on its own merits.** The first version made them members, which is the obvious shape and the wrong one: adding an inline member to a struct this module exports changes what importers materialise from its BMI, and `Profile` carries a `std::optional` that is already known to break under clang with the diff --git a/CHANGELOG.md b/CHANGELOG.md index 74b2bf15..0cd93b6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,22 @@ GCC 的接口拼法本就是纯语言,因此同一个工程在 GCC 上能构建 第四处随之关闭:xpkg 的闸门漏掉了它自己会填的 `privateIncludeDirs`。索引里 228 份 descriptor 用新旧两个二进制解析,结果逐字节相同。 +### `Profile.dependency_linkage` 不再是 `std::optional` 成员 + +导出结构体的 `std::optional` **数据成员**会迫使本模块的接口实例化该特化的 +特殊成员机制,在 clang + MSVC 标准库下无法编译: + +``` +optional:262: error: no matching constructor for initialization of +'_SMF_control<_Optional_construct_base>, ...>' +``` + +路径是 `Manifest` -> `std::map` -> `Profile`。同一文件里 +`TargetEntry::sysroot` 的注释早已禁止这个形状并给出修法(两个普通成员),`Profile` 是 +模块里最后一个仍是该形状的成员。现按同一修法改为 +`dependencyLinkage` + `dependencyLinkageDeclared`,语义不变:未声明仍表示「沿用 +`[build]` 的值」。 + ### `[runtime]` 与 `[target.<谓词>.runtime]` 的未知键会被报出 不受支持的键被报出而不是丢弃,是 `[build]`、`[target.]` 与 diff --git a/modules/manifest/src/toml.cppm b/modules/manifest/src/toml.cppm index 857aa6a9..9e567f5b 100644 --- a/modules/manifest/src/toml.cppm +++ b/modules/manifest/src/toml.cppm @@ -705,8 +705,10 @@ std::expected parse_string(std::string_view content, if (v.is_string()) out.push_back(v.as_string()); }; if (auto it = tt.find("dependency_linkage"); - it != tt.end() && it->second.is_string()) + it != tt.end() && it->second.is_string()) { pr.dependencyLinkage = it->second.as_string(); + pr.dependencyLinkageDeclared = true; + } read_list("cflags", pr.cflags); read_list("cxxflags", pr.cxxflags); read_list("ldflags", pr.ldflags); diff --git a/modules/manifest/src/types.cppm b/modules/manifest/src/types.cppm index bc132576..b8b3718f 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -1401,12 +1401,29 @@ struct Profile { bool strip = false; // `dependency_linkage`, per profile (#519). // - // OPTIONAL, and that is load-bearing rather than stylistic: resolving a - // profile REPLACES the whole struct with the declared one, so a plain - // value would make `[profile.dev] opt = 0` silently reset a - // `[build] dependency_linkage = "shared"` back to the field default. - // Absent means "whatever [build] said". - std::optional dependencyLinkage; + // DECLARED-OR-NOT IS LOAD-BEARING, and that is why there are two members + // rather than one: resolving a profile REPLACES the whole struct with the + // declared one, so a plain value alone would make `[profile.dev] opt = 0` + // silently reset a `[build] dependency_linkage = "shared"` back to the + // field default. Not declared means "whatever [build] said". + // + // TWO MEMBERS AND NOT AN `std::optional`, for exactly the + // reason `TargetEntry::sysroot` gives above, and this was the last member + // in the module still shaped the way that note forbids. An + // `std::optional` DATA MEMBER of an exported struct forces + // this module's interface to materialise that specialisation's + // special-member machinery, and under clang with the MSVC standard library + // it does not compile: + // + // optional:262: error: no matching constructor for initialization of + // '_SMF_control<_Optional_construct_base>, ...>' + // + // reached through `Manifest` -> `std::map` -> + // `Profile`. The error names whichever translation unit happens to copy a + // Manifest -- `tests/unit/test_modgraph.cpp` is one -- and says nothing + // about the member that caused it. + std::string dependencyLinkage; + bool dependencyLinkageDeclared = false; // Passthrough escape hatch (fixed keys, open values — I6 completeness): std::vector cflags; std::vector cxxflags; diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm index 776a6467..87e22275 100644 --- a/src/build/prepare.cppm +++ b/src/build/prepare.cppm @@ -2071,8 +2071,8 @@ prepare_build(bool print_fingerprint, // profile that does not mention it leaves `[build]` standing; a plain // value would reset it, because the block above REPLACES `pr` wholesale // with the declared profile. - if (pr.dependencyLinkage) - m->buildConfig.dependencyLinkage = *pr.dependencyLinkage; + if (pr.dependencyLinkageDeclared) + m->buildConfig.dependencyLinkage = pr.dependencyLinkage; m->buildConfig.optLevel = pr.optLevel; m->buildConfig.debug = pr.debug; m->buildConfig.lto = pr.lto; diff --git a/tests/unit/test_manifest.cpp b/tests/unit/test_manifest.cpp index e4b4b314..94ebfa09 100644 --- a/tests/unit/test_manifest.cpp +++ b/tests/unit/test_manifest.cpp @@ -3639,10 +3639,10 @@ dependency_linkage = "static" )"); ASSERT_TRUE(m); ASSERT_TRUE(m->profiles.contains("dev")); - EXPECT_FALSE(m->profiles.at("dev").dependencyLinkage.has_value()); + EXPECT_FALSE(m->profiles.at("dev").dependencyLinkageDeclared); ASSERT_TRUE(m->profiles.contains("fast")); - ASSERT_TRUE(m->profiles.at("fast").dependencyLinkage.has_value()); - EXPECT_EQ(*m->profiles.at("fast").dependencyLinkage, "static"); + ASSERT_TRUE(m->profiles.at("fast").dependencyLinkageDeclared); + EXPECT_EQ(m->profiles.at("fast").dependencyLinkage, "static"); } TEST(Manifest, ADependencyEdgeLinkageIsAClosedVocabularyToo) {