fix(features): converge feature request derivation onto the dependency edge graph (#242 transitive; #241 naming)

Sunrisepeak · Sunrisepeak · commit 0cca9e0eaa1f · 2026-07-19T06:46:13.000+08:00
Post-#242 architecture review found the feature subsystem derived a package's requested-feature set in two places that disagreed on transitive edges: resolution (mergeActiveFeatureDeps) read the true per-edge spec, but activation (apply) + the dep build.mcpp env re-derived it by scanning ONLY the root manifest's direct deps. So a transitive dep's requested features and its consumer's `default-features = false` were silently dropped — activation kept seeding a default feature (defining its macro / keeping default-gated sources) that resolution had already skipped, which at best ignores the opt-out and at worst compiles default-gated sources against a dep that was never resolved. - DependencyEdge now carries the per-edge requestedFeatures + defaultFeatures (recorded in recordDependencyEdge from the spec). - New aggregatedRequest(depPkgIndex): union requested features and OR default-features over ALL incoming edges (Cargo diamond semantics), sourced from the authoritative edge graph. Both feature activation and the dep build.mcpp env consume it. Direct-dep behavior is unchanged (root edge carries root spec); transitive edges are now honored, closing the pre-existing "transitive dep->dep feature requests not propagated" gap too. - #241 (review finding): emit MCPP_DEP_<NAME>_DIR under BOTH the dep's canonical name AND its short (namespace-stripped) name, so dep_dir("compat.zlib") and dep_dir("zlib") both resolve (code/doc no longer contradict); + a collision guard in contract_env (two names sanitizing to one var warns instead of silent last-wins). e2e 127 (transitive opt-out honored + control). All feature e2e (67/71/72/79/ 80/81/82/83/100/106/125/126) + unit 35/35 stay green.
diff --git a/src/build/build_program.cppm b/src/build/build_program.cppm
@@ -41,10 +41,12 @@ struct BuildProgramEnv {
     // root-project contract, unchanged). Dependencies point this at OUT_DIR so
     // a shared package root is never written to.
     std::filesystem::path genBase;
-    // mcpp#241: this package's resolved dependencies, as (declared-name → dir).
-    // Emitted as MCPP_DEP_<SANITIZED_NAME>_DIR so a build.mcpp can locate a
-    // dependency's payload (e.g. a data-asset package) by name instead of
-    // reverse-engineering the store layout. Same sanitizer as MCPP_FEATURE_.
+    // mcpp#241: this package's resolved dependencies, as (name → dir) pairs.
+    // The caller emits each dep under BOTH its canonical package name and its
+    // short (namespace-stripped) name, so a build.mcpp can locate a dependency's
+    // payload (e.g. a data-asset package) via either spelling. Emitted as
+    // MCPP_DEP_<SANITIZED_NAME>_DIR (same sanitizer as MCPP_FEATURE_) instead of
+    // reverse-engineering the store layout.
     std::vector<std::pair<std::string, std::filesystem::path>> depDirs;
 };
 
@@ -369,11 +371,25 @@ contract_env(const fs::path& root, const fs::path& outDir, const BuildProgramEnv
         e.emplace_back("MCPP_FEATURE_" + sanitize_feature_env(f), "1");
     }
     e.emplace_back("MCPP_FEATURES", csv);
-    // mcpp#241: per-dependency payload dir. Sanitize the DECLARED dep name (what
-    // the author wrote in `deps`) so the var name is predictable from the
-    // manifest, not from store internals.
-    for (auto const& [name, dir] : env.depDirs)
-        e.emplace_back("MCPP_DEP_" + sanitize_feature_env(name) + "_DIR", dir.string());
+    // mcpp#241: per-dependency payload dir, under MCPP_DEP_<SANITIZED_NAME>_DIR
+    // (same sanitizer as MCPP_FEATURE_ — predictable from the manifest, not
+    // store internals). Two distinct dep names can sanitize to the same var
+    // (e.g. `foo.bar` vs `foo-bar`, or a bare `zlib` vs another dep's short
+    // `zlib`); guard so a silent last-wins can't hand one dep another's dir —
+    // keep the first and warn on a conflicting value.
+    std::map<std::string, std::string> depVarValue;
+    for (auto const& [name, dir] : env.depDirs) {
+        auto var = "MCPP_DEP_" + sanitize_feature_env(name) + "_DIR";
+        auto [it, inserted] = depVarValue.try_emplace(var, dir.string());
+        if (inserted) {
+            e.emplace_back(var, dir.string());
+        } else if (it->second != dir.string()) {
+            mcpp::ui::warning(std::format(
+                "build.mcpp: dependency name collides on {} (kept '{}', ignored "
+                "'{}') — rename one dependency to disambiguate", var,
+                it->second, dir.string()));
+        }
+    }
     return e;
 }
 
