Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Implement FromStr and Into<String> for CrateName #2061

Merged
merged 1 commit into from
Jul 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/lsp/src/lib_hacky.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use lsp_types::{
use noirc_driver::{check_crate, create_local_crate, create_non_local_crate, propagate_dep};
use noirc_errors::{DiagnosticKind, FileDiagnostic};
use noirc_frontend::{
graph::{CrateGraph, CrateId, CrateName, CrateType},
graph::{CrateGraph, CrateId, CrateType},
hir::Context,
};

Expand Down Expand Up @@ -299,7 +299,7 @@ fn create_context_at_path(
.join(PathBuf::from(&dependency_path).join("src").join("lib.nr"));
let library_crate =
create_non_local_crate(&mut context, &path_to_lib, CrateType::Library);
propagate_dep(&mut context, library_crate, &CrateName::new(crate_name).unwrap());
propagate_dep(&mut context, library_crate, &crate_name.parse().unwrap());
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions crates/nargo_cli/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ fn resolve_package_manifest(
}

fn crate_name(name: Option<CrateName>) -> String {
name.map(|name| name.as_string()).unwrap_or_else(|| "[unnamed]".to_string())
name.map(|name| name.into()).unwrap_or_else(|| "[unnamed]".to_string())
}

fn resolve_workspace_manifest(
Expand Down Expand Up @@ -191,8 +191,7 @@ fn resolve_workspace_manifest(
.package
.name
.map(|name| {
CrateName::new(&name)
.map_err(|_name| DependencyResolutionError::InvalidPackageName)
name.parse().map_err(|_name| DependencyResolutionError::InvalidPackageName)
})
.transpose()?;

Expand All @@ -201,7 +200,7 @@ fn resolve_workspace_manifest(
}

if local_package.is_none() && workspace.default_member.as_ref() == Some(member) {
local_package = name.as_ref().map(CrateName::as_string);
local_package = name.map(Into::into);
}
}
Manifest::Workspace(_) => {
Expand All @@ -214,7 +213,8 @@ fn resolve_workspace_manifest(
}

let local_package = match local_package {
Some(local_package) => CrateName::new(&local_package)
Some(local_package) => local_package
.parse::<CrateName>()
.map_err(|_| DependencyResolutionError::InvalidPackageName)?
.into(),
None => packages.keys().last().expect("non-empty packages").clone(),
Expand Down
6 changes: 3 additions & 3 deletions crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ pub fn create_non_local_crate(

/// Adds a edge in the crate graph for two crates
pub fn add_dep(context: &mut Context, this_crate: CrateId, depends_on: CrateId, crate_name: &str) {
let crate_name = CrateName::new(crate_name)
.expect("crate name contains blacklisted characters, please remove");
let crate_name =
crate_name.parse().expect("crate name contains blacklisted characters, please remove");

// Cannot depend on a binary
if context.crate_graph.crate_type(depends_on) == CrateType::Binary {
Expand Down Expand Up @@ -139,7 +139,7 @@ pub fn check_crate(
// You can add any crate type to the crate graph
// but you cannot depend on Binaries
let std_crate = context.crate_graph.add_stdlib(CrateType::Library, root_file_id);
propagate_dep(context, std_crate, &CrateName::new(std_crate_name).unwrap());
propagate_dep(context, std_crate, &std_crate_name.parse().unwrap());

let mut errors = vec![];
match context.crate_graph.crate_type(crate_id) {
Expand Down
42 changes: 24 additions & 18 deletions crates/noirc_frontend/src/graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
// This version is also simpler due to not having macro_defs or proc_macros
// XXX: Edition may be reintroduced or some sort of versioning

use std::str::FromStr;

use fm::FileId;
use rustc_hash::{FxHashMap, FxHashSet};
use smol_str::SmolStr;
Expand All @@ -27,23 +29,27 @@ impl CrateId {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct CrateName(SmolStr);

impl CrateName {
/// Creates a new CrateName rejecting any crate name that
/// has a character on the blacklist.
/// The difference between RA and this implementation is that
/// characters on the blacklist are never allowed; there is no normalization.
pub fn new(name: &str) -> Result<CrateName, &str> {
impl From<CrateName> for String {
fn from(crate_name: CrateName) -> Self {
crate_name.0.into()
}
}

/// Creates a new CrateName rejecting any crate name that
/// has a character on the blacklist.
/// The difference between RA and this implementation is that
/// characters on the blacklist are never allowed; there is no normalization.
impl FromStr for CrateName {
type Err = String;

fn from_str(name: &str) -> Result<Self, Self::Err> {
let is_invalid = name.chars().any(|n| CHARACTER_BLACK_LIST.contains(&n));
if is_invalid {
Err(name)
Err(name.into())
} else {
Ok(Self(SmolStr::new(name)))
}
}

pub fn as_string(&self) -> String {
self.0.clone().into()
}
}

#[derive(Debug, Clone, Default, PartialEq, Eq)]
Expand Down Expand Up @@ -80,7 +86,7 @@ pub struct Dependency {

impl Dependency {
pub fn as_name(&self) -> String {
self.name.as_string()
self.name.clone().into()
}
}

Expand Down Expand Up @@ -212,7 +218,7 @@ pub struct CyclicDependenciesError {
mod tests {
use std::path::PathBuf;

use super::{CrateGraph, CrateName, CrateType, FileId};
use super::{CrateGraph, CrateType, FileId};

fn dummy_file_ids(n: usize) -> Vec<FileId> {
use fm::{FileMap, FILE_EXTENSION};
Expand All @@ -238,9 +244,9 @@ mod tests {
let crate2 = graph.add_crate_root(CrateType::Library, file_ids[1]);
let crate3 = graph.add_crate_root(CrateType::Library, file_ids[2]);

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());
assert!(graph.add_dep(crate1, "crate2".parse().unwrap(), crate2).is_ok());
assert!(graph.add_dep(crate2, "crate3".parse().unwrap(), crate3).is_ok());
assert!(graph.add_dep(crate3, "crate1".parse().unwrap(), crate1).is_err());
}

#[test]
Expand All @@ -253,8 +259,8 @@ mod tests {
let crate1 = graph.add_crate_root(CrateType::Library, file_id_0);
let crate2 = graph.add_crate_root(CrateType::Library, file_id_1);
let crate3 = graph.add_crate_root(CrateType::Library, file_id_2);
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(crate1, "crate2".parse().unwrap(), crate2).is_ok());
assert!(graph.add_dep(crate2, "crate3".parse().unwrap(), crate3).is_ok());
}
#[test]
fn it_works2() {
Expand Down
4 changes: 2 additions & 2 deletions crates/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use noirc_driver::{
propagate_dep, CompileOptions, CompiledContract,
};
use noirc_frontend::{
graph::{CrateGraph, CrateName, CrateType},
graph::{CrateGraph, CrateType},
hir::Context,
};
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -65,7 +65,7 @@ fn add_noir_lib(context: &mut Context, crate_name: &str) {
let path_to_lib = Path::new(&crate_name).join("lib.nr");
let library_crate = create_non_local_crate(context, &path_to_lib, CrateType::Library);

propagate_dep(context, library_crate, &CrateName::new(crate_name).unwrap());
propagate_dep(context, library_crate, &crate_name.parse().unwrap());
}

#[wasm_bindgen]
Expand Down