Add cargo_cfg_target_family_multivalued FCW

compiler/rustc_lint/messages.ftl

@@ -768,6 +768,16 @@ lint_supertrait_as_deref_target = this `Deref` implementation is covered by an i
 lint_surrogate_char_cast = surrogate values are not valid for `char`
     .note = `0xD800..=0xDFFF` are reserved for Unicode surrogates and are not valid `char` values
 
+lint_suspicious_cargo_cfg_target_family_comparison =
+    comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    .note = `CARGO_CFG_TARGET_FAMILY` can contain multiple comma-separated values
+    .suggestion = compare against each family instead
+
+lint_suspicious_cargo_cfg_target_family_match =
+    matching on `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    .note = `CARGO_CFG_TARGET_FAMILY` can contain multiple comma-separated values
+    .suggestion = compare against each family instead
+
 lint_suspicious_double_ref_clone =
     using `.clone()` on a double reference, which returns `{$ty}` instead of cloning the inner type
 

compiler/rustc_lint/src/cargo_cfg.rs

@@ -0,0 +1,198 @@
+use rustc_ast::{BinOpKind, UnOp};
+use rustc_hir::{self as hir, Expr, ExprKind, HirIdSet, LangItem, QPath, Stmt, StmtKind};
+use rustc_macros::{LintDiagnostic, Subdiagnostic};
+use rustc_middle::ty;
+use rustc_session::lint::FutureIncompatibilityReason;
+use rustc_session::{declare_lint, impl_lint_pass};
+use rustc_span::{Span, sym};
+
+use crate::{LateContext, LateLintPass, LintContext};
+
+declare_lint! {
+    /// The `suspicious_cargo_cfg_target_family_comparisons` lint detects single-valued comparisons
+    /// of [the `CARGO_CFG_TARGET_FAMILY`][CARGO_CFG_TARGET_FAMILY] environment variable.
+    ///
+    /// This variable is set by Cargo in build scripts.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,no_run
+    /// // build.rs
+    /// fn main() {
+    ///     let target_family = std::env::var("CARGO_CFG_TARGET_FAMILY").unwrap();
+    ///
+    ///     if target_family == "unix" {
+    ///         // Do something specific to Unix platforms
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// `CARGO_CFG_TARGET_FAMILY` is taken from [the `target_family` cfg][cfg-target_family], which
+    /// may be set multiple times. This means that `CARGO_CFG_TARGET_FAMILY` can consist of multiple
+    /// values, separated by commas. Comparing against a single value is thus not cross-platform.
+    ///
+    /// Note that most targets currently only have a single `target_family`, so oftentimes you
+    /// wouldn't hit this. This is a [future-incompatible] lint, since the compiler may at some
+    /// point introduce further target families for existing targets, and then a simple comparison
+    /// would no longer work.
+    ///
+    /// [CARGO_CFG_TARGET_FAMILY]: https://doc.rust-lang.org/cargo/reference/environment-variables.html#:~:text=CARGO_CFG_TARGET_FAMILY
+    /// [cfg-target_family]: https://doc.rust-lang.org/reference/conditional-compilation.html#target_family
+    SUSPICIOUS_CARGO_CFG_TARGET_FAMILY_COMPARISONS,
+    Warn,
+    "comparing `CARGO_CFG_TARGET_FAMILY` env var with a single value",
+    @future_incompatible = FutureIncompatibleInfo {
+        reason: FutureIncompatibilityReason::FutureReleaseSemanticsChange,
+        reference: "issue #100343 <https://github.com/rust-lang/rust/issues/100343>",
+        explain_reason: false,
+    };
+}
+
+#[derive(Default)]
+pub(crate) struct SuspiciousCargoCfgTargetFamilyComparisons {
+    /// A side table of locals that are initialized from
+    /// `std::env::var("CARGO_CFG_TARGET_FAMILY")` or similar.
+    target_family_locals: HirIdSet,
+}
+
+impl_lint_pass!(SuspiciousCargoCfgTargetFamilyComparisons => [SUSPICIOUS_CARGO_CFG_TARGET_FAMILY_COMPARISONS]);
+
+#[derive(LintDiagnostic)]
+#[diag(lint_suspicious_cargo_cfg_target_family_comparison)]
+#[note]
+struct SingleValuedComparison {
+    #[subdiagnostic]
+    sugg: Option<ReplaceWithSplitAny>,
+}
+
+#[derive(Subdiagnostic)]
+#[multipart_suggestion(lint_suggestion, applicability = "machine-applicable")]
+struct ReplaceWithSplitAny {
+    #[suggestion_part(code = "!")]
+    negate: Option<Span>,
+    #[suggestion_part(code = ".split(',').any(|x| x == ")]
+    op: Span,
+    #[suggestion_part(code = ")")]
+    end: Span,
+}
+
+#[derive(LintDiagnostic)]
+#[diag(lint_suspicious_cargo_cfg_target_family_match)]
+#[note]
+#[note(lint_suggestion)]
+struct SingleValuedMatch;
+
+// NOTE: We choose not to do a check for when in a build script, like:
+// matches!(&sess.opts.crate_name, Some(crate_name) if crate_name == "build_script_build")
+// Since we might be building a library that is used as a build script dependency (`cc-rs` etc).
+impl<'tcx> LateLintPass<'tcx> for SuspiciousCargoCfgTargetFamilyComparisons {
+    fn check_stmt(&mut self, _cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) {
+        // Find locals that are initialized from `CARGO_CFG_TARGET_FAMILY`, and save them for later
+        // checking.
+        if let StmtKind::Let(stmt) = &stmt.kind {
+            if let Some(init) = stmt.init {
+                if self.accesses_target_family_env(init) {
+                    stmt.pat.each_binding(|_, hir_id, _, _| {
+                        self.target_family_locals.insert(hir_id);
+                    });
+                }
+            }
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        // Check expressions that do single-valued comparisons.
+        match &expr.kind {
+            ExprKind::Binary(op, a, b) if matches!(op.node, BinOpKind::Eq | BinOpKind::Ne) => {
+                if self.accesses_target_family_env(a) {
+                    // If this is a &str or String, we can confidently give a `.split()` suggestion.
+                    let a_ty = cx.typeck_results().expr_ty(a);
+                    let is_str = matches!(
+                        a_ty.kind(),
+                        ty::Ref(_, r, _) if r.is_str(),
+                    ) || matches!(
+                        a_ty.ty_adt_def(),
+                        Some(ty_def) if cx.tcx.is_lang_item(ty_def.did(), LangItem::String),
+                    );
+                    let sugg = is_str.then(|| ReplaceWithSplitAny {
+                        negate: (op.node == BinOpKind::Ne).then(|| expr.span.shrink_to_lo()),
+                        op: a.span.between(b.span),
+                        end: b.span.shrink_to_hi(),
+                    });
+
+                    cx.emit_span_lint(
+                        SUSPICIOUS_CARGO_CFG_TARGET_FAMILY_COMPARISONS,
+                        expr.span,
+                        SingleValuedComparison { sugg },
+                    );
+                } else if self.accesses_target_family_env(b) {
+                    cx.emit_span_lint(
+                        SUSPICIOUS_CARGO_CFG_TARGET_FAMILY_COMPARISONS,
+                        expr.span,
+                        // Unsure how to emit a suggestion when we need to reorder `a` and `b`.
+                        SingleValuedComparison { sugg: None },
+                    );
+                }
+            }
+            ExprKind::Match(expr, _, _) if self.accesses_target_family_env(expr) => {
+                cx.emit_span_lint(
+                    SUSPICIOUS_CARGO_CFG_TARGET_FAMILY_COMPARISONS,
+                    expr.span,
+                    SingleValuedMatch,
+                );
+            }
+            // We don't handle method calls like `PartialEq::eq`, that's probably fine though,
+            // those are uncommon in real-world code.
+            _ => {}
+        }
+    }
+}
+
+impl SuspiciousCargoCfgTargetFamilyComparisons {
+    /// Check if an expression is likely derived from the `CARGO_CFG_TARGET_FAMILY` env var.
+    fn accesses_target_family_env(&self, expr: &Expr<'_>) -> bool {
+        match &expr.kind {
+            // A call to `std::env::var[_os]("CARGO_CFG_TARGET_FAMILY")`.
+            //
+            // NOTE: This actually matches all functions that take as a single value
+            // `"CARGO_CFG_TARGET_FAMILY"`. We could restrict this by matching only functions that
+            // match `"std::env::var"` or `"std::env::var_os"` by doing something like:
+            //
+            // && let Expr { kind: ExprKind::Path(QPath::Resolved(_, path)), .. } = func
+            // && let Some(fn_def_id) = path.res.opt_def_id()
+            // && cx.tcx.is_diagnostic_item(sym::std_env_var, fn_def_id)
+            //
+            // But users often define wrapper functions around these, and so we wouldn't catch it
+            // when they do.
+            //
+            // This is probably fine, `"CARGO_CFG_TARGET_FAMILY"` is unique enough of a name that
+            // it's unlikely that people will be using it for anything else.
+            ExprKind::Call(_, [arg])
+                if let ExprKind::Lit(lit) = &arg.kind
+                    && lit.node.str() == Some(sym::cargo_cfg_target_family) =>
+            {
+                true
+            }
+            // On local variables, try to see if it was initialized from target family earlier.
+            ExprKind::Path(QPath::Resolved(_, path))
+                if let hir::def::Res::Local(local_hir_id) = &path.res =>
+            {
+                self.target_family_locals.contains(local_hir_id)
+            }
+            // Recurse through references and dereferences.
+            ExprKind::AddrOf(_, _, expr) | ExprKind::Unary(UnOp::Deref, expr) => {
+                self.accesses_target_family_env(expr)
+            }
+            // Recurse on every method call to allow `.unwrap()`, `.as_deref()` and similar.
+            //
+            // NOTE: We could consider only recursing on specific `Option`/`Result` methods, but the
+            // full list of the ones we'd want becomes unwieldy pretty quickly.
+            ExprKind::MethodCall(_, receiver, _, _) => self.accesses_target_family_env(receiver),
+            _ => false,
+        }
+    }
+}

compiler/rustc_lint/src/lib.rs

@@ -34,6 +34,7 @@ mod async_closures;
 mod async_fn_in_trait;
 mod autorefs;
 pub mod builtin;
+mod cargo_cfg;
 mod context;
 mod dangling;
 mod default_could_be_derived;
@@ -83,6 +84,7 @@ use async_closures::AsyncClosureUsage;
 use async_fn_in_trait::AsyncFnInTrait;
 use autorefs::*;
 use builtin::*;
+use cargo_cfg::*;
 use dangling::*;
 use default_could_be_derived::DefaultCouldBeDerived;
 use deref_into_dyn_supertrait::*;
@@ -243,6 +245,7 @@ late_lint_methods!(
             UnqualifiedLocalImports: UnqualifiedLocalImports,
             CheckTransmutes: CheckTransmutes,
             LifetimeSyntax: LifetimeSyntax,
+            SuspiciousCargoCfgTargetFamilyComparisons: SuspiciousCargoCfgTargetFamilyComparisons::default(),
         ]
     ]
 );

compiler/rustc_span/src/symbol.rs

@@ -618,6 +618,7 @@ symbols! {
         call_ref_future,
         caller_location,
         capture_disjoint_fields,
+        cargo_cfg_target_family: "CARGO_CFG_TARGET_FAMILY",
         carrying_mul_add,
         catch_unwind,
         cause,

tests/ui/lint/suspicious_cargo_cfg_target_family_comparisons.rs

@@ -0,0 +1,146 @@
+// Test the `suspicious_cargo_cfg_target_family_comparisons` lint.
+
+//@ check-pass
+//@ exec-env:CARGO_CFG_TARGET_FAMILY=unix
+
+use std::env;
+
+fn main() {
+    // Check that direct comparisons warn.
+    let is_unix = env::var("CARGO_CFG_TARGET_FAMILY").unwrap() == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // But that later usage doesn't warn.
+    if is_unix {}
+
+    // Assigning to local variable is fine.
+    let target_family = env::var("CARGO_CFG_TARGET_FAMILY").unwrap();
+
+    // Using local in an `==` comparison.
+    if target_family == "unix" {
+        //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    }
+
+    // Using local in a match.
+    match &*target_family {
+        //~^ WARN matching on `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+        "unix" => {}
+        _ => {}
+    }
+
+    // Correct handling doesn't warn.
+    if target_family.contains("unix") {}
+    if target_family.split(',').any(|x| x == "unix") {}
+
+    // Test supression.
+    #[allow(suspicious_cargo_cfg_target_family_comparisons)]
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY").unwrap() == "unix";
+
+    // Negative comparison.
+    let _ = target_family != "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // Local variable propagation.
+    let target_family = env::var("CARGO_CFG_TARGET_FAMILY").unwrap();
+    let target_family: &str = target_family.as_ref();
+    let _ = target_family == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // Custom wrapper.
+    fn get_and_track_env_var(env_var_name: &str) -> String {
+        // This is actually unnecessary, Cargo already tracks changes to the target family, but it's
+        // nonetheless a fairly common pattern.
+        println!("cargo:rerun-if-env-changed={env_var_name}");
+        env::var(env_var_name).unwrap()
+    }
+    let _ = get_and_track_env_var("CARGO_CFG_TARGET_FAMILY") == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // Various.
+    let _ = ::std::env::var("CARGO_CFG_TARGET_FAMILY").unwrap() == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY").expect("should be set") == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY").unwrap_or_default() == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var_os("CARGO_CFG_TARGET_FAMILY").unwrap() == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY") == Ok("unix".to_string());
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var_os("CARGO_CFG_TARGET_FAMILY") == Some("unix".into());
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY").as_deref() == Ok("unix");
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var_os("CARGO_CFG_TARGET_FAMILY").as_deref() == Some("unix".as_ref());
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY").ok().as_deref() == Some("unix".as_ref());
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    false_negatives();
+    false_positives();
+}
+
+// This lint has many false negatives, the problem is intractable in the general case.
+fn false_negatives() {
+    // Cannot detect if the env var is not specified inline (such as when dynamically generated).
+    let var = "CARGO_CFG_TARGET_FAMILY";
+    let _ = env::var(var).unwrap() == "unix";
+
+    // Cannot detect if env var value comes from somewhere more complex.
+    fn get_env_var() -> String {
+        env::var("CARGO_CFG_TARGET_FAMILY").unwrap()
+    }
+    let _ = get_env_var() == "unix";
+
+    // Doesn't detect more complex expressions.
+    let _ = std::convert::identity(env::var_os("CARGO_CFG_TARGET_FAMILY").unwrap()) == "unix";
+    let _ = *Box::new(env::var_os("CARGO_CFG_TARGET_FAMILY").unwrap()) == "unix";
+
+    // Doesn't detect variables that are initialized later.
+    let later_init;
+    later_init = env::var("CARGO_CFG_TARGET_FAMILY").unwrap();
+    if later_init == "unix" {}
+
+    // Doesn't detect if placed inside a struct.
+    struct Target {
+        family: String,
+    }
+    let target = Target { family: env::var("CARGO_CFG_TARGET_FAMILY").unwrap() };
+    if target.family == "unix" {}
+}
+
+// This lint also has false positives, these are probably unlikely to be hit in practice.
+fn false_positives() {
+    // Cannot detect later changes to assigned variable.
+    let mut overwritten = env::var("CARGO_CFG_TARGET_FAMILY").unwrap();
+    if true {
+        overwritten = "unix".to_string();
+    }
+    if overwritten == "unix" {}
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // Non-std::env::var usage.
+    let _ = std::convert::identity("CARGO_CFG_TARGET_FAMILY") == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // Call unusual `Option`/`Result` method, and then compare that result.
+    let _ = env::var_os("CARGO_CFG_TARGET_FAMILY").is_some() == true;
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+
+    // Match with match arms that contains checks.
+    match env::var("CARGO_CFG_TARGET_FAMILY") {
+        //~^ WARN matching on `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+        Ok(os) if os.contains("unix") => {}
+        _ => {}
+    }
+
+    // Unusual method call.
+    trait Foo {
+        fn returns_string(&self) -> &str {
+            "unix"
+        }
+    }
+    impl Foo for String {}
+    let _ = env::var("CARGO_CFG_TARGET_FAMILY").unwrap().returns_string() == "unix";
+    //~^ WARN comparing against `CARGO_CFG_TARGET_FAMILY` directly may break in the future
+}