Auto merge of #7959 - ehuss:restricted-crate-names, r=alexcrichton

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).

Author: bors

Cargo.toml

@@ -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"

src/cargo/core/compiler/layout.rs

@@ -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.
     ///

src/cargo/core/compiler/mod.rs

@@ -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};

src/cargo/ops/cargo_new.rs

@@ -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;
@@ -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(())
 }
 
@@ -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,
@@ -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![];
 
@@ -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;
 
@@ -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),

src/cargo/ops/cargo_package.rs

@@ -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,
@@ -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(|| {
@@ -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(()),
@@ -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(())
 }

src/cargo/util/mod.rs

@@ -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;
@@ -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;
@@ -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()

src/cargo/util/restricted_names.rs

@@ -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(())
+}