Skip to content

Commit

Permalink
Auto merge of #7959 - ehuss:restricted-crate-names, r=alexcrichton
Browse files Browse the repository at this point in the history
Try to better handle restricted crate names.

This attempts to improve handling of restricted crate names, particularly for `cargo new` and `cargo init`. Hopefully the code is straightforward to follow, but in summary the changes are:

**General changes**

* Add more details to the error messages about why a name is not allowed, and what is allowed.
* Change the valid package name check to be restricted to Unicode XID. This brings it in line with non_ascii_idents support in rustc. For the most part, this is pretty much the same as before. Note: this is used for the package name, and registry names. The differences are:
    * Package names cannot start with numbers. Previously this was only rejected in `cargo new`. crates.io also rejects numbers. Numbers are also not valid crate names.
    * Package names cannot start with dash `-`. This is a somewhat arbitrary change, but seems like it would stem problems. crates.io also rejects this.
    * Package names cannot start with these characters that were previously allowed: https://gist.github.com/ehuss/804a797950001b5226e1264b6f65211f#file-not_start_but_alphanumeric-txt
        * Most of these are wacky numbers or other strange things.
    * Package names cannot contain these characters that were previously allowed: https://gist.github.com/ehuss/804a797950001b5226e1264b6f65211f#file-not_continue_but_alphanumeric-txt
        * These are mostly odd things that for whatever reason the Unicode people decided not to include. It seems unlikely to me that someone would want to use one of these.
* Display a warning on Windows if a Cargo target is a special Windows filename. The build error tends to be hard to understand, so the hope is the warning will make it evident.
* `cargo package/publish`: Warn if a special Windows name is in the package.

**cargo new/init specific changes**

* Update keyword list to 2018 edition.
* Add warning if creating a library that has one of the conflicting names (deps/examples/build/incremental).
* Warn about conflicting std names (core/std/alloc/proc-macro).
* Windows reserved names: Rejected on windows, warned on other platforms.
* Warn about non-ASCII package names.
* Only print the `--name` suggestion for `cargo init`. I found the suggestion confusing, and I feel like it doesn't really make sense for `cargo new` (since it would only affect the directory name).
  • Loading branch information
bors committed Mar 4, 2020
2 parents 208e499 + 2a874aa commit 5fe8ab5
Show file tree
Hide file tree
Showing 15 changed files with 363 additions and 84 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ tar = { version = "0.4.26", default-features = false }
tempfile = "3.0"
termcolor = "1.0"
toml = "0.5.3"
unicode-xid = "0.2.0"
url = "2.0"
walkdir = "2.2"
clap = "2.31.2"
Expand Down
6 changes: 0 additions & 6 deletions src/cargo/core/compiler/layout.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,6 @@ pub struct Layout {
_lock: FileLock,
}

pub fn is_bad_artifact_name(name: &str) -> bool {
["deps", "examples", "build", "incremental"]
.iter()
.any(|&reserved| reserved == name)
}

impl Layout {
/// Calculate the paths for build output, lock the build directory, and return as a Layout.
///
Expand Down
1 change: 0 additions & 1 deletion src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ pub use self::custom_build::{BuildOutput, BuildScriptOutputs, BuildScripts};
pub use self::job::Freshness;
use self::job::{Job, Work};
use self::job_queue::{JobQueue, JobState};
pub use self::layout::is_bad_artifact_name;
use self::output_depinfo::output_depinfo;
use self::unit_dependencies::UnitDep;
pub use crate::core::compiler::unit::{Unit, UnitInterner};
Expand Down
97 changes: 67 additions & 30 deletions src/cargo/ops/cargo_new.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::core::{compiler, Workspace};
use crate::core::{Shell, Workspace};
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::{existing_vcs_repo, FossilRepo, GitRepo, HgRepo, PijulRepo};
use crate::util::{paths, validate_package_name, Config};
use crate::util::{paths, restricted_names, Config};
use git2::Config as GitConfig;
use git2::Repository as GitRepository;
use serde::de;
Expand Down Expand Up @@ -155,41 +155,71 @@ fn get_name<'a>(path: &'a Path, opts: &'a NewOptions) -> CargoResult<&'a str> {
})
}

