From 78092c7c66a665ada55ea09476a387014f6665b0 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Wed, 5 Feb 2020 12:47:28 +0200 Subject: Apply the reviews suggestions --- crates/ra_db/src/fixture.rs | 6 +++--- crates/ra_db/src/input.rs | 37 ++++++++++++++++++++------------ crates/ra_db/src/lib.rs | 4 +++- crates/ra_ide/src/mock_analysis.rs | 6 ++++-- crates/ra_project_model/src/lib.rs | 43 ++++++++++++++++++++++++++------------ 5 files changed, 64 insertions(+), 32 deletions(-) diff --git a/crates/ra_db/src/fixture.rs b/crates/ra_db/src/fixture.rs index 30b598e9a..17cd138c2 100644 --- a/crates/ra_db/src/fixture.rs +++ b/crates/ra_db/src/fixture.rs @@ -8,8 +8,8 @@ use rustc_hash::FxHashMap; use test_utils::{extract_offset, parse_fixture, CURSOR_MARKER}; use crate::{ - CrateGraph, CrateId, Edition, Env, FileId, FilePosition, RelativePathBuf, SourceDatabaseExt, - SourceRoot, SourceRootId, + input::CrateName, CrateGraph, CrateId, Edition, Env, FileId, FilePosition, RelativePathBuf, + SourceDatabaseExt, SourceRoot, SourceRootId, }; pub const WORKSPACE: SourceRootId = SourceRootId(0); @@ -139,7 +139,7 @@ fn with_files(db: &mut dyn SourceDatabaseExt, fixture: &str) -> Option> From for CrateName { - fn from(name: T) -> Self { - // For root projects with dashes in their name, - // cargo metadata does not do any normalization - // so we do it ourselves - Self(SmolStr::new(name.as_ref().replace('-', "_"))) +impl CrateName { + /// Crates a crate name, checking for dashes in the string provided. + /// Dashes are not allowed in the crate names, + /// hence the input string is returned as `Err` for those cases. + pub fn new(name: &str) -> Result { + if name.contains('-') { + Err(name) + } else { + Ok(Self(SmolStr::new(name))) + } + } + + /// Crates a crate name, unconditionally replacing the dashes with underscores. + pub fn normalize_dashes(name: &str) -> CrateName { + Self(SmolStr::new(name.replace('-', "_"))) } } @@ -279,7 +288,7 @@ pub struct CyclicDependenciesError; #[cfg(test)] mod tests { - use super::{CfgOptions, CrateGraph, Dependency, Edition::Edition2018, Env, FileId}; + use super::{CfgOptions, CrateGraph, CrateName, Dependency, Edition::Edition2018, Env, FileId}; #[test] fn it_should_panic_because_of_cycle_dependencies() { @@ -290,9 +299,9 @@ mod tests { graph.add_crate_root(FileId(2u32), Edition2018, CfgOptions::default(), Env::default()); let crate3 = graph.add_crate_root(FileId(3u32), Edition2018, CfgOptions::default(), Env::default()); - assert!(graph.add_dep(crate1, "crate2".into(), crate2).is_ok()); - assert!(graph.add_dep(crate2, "crate3".into(), crate3).is_ok()); - assert!(graph.add_dep(crate3, "crate1".into(), crate1).is_err()); + assert!(graph.add_dep(crate1, CrateName::new("crate2").unwrap(), crate2).is_ok()); + assert!(graph.add_dep(crate2, CrateName::new("crate3").unwrap(), crate3).is_ok()); + assert!(graph.add_dep(crate3, CrateName::new("crate1").unwrap(), crate1).is_err()); } #[test] @@ -304,8 +313,8 @@ mod tests { graph.add_crate_root(FileId(2u32), Edition2018, CfgOptions::default(), Env::default()); let crate3 = graph.add_crate_root(FileId(3u32), Edition2018, CfgOptions::default(), Env::default()); - assert!(graph.add_dep(crate1, "crate2".into(), crate2).is_ok()); - assert!(graph.add_dep(crate2, "crate3".into(), crate3).is_ok()); + assert!(graph.add_dep(crate1, CrateName::new("crate2").unwrap(), crate2).is_ok()); + assert!(graph.add_dep(crate2, CrateName::new("crate3").unwrap(), crate3).is_ok()); } #[test] @@ -315,7 +324,9 @@ mod tests { graph.add_crate_root(FileId(1u32), Edition2018, CfgOptions::default(), Env::default()); let crate2 = graph.add_crate_root(FileId(2u32), Edition2018, CfgOptions::default(), Env::default()); - assert!(graph.add_dep(crate1, "crate-name-with-dashes".into(), crate2).is_ok()); + assert!(graph + .add_dep(crate1, CrateName::normalize_dashes("crate-name-with-dashes"), crate2) + .is_ok()); assert_eq!( graph.dependencies(crate1).collect::>(), vec![&Dependency { crate_id: crate2, name: "crate_name_with_dashes".into() }] diff --git a/crates/ra_db/src/lib.rs b/crates/ra_db/src/lib.rs index 21341b769..fb002d717 100644 --- a/crates/ra_db/src/lib.rs +++ b/crates/ra_db/src/lib.rs @@ -10,7 +10,9 @@ use ra_syntax::{ast, Parse, SourceFile, TextRange, TextUnit}; pub use crate::{ cancellation::Canceled, - input::{CrateGraph, CrateId, Dependency, Edition, Env, FileId, SourceRoot, SourceRootId}, + input::{ + CrateGraph, CrateId, CrateName, Dependency, Edition, Env, FileId, SourceRoot, SourceRootId, + }, }; pub use relative_path::{RelativePath, RelativePathBuf}; pub use salsa; diff --git a/crates/ra_ide/src/mock_analysis.rs b/crates/ra_ide/src/mock_analysis.rs index bf8a54932..081aaee8c 100644 --- a/crates/ra_ide/src/mock_analysis.rs +++ b/crates/ra_ide/src/mock_analysis.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use ra_cfg::CfgOptions; -use ra_db::{Env, RelativePathBuf}; +use ra_db::{CrateName, Env, RelativePathBuf}; use test_utils::{extract_offset, extract_range, parse_fixture, CURSOR_MARKER}; use crate::{ @@ -107,7 +107,9 @@ impl MockAnalysis { crate_graph.add_crate_root(file_id, Edition2018, cfg_options, Env::default()); let crate_name = path.parent().unwrap().file_name().unwrap(); if let Some(root_crate) = root_crate { - crate_graph.add_dep(root_crate, crate_name.into(), other_crate).unwrap(); + crate_graph + .add_dep(root_crate, CrateName::new(crate_name).unwrap(), other_crate) + .unwrap(); } } change.add_file(source_root, file_id, path, Arc::new(contents)); diff --git a/crates/ra_project_model/src/lib.rs b/crates/ra_project_model/src/lib.rs index 6a104e6f2..bc1d15406 100644 --- a/crates/ra_project_model/src/lib.rs +++ b/crates/ra_project_model/src/lib.rs @@ -13,7 +13,7 @@ use std::{ }; use ra_cfg::CfgOptions; -use ra_db::{CrateGraph, CrateId, Edition, Env, FileId}; +use ra_db::{CrateGraph, CrateId, CrateName, Edition, Env, FileId}; use rustc_hash::FxHashMap; use serde_json::from_reader; @@ -177,7 +177,9 @@ impl ProjectWorkspace { if let (Some(&from), Some(&to)) = (crates.get(&from_crate_id), crates.get(&to_crate_id)) { - if let Err(_) = crate_graph.add_dep(from, dep.name.clone().into(), to) { + if let Err(_) = + crate_graph.add_dep(from, CrateName::new(&dep.name).unwrap(), to) + { log::error!( "cyclic dependency {:?} -> {:?}", from_crate_id, @@ -215,7 +217,9 @@ impl ProjectWorkspace { if let (Some(&from), Some(&to)) = (sysroot_crates.get(&from), sysroot_crates.get(&to)) { - if let Err(_) = crate_graph.add_dep(from, name.into(), to) { + if let Err(_) = + crate_graph.add_dep(from, CrateName::new(name).unwrap(), to) + { log::error!("cyclic dependency between sysroot crates") } } @@ -257,7 +261,7 @@ impl ProjectWorkspace { if let Some(proc_macro) = libproc_macro { if let Err(_) = crate_graph.add_dep( crate_id, - "proc_macro".into(), + CrateName::new("proc_macro").unwrap(), proc_macro, ) { log::error!( @@ -276,9 +280,14 @@ impl ProjectWorkspace { for &from in pkg_crates.get(&pkg).into_iter().flatten() { if let Some(to) = lib_tgt { if to != from { - if let Err(_) = - crate_graph.add_dep(from, pkg.name(&cargo).into(), to) - { + if let Err(_) = crate_graph.add_dep( + from, + // For root projects with dashes in their name, + // cargo metadata does not do any normalization, + // so we do it ourselves currently + CrateName::normalize_dashes(pkg.name(&cargo)), + to, + ) { log::error!( "cyclic dependency between targets of {}", pkg.name(&cargo) @@ -289,17 +298,23 @@ impl ProjectWorkspace { // core is added as a dependency before std in order to // mimic rustcs dependency order if let Some(core) = libcore { - if let Err(_) = crate_graph.add_dep(from, "core".into(), core) { + if let Err(_) = + crate_graph.add_dep(from, CrateName::new("core").unwrap(), core) + { log::error!("cyclic dependency on core for {}", pkg.name(&cargo)) } } if let Some(alloc) = liballoc { - if let Err(_) = crate_graph.add_dep(from, "alloc".into(), alloc) { + if let Err(_) = + crate_graph.add_dep(from, CrateName::new("alloc").unwrap(), alloc) + { log::error!("cyclic dependency on alloc for {}", pkg.name(&cargo)) } } if let Some(std) = libstd { - if let Err(_) = crate_graph.add_dep(from, "std".into(), std) { + if let Err(_) = + crate_graph.add_dep(from, CrateName::new("std").unwrap(), std) + { log::error!("cyclic dependency on std for {}", pkg.name(&cargo)) } } @@ -312,9 +327,11 @@ impl ProjectWorkspace { for dep in pkg.dependencies(&cargo) { if let Some(&to) = pkg_to_lib_crate.get(&dep.pkg) { for &from in pkg_crates.get(&pkg).into_iter().flatten() { - if let Err(_) = - crate_graph.add_dep(from, dep.name.clone().into(), to) - { + if let Err(_) = crate_graph.add_dep( + from, + CrateName::new(&dep.name).unwrap(), + to, + ) { log::error!( "cyclic dependency {} -> {}", pkg.name(&cargo), -- cgit v1.2.3