From aa93f61f216b01e401d32545f2dc844dc2e50b14 Mon Sep 17 00:00:00 2001 From: Benedikt Werner <1benediktwerner@gmail.com> Date: Sat, 7 Dec 2019 09:10:06 +0100 Subject: [PATCH] Warn instead of error --- crates/cargo-platform/src/cfg.rs | 66 +-------------- crates/cargo-platform/src/error.rs | 4 - crates/cargo-platform/src/lib.rs | 47 ++++++++++- crates/cargo-platform/tests/test_cfg.rs | 83 ++++++++++++++++--- src/cargo/util/toml/mod.rs | 6 +- .../src/reference/specifying-dependencies.md | 2 + 6 files changed, 124 insertions(+), 84 deletions(-) diff --git a/crates/cargo-platform/src/cfg.rs b/crates/cargo-platform/src/cfg.rs index efa3919d06f..662c7f97bee 100644 --- a/crates/cargo-platform/src/cfg.rs +++ b/crates/cargo-platform/src/cfg.rs @@ -1,4 +1,4 @@ -use crate::error::{ParseError, ParseErrorKind, ParseErrorKind::*}; +use crate::error::{ParseError, ParseErrorKind::*}; use std::fmt; use std::iter; use std::str::{self, FromStr}; @@ -41,21 +41,6 @@ struct Parser<'a> { t: Tokenizer<'a>, } -impl Cfg { - pub(crate) fn validate_as_target(&self) -> Result<(), ParseErrorKind> { - match self { - Cfg::Name(name) => match name.as_str() { - "test" | "debug_assertions" | "proc_macro" => Err(InvalidCfgName(name.to_string())), - _ => Ok(()), - }, - Cfg::KeyPair(name, _) => match name.as_str() { - "feature" => Err(InvalidCfgKey(name.to_string())), - _ => Ok(()), - }, - } - } -} - impl FromStr for Cfg { type Err = ParseError; @@ -104,19 +89,6 @@ impl CfgExpr { CfgExpr::Value(ref e) => cfg.contains(e), } } - - pub(crate) fn validate_as_target(&self) -> Result<(), ParseErrorKind> { - match *self { - CfgExpr::Not(ref e) => e.validate_as_target()?, - CfgExpr::All(ref e) | CfgExpr::Any(ref e) => { - for e in e { - e.validate_as_target()?; - } - } - CfgExpr::Value(ref e) => e.validate_as_target()?, - } - Ok(()) - } } impl FromStr for CfgExpr { @@ -345,39 +317,3 @@ impl<'a> Token<'a> { } } } - -#[test] -fn cfg_validate_as_target() { - fn p(s: &str) -> CfgExpr { - s.parse().unwrap() - } - - assert!(p("unix").validate_as_target().is_ok()); - assert!(p("windows").validate_as_target().is_ok()); - assert!(p("any(not(unix), windows)").validate_as_target().is_ok()); - assert!(p("foo").validate_as_target().is_ok()); - - assert!(p("target_arch = \"abc\"").validate_as_target().is_ok()); - assert!(p("target_feature = \"abc\"").validate_as_target().is_ok()); - assert!(p("target_os = \"abc\"").validate_as_target().is_ok()); - assert!(p("target_family = \"abc\"").validate_as_target().is_ok()); - assert!(p("target_env = \"abc\"").validate_as_target().is_ok()); - assert!(p("target_endian = \"abc\"").validate_as_target().is_ok()); - assert!(p("target_pointer_width = \"abc\"") - .validate_as_target() - .is_ok()); - assert!(p("target_vendor = \"abc\"").validate_as_target().is_ok()); - assert!(p("bar = \"def\"").validate_as_target().is_ok()); - - assert!(p("test").validate_as_target().is_err()); - assert!(p("debug_assertions").validate_as_target().is_err()); - assert!(p("proc_macro").validate_as_target().is_err()); - assert!(p("any(not(debug_assertions), windows)") - .validate_as_target() - .is_err()); - - assert!(p("feature = \"abc\"").validate_as_target().is_err()); - assert!(p("any(not(feature = \"def\"), target_arch = \"abc\")") - .validate_as_target() - .is_err()); -} diff --git a/crates/cargo-platform/src/error.rs b/crates/cargo-platform/src/error.rs index ce0fb49f350..19a15a1e132 100644 --- a/crates/cargo-platform/src/error.rs +++ b/crates/cargo-platform/src/error.rs @@ -17,8 +17,6 @@ pub enum ParseErrorKind { IncompleteExpr(&'static str), UnterminatedExpression(String), InvalidTarget(String), - InvalidCfgName(String), - InvalidCfgKey(String), #[doc(hidden)] __Nonexhaustive, @@ -55,8 +53,6 @@ impl fmt::Display for ParseErrorKind { write!(f, "unexpected content `{}` found after cfg expression", s) } InvalidTarget(s) => write!(f, "invalid target specifier: {}", s), - InvalidCfgName(name) => write!(f, "invalid name in target cfg: {}", name), - InvalidCfgKey(name) => write!(f, "invalid key in target cfg: {}", name), __Nonexhaustive => unreachable!(), } } diff --git a/crates/cargo-platform/src/lib.rs b/crates/cargo-platform/src/lib.rs index 9b3033da4a5..e647643ca16 100644 --- a/crates/cargo-platform/src/lib.rs +++ b/crates/cargo-platform/src/lib.rs @@ -61,6 +61,48 @@ impl Platform { } Ok(()) } + + pub fn check_cfg_attributes(&self, warnings: &mut Vec) { + fn check_cfg_expr(expr: &CfgExpr, warnings: &mut Vec) { + match *expr { + CfgExpr::Not(ref e) => check_cfg_expr(e, warnings), + CfgExpr::All(ref e) | CfgExpr::Any(ref e) => { + for e in e { + check_cfg_expr(e, warnings); + } + } + CfgExpr::Value(ref e) => match e { + Cfg::Name(name) => match name.as_str() { + "test" | "debug_assertions" | "proc_macro" => + warnings.push(format!( + "Found `{}` in `target.'cfg(...)'.dependencies`. \ + This value is not supported for selecting dependencies \ + and will not work as expected. \ + To learn more visit \ + https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies", + name + )), + _ => (), + }, + Cfg::KeyPair(name, _) => match name.as_str() { + "feature" => + warnings.push(String::from( + "Found `feature = ...` in `target.'cfg(...)'.dependencies`. \ + This key is not supported for selecting dependencies \ + and will not work as expected. \ + Use the [features] section instead: \ + https://doc.rust-lang.org/cargo/reference/manifest.html#the-features-section" + )), + _ => (), + }, + } + } + } + + if let Platform::Cfg(cfg) = self { + check_cfg_expr(cfg, warnings); + } + } } impl serde::Serialize for Platform { @@ -88,10 +130,7 @@ impl FromStr for Platform { fn from_str(s: &str) -> Result { if s.starts_with("cfg(") && s.ends_with(')') { let s = &s[4..s.len() - 1]; - let cfg: CfgExpr = s.parse()?; - cfg.validate_as_target() - .map_err(|kind| ParseError::new(s, kind))?; - Ok(Platform::Cfg(cfg)) + s.parse().map(Platform::Cfg) } else { Platform::validate_named_platform(s)?; Ok(Platform::Name(s.to_string())) diff --git a/crates/cargo-platform/tests/test_cfg.rs b/crates/cargo-platform/tests/test_cfg.rs index 703acc908d3..dd99d9a79b8 100644 --- a/crates/cargo-platform/tests/test_cfg.rs +++ b/crates/cargo-platform/tests/test_cfg.rs @@ -155,16 +155,6 @@ fn bad_target_name() { "failed to parse `!foo` as a cfg expression: \ invalid target specifier: unexpected character ! in target name", ); - bad::( - "cfg(debug_assertions)", - "failed to parse `debug_assertions` as a cfg expression: \ - invalid name in target cfg: debug_assertions", - ); - bad::( - "cfg(feature = \"abc\")", - "failed to parse `feature = \"abc\"` as a cfg expression: \ - invalid key in target cfg: feature", - ); } #[test] @@ -186,3 +176,76 @@ fn round_trip_platform() { all(target_os = \"freebsd\", target_arch = \"x86_64\")))", ); } + +#[test] +fn check_cfg_attributes() { + fn ok(s: &str) { + let p = Platform::Cfg(s.parse().unwrap()); + let mut warnings = Vec::new(); + p.check_cfg_attributes(&mut warnings); + assert!( + warnings.is_empty(), + "Expected no warnings but got: {:?}", + warnings, + ); + } + + fn warn(s: &str, names: &[&str]) { + let p = Platform::Cfg(s.parse().unwrap()); + let mut warnings = Vec::new(); + p.check_cfg_attributes(&mut warnings); + assert_eq!( + warnings.len(), + names.len(), + "Expecter warnings about {:?} but got {:?}", + names, + warnings, + ); + for (name, warning) in names.iter().zip(warnings.iter()) { + assert!( + warning.contains(name), + "Expected warning about '{}' but got: {}", + name, + warning, + ); + } + } + + ok("unix"); + ok("windows"); + ok("any(not(unix), windows)"); + ok("foo"); + + ok("target_arch = \"abc\""); + ok("target_feature = \"abc\""); + ok("target_os = \"abc\""); + ok("target_family = \"abc\""); + ok("target_env = \"abc\""); + ok("target_endian = \"abc\""); + ok("target_pointer_width = \"abc\""); + ok("target_vendor = \"abc\""); + ok("bar = \"def\""); + + warn("test", &["test"]); + warn("debug_assertions", &["debug_assertions"]); + warn("proc_macro", &["proc_macro"]); + warn("feature = \"abc\"", &["feature"]); + + warn("any(not(debug_assertions), windows)", &["debug_assertions"]); + warn( + "any(not(feature = \"def\"), target_arch = \"abc\")", + &["feature"], + ); + warn( + "any(not(target_os = \"windows\"), proc_macro)", + &["proc_macro"], + ); + warn( + "any(not(feature = \"windows\"), proc_macro)", + &["feature", "proc_macro"], + ); + warn( + "all(not(debug_assertions), any(windows, proc_macro))", + &["debug_assertions", "proc_macro"], + ); +} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 1553553010d..8ce11f22ece 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1085,7 +1085,11 @@ impl TomlManifest { process_dependencies(&mut cx, build_deps, Some(Kind::Build))?; for (name, platform) in me.target.iter().flatten() { - cx.platform = Some(name.parse()?); + cx.platform = { + let platform: Platform = name.parse()?; + platform.check_cfg_attributes(&mut cx.warnings); + Some(platform) + }; process_dependencies(&mut cx, platform.dependencies.as_ref(), None)?; let build_deps = platform .build_dependencies diff --git a/src/doc/src/reference/specifying-dependencies.md b/src/doc/src/reference/specifying-dependencies.md index 38488fc94ee..aa2881df00b 100644 --- a/src/doc/src/reference/specifying-dependencies.md +++ b/src/doc/src/reference/specifying-dependencies.md @@ -477,6 +477,8 @@ Use [the `[features]` section](manifest.md#the-features-section) instead. The same applies to `cfg(debug_assertions)`, `cfg(test)` and `cfg(prog_macro)`. +These values will not work as expected and will always have the default value +returned by `rustc --print=cfg`. There is currently no way to add dependencies based on these configuration values. In addition to `#[cfg]` syntax, Cargo also supports listing out the full target