Auto merge of #9059 - ehuss:fix-unit-for-host, r=alexcrichton

Fix unit_for computation on proc-macros in shared workspace.

There was a bug where the UnitFor was not being computed properly for proc-macros in a workspace with shared dependencies, integration tests, and a mixture of various flags (`--workspace --all-targets --all-features`). The issue is that [this line](https://github.com/rust-lang/cargo/blob/faf05ac316cd71100a691799cd8da02fce6dd85d/src/cargo/core/compiler/unit_dependencies.rs#L474), which is used when attaching the implicit dependency from an integration test to its library, was using the wrong unit_for value (it was not checking if the implicit lib is a proc-macro). The consequence is that the graph could be built inconsistently, causing features to be randomly selected incorrectly if the integration test happened to be the first unit processed.

The solution here is to use a common function for transitioning the unit_for value. The with_for_host/with_host_features split was mostly a consequence of how things evolved over time, and keeping them separate wasn't really necessary.

src/cargo/core/compiler/unit_dependencies.rs

@@ -265,10 +265,7 @@ fn compute_deps(
             None => continue,
         };
         let mode = check_or_build_mode(unit.mode, lib);
-        let dep_unit_for = unit_for
-            .with_for_host(lib.for_host())
-            // If it is a custom build script, then it *only* has build dependencies.
-            .with_host_features(unit.target.is_custom_build() || lib.proc_macro());
+        let dep_unit_for = unit_for.with_dependency(unit, lib);
 
         if state.config.cli_unstable().dual_proc_macros && lib.proc_macro() && !unit.kind.is_host()
         {
@@ -417,9 +414,7 @@ fn compute_deps_doc(unit: &Unit, state: &mut State<'_, '_>) -> CargoResult<Vec<U
         // Rustdoc only needs rmeta files for regular dependencies.
         // However, for plugins/proc macros, deps should be built like normal.
         let mode = check_or_build_mode(unit.mode, lib);
-        let dep_unit_for = UnitFor::new_normal()
-            .with_for_host(lib.for_host())
-            .with_host_features(lib.proc_macro());
+        let dep_unit_for = UnitFor::new_normal().with_dependency(unit, lib);
         let lib_unit_dep = new_unit_dep(
             state,
             unit,
@@ -466,12 +461,13 @@ fn maybe_lib(
         .find(|t| t.is_linkable())
         .map(|t| {
             let mode = check_or_build_mode(unit.mode, t);
+            let dep_unit_for = unit_for.with_dependency(unit, t);
             new_unit_dep(
                 state,
                 unit,
                 &unit.pkg,
                 t,
-                unit_for,
+                dep_unit_for,
                 unit.kind.for_target(t),
                 mode,
             )

src/cargo/core/profiles.rs

@@ -1,6 +1,6 @@
-use crate::core::compiler::CompileMode;
+use crate::core::compiler::{CompileMode, Unit};
 use crate::core::resolver::features::FeaturesFor;
-use crate::core::{Feature, Features, PackageId, PackageIdSpec, Resolve, Shell};
+use crate::core::{Feature, Features, PackageId, PackageIdSpec, Resolve, Shell, Target};
 use crate::util::errors::CargoResultExt;
 use crate::util::interning::InternedString;
 use crate::util::toml::{ProfilePackageSpec, StringOrBool, TomlProfile, TomlProfiles, U32OrBool};
@@ -976,36 +976,38 @@ impl UnitFor {
         unit_for
     }
 
-    /// Returns a new copy based on `for_host` setting.
+    /// Returns a new copy updated based on the target dependency.
     ///
-    /// When `for_host` is true, this clears `panic_abort_ok` in a sticky
-    /// fashion so that all its dependencies also have `panic_abort_ok=false`.
-    /// This'll help ensure that once we start compiling for the host platform
-    /// (build scripts, plugins, proc macros, etc) we'll share the same build
-    /// graph where everything is `panic=unwind`.
-    pub fn with_for_host(self, for_host: bool) -> UnitFor {
+    /// This is where the magic happens that the host/host_features settings
+    /// transition in a sticky fashion. As the dependency graph is being
+    /// built, once those flags are set, they stay set for the duration of
+    /// that portion of tree.
+    pub fn with_dependency(self, parent: &Unit, dep_target: &Target) -> UnitFor {
+        // A build script or proc-macro transitions this to being built for the host.
+        let dep_for_host = dep_target.for_host();
+        // This is where feature decoupling of host versus target happens.
+        //
+        // Once host features are desired, they are always desired.
+        //
+        // A proc-macro should always use host features.
+        //
+        // Dependencies of a build script should use host features (subtle
+        // point: the build script itself does *not* use host features, that's
+        // why the parent is checked here, and not the dependency).
+        let host_features =
+            self.host_features || parent.target.is_custom_build() || dep_target.proc_macro();
+        // Build scripts and proc macros, and all of their dependencies are
+        // AlwaysUnwind.
+        let panic_setting = if dep_for_host {
+            PanicSetting::AlwaysUnwind
+        } else {
+            self.panic_setting
+        };
         UnitFor {
-            host: self.host || for_host,
-            host_features: self.host_features,
-            panic_setting: if for_host {
-                PanicSetting::AlwaysUnwind
-            } else {
-                self.panic_setting
-            },
-        }
-    }
-
-    /// Returns a new copy updating it whether or not it should use features
-    /// for build dependencies and proc-macros.
-    ///
-    /// This is part of the machinery responsible for handling feature
-    /// decoupling for build dependencies in the new feature resolver.
-    pub fn with_host_features(mut self, host_features: bool) -> UnitFor {
-        if host_features {
-            assert!(self.host);
+            host: self.host || dep_for_host,
+            host_features,
+            panic_setting,
         }
-        self.host_features = self.host_features || host_features;
-        self
     }
 
     /// Returns `true` if this unit is for a build script or any of its

tests/testsuite/features2.rs

@@ -2147,3 +2147,133 @@ foo v0.1.0 ([ROOT]/foo)
         .run();
     clear();
 }
+
+#[cargo_test]
+fn pm_with_int_shared() {
+    // This is a somewhat complex scenario of a proc-macro in a workspace with
+    // an integration test where the proc-macro is used for other things, and
+    // *everything* is built at once (`--workspace --all-targets
+    // --all-features`). There was a bug where the UnitFor settings were being
+    // incorrectly computed based on the order that the graph was traversed.
+    //
+    // There are some uncertainties about exactly how proc-macros should behave
+    // with `--workspace`, see https://github.com/rust-lang/cargo/issues/8312.
+    //
+    // This uses a const-eval hack to do compile-time feature checking.
+    let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [workspace]
+                members = ["foo", "pm", "shared"]
+                resolver = "2"
+            "#,
+        )
+        .file(
+            "foo/Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.1.0"
+                edition = "2018"
+
+                [dependencies]
+                pm = { path = "../pm" }
+                shared = { path = "../shared", features = ["norm-feat"] }
+            "#,
+        )
+        .file(
+            "foo/src/lib.rs",
+            r#"
+                // foo->shared always has both features set
+                const _CHECK: [(); 0] = [(); 0-!(shared::FEATS==3) as usize];
+            "#,
+        )
+        .file(
+            "pm/Cargo.toml",
+            r#"
+                [package]
+                name = "pm"
+                version = "0.1.0"
+
+                [lib]
+                proc-macro = true
+
+                [dependencies]
+                shared = { path = "../shared", features = ["host-feat"] }
+            "#,
+        )
+        .file(
+            "pm/src/lib.rs",
+            r#"
+                // pm->shared always has just host
+                const _CHECK: [(); 0] = [(); 0-!(shared::FEATS==1) as usize];
+            "#,
+        )
+        .file(
+            "pm/tests/pm_test.rs",
+            r#"
+                // integration test gets both set
+                const _CHECK: [(); 0] = [(); 0-!(shared::FEATS==3) as usize];
+            "#,
+        )
+        .file(
+            "shared/Cargo.toml",
+            r#"
+                [package]
+                name = "shared"
+                version = "0.1.0"
+
+                [features]
+                norm-feat = []
+                host-feat = []
+            "#,
+        )
+        .file(
+            "shared/src/lib.rs",
+            r#"
+                pub const FEATS: u32 = {
+                    if cfg!(feature="norm-feat") && cfg!(feature="host-feat") {
+                        3
+                    } else if cfg!(feature="norm-feat") {
+                        2
+                    } else if cfg!(feature="host-feat") {
+                        1
+                    } else {
+                        0
+                    }
+                };
+            "#,
+        )
+        .build();
+
+    p.cargo("build --workspace --all-targets --all-features -v")
+        .with_stderr_unordered(
+            "\
+[COMPILING] shared [..]
+[RUNNING] `rustc --crate-name shared [..]--crate-type lib [..]
+[RUNNING] `rustc --crate-name shared [..]--crate-type lib [..]
+[RUNNING] `rustc --crate-name shared [..]--test[..]
+[COMPILING] pm [..]
+[RUNNING] `rustc --crate-name pm [..]--crate-type proc-macro[..]
+[RUNNING] `rustc --crate-name pm [..]--test[..]
+[COMPILING] foo [..]
+[RUNNING] `rustc --crate-name foo [..]--test[..]
+[RUNNING] `rustc --crate-name pm_test [..]--test[..]
+[RUNNING] `rustc --crate-name foo [..]--crate-type lib[..]
+[FINISHED] [..]
+",
+        )
+        .run();
+
+    // And again, should stay fresh.
+    p.cargo("build --workspace --all-targets --all-features -v")
+        .with_stderr_unordered(
+            "\
+[FRESH] shared [..]
+[FRESH] pm [..]
+[FRESH] foo [..]
+[FINISHED] [..]",
+        )
+        .run();
+}