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 ccc4a1a64c56f..1bb8cf7d2bf41 100644 --- a/compiler/rustc_attr_parsing/src/attributes/cfg.rs +++ b/compiler/rustc_attr_parsing/src/attributes/cfg.rs @@ -88,6 +88,24 @@ pub fn parse_cfg_entry( cx: &mut AcceptContext<'_, '_, S>, item: &MetaItemOrLitParser, ) -> Result { + let entry = parse_cfg_entry_inner(cx, item)?; + let mut spans = Vec::new(); + let total = misleading_cfgs(&entry, &mut spans); + if !spans.is_empty() { + cx.emit_lint( + UNEXPECTED_CFGS, + AttributeLintKind::UnexpectedCfgName((name, name_span), value), + span, + ); + } + Ok(entry) +} + +fn parse_cfg_entry_inner( + cx: &mut AcceptContext<'_, '_, S>, + item: &MetaItemOrLitParser, +) -> Result { + Ok(match item { MetaItemOrLitParser::MetaItemParser(meta) => match meta.args() { ArgParser::List(list) => match meta.path().word_sym() { diff --git a/compiler/rustc_lint/src/early/diagnostics/check_cfg.rs b/compiler/rustc_lint/src/early/diagnostics/check_cfg.rs index 9fcabfa623d80..2d2daab635de8 100644 --- a/compiler/rustc_lint/src/early/diagnostics/check_cfg.rs +++ b/compiler/rustc_lint/src/early/diagnostics/check_cfg.rs @@ -66,7 +66,7 @@ fn cargo_help_sub( inst: &impl Fn(EscapeQuotes) -> 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/src/lib.rs b/compiler/rustc_lint/src/lib.rs index 6f6aa0f96e75e..da4b2ec1c5848 100644 --- a/compiler/rustc_lint/src/lib.rs +++ b/compiler/rustc_lint/src/lib.rs @@ -57,6 +57,7 @@ pub mod lifetime_syntax; mod lints; mod macro_expr_fragment_specifier_2024_migration; mod map_unit_fn; +mod misleading_cfg_in_build_script; mod multiple_supertrait_upcastable; mod non_ascii_idents; mod non_fmt_panic; @@ -101,6 +102,7 @@ use let_underscore::*; use lifetime_syntax::*; use macro_expr_fragment_specifier_2024_migration::*; use map_unit_fn::*; +use misleading_cfg_in_build_script::MisleadingCfgInBuildScript; use multiple_supertrait_upcastable::*; use non_ascii_idents::*; use non_fmt_panic::NonPanicFmt; @@ -157,6 +159,7 @@ early_lint_methods!( pub BuiltinCombinedPreExpansionLintPass, [ KeywordIdents: KeywordIdents, + MisleadingCfgInBuildScript: MisleadingCfgInBuildScript, ] ] ); diff --git a/compiler/rustc_lint/src/misleading_cfg_in_build_script.rs b/compiler/rustc_lint/src/misleading_cfg_in_build_script.rs new file mode 100644 index 0000000000000..f826078332dcb --- /dev/null +++ b/compiler/rustc_lint/src/misleading_cfg_in_build_script.rs @@ -0,0 +1,275 @@ +use rustc_ast::ast::{Attribute, MacCall}; +use rustc_ast::tokenstream::TokenStream; +use rustc_ast::{MetaItem, MetaItemInner, MetaItemKind}; +use rustc_errors::{Applicability, DiagDecorator}; +use rustc_session::{declare_lint, declare_lint_pass}; +use rustc_span::{Span, Symbol, sym}; + +use crate::{EarlyContext, EarlyLintPass, LintContext}; + +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" +} + +declare_lint_pass!(MisleadingCfgInBuildScript => [MISLEADING_CFG_IN_BUILD_SCRIPT]); + +/// Represents the AST of `cfg` attribute and `cfg!` macro. +#[derive(Debug)] +enum CfgAst { + /// Represents an OS family such as "unix" or "windows". + OsFamily(Symbol), + /// The `any()` attribute. + Any(Vec), + /// The `all()` attribute. + All(Vec), + /// The `not()` attribute. + Not(Box), + /// Any `target_* = ""` key/value attribute. + TargetKeyValue(Symbol, Symbol), + /// the `feature = ""` attribute with its associated value. + Feature(Symbol), +} + +impl CfgAst { + fn has_only_features(&self) -> bool { + match self { + Self::OsFamily(_) | Self::TargetKeyValue(_, _) => false, + Self::Any(v) | Self::All(v) => v.is_empty() || v.iter().all(CfgAst::has_only_features), + Self::Not(v) => v.has_only_features(), + Self::Feature(_) => true, + } + } + + fn generate_replacement(&self) -> String { + self.generate_replacement_inner(true, false) + } + + fn generate_replacement_inner(&self, is_top_level: bool, parent_is_not: bool) -> String { + match self { + Self::OsFamily(os) => format!( + "std::env::var(\"CARGO_CFG_{}\"){}", + os.as_str().to_uppercase(), + if parent_is_not { ".is_err()" } else { ".is_ok()" }, + ), + Self::TargetKeyValue(cfg_target, s) => format!( + "{}std::env::var(\"CARGO_CFG_{}\").unwrap_or_default() == \"{s}\"", + if parent_is_not { "!" } else { "" }, + cfg_target.as_str().to_uppercase(), + ), + Self::Any(v) => { + if v.is_empty() { + if parent_is_not { "true" } else { "false" }.to_string() + } else if v.len() == 1 { + v[0].generate_replacement_inner(is_top_level, parent_is_not) + } else { + format!( + "{not}{open_paren}{cond}{closing_paren}", + not = if parent_is_not { "!" } else { "" }, + open_paren = if !parent_is_not && is_top_level { "" } else { "(" }, + cond = v + .iter() + .map(|i| i.generate_replacement_inner(false, false)) + .collect::>() + .join(" || "), + closing_paren = if !parent_is_not && is_top_level { "" } else { ")" }, + ) + } + } + Self::All(v) => { + if v.is_empty() { + if parent_is_not { "false" } else { "true" }.to_string() + } else if v.len() == 1 { + v[0].generate_replacement_inner(is_top_level, parent_is_not) + } else { + format!( + "{not}{open_paren}{cond}{closing_paren}", + not = if parent_is_not { "!" } else { "" }, + open_paren = if !parent_is_not && is_top_level { "" } else { "(" }, + cond = v + .iter() + .map(|i| i.generate_replacement_inner(false, false)) + .collect::>() + .join(" && "), + closing_paren = if !parent_is_not && is_top_level { "" } else { ")" }, + ) + } + } + Self::Not(i) => i.generate_replacement_inner(is_top_level, true), + Self::Feature(s) => format!( + "cfg!({}feature = {s}{})", + if parent_is_not { "not(" } else { "" }, + if parent_is_not { ")" } else { "" }, + ), + } + } +} + +fn parse_meta_item(meta: MetaItem, has_unknown: &mut bool, out: &mut Vec) { + let Some(name) = meta.name() else { + *has_unknown = true; + return; + }; + match meta.kind { + MetaItemKind::Word => { + if [sym::windows, sym::unix].contains(&name) { + out.push(CfgAst::OsFamily(name)); + return; + } + } + MetaItemKind::NameValue(item) => { + if name == sym::feature { + out.push(CfgAst::Feature(item.symbol)); + return; + } else if name.as_str().starts_with("target_") { + out.push(CfgAst::TargetKeyValue(name, item.symbol)); + return; + } + } + MetaItemKind::List(item) => { + if !*has_unknown && [sym::any, sym::not, sym::all].contains(&name) { + let mut sub_out = Vec::new(); + + for sub in item { + if let MetaItemInner::MetaItem(item) = sub { + parse_meta_item(item, has_unknown, &mut sub_out); + if *has_unknown { + return; + } + } + } + if name == sym::any { + out.push(CfgAst::Any(sub_out)); + return; + } else if name == sym::all { + out.push(CfgAst::All(sub_out)); + return; + // `not()` can only have one argument. + } else if sub_out.len() == 1 { + out.push(CfgAst::Not(Box::new(sub_out.pop().unwrap()))); + return; + } + } + } + } + *has_unknown = true; +} + +fn parse_macro_args(tokens: &TokenStream, has_unknown: &mut bool, out: &mut Vec) { + if let Some(meta) = MetaItem::from_tokens(&mut tokens.iter()) { + parse_meta_item(meta, has_unknown, out); + } +} + +fn get_invalid_cfg_attrs(attr: &MetaItem, spans: &mut Vec, has_unknown: &mut bool) { + let Some(ident) = attr.ident() else { return }; + if [sym::unix, sym::windows].contains(&ident.name) { + spans.push(attr.span); + } else if attr.value_str().is_some() && ident.name.as_str().starts_with("target_") { + spans.push(attr.span); + } else if let Some(sub_attrs) = attr.meta_item_list() { + for sub_attr in sub_attrs { + if let Some(meta) = sub_attr.meta_item() { + get_invalid_cfg_attrs(meta, spans, has_unknown); + } + } + } else { + *has_unknown = true; + } +} + +const ERROR_MESSAGE: &str = "target-based cfg should be avoided in build scripts"; + +impl EarlyLintPass for MisleadingCfgInBuildScript { + fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &Attribute) { + match attr.name() { + Some(sym::cfg) if let Some(meta) = attr.meta() => { + get_invalid_cfg_attrs(&meta, &mut spans, &mut has_unknown); + } + Some(sym::cfg_attr) + if let Some(sub_attrs) = attr.meta_item_list() + && let Some(meta) = sub_attrs.first().and_then(|a| a.meta_item()) => + { + get_invalid_cfg_attrs(meta, &mut spans, &mut has_unknown); + } + _ => return, + } + if !spans.is_empty() { + if has_unknown { + // If the `cfg`/`cfg_attr` attribute contains not only invalid items, we display + // spans of all invalid items. + cx.emit_span_lint( + MISLEADING_CFG_IN_BUILD_SCRIPT, + spans, + DiagDecorator(|diag| { + diag.primary_message(ERROR_MESSAGE); + }), + ); + } else { + // No "good" item in the `cfg`/`cfg_attr` attribute so we can use the span of the + // whole attribute directly. + cx.emit_span_lint( + MISLEADING_CFG_IN_BUILD_SCRIPT, + attr.span, + DiagDecorator(|diag| { + diag.primary_message(ERROR_MESSAGE); + }), + ); + } + } + } + + fn check_mac(&mut self, cx: &EarlyContext<'_>, call: &MacCall) { + if call.path.segments.len() == 1 && call.path.segments[0].ident.name == sym::cfg { + let mut ast = Vec::new(); + let mut has_unknown = false; + parse_macro_args(&call.args.tokens, &mut has_unknown, &mut ast); + if !has_unknown && ast.len() > 1 { + cx.emit_span_lint( + MISLEADING_CFG_IN_BUILD_SCRIPT, + call.span(), + DiagDecorator(|diag| { + diag.primary_message(ERROR_MESSAGE); + }), + ); + } else if let Some(ast) = ast.get(0) + && !ast.has_only_features() + { + let span = call.span(); + cx.emit_span_lint( + MISLEADING_CFG_IN_BUILD_SCRIPT, + span, + DiagDecorator(|diag| { + diag.primary_message(ERROR_MESSAGE).span_suggestion( + span, + "use cargo environment variables if possible", + ast.generate_replacement(), + Applicability::MaybeIncorrect, + ); + }), + ); + } + } + } +} diff --git a/compiler/rustc_session/src/parse.rs b/compiler/rustc_session/src/parse.rs index 8251050b6aead..636c87e87cba9 100644 --- a/compiler/rustc_session/src/parse.rs +++ b/compiler/rustc_session/src/parse.rs @@ -18,7 +18,7 @@ use rustc_feature::{GateIssue, UnstableFeatures, find_feature_issue}; use rustc_span::edition::Edition; use rustc_span::hygiene::ExpnId; use rustc_span::source_map::{FilePathMapping, SourceMap}; -use rustc_span::{Span, Symbol, sym}; +use rustc_span::{ErrorGuaranteed, Span, Symbol, sym}; use crate::Session; use crate::config::{Cfg, CheckCfg}; @@ -379,3 +379,30 @@ impl ParseSess { self.dcx.handle() } } + +pub fn parse_cfg(parser: &mut rustc_parse::Parser<'_>, sess: &rustc_session::Session<'_>, span: Span, tts: TokenStream) -> Result { + let meta = MetaItemOrLitParser::parse_single( + &mut parser, + ShouldEmit::Nothing, + AllowExprMetavar::Yes, + ) + .map_err(|_| diag.emit())?; + AttributeParser::parse_single_args( + sess, + span, + span, + AttrStyle::Inner, + AttrPath { segments: vec![sym::cfg].into_boxed_slice(), span }, + None, + ParsedDescription::Macro, + span, + cx.current_expansion.lint_node_id, + // Doesn't matter what the target actually is here. + Target::Crate, + Some(cx.ecfg.features), + ShouldEmit::Nothing, + &meta, + parse_cfg_entry, + &CFG_TEMPLATE, + ) +} \ No newline at end of file diff --git a/src/tools/lint-docs/src/lib.rs b/src/tools/lint-docs/src/lib.rs index bc38e931fe515..3ffc4f179b9e8 100644 --- a/src/tools/lint-docs/src/lib.rs +++ b/src/tools/lint-docs/src/lib.rs @@ -333,6 +333,7 @@ impl<'a> LintExtractor<'a> { lint.name.as_str(), "unused_features" // broken lint | "soft_unstable" // cannot have a stable example + | "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 +