diff --git a/src/build/prepare.cppm b/src/build/prepare.cppm
@@ -1851,6 +1851,14 @@ prepare_build(bool print_fingerprint,
         std::size_t dependencyPackageIndex = 0;
         mcpp::modgraph::DependencyVisibility visibility =
             mcpp::modgraph::DependencyVisibility::Public;
+        // #242/#243: the per-edge feature request that THIS consumer made of
+        // THIS dependency. Feature activation must consume these off the edge
+        // graph (union over all incoming edges) rather than re-scanning only
+        // the root manifest's direct deps — otherwise a transitive dep's
+        // requested features and its consumer's `default-features = false` are
+        // silently dropped (resolution honors them per-edge; activation did not).
+        std::vector<std::string> requestedFeatures;
+        bool defaultFeatures = true;
     };
     std::vector<DependencyEdge> dependencyEdges;
 
@@ -1961,6 +1969,8 @@ prepare_build(bool print_fingerprint,
             .consumerPackageIndex = consumerPackageIndex,
             .dependencyPackageIndex = dependencyPackageIndex,
             .visibility = visibility,
+            .requestedFeatures = spec.features,
+            .defaultFeatures = spec.defaultFeatures,
         });
     };
 
@@ -2836,17 +2846,32 @@ prepare_build(bool print_fingerprint,
             apply(packages[0], rootReq);
             for (auto& f : activate(*m, rootReq)) activeRootFeatures.insert(f);
         }
