diff --git a/compiler/rustc_ast/src/attr/mod.rs b/compiler/rustc_ast/src/attr/mod.rs index 369fe12539fa8..94b138be11dab 100644 --- a/compiler/rustc_ast/src/attr/mod.rs +++ b/compiler/rustc_ast/src/attr/mod.rs @@ -481,7 +481,7 @@ impl MetaItem { } } - fn from_tokens(iter: &mut TokenStreamIter<'_>) -> Option { + pub fn from_tokens(iter: &mut TokenStreamIter<'_>) -> Option { // FIXME: Share code with `parse_path`. let tt = iter.next().map(|tt| TokenTree::uninterpolate(tt)); let path = match tt.as_deref() { diff --git a/compiler/rustc_attr_parsing/src/attributes/cfg.rs b/compiler/rustc_attr_parsing/src/attributes/cfg.rs index 84c83be8b4a5d..dcd67e70b5e7e 100644 --- a/compiler/rustc_attr_parsing/src/attributes/cfg.rs +++ b/compiler/rustc_attr_parsing/src/attributes/cfg.rs @@ -3,7 +3,7 @@ use std::convert::identity; use rustc_ast::token::Delimiter; use rustc_ast::tokenstream::DelimSpan; use rustc_ast::{AttrItem, Attribute, LitKind, ast, token}; -use rustc_errors::{Applicability, PResult, msg}; +use rustc_errors::{Applicability, Diagnostic, PResult, msg}; use rustc_feature::{ AttrSuggestionStyle, AttributeTemplate, Features, GatedCfg, find_gated_cfg, template, }; @@ -85,9 +85,151 @@ pub fn parse_cfg( parse_cfg_entry(cx, single).ok() } +fn generate_cfg_replacement(entry: &CfgEntry) -> String { + generate_cfg_replacement_inner(entry, true, false) +} + +fn generate_cfg_replacement_inner(entry: &CfgEntry, is_top_level: bool, parent_is_not: bool) -> String { + match entry { + CfgEntry::NameValue { name, value, .. } => { + let name = name.as_str(); + match value { + Some(value) => format!( + "{}std::env::var(\"CARGO_CFG_{}\").unwrap_or_default() == \"{value}\"", + if parent_is_not { "!" } else { "" }, + name.to_uppercase(), + ), + None => format!( + "std::env::var(\"CARGO_CFG_{}\"){}", + name.to_uppercase(), + if parent_is_not { ".is_err()" } else { ".is_ok()" }, + ), + } + } + CfgEntry::Any(entries, _) => { + match entries.as_slice() { + [] => if parent_is_not { "true" } else { "false" }.to_string(), + [entry] => generate_cfg_replacement_inner(&entry, is_top_level, parent_is_not), + _ => format!( + "{not}{open_paren}{cond}{closing_paren}", + not = if parent_is_not { "!" } else { "" }, + open_paren = if !parent_is_not && is_top_level { "" } else { "(" }, + cond = entries + .iter() + .map(|cfg| generate_cfg_replacement_inner(cfg, false, false)) + .collect::>() + .join(" || "), + closing_paren = if !parent_is_not && is_top_level { "" } else { ")" }, + ), + } + } + CfgEntry::All(entries, _) => { + match entries.as_slice() { + [] => if parent_is_not { "false" } else { "true" }.to_string(), + [entry] => generate_cfg_replacement_inner(&entry, is_top_level, parent_is_not), + _ => format!( + "{not}{open_paren}{cond}{closing_paren}", + not = if parent_is_not { "!" } else { "" }, + open_paren = if !parent_is_not && is_top_level { "" } else { "(" }, + cond = entries + .iter() + .map(|cfg| generate_cfg_replacement_inner(cfg, false, false)) + .collect::>() + .join(" && "), + closing_paren = if !parent_is_not && is_top_level { "" } else { ")" }, + ), + } + } + CfgEntry::Not(cfg, _) => generate_cfg_replacement_inner(cfg, is_top_level, true), + _ => String::new(), + } +} + +fn misleading_cfgs(entry: &CfgEntry, spans: &mut Vec, has_ok_cfgs: &mut bool) { + match entry { + CfgEntry::All(entries, _) | CfgEntry::Any(entries, _) => { + for entry in entries { + misleading_cfgs(entry, spans, has_ok_cfgs); + } + } + CfgEntry::Not(entry, _) => misleading_cfgs(entry, spans, has_ok_cfgs), + CfgEntry::Bool(..) | CfgEntry::Version(..) => { + *has_ok_cfgs = true; + } + CfgEntry::NameValue { name, value, span } => { + match value { + Some(_) => { + let name = name.as_str(); + if name.starts_with("target_") { + spans.push(*span); + } else { + *has_ok_cfgs = true; + } + } + None => { + if [sym::windows, sym::unix].contains(&name) { + spans.push(*span); + } else { + *has_ok_cfgs = true; + } + } + } + } + } +} + +fn check_unexpected_cfgs(cx: &mut AcceptContext<'_, '_, S>, entry: &CfgEntry, _span: Span, is_cfg_macro: bool) { + let mut spans = Vec::new(); + let mut has_ok_cfgs = false; + misleading_cfgs(entry, &mut spans, &mut has_ok_cfgs); + if spans.is_empty() { + return; + } + if !is_cfg_macro || has_ok_cfgs { + cx.emit_dyn_lint( + UNEXPECTED_CFGS, + |dcx, level| crate::errors::UnexpectedCfg { + span: None, + suggestion_message: "", + }.into_diag(dcx, level), + spans, + ); + return; + } + let replacement = generate_cfg_replacement(entry); + // The span including the `cfg!()` macro. + let span = cx.attr_span; + cx.emit_dyn_lint( + UNEXPECTED_CFGS, + move |dcx, level| crate::errors::UnexpectedCfg { + span: Some(span), + suggestion_message: &replacement, + }.into_diag(dcx, level), + span, + ); +} + +pub fn parse_cfg_entry_macro( + cx: &mut AcceptContext<'_, '_, S>, + item: &MetaItemOrLitParser, +) -> Result { + let entry = parse_cfg_entry_inner(cx, item)?; + check_unexpected_cfgs(cx, &entry, item.span(), true); + Ok(entry) +} + pub fn parse_cfg_entry( cx: &mut AcceptContext<'_, '_, S>, item: &MetaItemOrLitParser, +) -> Result { + let entry = parse_cfg_entry_inner(cx, item)?; + check_unexpected_cfgs(cx, &entry, item.span(), false); + Ok(entry) +} + +fn parse_cfg_entry_inner( + cx: &mut AcceptContext<'_, '_, S>, + item: &MetaItemOrLitParser, ) -> Result { Ok(match item { MetaItemOrLitParser::MetaItemParser(meta) => match meta.args() { @@ -96,14 +238,14 @@ pub fn parse_cfg_entry( let Some(single) = list.single() else { return Err(cx.adcx().expected_single_argument(list.span, list.len())); }; - CfgEntry::Not(Box::new(parse_cfg_entry(cx, single)?), list.span) + CfgEntry::Not(Box::new(parse_cfg_entry_inner(cx, single)?), list.span) } Some(sym::any) => CfgEntry::Any( - list.mixed().flat_map(|sub_item| parse_cfg_entry(cx, sub_item)).collect(), + list.mixed().flat_map(|sub_item| parse_cfg_entry_inner(cx, sub_item)).collect(), list.span, ), Some(sym::all) => CfgEntry::All( - list.mixed().flat_map(|sub_item| parse_cfg_entry(cx, sub_item)).collect(), + list.mixed().flat_map(|sub_item| parse_cfg_entry_inner(cx, sub_item)).collect(), list.span, ), Some(sym::target) => parse_cfg_entry_target(cx, list, meta.span())?, diff --git a/compiler/rustc_attr_parsing/src/errors.rs b/compiler/rustc_attr_parsing/src/errors.rs index 96a4c473c3ae2..1b80e64de5454 100644 --- a/compiler/rustc_attr_parsing/src/errors.rs +++ b/compiler/rustc_attr_parsing/src/errors.rs @@ -321,3 +321,15 @@ pub(crate) struct IncorrectDoNotRecommendLocation { #[label("not a trait implementation")] pub target_span: Span, } + +#[derive(Diagnostic)] +#[diag("target-based cfg should be avoided in build scripts")] +pub(crate) struct UnexpectedCfg<'a> { + #[suggestion( + "use cargo environment variables if possible", + code = "{suggestion_message}", + applicability = "maybe-incorrect", + )] + pub span: Option, + pub suggestion_message: &'a str, +} diff --git a/compiler/rustc_attr_parsing/src/lib.rs b/compiler/rustc_attr_parsing/src/lib.rs index 73618dbfbf30c..8caa5fff81ba9 100644 --- a/compiler/rustc_attr_parsing/src/lib.rs +++ b/compiler/rustc_attr_parsing/src/lib.rs @@ -109,6 +109,7 @@ pub mod validate_attr; pub use attributes::AttributeSafety; pub use attributes::cfg::{ CFG_TEMPLATE, EvalConfigResult, eval_config_entry, parse_cfg, parse_cfg_attr, parse_cfg_entry, + parse_cfg_entry_macro, }; pub use attributes::cfg_select::*; pub use attributes::util::{is_builtin_attr, parse_version}; diff --git a/compiler/rustc_builtin_macros/src/cfg.rs b/compiler/rustc_builtin_macros/src/cfg.rs index 2872cff0fdc7a..892f7326c4e7b 100644 --- a/compiler/rustc_builtin_macros/src/cfg.rs +++ b/compiler/rustc_builtin_macros/src/cfg.rs @@ -7,7 +7,7 @@ use rustc_ast::{AttrStyle, token}; use rustc_attr_parsing::parser::{AllowExprMetavar, MetaItemOrLitParser}; use rustc_attr_parsing::{ self as attr, AttributeParser, AttributeSafety, CFG_TEMPLATE, ParsedDescription, ShouldEmit, - parse_cfg_entry, + parse_cfg_entry_macro, }; use rustc_expand::base::{DummyResult, ExpandResult, ExtCtxt, MacEager, MacroExpanderResult}; use rustc_hir::attrs::CfgEntry; @@ -63,7 +63,7 @@ fn parse_cfg(cx: &ExtCtxt<'_>, span: Span, tts: TokenStream) -> Result String, ) -> lints::UnexpectedCfgCargoHelp { // We don't want to suggest the `build.rs` way to expected cfgs if we are already in a - // `build.rs`. We therefor do a best effort check (looking if the `--crate-name` is + // `build.rs`. We therefore do a best effort check (looking if the `--crate-name` is // `build_script_build`) to try to figure out if we are building a Cargo build script let unescaped = &inst(EscapeQuotes::No); diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index b027872dd99cc..778039b51cc40 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -73,6 +73,7 @@ declare_lint_pass! { MALFORMED_DIAGNOSTIC_ATTRIBUTES, MALFORMED_DIAGNOSTIC_FORMAT_LITERALS, META_VARIABLE_MISUSE, + MISLEADING_CFG_IN_BUILD_SCRIPT, MISPLACED_DIAGNOSTIC_ATTRIBUTES, MISSING_ABI, MISSING_UNSAFE_ON_EXTERN, @@ -5695,3 +5696,29 @@ declare_lint! { report_in_deps: false, }; } + +declare_lint! { + /// Checks for usage of `#[cfg]`/`#[cfg_attr]`/`cfg!()` in `build.rs` scripts. + /// + /// ### Explanation + /// + /// It checks the `cfg` values for the *host*, not the target. For example, `cfg!(windows)` is + /// true when compiling on Windows, so it will give the wrong answer if you are cross compiling. + /// This is because build scripts run on the machine performing compilation, rather than on the + /// target. + /// + /// ### Example + /// + /// ```rust,ignore (should only be run in cargo build scripts) + /// if cfg!(windows) {} + /// ``` + /// + /// Use instead: + /// + /// ```rust + /// if std::env::var("CARGO_CFG_WINDOWS").is_ok() {} + /// ``` + pub MISLEADING_CFG_IN_BUILD_SCRIPT, + Allow, + "use of host configs in `build.rs` scripts" +} diff --git a/src/tools/lint-docs/src/lib.rs b/src/tools/lint-docs/src/lib.rs index f7d487333e32b..739de9e801a59 100644 --- a/src/tools/lint-docs/src/lib.rs +++ b/src/tools/lint-docs/src/lib.rs @@ -332,6 +332,7 @@ impl<'a> LintExtractor<'a> { if matches!( lint.name.as_str(), "unused_features" // broken lint + | "misleading_cfg_in_build_script" // only run in cargo build scripts ) { return Ok(()); } diff --git a/tests/ui/lint/misleading_cfg_in_build_script.rs b/tests/ui/lint/misleading_cfg_in_build_script.rs new file mode 100644 index 0000000000000..e7c840d596ac5 --- /dev/null +++ b/tests/ui/lint/misleading_cfg_in_build_script.rs @@ -0,0 +1,47 @@ +// This test checks the `cfg()` attributes/macros in cargo build scripts. +// +//@ no-auto-check-cfg + +#![deny(misleading_cfg_in_build_script)] +#![allow(dead_code)] + +#[cfg(windows)] +//~^ ERROR: misleading_cfg_in_build_script +fn unused_windows_fn() {} +#[cfg(not(windows))] +//~^ ERROR: misleading_cfg_in_build_script +fn unused_not_windows_fn() {} +#[cfg(any(windows, feature = "yellow", unix))] +//~^ ERROR: misleading_cfg_in_build_script +fn pink() {} + +// Should not lint. +#[cfg(feature = "green")] +fn pink() {} +#[cfg(bob)] +fn bob() {} + +fn main() { + if cfg!(windows) {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(not(windows)) {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(target_os = "freebsd") {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(any(target_os = "freebsd", windows)) {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(not(any(target_os = "freebsd", windows))) {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(all(target_os = "freebsd", windows)) {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(not(all(target_os = "freebsd", windows))) {} + //~^ ERROR: misleading_cfg_in_build_script + if cfg!(all(target_os = "freebsd", any(windows, not(feature = "red")))) {} + //~^ ERROR: misleading_cfg_in_build_script + + // Should not warn. + if cfg!(any()) {} + if cfg!(all()) {} + if cfg!(feature = "blue") {} + if cfg!(bob) {} +} diff --git a/tests/ui/lint/misleading_cfg_in_build_script.stderr b/tests/ui/lint/misleading_cfg_in_build_script.stderr new file mode 100644 index 0000000000000..e2c75999db929 --- /dev/null +++ b/tests/ui/lint/misleading_cfg_in_build_script.stderr @@ -0,0 +1,74 @@ +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:8:1 + | +LL | #[cfg(windows)] + | ^^^^^^^^^^^^^^^ + | +note: the lint level is defined here + --> $DIR/misleading_cfg_in_build_script.rs:5:9 + | +LL | #![deny(misleading_cfg_in_build_script)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:11:1 + | +LL | #[cfg(not(windows))] + | ^^^^^^^^^^^^^^^^^^^^ + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:14:11 + | +LL | #[cfg(any(windows, feature = "yellow", unix))] + | ^^^^^^^ ^^^^ + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:25:8 + | +LL | if cfg!(windows) {} + | ^^^^^^^^^^^^^ help: use cargo environment variables if possible: `std::env::var("CARGO_CFG_WINDOWS").is_ok()` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:27:8 + | +LL | if cfg!(not(windows)) {} + | ^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `std::env::var("CARGO_CFG_WINDOWS").is_err()` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:29:8 + | +LL | if cfg!(target_os = "freebsd") {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "freebsd"` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:31:8 + | +LL | if cfg!(any(target_os = "freebsd", windows)) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "freebsd" || std::env::var("CARGO_CFG_WINDOWS").is_ok()` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:33:8 + | +LL | if cfg!(not(any(target_os = "freebsd", windows))) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `!(std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "freebsd" || std::env::var("CARGO_CFG_WINDOWS").is_ok())` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:35:8 + | +LL | if cfg!(all(target_os = "freebsd", windows)) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "freebsd" && std::env::var("CARGO_CFG_WINDOWS").is_ok()` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:37:8 + | +LL | if cfg!(not(all(target_os = "freebsd", windows))) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `!(std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "freebsd" && std::env::var("CARGO_CFG_WINDOWS").is_ok())` + +error: target-based cfg should be avoided in build scripts + --> $DIR/misleading_cfg_in_build_script.rs:39:8 + | +LL | if cfg!(all(target_os = "freebsd", any(windows, not(feature = "red")))) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use cargo environment variables if possible: `std::env::var("CARGO_CFG_TARGET_OS").unwrap_or_default() == "freebsd" && (std::env::var("CARGO_CFG_WINDOWS").is_ok() || cfg!(not(feature = red)))` + +error: aborting due to 11 previous errors +