Skip to content

Commit

Permalink
Add future-incompatibility warning against some keyword-as-ident
Browse files Browse the repository at this point in the history
  • Loading branch information
Urgau committed Nov 9, 2024
1 parent bfa0e1c commit 6577c4f
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 3 deletions.
8 changes: 8 additions & 0 deletions crates/cargo-platform/src/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ enum Token<'a> {
String(&'a str),
}

/// The list of keywords.
///
/// We should consider all the keywords, but some are conditional on
/// the edition so for now we just consider true/false.
///
/// <https://doc.rust-lang.org/reference/keywords.html>
pub(crate) const KEYWORDS: &[&str; 2] = &["true", "false"];

#[derive(Clone)]
struct Tokenizer<'a> {
s: iter::Peekable<str::CharIndices<'a>>,
Expand Down
43 changes: 42 additions & 1 deletion crates/cargo-platform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@
//!
//! [`Platform`]: enum.Platform.html
use std::fmt;
use std::str::FromStr;
use std::{fmt, path::Path};

mod cfg;
mod error;

use cfg::KEYWORDS;
pub use cfg::{Cfg, CfgExpr};
pub use error::{ParseError, ParseErrorKind};

Expand Down Expand Up @@ -104,6 +105,46 @@ impl Platform {
check_cfg_expr(cfg, warnings);
}
}

pub fn check_cfg_keywords(&self, warnings: &mut Vec<String>, path: &Path) {
fn check_cfg_expr(expr: &CfgExpr, warnings: &mut Vec<String>, path: &Path) {
match *expr {
CfgExpr::Not(ref e) => check_cfg_expr(e, warnings, path),
CfgExpr::All(ref e) | CfgExpr::Any(ref e) => {
for e in e {
check_cfg_expr(e, warnings, path);
}
}
CfgExpr::Value(ref e) => match e {
Cfg::Name(name) | Cfg::KeyPair(name, _) => {
if KEYWORDS.contains(&name.as_str()) {
if name.as_str() == "true" || name.as_str() == "false" {
warnings.push(format!(
"[{}] future-incompatibility: the meaning of `cfg({e})` will change in the future\n \
| Cargo is erroneously allowing `cfg(true)` and `cfg(false)`, but both forms are interpreted as false unless manually overridden with `--cfg`.\n \
| In the future these will be built-in defines that will have the corresponding true/false value.\n \
| It is recommended to avoid using these configs until they are properly supported.\n \
| See <https://github.com/rust-lang/rust/issues/131204> for more information.",
path.display()
));
} else {
warnings.push(format!(
"[{}] future-incompatibility: `cfg({e})` is deprecated as `{name}` is a keyword \
and not an identifier and should not have have been accepted in this position.\n \
| this was previously accepted by Cargo but is being phased out; it will become a hard error in a future release!",
path.display()
));
}
}
}
},
}
}

if let Platform::Cfg(cfg) = self {
check_cfg_expr(cfg, warnings, path);
}
}
}

impl serde::Serialize for Platform {
Expand Down
9 changes: 8 additions & 1 deletion src/cargo/util/context/target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::core::compiler::{BuildOutput, LinkArgTarget};
use crate::util::CargoResult;
use serde::Deserialize;
use std::collections::{BTreeMap, HashMap};
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::rc::Rc;

/// Config definition of a `[target.'cfg(…)']` table.
Expand Down Expand Up @@ -53,6 +53,13 @@ pub(super) fn load_target_cfgs(
let target: BTreeMap<String, TargetCfgConfig> = gctx.get("target")?;
tracing::debug!("Got all targets {:#?}", target);
for (key, cfg) in target {
if let Ok(platform) = key.parse::<cargo_platform::Platform>() {
let mut warnings = Vec::new();
platform.check_cfg_keywords(&mut warnings, &Path::new(".cargo/config.toml"));
for w in warnings {
gctx.shell().warn(w)?;
}
}
if key.starts_with("cfg(") {
// Unfortunately this is not able to display the location of the
// unused key. Using config::Value<toml::Value> doesn't work. One
Expand Down
1 change: 1 addition & 0 deletions src/cargo/util/toml/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1326,6 +1326,7 @@ pub fn to_real_manifest(
for (name, platform) in original_toml.target.iter().flatten() {
let platform_kind: Platform = name.parse()?;
platform_kind.check_cfg_attributes(warnings);
platform_kind.check_cfg_keywords(warnings, manifest_file);
let platform_kind = Some(platform_kind);
validate_dependencies(
platform.dependencies.as_ref(),
Expand Down
13 changes: 12 additions & 1 deletion tests/testsuite/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,9 +550,20 @@ fn cfg_keywords() {

p.cargo("check")
.with_stderr_data(str![[r#"
[WARNING] [[ROOT]/foo/Cargo.toml] future-incompatibility: the meaning of `cfg(true)` will change in the future
| Cargo is erroneously allowing `cfg(true)` and `cfg(false)`, but both forms are interpreted as false unless manually overridden with `--cfg`.
| In the future these will be built-in defines that will have the corresponding true/false value.
| It is recommended to avoid using these configs until they are properly supported.
| See <https://github.com/rust-lang/rust/issues/131204> for more information.
[WARNING] [.cargo/config.toml] future-incompatibility: the meaning of `cfg(false)` will change in the future
| Cargo is erroneously allowing `cfg(true)` and `cfg(false)`, but both forms are interpreted as false unless manually overridden with `--cfg`.
| In the future these will be built-in defines that will have the corresponding true/false value.
| It is recommended to avoid using these configs until they are properly supported.
| See <https://github.com/rust-lang/rust/issues/131204> for more information.
[LOCKING] 1 package to latest compatible version
[CHECKING] foo v0.1.0 ([ROOT]/foo)
...
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
"#]])
.run();
}

0 comments on commit 6577c4f

Please sign in to comment.