+        // #242/#243: the feature request for a dependency PACKAGE, aggregated
+        // over ALL its incoming edges (a package may be depended on by several
+        // consumers — diamond — or reached only transitively). Cargo semantics:
+        // requested features UNION; default-features stays on unless EVERY
+        // consumer opted out. Sourcing this from the authoritative edge graph —
+        // rather than scanning only the root manifest's direct deps — makes
+        // activation AGREE with resolution (mergeActiveFeatureDeps, which reads
+        // the true per-edge spec): a transitive dep's requested features and its
+        // consumer's `default-features = false` are no longer silently dropped.
+        auto aggregatedRequest = [&](std::size_t depPkgIndex)
+            -> std::pair<std::vector<std::string>, bool> {
+            std::vector<std::string> feats;
+            bool anyEdge = false, anyDefault = false;
+            for (auto const& edge : dependencyEdges) {
+                if (edge.dependencyPackageIndex != depPkgIndex) continue;
+                anyEdge = true;
+                if (edge.defaultFeatures) anyDefault = true;
+                for (auto const& f : edge.requestedFeatures)
+                    if (std::find(feats.begin(), feats.end(), f) == feats.end())
+                        feats.push_back(f);
+            }
+            return { std::move(feats), anyEdge ? anyDefault : true };
+        };
         for (std::size_t i = 1; i < packages.size(); ++i) {
             auto& pname = packages[i].manifest.package.name;
-            std::vector<std::string> req;
-            bool depDefaultFeatures = true;   // #242: consumer opt-out
-            for (auto& [dname, dspec] : m->dependencies) {
-                if (dname == pname || dspec.shortName == pname) {
-                    req = dspec.features;
-                    depDefaultFeatures = dspec.defaultFeatures;
-                    break;
-                }
-            }
+            auto [req, depDefaultFeatures] = aggregatedRequest(i);
             if (!req.empty() && !packages[i].manifest.featuresMap.empty()) {
                 for (auto& f : req) {
                     if (packages[i].manifest.featuresMap.contains(f)) continue;
@@ -2879,16 +2904,10 @@ prepare_build(bool print_fingerprint,
             if (!std::filesystem::exists(pkg.root / "build.mcpp", bpEc)) continue;
             auto host = host_tc_for_build_program();
             if (!host) return std::unexpected(host.error());
-            std::vector<std::string> req;
-            bool depDefaultFeatures = true;   // #242: consumer opt-out
-            for (auto& [dname, dspec] : m->dependencies) {
-                if (dname == pkg.manifest.package.name
-                    || dspec.shortName == pkg.manifest.package.name) {
-                    req = dspec.features;
-                    depDefaultFeatures = dspec.defaultFeatures;
-                    break;
-                }
-            }
+            // Same edge-graph aggregation as feature activation above, so a
+            // dep build.mcpp sees the SAME active feature set the dep is built
+            // with (incl. transitive requests / default-features opt-out).
+            auto [req, depDefaultFeatures] = aggregatedRequest(i);
             auto dirSafe = [](std::string s) {
                 for (auto& c : s) if (c == '/' || c == '\\' || c == ':') c = '_';
                 return s;
@@ -2901,17 +2920,24 @@ prepare_build(bool print_fingerprint,
                 / (dirSafe(pkg.manifest.package.name) + "@" + pkg.manifest.package.version);
             bpEnv.genBase      = bpEnv.artifactsDir / "out";
             // mcpp#241: expose this package's resolved dependencies (verdir /
-            // payload root) as MCPP_DEP_<NAME>_DIR, keyed by the dependency's
-            // canonical package name. Uses the authoritative consumer→dep edge
-            // graph (no name-guessing); covers feature-activated deps too, since
-            // mergeActiveFeatureDeps folded them into `dependencies` before the
-            // edges were recorded. (The ROOT project's own build.mcpp runs
+            // payload root) as MCPP_DEP_<NAME>_DIR. Uses the authoritative
+            // consumer→dep edge graph (no name-guessing); covers feature-
+            // activated deps too (mergeActiveFeatureDeps folded them into
+            // `dependencies` before the edges were recorded). A dep is emitted
+            // under BOTH its canonical package name AND its namespace-stripped
+            // short name, so `mcpp::dep_dir("compat.zlib")` and
+            // `mcpp::dep_dir("zlib")` both resolve regardless of which spelling
+            // the author used in `deps`. (The ROOT project's own build.mcpp runs
             // before dependency resolution, so it does not yet receive these —
             // tracked as a follow-up in the #230-#243 ledger.)
             for (auto const& edge : dependencyEdges) {
                 if (edge.consumerPackageIndex != i) continue;
                 auto const& depPkg = packages[edge.dependencyPackageIndex];
-                bpEnv.depDirs.emplace_back(depPkg.manifest.package.name, depPkg.root);
+                const auto& canon = depPkg.manifest.package.name;
+                bpEnv.depDirs.emplace_back(canon, depPkg.root);
+                if (auto dot = canon.rfind('.'); dot != std::string::npos
+                        && dot + 1 < canon.size())
+                    bpEnv.depDirs.emplace_back(canon.substr(dot + 1), depPkg.root);
             }
             auto& bcDep = pkg.manifest.buildConfig;
             const auto cN = bcDep.cflags.size(), cxN = bcDep.cxxflags.size(),
diff --git a/tests/e2e/127_transitive_default_features_optout.sh b/tests/e2e/127_transitive_default_features_optout.sh
@@ -0,0 +1,117 @@
+#!/usr/bin/env bash
+# requires: elf gcc
+# mcpp#242 (transitive edge, found in the 0.0.99 architecture review): a
+# consumer's `default-features = false` must be honored even when the consumer
+# is itself a DEPENDENCY (transitive edge), not only for the root's direct deps.
+#
+# Before the edge-graph convergence, feature *resolution* honored the per-edge
+# opt-out (mergeActiveFeatureDeps) but feature *activation* re-derived the flag
+# by scanning only the ROOT manifest's direct deps — so a transitive dep's
+# opt-out was silently dropped: activation still seeded the dep's default
+# feature (defining its macro / keeping its default-gated sources) that
+# resolution had already skipped. Now both consume the authoritative
+# consumer→dep edge graph, so they agree.
+#
+# Layout: root(bin) -> A(lib) -> B(lib, `default = ["heavy"]`, heavy defines
+# B_HEAVY). A depends on B with `default-features = false`. B's heavy macro must
+# therefore be OFF in the transitive build.
+set -e
+
+TMP=$(mktemp -d)
+trap "rm -rf $TMP" EXIT
+cd "$TMP"
+
+# B: a library with a default feature `heavy` that defines B_HEAVY.
+mkdir -p B/src
+cat > B/mcpp.toml <<'EOF'
+[package]
+name    = "B"
+version = "0.1.0"
+[modules]
+sources = ["src/**/*.cpp"]
+[features]
+default = ["heavy"]
+heavy   = { defines = ["B_HEAVY=1"] }
+[targets.B]
+kind = "lib"
+EOF
+cat > B/src/b.cpp <<'EOF'
+int b_heavy() {
+#ifdef B_HEAVY
+    return 1;
+#else
+    return 0;
+#endif
+}
+EOF
+
+# A: depends on B, opting OUT of B's default features.
+mkdir -p A/src
+cat > A/mcpp.toml <<'EOF'
+[package]
+name    = "A"
+version = "0.1.0"
+[modules]
+sources = ["src/**/*.cpp"]
+[dependencies]
+B = { path = "../B", default-features = false }
+[targets.A]
+kind = "lib"
+EOF
+cat > A/src/a.cpp <<'EOF'
+extern int b_heavy();
+int a_val() { return b_heavy(); }
+EOF
+
+# root consumer -> A (which transitively pulls B).
+mkdir -p app/src
+cat > app/mcpp.toml <<'EOF'
+[package]
+name    = "app"
+version = "0.1.0"
+[modules]
+sources = ["src/**/*.cpp"]
+[dependencies]
+A = { path = "../A" }
+[targets.app]
+kind = "bin"
+main = "src/main.cpp"
+EOF
+cat > app/src/main.cpp <<'EOF'
+import std;
+extern int a_val();
+int main() { std::println("heavy={}", a_val()); return 0; }
+EOF
+
+cd app
+"$MCPP" build > build.log 2>&1 || { cat build.log; echo "FAIL: build failed"; exit 1; }
+out="$("$MCPP" run 2>&1 | grep '^heavy=' | tail -1)"
+[[ "$out" == "heavy=0" ]] || {
+    echo "FAIL: transitive default-features opt-out not honored (got '$out', want heavy=0)"
+    exit 1
+}
+
+# Control: flip A to KEEP B's defaults -> heavy must come back on. Clean first:
+# changing a transitive dep's active feature set across an in-place rebuild is a
+# separate fingerprint concern; this test isolates the activation logic with a
+# fresh build.
+cat > ../A/mcpp.toml <<'EOF'
+[package]
+name    = "A"
+version = "0.1.0"
+[modules]
+sources = ["src/**/*.cpp"]
+[dependencies]
+B = { path = "../B" }
+[targets.A]
+kind = "lib"
+EOF
+"$MCPP" clean > /dev/null 2>&1 || true
+"$MCPP" build > build2.log 2>&1 || { cat build2.log; echo "FAIL: control build failed"; exit 1; }
+out2="$("$MCPP" run 2>&1 | grep '^heavy=' | tail -1)"
+[[ "$out2" == "heavy=1" ]] || {
+    echo "FAIL: control (defaults kept) expected heavy=1, got '$out2'"
+    exit 1
+}
+
+echo "OK"