fn check_name(name: &str, opts: &NewOptions) -> CargoResult<()> {
// If --name is already used to override, no point in suggesting it
// again as a fix.
let name_help = match opts.name {
Some(_) => "",
None => "\nuse --name to override crate name",
};
fn check_name(name: &str, name_help: &str, has_bin: bool, shell: &mut Shell) -> CargoResult<()> {
restricted_names::validate_package_name(name, "crate name", name_help)?;

// Ban keywords + test list found at
// https://doc.rust-lang.org/reference/keywords.html
let blacklist = [
"abstract", "alignof", "as", "become", "box", "break", "const", "continue", "crate", "do",
"else", "enum", "extern", "false", "final", "fn", "for", "if", "impl", "in", "let", "loop",
"macro", "match", "mod", "move", "mut", "offsetof", "override", "priv", "proc", "pub",
"pure", "ref", "return", "self", "sizeof", "static", "struct", "super", "test", "trait",
"true", "type", "typeof", "unsafe", "unsized", "use", "virtual", "where", "while", "yield",
];
if blacklist.contains(&name) || (opts.kind.is_bin() && compiler::is_bad_artifact_name(name)) {
if restricted_names::is_keyword(name) {
anyhow::bail!(
"The name `{}` cannot be used as a crate name{}",
"the name `{}` cannot be used as a crate name, it is a Rust keyword{}",
name,
name_help
)
);
}

if let Some(ref c) = name.chars().next() {
if c.is_digit(10) {
if restricted_names::is_conflicting_artifact_name(name) {
if has_bin {
anyhow::bail!(
"Package names starting with a digit cannot be used as a crate name{}",
"the name `{}` cannot be used as a crate name, \
it conflicts with cargo's build directory names{}",
name,
name_help
)
);
} else {
shell.warn(format!(
"the name `{}` will not support binary \
executables with that name, \
it conflicts with cargo's build directory names",
name
))?;
}
}
if name == "test" {
anyhow::bail!(
"the name `test` cannot be used as a crate name, \
it conflicts with Rust's built-in test library{}",
name_help
);
}
if ["core", "std", "alloc", "proc_macro", "proc-macro"].contains(&name) {
shell.warn(format!(
"the name `{}` is part of Rust's standard library\n\
It is recommended to use a different name to avoid problems.",
name
))?;
}
if restricted_names::is_windows_reserved(name) {
if cfg!(windows) {
anyhow::bail!(
"cannot use name `{}`, it is a reserved Windows filename{}",
name,
name_help
);
} else {
shell.warn(format!(
"the name `{}` is a reserved Windows filename\n\
This package will not work on Windows platforms.",
name
))?;
}
}
if restricted_names::is_non_ascii_name(name) {
shell.warn(format!(
"the name `{}` contains non-ASCII characters\n\
Support for non-ASCII crate names is experimental and only valid \
on the nightly toolchain.",
name
))?;
}

validate_package_name(name, "crate name", name_help)?;
Ok(())
}

Expand Down Expand Up @@ -337,7 +367,7 @@ pub fn new(opts: &NewOptions, config: &Config) -> CargoResult<()> {
}

let name = get_name(path, opts)?;
check_name(name, opts)?;
check_name(name, "", opts.kind.is_bin(), &mut config.shell())?;

let mkopts = MkOptions {
version_control: opts.version_control,
Expand Down Expand Up @@ -372,7 +402,6 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
}

let name = get_name(path, opts)?;
check_name(name, opts)?;

let mut src_paths_types = vec![];

Expand All @@ -385,6 +414,14 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
// Maybe when doing `cargo init --bin` inside a library package stub,
// user may mean "initialize for library, but also add binary target"
}
let has_bin = src_paths_types.iter().any(|x| x.bin);
// If --name is already used to override, no point in suggesting it
// again as a fix.
let name_help = match opts.name {
Some(_) => "",
None => "\nuse --name to override crate name",
};
check_name(name, name_help, has_bin, &mut config.shell())?;

let mut version_control = opts.version_control;

Expand Down Expand Up @@ -426,7 +463,7 @@ pub fn init(opts: &NewOptions, config: &Config) -> CargoResult<()> {
version_control,
path,
name,
bin: src_paths_types.iter().any(|x| x.bin),
bin: has_bin,
source_files: src_paths_types,
edition: opts.edition.as_ref().map(|s| &**s),
registry: opts.registry.as_ref().map(|s| &**s),
Expand Down
28 changes: 24 additions & 4 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ use log::debug;
use tar::{Archive, Builder, EntryType, Header};

use crate::core::compiler::{BuildConfig, CompileMode, DefaultExecutor, Executor};
use crate::core::{Feature, Verbosity, Workspace};
use crate::core::{Feature, Shell, Verbosity, Workspace};
use crate::core::{Package, PackageId, PackageSet, Resolve, Source, SourceId};
use crate::ops;
use crate::sources::PathSource;
use crate::util::errors::{CargoResult, CargoResultExt};
use crate::util::paths;
use crate::util::toml::TomlManifest;
use crate::util::{self, Config, FileLock};
use crate::util::{self, restricted_names, Config, FileLock};

pub struct PackageOpts<'cfg> {
pub config: &'cfg Config,
Expand Down Expand Up @@ -142,7 +142,7 @@ fn build_ar_list(
let root = pkg.root();
for src_file in src_files {
let rel_path = src_file.strip_prefix(&root)?.to_path_buf();
check_filename(&rel_path)?;
check_filename(&rel_path, &mut ws.config().shell())?;
let rel_str = rel_path
.to_str()
.ok_or_else(|| {
Expand Down Expand Up @@ -804,7 +804,7 @@ fn report_hash_difference(orig: &HashMap<PathBuf, u64>, after: &HashMap<PathBuf,
//
// To help out in situations like this, issue about weird filenames when
// packaging as a "heads up" that something may not work on other platforms.
fn check_filename(file: &Path) -> CargoResult<()> {
fn check_filename(file: &Path, shell: &mut Shell) -> CargoResult<()> {
let name = match file.file_name() {
Some(name) => name,
None => return Ok(()),
Expand All @@ -825,5 +825,25 @@ fn check_filename(file: &Path) -> CargoResult<()> {
file.display()
)
}
let mut check_windows = |name| -> CargoResult<()> {
if restricted_names::is_windows_reserved(name) {
shell.warn(format!(
"file {} is a reserved Windows filename, \
it will not work on Windows platforms",
file.display()
))?;
}
Ok(())
};
for component in file.iter() {
if let Some(component) = component.to_str() {
check_windows(component)?;
}
}
if file.extension().is_some() {
if let Some(stem) = file.file_stem().and_then(|s| s.to_str()) {
check_windows(stem)?;
}
}
Ok(())
}
18 changes: 2 additions & 16 deletions src/cargo/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ pub use self::paths::{dylib_path_envvar, normalize_path};
pub use self::process_builder::{process, ProcessBuilder};
pub use self::progress::{Progress, ProgressStyle};
pub use self::read2::read2;
pub use self::restricted_names::validate_package_name;
pub use self::rustc::Rustc;
pub use self::sha256::Sha256;
pub use self::to_semver::ToSemver;
Expand Down Expand Up @@ -51,6 +52,7 @@ pub mod process_builder;
pub mod profile;
mod progress;
mod read2;
pub mod restricted_names;
pub mod rustc;
mod sha256;
pub mod to_semver;
Expand All @@ -68,22 +70,6 @@ pub fn elapsed(duration: Duration) -> String {
}
}

/// Check the base requirements for a package name.
///
/// This can be used for other things than package names, to enforce some
/// level of sanity. Note that package names have other restrictions
/// elsewhere. `cargo new` has a few restrictions, such as checking for
/// reserved names. crates.io has even more restrictions.
pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<()> {
if let Some(ch) = name
.chars()
.find(|ch| !ch.is_alphanumeric() && *ch != '_' && *ch != '-')
{
anyhow::bail!("Invalid character `{}` in {}: `{}`{}", ch, what, name, help);
}
Ok(())
}

/// Whether or not this running in a Continuous Integration environment.
pub fn is_ci() -> bool {
std::env::var("CI").is_ok() || std::env::var("TF_BUILD").is_ok()
Expand Down
83 changes: 83 additions & 0 deletions src/cargo/util/restricted_names.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//! Helpers for validating and checking names like package and crate names.
use crate::util::CargoResult;
use anyhow::bail;

/// Returns `true` if the name contains non-ASCII characters.
pub fn is_non_ascii_name(name: &str) -> bool {
name.chars().any(|ch| ch > '\x7f')
}

/// A Rust keyword.
pub fn is_keyword(name: &str) -> bool {
// See https://doc.rust-lang.org/reference/keywords.html
[
"Self", "abstract", "as", "async", "await", "become", "box", "break", "const", "continue",
"crate", "do", "dyn", "else", "enum", "extern", "false", "final", "fn", "for", "if",
"impl", "in", "let", "loop", "macro", "match", "mod", "move", "mut", "override", "priv",
"pub", "ref", "return", "self", "static", "struct", "super", "trait", "true", "try",
"type", "typeof", "unsafe", "unsized", "use", "virtual", "where", "while", "yield",
]
.contains(&name)
}

/// These names cannot be used on Windows, even with an extension.
pub fn is_windows_reserved(name: &str) -> bool {
[
"con", "prn", "aux", "nul", "com1", "com2", "com3", "com4", "com5", "com6", "com7", "com8",
"com9", "lpt1", "lpt2", "lpt3", "lpt4", "lpt5", "lpt6", "lpt7", "lpt8", "lpt9",
]
.contains(&name.to_ascii_lowercase().as_str())
}

/// An artifact with this name will conflict with one of Cargo's build directories.
pub fn is_conflicting_artifact_name(name: &str) -> bool {
["deps", "examples", "build", "incremental"].contains(&name)
}

/// Check the base requirements for a package name.
///
/// This can be used for other things than package names, to enforce some
/// level of sanity. Note that package names have other restrictions
/// elsewhere. `cargo new` has a few restrictions, such as checking for
/// reserved names. crates.io has even more restrictions.
pub fn validate_package_name(name: &str, what: &str, help: &str) -> CargoResult<()> {
let mut chars = name.chars();
if let Some(ch) = chars.next() {
if ch.is_digit(10) {
// A specific error for a potentially common case.
bail!(
"the name `{}` cannot be used as a {}, \
the name cannot start with a digit{}",
name,
what,
help
);
}
if !(unicode_xid::UnicodeXID::is_xid_start(ch) || ch == '_') {
bail!(
"invalid character `{}` in {}: `{}`, \
the first character must be a Unicode XID start character \
(most letters or `_`){}",
ch,
what,
name,
help
);
}
}
for ch in chars {
if !(unicode_xid::UnicodeXID::is_xid_continue(ch) || ch == '-') {
bail!(
"invalid character `{}` in {}: `{}`, \
characters must be Unicode XID characters \
(numbers, `-`, `_`, or most letters){}",
ch,
what,
name,
help
);
}
}
Ok(())
}
Loading

0 comments on commit 5fe8ab5

Please sign in to comment.