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..8ff10e5c --- /dev/null +++ b/.agents/docs/2026-09-09-two-answers-and-two-silences.md @@ -0,0 +1,567 @@ +--- +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 | `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 +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. + +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. + +`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. + +**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 +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 +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. + +`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 +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. + +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` / +`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..0cd93b6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,95 @@ ## [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` 就能让它生效。 + +闸门改为 `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 用新旧两个二进制解析,结果逐字节相同。 + +### `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.]` 与 +`[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..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); @@ -2137,9 +2139,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 +2634,33 @@ 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. + // + // 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 (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: {}. 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()) { auto& bt = bit->second.as_table(); @@ -2862,14 +2920,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()) + // `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 (!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 fdf1efb0..b8b3718f 100644 --- a/modules/manifest/src/types.cppm +++ b/modules/manifest/src/types.cppm @@ -311,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 @@ -1124,8 +1145,38 @@ 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. }; +// 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 @@ -1350,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/modules/manifest/src/xpkg.cppm b/modules/manifest/src/xpkg.cppm index ef3d5f7d..9f5d382d 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 (!mcpp::manifest::is_empty(cc)) 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/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/src/modgraph/scanner.cppm b/src/modgraph/scanner.cppm index d96b4c30..d885e3a7 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,70 @@ 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)) { + // 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;`, and a name is a " + "dot-separated sequence of ASCII identifiers.", + name.empty() ? std::string(decl) : 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..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) { @@ -5013,3 +5013,156 @@ 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" +[[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"] +)"; + 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"); + // `[[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. +// 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..5301ade7 100644 --- a/tests/unit/test_modgraph.cpp +++ b/tests/unit/test_modgraph.cpp @@ -1017,3 +1017,130 @@ 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 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) { + 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; +}