From e3eda309263300b935088ca16c485f36e1c50062 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 13:14:37 -0600 Subject: [PATCH 1/9] refactor(config): Pull in toml parse wrapper --- src/cargo/util/config/mod.rs | 10 +++++++--- src/cargo/util/toml/mod.rs | 5 ----- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/cargo/util/config/mod.rs b/src/cargo/util/config/mod.rs index 3ed2bc52ecd..50153466b82 100644 --- a/src/cargo/util/config/mod.rs +++ b/src/cargo/util/config/mod.rs @@ -76,7 +76,6 @@ use crate::sources::CRATES_IO_REGISTRY; use crate::util::errors::CargoResult; use crate::util::network::http::configure_http_handle; use crate::util::network::http::http_handle; -use crate::util::toml as cargo_toml; use crate::util::{internal, CanonicalUrl}; use crate::util::{try_canonicalize, validate_package_name}; use crate::util::{Filesystem, IntoUrl, IntoUrlWithBase, Rustc}; @@ -1198,7 +1197,7 @@ impl Config { } let contents = fs::read_to_string(path) .with_context(|| format!("failed to read configuration file `{}`", path.display()))?; - let toml = cargo_toml::parse_document(&contents, path, self).with_context(|| { + let toml = parse_document(&contents, path, self).with_context(|| { format!("could not parse TOML configuration in `{}`", path.display()) })?; let def = match why_load { @@ -2249,7 +2248,7 @@ pub fn save_credentials( ) })?; - let mut toml = cargo_toml::parse_document(&contents, file.path(), cfg)?; + let mut toml = parse_document(&contents, file.path(), cfg)?; // Move the old token location to the new one. if let Some(token) = toml.remove("token") { @@ -2716,6 +2715,11 @@ impl EnvConfigValue { pub type EnvConfig = HashMap; +fn parse_document(toml: &str, _file: &Path, _config: &Config) -> CargoResult { + // At the moment, no compatibility checks are needed. + toml.parse().map_err(Into::into) +} + /// A type to deserialize a list of strings from a toml file. /// /// Supports deserializing either a whitespace-separated list of arguments in a diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 3c1dc48f3ee..ccb19397a19 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -175,11 +175,6 @@ fn read_manifest_from_str( } } -pub fn parse_document(toml: &str, _file: &Path, _config: &Config) -> CargoResult { - // At the moment, no compatibility checks are needed. - toml.parse().map_err(Into::into) -} - /// Warn about paths that have been deprecated and may conflict. fn warn_on_deprecated(new_path: &str, name: &str, kind: &str, warnings: &mut Vec) { let old_path = new_path.replace("-", "_"); From 90e2995eb1430203b0892461ef4e52b2fd0f8a97 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 13:16:44 -0600 Subject: [PATCH 2/9] refactor(toml): Reduce visibility This will make it clearer what each piece of logic belongs to --- src/cargo/util/toml/embedded.rs | 4 ++-- src/cargo/util/toml/mod.rs | 25 ++++++++++++------------- src/cargo/util/toml/targets.rs | 2 +- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/cargo/util/toml/embedded.rs b/src/cargo/util/toml/embedded.rs index 8e3912288bf..4c57195d4f4 100644 --- a/src/cargo/util/toml/embedded.rs +++ b/src/cargo/util/toml/embedded.rs @@ -8,7 +8,7 @@ const DEFAULT_EDITION: crate::core::features::Edition = crate::core::features::Edition::LATEST_STABLE; const AUTO_FIELDS: &[&str] = &["autobins", "autoexamples", "autotests", "autobenches"]; -pub fn expand_manifest( +pub(super) fn expand_manifest( content: &str, path: &std::path::Path, config: &Config, @@ -329,7 +329,7 @@ impl DocFragment { } #[derive(Clone, Copy, PartialEq, Debug)] -pub enum CommentKind { +enum CommentKind { Line, Block, } diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index ccb19397a19..7af8ced4429 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -28,7 +28,7 @@ use crate::util::{ RustVersion, }; -pub mod embedded; +mod embedded; pub mod schema; mod targets; use self::targets::targets; @@ -1446,7 +1446,7 @@ fn inheritable_from_path( } /// Returns the name of the README file for a [`schema::TomlPackage`]. -pub fn readme_for_package( +fn readme_for_package( package_root: &Path, readme: Option<&schema::StringOrBool>, ) -> Option { @@ -1505,7 +1505,7 @@ macro_rules! inheritable_field_getter { ( $(($key:literal, $field:ident -> $ret:ty),)* ) => ( $( #[doc = concat!("Gets the field `workspace.", $key, "`.")] - pub fn $field(&self) -> CargoResult<$ret> { + fn $field(&self) -> CargoResult<$ret> { let Some(val) = &self.$field else { bail!("`workspace.{}` was not defined", $key); }; @@ -1518,7 +1518,6 @@ macro_rules! inheritable_field_getter { impl schema::InheritableFields { inheritable_field_getter! { // Please keep this list lexicographically ordered. - ("dependencies", dependencies -> BTreeMap), ("lints", lints -> schema::TomlLints), ("package.authors", authors -> Vec), ("package.badges", badges -> BTreeMap>), @@ -1538,7 +1537,7 @@ impl schema::InheritableFields { } /// Gets a workspace dependency with the `name`. - pub fn get_dependency( + fn get_dependency( &self, name: &str, package_root: &Path, @@ -1557,7 +1556,7 @@ impl schema::InheritableFields { } /// Gets the field `workspace.package.license-file`. - pub fn license_file(&self, package_root: &Path) -> CargoResult { + fn license_file(&self, package_root: &Path) -> CargoResult { let Some(license_file) = &self.license_file else { bail!("`workspace.package.license-file` was not defined"); }; @@ -1565,7 +1564,7 @@ impl schema::InheritableFields { } /// Gets the field `workspace.package.readme`. - pub fn readme(&self, package_root: &Path) -> CargoResult { + fn readme(&self, package_root: &Path) -> CargoResult { let Some(readme) = readme_for_package(self.ws_root.as_path(), self.readme.as_ref()) else { bail!("`workspace.package.readme` was not defined"); }; @@ -1573,25 +1572,25 @@ impl schema::InheritableFields { .map(schema::StringOrBool::String) } - pub fn ws_root(&self) -> &PathBuf { + fn ws_root(&self) -> &PathBuf { &self.ws_root } - pub fn update_deps(&mut self, deps: Option>) { + fn update_deps(&mut self, deps: Option>) { self.dependencies = deps; } - pub fn update_lints(&mut self, lints: Option) { + fn update_lints(&mut self, lints: Option) { self.lints = lints; } - pub fn update_ws_path(&mut self, ws_root: PathBuf) { + fn update_ws_path(&mut self, ws_root: PathBuf) { self.ws_root = ws_root; } } impl schema::TomlPackage { - pub fn to_package_id(&self, source_id: SourceId, version: semver::Version) -> PackageId { + fn to_package_id(&self, source_id: SourceId, version: semver::Version) -> PackageId { PackageId::pure(self.name.as_str().into(), version, source_id) } } @@ -2084,7 +2083,7 @@ impl schema::TomlProfiles { /// /// It's a bit unfortunate both `-Z` flags and `cargo-features` are required, /// because profiles can now be set in either `Cargo.toml` or `config.toml`. - pub fn validate( + fn validate( &self, cli_unstable: &CliUnstable, features: &Features, diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index bfc1419c89f..2a05c3d4320 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -31,7 +31,7 @@ const DEFAULT_BENCH_DIR_NAME: &'static str = "benches"; const DEFAULT_EXAMPLE_DIR_NAME: &'static str = "examples"; const DEFAULT_BIN_DIR_NAME: &'static str = "bin"; -pub fn targets( +pub(super) fn targets( features: &Features, manifest: &TomlManifest, package_name: &str, From 2a314c7642567706ec90e6fadac0d67c54b6d40e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 13:25:55 -0600 Subject: [PATCH 3/9] refactor(toml): Pull out profile name validation It now lives with other name validation logic. --- src/cargo/util/command_prelude.rs | 13 +++-- src/cargo/util/restricted_names.rs | 79 ++++++++++++++++++++++++++++ src/cargo/util/toml/mod.rs | 82 +----------------------------- 3 files changed, 87 insertions(+), 87 deletions(-) diff --git a/src/cargo/util/command_prelude.rs b/src/cargo/util/command_prelude.rs index 6e90d3228d2..3888b80c47f 100644 --- a/src/cargo/util/command_prelude.rs +++ b/src/cargo/util/command_prelude.rs @@ -6,9 +6,8 @@ use crate::ops::{CompileFilter, CompileOptions, NewOptions, Packages, VersionCon use crate::util::important_paths::find_root_manifest_for_wd; use crate::util::interning::InternedString; use crate::util::is_rustup; -use crate::util::restricted_names::is_glob_pattern; -use crate::util::toml::schema::{StringOrVec, TomlProfile}; -use crate::util::validate_package_name; +use crate::util::restricted_names; +use crate::util::toml::schema::StringOrVec; use crate::util::{ print_available_benches, print_available_binaries, print_available_examples, print_available_packages, print_available_tests, @@ -607,7 +606,7 @@ Run `{cmd}` to see possible targets." bail!("profile `doc` is reserved and not allowed to be explicitly specified") } (_, _, Some(name)) => { - TomlProfile::validate_name(name)?; + restricted_names::validate_profile_name(name)?; name } }; @@ -801,7 +800,7 @@ Run `{cmd}` to see possible targets." ) -> CargoResult { let mut compile_opts = self.compile_options(config, mode, workspace, profile_checking)?; let spec = self._values_of("package"); - if spec.iter().any(is_glob_pattern) { + if spec.iter().any(restricted_names::is_glob_pattern) { anyhow::bail!("Glob patterns on package selection are not supported.") } compile_opts.spec = Packages::Packages(spec); @@ -835,7 +834,7 @@ Run `{cmd}` to see possible targets." (None, None) => config.default_registry()?.map(RegistryOrIndex::Registry), (None, Some(i)) => Some(RegistryOrIndex::Index(i.into_url()?)), (Some(r), None) => { - validate_package_name(r, "registry name", "")?; + restricted_names::validate_package_name(r, "registry name", "")?; Some(RegistryOrIndex::Registry(r.to_string())) } (Some(_), Some(_)) => { @@ -850,7 +849,7 @@ Run `{cmd}` to see possible targets." match self._value_of("registry").map(|s| s.to_string()) { None => config.default_registry(), Some(registry) => { - validate_package_name(®istry, "registry name", "")?; + restricted_names::validate_package_name(®istry, "registry name", "")?; Ok(Some(registry)) } } diff --git a/src/cargo/util/restricted_names.rs b/src/cargo/util/restricted_names.rs index 2c3eaa9e1f8..f61249775d3 100644 --- a/src/cargo/util/restricted_names.rs +++ b/src/cargo/util/restricted_names.rs @@ -120,3 +120,82 @@ pub fn is_windows_reserved_path(path: &Path) -> bool { pub fn is_glob_pattern>(name: T) -> bool { name.as_ref().contains(&['*', '?', '[', ']'][..]) } + +/// Validate dir-names and profile names according to RFC 2678. +pub fn validate_profile_name(name: &str) -> CargoResult<()> { + if let Some(ch) = name + .chars() + .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') + { + bail!( + "invalid character `{}` in profile name `{}`\n\ + Allowed characters are letters, numbers, underscore, and hyphen.", + ch, + name + ); + } + + const SEE_DOCS: &str = "See https://doc.rust-lang.org/cargo/reference/profiles.html \ + for more on configuring profiles."; + + let lower_name = name.to_lowercase(); + if lower_name == "debug" { + bail!( + "profile name `{}` is reserved\n\ + To configure the default development profile, use the name `dev` \ + as in [profile.dev]\n\ + {}", + name, + SEE_DOCS + ); + } + if lower_name == "build-override" { + bail!( + "profile name `{}` is reserved\n\ + To configure build dependency settings, use [profile.dev.build-override] \ + and [profile.release.build-override]\n\ + {}", + name, + SEE_DOCS + ); + } + + // These are some arbitrary reservations. We have no plans to use + // these, but it seems safer to reserve a few just in case we want to + // add more built-in profiles in the future. We can also uses special + // syntax like cargo:foo if needed. But it is unlikely these will ever + // be used. + if matches!( + lower_name.as_str(), + "build" + | "check" + | "clean" + | "config" + | "fetch" + | "fix" + | "install" + | "metadata" + | "package" + | "publish" + | "report" + | "root" + | "run" + | "rust" + | "rustc" + | "rustdoc" + | "target" + | "tmp" + | "uninstall" + ) || lower_name.starts_with("cargo") + { + bail!( + "profile name `{}` is reserved\n\ + Please choose a different name.\n\ + {}", + name, + SEE_DOCS + ); + } + + Ok(()) +} diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 7af8ced4429..d470a0cddfa 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -23,6 +23,7 @@ use crate::core::{GitReference, PackageIdSpec, SourceId, WorkspaceConfig, Worksp use crate::sources::{CRATES_IO_INDEX, CRATES_IO_REGISTRY}; use crate::util::errors::{CargoResult, ManifestError}; use crate::util::interning::InternedString; +use crate::util::restricted_names; use crate::util::{ self, config::ConfigRelativePath, validate_package_name, Config, IntoUrl, OptVersionReq, RustVersion, @@ -2122,7 +2123,7 @@ impl schema::TomlProfile { } // Profile name validation - Self::validate_name(name)?; + restricted_names::validate_profile_name(name)?; if let Some(dir_name) = &self.dir_name { // This is disabled for now, as we would like to stabilize named @@ -2180,85 +2181,6 @@ impl schema::TomlProfile { Ok(()) } - /// Validate dir-names and profile names according to RFC 2678. - pub fn validate_name(name: &str) -> CargoResult<()> { - if let Some(ch) = name - .chars() - .find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-') - { - bail!( - "invalid character `{}` in profile name `{}`\n\ - Allowed characters are letters, numbers, underscore, and hyphen.", - ch, - name - ); - } - - const SEE_DOCS: &str = "See https://doc.rust-lang.org/cargo/reference/profiles.html \ - for more on configuring profiles."; - - let lower_name = name.to_lowercase(); - if lower_name == "debug" { - bail!( - "profile name `{}` is reserved\n\ - To configure the default development profile, use the name `dev` \ - as in [profile.dev]\n\ - {}", - name, - SEE_DOCS - ); - } - if lower_name == "build-override" { - bail!( - "profile name `{}` is reserved\n\ - To configure build dependency settings, use [profile.dev.build-override] \ - and [profile.release.build-override]\n\ - {}", - name, - SEE_DOCS - ); - } - - // These are some arbitrary reservations. We have no plans to use - // these, but it seems safer to reserve a few just in case we want to - // add more built-in profiles in the future. We can also uses special - // syntax like cargo:foo if needed. But it is unlikely these will ever - // be used. - if matches!( - lower_name.as_str(), - "build" - | "check" - | "clean" - | "config" - | "fetch" - | "fix" - | "install" - | "metadata" - | "package" - | "publish" - | "report" - | "root" - | "run" - | "rust" - | "rustc" - | "rustdoc" - | "target" - | "tmp" - | "uninstall" - ) || lower_name.starts_with("cargo") - { - bail!( - "profile name `{}` is reserved\n\ - Please choose a different name.\n\ - {}", - name, - SEE_DOCS - ); - } - - Ok(()) - } - /// Validates a profile. /// /// This is a shallow check, which is reused for the profile itself and any overrides. From 807d7ed2f048da6dd9b5527ca42ea4957590f547 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 14:22:52 -0600 Subject: [PATCH 4/9] refactor(toml): Remove unused Rc --- src/cargo/ops/cargo_package.rs | 13 +++++-------- src/cargo/util/toml/mod.rs | 11 +++++------ 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/cargo/ops/cargo_package.rs b/src/cargo/ops/cargo_package.rs index 4d145d887c0..6ac09dc771d 100644 --- a/src/cargo/ops/cargo_package.rs +++ b/src/cargo/ops/cargo_package.rs @@ -3,7 +3,6 @@ use std::fs::{self, File}; use std::io::prelude::*; use std::io::SeekFrom; use std::path::{Path, PathBuf}; -use std::rc::Rc; use std::sync::Arc; use std::task::Poll; @@ -412,16 +411,14 @@ fn build_lock(ws: &Workspace<'_>, orig_pkg: &Package) -> CargoResult { let orig_resolve = ops::load_pkg_lockfile(ws)?; // Convert Package -> TomlManifest -> Manifest -> Package - let toml_manifest = Rc::new( - orig_pkg - .manifest() - .original() - .prepare_for_publish(ws, orig_pkg.root())?, - ); + let toml_manifest = orig_pkg + .manifest() + .original() + .prepare_for_publish(ws, orig_pkg.root())?; let package_root = orig_pkg.root(); let source_id = orig_pkg.package_id().source_id(); let (manifest, _nested_paths) = - TomlManifest::to_real_manifest(&toml_manifest, false, source_id, package_root, config)?; + TomlManifest::to_real_manifest(toml_manifest, false, source_id, package_root, config)?; let new_pkg = Package::new(manifest, orig_pkg.manifest_path()); let max_rust_version = new_pkg.rust_version().cloned(); diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index d470a0cddfa..349d1b4d41f 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -111,7 +111,6 @@ fn read_manifest_from_str( } }; - let manifest = Rc::new(manifest); if let Some(deps) = manifest .workspace .as_ref() @@ -128,7 +127,7 @@ fn read_manifest_from_str( } return if manifest.project.is_some() || manifest.package.is_some() { let (mut manifest, paths) = schema::TomlManifest::to_real_manifest( - &manifest, + manifest, embedded, source_id, package_root, @@ -145,7 +144,7 @@ fn read_manifest_from_str( Ok((EitherManifest::Real(manifest), paths)) } else { let (mut m, paths) = - schema::TomlManifest::to_virtual_manifest(&manifest, source_id, package_root, config)?; + schema::TomlManifest::to_virtual_manifest(manifest, source_id, package_root, config)?; add_unused(m.warnings_mut()); Ok((EitherManifest::Virtual(m), paths)) }; @@ -391,7 +390,7 @@ impl schema::TomlManifest { } pub fn to_real_manifest( - me: &Rc, + me: schema::TomlManifest, embedded: bool, source_id: SourceId, package_root: &Path, @@ -603,7 +602,7 @@ impl schema::TomlManifest { // If we have a lib with no path, use the inferred lib or else the package name. let targets = targets( &features, - me, + &me, package_name, package_root, edition, @@ -1119,7 +1118,7 @@ impl schema::TomlManifest { } fn to_virtual_manifest( - me: &Rc, + me: schema::TomlManifest, source_id: SourceId, root: &Path, config: &Config, From 778dc4d4b328b9a92b8ea120746e3f93d44832d2 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 15:19:05 -0600 Subject: [PATCH 5/9] refactor(toml): Move target build.rs logic next to use --- src/cargo/util/toml/mod.rs | 24 ------------------------ src/cargo/util/toml/targets.rs | 22 +++++++++++++++++++++- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 349d1b4d41f..b25d11dea04 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -1310,30 +1310,6 @@ impl schema::TomlManifest { } Ok(patch) } - - /// Returns the path to the build script if one exists for this crate. - fn maybe_custom_build( - &self, - build: &Option, - package_root: &Path, - ) -> Option { - let build_rs = package_root.join("build.rs"); - match *build { - // Explicitly no build script. - Some(schema::StringOrBool::Bool(false)) => None, - Some(schema::StringOrBool::Bool(true)) => Some(build_rs), - Some(schema::StringOrBool::String(ref s)) => Some(PathBuf::from(s)), - None => { - // If there is a `build.rs` file next to the `Cargo.toml`, assume it is - // a build script. - if build_rs.is_file() { - Some(build_rs) - } else { - None - } - } - } - } } struct Context<'a, 'b> { diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 2a05c3d4320..9624312f0ac 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -105,7 +105,7 @@ pub(super) fn targets( )?); // processing the custom build script - if let Some(custom_build) = manifest.maybe_custom_build(custom_build, package_root) { + if let Some(custom_build) = maybe_custom_build(custom_build, package_root) { if metabuild.is_some() { anyhow::bail!("cannot specify both `metabuild` and `build`"); } @@ -964,3 +964,23 @@ Cargo doesn't know which to use because multiple target files found at `{}` and (None, Some(_)) => unreachable!(), } } + +/// Returns the path to the build script if one exists for this crate. +fn maybe_custom_build(build: &Option, package_root: &Path) -> Option { + let build_rs = package_root.join("build.rs"); + match *build { + // Explicitly no build script. + Some(StringOrBool::Bool(false)) => None, + Some(StringOrBool::Bool(true)) => Some(build_rs), + Some(StringOrBool::String(ref s)) => Some(PathBuf::from(s)), + None => { + // If there is a `build.rs` file next to the `Cargo.toml`, assume it is + // a build script. + if build_rs.is_file() { + Some(build_rs) + } else { + None + } + } + } +} From d87b92cce54a6308b84f02be031a9edbf3bae7aa Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 15:34:57 -0600 Subject: [PATCH 6/9] refactor(toml): Let schema abstract over duplicate fields --- src/cargo/util/toml/mod.rs | 17 ----------------- src/cargo/util/toml/schema.rs | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index b25d11dea04..b81152a8978 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2333,17 +2333,6 @@ impl schema::TomlTarget { } } - fn proc_macro(&self) -> Option { - self.proc_macro_raw.or(self.proc_macro_raw2).or_else(|| { - if let Some(types) = self.crate_types() { - if types.contains(&"proc-macro".to_string()) { - return Some(true); - } - } - None - }) - } - fn validate_crate_types(&self, target_kind_human: &str, warnings: &mut Vec) { if self.crate_type.is_some() && self.crate_type2.is_some() { warn_on_deprecated( @@ -2354,12 +2343,6 @@ impl schema::TomlTarget { ); } } - - fn crate_types(&self) -> Option<&Vec> { - self.crate_type - .as_ref() - .or_else(|| self.crate_type2.as_ref()) - } } impl schema::MaybeWorkspaceLints { diff --git a/src/cargo/util/toml/schema.rs b/src/cargo/util/toml/schema.rs index f884195d1cc..eaeedac1fc6 100644 --- a/src/cargo/util/toml/schema.rs +++ b/src/cargo/util/toml/schema.rs @@ -939,6 +939,23 @@ impl TomlTarget { pub fn new() -> TomlTarget { TomlTarget::default() } + + pub fn proc_macro(&self) -> Option { + self.proc_macro_raw.or(self.proc_macro_raw2).or_else(|| { + if let Some(types) = self.crate_types() { + if types.contains(&"proc-macro".to_string()) { + return Some(true); + } + } + None + }) + } + + pub fn crate_types(&self) -> Option<&Vec> { + self.crate_type + .as_ref() + .or_else(|| self.crate_type2.as_ref()) + } } /// Corresponds to a `target` entry, but `TomlTarget` is already used. From 0a6fe0abdd31ce2050abd28f4a479d5abb5db8e7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 15:38:18 -0600 Subject: [PATCH 7/9] refactor(toml): Generalize schema abstracting over duplicate fields --- src/cargo/util/toml/mod.rs | 61 ++++++++--------------------------- src/cargo/util/toml/schema.rs | 38 ++++++++++++++++++++++ 2 files changed, 51 insertions(+), 48 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index b81152a8978..22e9926f5d9 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -280,19 +280,11 @@ impl schema::TomlManifest { dependencies: map_deps(config, self.dependencies.as_ref(), all)?, dev_dependencies: map_deps( config, - self.dev_dependencies - .as_ref() - .or_else(|| self.dev_dependencies2.as_ref()), + self.dev_dependencies(), schema::TomlDependency::is_version_specified, )?, dev_dependencies2: None, - build_dependencies: map_deps( - config, - self.build_dependencies - .as_ref() - .or_else(|| self.build_dependencies2.as_ref()), - all, - )?, + build_dependencies: map_deps(config, self.build_dependencies(), all)?, build_dependencies2: None, features: self.features.clone(), target: match self.target.as_ref().map(|target_map| { @@ -305,19 +297,11 @@ impl schema::TomlManifest { dependencies: map_deps(config, v.dependencies.as_ref(), all)?, dev_dependencies: map_deps( config, - v.dev_dependencies - .as_ref() - .or_else(|| v.dev_dependencies2.as_ref()), + v.dev_dependencies(), schema::TomlDependency::is_version_specified, )?, dev_dependencies2: None, - build_dependencies: map_deps( - config, - v.build_dependencies - .as_ref() - .or_else(|| v.build_dependencies2.as_ref()), - all, - )?, + build_dependencies: map_deps(config, v.build_dependencies(), all)?, build_dependencies2: None, }, )) @@ -715,10 +699,7 @@ impl schema::TomlManifest { if me.dev_dependencies.is_some() && me.dev_dependencies2.is_some() { warn_on_deprecated("dev-dependencies", package_name, "package", cx.warnings); } - let dev_deps = me - .dev_dependencies - .as_ref() - .or_else(|| me.dev_dependencies2.as_ref()); + let dev_deps = me.dev_dependencies(); let dev_deps = process_dependencies( &mut cx, dev_deps, @@ -729,10 +710,7 @@ impl schema::TomlManifest { if me.build_dependencies.is_some() && me.build_dependencies2.is_some() { warn_on_deprecated("build-dependencies", package_name, "package", cx.warnings); } - let build_deps = me - .build_dependencies - .as_ref() - .or_else(|| me.build_dependencies2.as_ref()); + let build_deps = me.build_dependencies(); let build_deps = process_dependencies( &mut cx, build_deps, @@ -767,10 +745,7 @@ impl schema::TomlManifest { if platform.build_dependencies.is_some() && platform.build_dependencies2.is_some() { warn_on_deprecated("build-dependencies", name, "platform target", cx.warnings); } - let build_deps = platform - .build_dependencies - .as_ref() - .or_else(|| platform.build_dependencies2.as_ref()); + let build_deps = platform.build_dependencies(); let build_deps = process_dependencies( &mut cx, build_deps, @@ -781,10 +756,7 @@ impl schema::TomlManifest { if platform.dev_dependencies.is_some() && platform.dev_dependencies2.is_some() { warn_on_deprecated("dev-dependencies", name, "platform target", cx.warnings); } - let dev_deps = platform - .dev_dependencies - .as_ref() - .or_else(|| platform.dev_dependencies2.as_ref()); + let dev_deps = platform.dev_dependencies(); let dev_deps = process_dependencies( &mut cx, dev_deps, @@ -1147,10 +1119,10 @@ impl schema::TomlManifest { if me.dependencies.is_some() { bail!("this virtual manifest specifies a [dependencies] section, which is not allowed"); } - if me.dev_dependencies.is_some() || me.dev_dependencies2.is_some() { + if me.dev_dependencies().is_some() { bail!("this virtual manifest specifies a [dev-dependencies] section, which is not allowed"); } - if me.build_dependencies.is_some() || me.build_dependencies2.is_some() { + if me.build_dependencies().is_some() { bail!("this virtual manifest specifies a [build-dependencies] section, which is not allowed"); } if me.features.is_some() { @@ -1662,7 +1634,7 @@ impl schema::TomlWorkspaceDependency { inheritable()?.get_dependency(name, cx.root).map(|d| { match d { schema::TomlDependency::Simple(s) => { - if let Some(false) = self.default_features.or(self.default_features2) { + if let Some(false) = self.default_features() { default_features_msg(name, None, cx); } if self.optional.is_some() || self.features.is_some() || self.public.is_some() { @@ -1679,10 +1651,7 @@ impl schema::TomlWorkspaceDependency { } schema::TomlDependency::Detailed(d) => { let mut d = d.clone(); - match ( - self.default_features.or(self.default_features2), - d.default_features.or(d.default_features2), - ) { + match (self.default_features(), d.default_features()) { // member: default-features = true and // workspace: default-features = false should turn on // default-features @@ -1982,11 +1951,7 @@ impl schema::DetailedTomlDependency

{ warn_on_deprecated("default-features", name_in_toml, "dependency", cx.warnings); } dep.set_features(self.features.iter().flatten()) - .set_default_features( - self.default_features - .or(self.default_features2) - .unwrap_or(true), - ) + .set_default_features(self.default_features().unwrap_or(true)) .set_optional(self.optional.unwrap_or(false)) .set_platform(cx.platform.clone()); if let Some(registry) = &self.registry { diff --git a/src/cargo/util/toml/schema.rs b/src/cargo/util/toml/schema.rs index eaeedac1fc6..6ea93e02193 100644 --- a/src/cargo/util/toml/schema.rs +++ b/src/cargo/util/toml/schema.rs @@ -45,6 +45,18 @@ impl TomlManifest { self.profile.is_some() } + pub fn dev_dependencies(&self) -> Option<&BTreeMap> { + self.dev_dependencies + .as_ref() + .or(self.dev_dependencies2.as_ref()) + } + + pub fn build_dependencies(&self) -> Option<&BTreeMap> { + self.build_dependencies + .as_ref() + .or(self.build_dependencies2.as_ref()) + } + pub fn features(&self) -> Option<&BTreeMap>> { self.features.as_ref() } @@ -462,6 +474,12 @@ pub struct TomlWorkspaceDependency { pub unused_keys: BTreeMap, } +impl TomlWorkspaceDependency { + pub fn default_features(&self) -> Option { + self.default_features.or(self.default_features2) + } +} + #[derive(Clone, Debug, Serialize)] #[serde(untagged)] pub enum TomlDependency { @@ -553,6 +571,12 @@ pub struct DetailedTomlDependency { pub unused_keys: BTreeMap, } +impl DetailedTomlDependency

{ + pub fn default_features(&self) -> Option { + self.default_features.or(self.default_features2) + } +} + // Explicit implementation so we avoid pulling in P: Default impl Default for DetailedTomlDependency

{ fn default() -> Self { @@ -971,6 +995,20 @@ pub struct TomlPlatform { pub dev_dependencies2: Option>, } +impl TomlPlatform { + pub fn dev_dependencies(&self) -> Option<&BTreeMap> { + self.dev_dependencies + .as_ref() + .or(self.dev_dependencies2.as_ref()) + } + + pub fn build_dependencies(&self) -> Option<&BTreeMap> { + self.build_dependencies + .as_ref() + .or(self.build_dependencies2.as_ref()) + } +} + #[derive(Deserialize, Serialize, Debug, Clone)] #[serde(expecting = "a lints table")] #[serde(rename_all = "kebab-case")] From ccccff112aca0249d2b163d7142a935f8b1cce6c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 21:17:32 -0600 Subject: [PATCH 8/9] refactor(toml): Move target duplicate-field logic next to use --- src/cargo/util/toml/mod.rs | 22 ---------------------- src/cargo/util/toml/targets.rs | 29 ++++++++++++++++++++++++++--- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 22e9926f5d9..017fbfb102a 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2286,28 +2286,6 @@ impl schema::TomlTarget { None => panic!("target name is required"), } } - - fn validate_proc_macro(&self, warnings: &mut Vec) { - if self.proc_macro_raw.is_some() && self.proc_macro_raw2.is_some() { - warn_on_deprecated( - "proc-macro", - self.name().as_str(), - "library target", - warnings, - ); - } - } - - fn validate_crate_types(&self, target_kind_human: &str, warnings: &mut Vec) { - if self.crate_type.is_some() && self.crate_type2.is_some() { - warn_on_deprecated( - "crate-type", - self.name().as_str(), - format!("{target_kind_human} target").as_str(), - warnings, - ); - } - } } impl schema::MaybeWorkspaceLints { diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 9624312f0ac..42ac650d91b 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -23,6 +23,7 @@ use crate::core::compiler::CrateType; use crate::core::{Edition, Feature, Features, Target}; use crate::util::errors::CargoResult; use crate::util::restricted_names; +use crate::util::toml::warn_on_deprecated; use anyhow::Context as _; @@ -172,8 +173,8 @@ fn clean_lib( }; let Some(ref lib) = lib else { return Ok(None) }; - lib.validate_proc_macro(warnings); - lib.validate_crate_types("library", warnings); + validate_proc_macro(lib, "library", warnings); + validate_crate_types(lib, "library", warnings); validate_target_name(lib, "library", "lib", warnings)?; @@ -398,7 +399,7 @@ fn clean_examples( let mut result = Vec::new(); for (path, toml) in targets { - toml.validate_crate_types("example", warnings); + validate_crate_types(&toml, "example", warnings); let crate_types = match toml.crate_types() { Some(kinds) => kinds.iter().map(|s| s.into()).collect(), None => Vec::new(), @@ -984,3 +985,25 @@ fn maybe_custom_build(build: &Option, package_root: &Path) -> Opti } } } + +fn validate_proc_macro(target: &TomlTarget, kind: &str, warnings: &mut Vec) { + if target.proc_macro_raw.is_some() && target.proc_macro_raw2.is_some() { + warn_on_deprecated( + "proc-macro", + target.name().as_str(), + format!("{kind} target").as_str(), + warnings, + ); + } +} + +fn validate_crate_types(target: &TomlTarget, kind: &str, warnings: &mut Vec) { + if target.crate_type.is_some() && target.crate_type2.is_some() { + warn_on_deprecated( + "crate-type", + target.name().as_str(), + format!("{kind} target").as_str(), + warnings, + ); + } +} From eba0091649dd87b0cde861dfb385c08da91439e4 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Nov 2023 21:56:15 -0600 Subject: [PATCH 9/9] refactor(toml): Move target name logic next to use --- src/cargo/util/toml/mod.rs | 9 ----- src/cargo/util/toml/targets.rs | 67 +++++++++++++++++++++------------- 2 files changed, 42 insertions(+), 34 deletions(-) diff --git a/src/cargo/util/toml/mod.rs b/src/cargo/util/toml/mod.rs index 017fbfb102a..cb841476b17 100644 --- a/src/cargo/util/toml/mod.rs +++ b/src/cargo/util/toml/mod.rs @@ -2279,15 +2279,6 @@ impl schema::TomlProfile { } } -impl schema::TomlTarget { - fn name(&self) -> String { - match self.name { - Some(ref name) => name.clone(), - None => panic!("target name is required"), - } - } -} - impl schema::MaybeWorkspaceLints { fn resolve<'a>( self, diff --git a/src/cargo/util/toml/targets.rs b/src/cargo/util/toml/targets.rs index 42ac650d91b..9d456ffd731 100644 --- a/src/cargo/util/toml/targets.rs +++ b/src/cargo/util/toml/targets.rs @@ -182,20 +182,22 @@ fn clean_lib( (Some(path), _) => package_root.join(&path.0), (None, Some(path)) => path, (None, None) => { - let legacy_path = package_root.join("src").join(format!("{}.rs", lib.name())); + let legacy_path = package_root + .join("src") + .join(format!("{}.rs", name_or_panic(lib))); if edition == Edition::Edition2015 && legacy_path.exists() { warnings.push(format!( "path `{}` was erroneously implicitly accepted for library `{}`,\n\ please rename the file to `src/lib.rs` or set lib.path in Cargo.toml", legacy_path.display(), - lib.name() + name_or_panic(lib) )); legacy_path } else { anyhow::bail!( "can't find library `{}`, \ rename file to `src/lib.rs` or specify lib.path", - lib.name() + name_or_panic(lib) ) } } @@ -217,7 +219,7 @@ fn clean_lib( { anyhow::bail!(format!( "library `{}` cannot set the crate type of both `dylib` and `cdylib`", - lib.name() + name_or_panic(lib) )); } (Some(kinds), _, _) if kinds.contains(&"proc-macro".to_string()) => { @@ -225,12 +227,12 @@ fn clean_lib( // This is a warning to retain backwards compatibility. warnings.push(format!( "proc-macro library `{}` should not specify `plugin = true`", - lib.name() + name_or_panic(lib) )); } warnings.push(format!( "library `{}` should only specify `proc-macro = true` instead of setting `crate-type`", - lib.name() + name_or_panic(lib) )); if kinds.len() > 1 { anyhow::bail!("cannot mix `proc-macro` crate type with others"); @@ -246,7 +248,7 @@ fn clean_lib( (None, _, _) => vec![CrateType::Lib], }; - let mut target = Target::lib_target(&lib.name(), crate_types, path, edition); + let mut target = Target::lib_target(name_or_panic(lib), crate_types, path, edition); configure(lib, &mut target)?; Ok(Some(target)) } @@ -286,7 +288,7 @@ fn clean_bins( validate_target_name(bin, "binary", "bin", warnings)?; - let name = bin.name(); + let name = name_or_panic(bin).to_owned(); if let Some(crate_types) = bin.crate_types() { if !crate_types.is_empty() { @@ -321,12 +323,12 @@ fn clean_bins( let mut result = Vec::new(); for bin in &bins { let path = target_path(bin, &inferred, "bin", package_root, edition, &mut |_| { - if let Some(legacy_path) = legacy_bin_path(package_root, &bin.name(), has_lib) { + if let Some(legacy_path) = legacy_bin_path(package_root, name_or_panic(bin), has_lib) { warnings.push(format!( "path `{}` was erroneously implicitly accepted for binary `{}`,\n\ please set bin.path in Cargo.toml", legacy_path.display(), - bin.name() + name_or_panic(bin) )); Some(legacy_path) } else { @@ -339,7 +341,7 @@ fn clean_bins( }; let mut target = Target::bin_target( - &bin.name(), + name_or_panic(bin), bin.filename.clone(), path, bin.required_features.clone(), @@ -406,7 +408,7 @@ fn clean_examples( }; let mut target = Target::example_target( - &toml.name(), + name_or_panic(&toml), crate_types, path, toml.required_features.clone(), @@ -444,8 +446,12 @@ fn clean_tests( let mut result = Vec::new(); for (path, toml) in targets { - let mut target = - Target::test_target(&toml.name(), path, toml.required_features.clone(), edition); + let mut target = Target::test_target( + name_or_panic(&toml), + path, + toml.required_features.clone(), + edition, + ); configure(&toml, &mut target)?; result.push(target); } @@ -465,14 +471,14 @@ fn clean_benches( let targets = { let mut legacy_bench_path = |bench: &TomlTarget| { let legacy_path = package_root.join("src").join("bench.rs"); - if !(bench.name() == "bench" && legacy_path.exists()) { + if !(name_or_panic(bench) == "bench" && legacy_path.exists()) { return None; } legacy_warnings.push(format!( "path `{}` was erroneously implicitly accepted for benchmark `{}`,\n\ please set bench.path in Cargo.toml", legacy_path.display(), - bench.name() + name_or_panic(bench) )); Some(legacy_path) }; @@ -498,8 +504,12 @@ fn clean_benches( let mut result = Vec::new(); for (path, toml) in targets { - let mut target = - Target::bench_target(&toml.name(), path, toml.required_features.clone(), edition); + let mut target = Target::bench_target( + name_or_panic(&toml), + path, + toml.required_features.clone(), + edition, + ); configure(&toml, &mut target)?; result.push(target); } @@ -785,8 +795,8 @@ fn validate_target_name( /// Will check a list of toml targets, and make sure the target names are unique within a vector. fn validate_unique_names(targets: &[TomlTarget], target_kind: &str) -> CargoResult<()> { let mut seen = HashSet::new(); - for name in targets.iter().map(|e| e.name()) { - if !seen.insert(name.clone()) { + for name in targets.iter().map(|e| name_or_panic(e)) { + if !seen.insert(name) { anyhow::bail!( "found duplicate {target_kind} name {name}, \ but all {target_kind} targets must have a unique name", @@ -876,7 +886,7 @@ fn target_path_not_found_error_message( return [target_path_file, target_path_subdir]; } - let target_name = target.name(); + let target_name = name_or_panic(target); let commonly_wrong_paths = possible_target_paths(&target_name, target_kind, true); let possible_paths = possible_target_paths(&target_name, target_kind, false); let existing_wrong_path_index = match ( @@ -923,7 +933,7 @@ fn target_path( // Should we verify that this path exists here? return Ok(package_root.join(&path.0)); } - let name = target.name(); + let name = name_or_panic(target).to_owned(); let mut matching = inferred .iter() @@ -956,7 +966,7 @@ fn target_path( "\ cannot infer path for `{}` {} Cargo doesn't know which to use because multiple target files found at `{}` and `{}`.", - target.name(), + name_or_panic(target), target_kind, p0.strip_prefix(package_root).unwrap_or(&p0).display(), p1.strip_prefix(package_root).unwrap_or(&p1).display(), @@ -986,11 +996,18 @@ fn maybe_custom_build(build: &Option, package_root: &Path) -> Opti } } +fn name_or_panic(target: &TomlTarget) -> &str { + target + .name + .as_deref() + .unwrap_or_else(|| panic!("target name is required")) +} + fn validate_proc_macro(target: &TomlTarget, kind: &str, warnings: &mut Vec) { if target.proc_macro_raw.is_some() && target.proc_macro_raw2.is_some() { warn_on_deprecated( "proc-macro", - target.name().as_str(), + name_or_panic(target), format!("{kind} target").as_str(), warnings, ); @@ -1001,7 +1018,7 @@ fn validate_crate_types(target: &TomlTarget, kind: &str, warnings: &mut Vec