Apply the reviews suggestions

This commit is contained in:
Kirill Bulatov 2020-02-05 12:47:28 +02:00
parent 2b9952625b
commit 78092c7c66
5 changed files with 64 additions and 32 deletions

View file

@ -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<FilePosit
for (from, to) in crate_deps {
let from_id = crates[&from];
let to_id = crates[&to];
crate_graph.add_dep(from_id, to.into(), to_id).unwrap();
crate_graph.add_dep(from_id, CrateName::new(&to).unwrap(), to_id).unwrap();
}
}

View file

@ -85,12 +85,21 @@ pub struct CrateId(pub u32);
pub struct CrateName(SmolStr);
impl<T: AsRef<str>> From<T> 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<CrateName, &str> {
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<_>>(),
vec![&Dependency { crate_id: crate2, name: "crate_name_with_dashes".into() }]

View file

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

View file

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

View file

@ -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),