Skip to content

Commit

Permalink
chore: remove most impl AsRef<str,Path> (foundry-rs#157)
Browse files Browse the repository at this point in the history
These mostly only bloat code size and compilation times, which are
already not great when we are already codegenning a billion structs and
ASTs with serde derives

For AsRef<str>, the only change is having to specify an extra 1 (one)
character (&) when using `String` so it derefs to `str`, otherwise it's
the same

For AsRef<Path> it's the same as AsRef<str> when using Path/PathBuf, but
it's a bit worse for string literals; but most of these functions are
called with variables in the first place. except in tests which will
have to use .as_ref() or Path::new

I've not removed these when using with &[] or impl IntoIterator since
that does help quite a bit, e.g if you have an owning iterator of
Strings, you cannot borrow inside of a map closure

I've also kept Into<PathBuf> since that makes more sense, you want to
avoid cloning if you can pass in an owned thing already, otherwise
you'll clone it yourself

Also fixes some inconsistencies in function signatures for str vs path
  • Loading branch information
DaniPopes authored Jun 28, 2024
1 parent 121f284 commit bc96471
Show file tree
Hide file tree
Showing 26 changed files with 457 additions and 637 deletions.
4 changes: 2 additions & 2 deletions crates/artifacts/solc/src/ast/lowfidelity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ pub struct Node {

impl Node {
/// Deserialize a serialized node attribute.
pub fn attribute<D: DeserializeOwned>(&self, key: impl AsRef<str>) -> Option<D> {
pub fn attribute<D: DeserializeOwned>(&self, key: &str) -> Option<D> {
// TODO: Can we avoid this clone?
self.other.get(key.as_ref()).and_then(|v| serde_json::from_value(v.clone()).ok())
self.other.get(key).and_then(|v| serde_json::from_value(v.clone()).ok())
}
}

Expand Down
40 changes: 19 additions & 21 deletions crates/artifacts/solc/src/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1100,30 +1100,28 @@ ast_node!(
#[cfg(test)]
mod tests {
use super::*;
use std::{fs, path::PathBuf};
use std::{fs, path::Path};

#[test]
fn can_parse_ast() {
fs::read_dir(
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../../test-data").join("ast"),
)
.unwrap()
.for_each(|path| {
let path = path.unwrap().path();
let path_str = path.to_string_lossy();

let input = fs::read_to_string(&path).unwrap();
let deserializer = &mut serde_json::Deserializer::from_str(&input);
let result: Result<SourceUnit, _> = serde_path_to_error::deserialize(deserializer);
match result {
Err(e) => {
println!("... {path_str} fail: {e}");
panic!();
fs::read_dir(Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../test-data").join("ast"))
.unwrap()
.for_each(|path| {
let path = path.unwrap().path();
let path_str = path.to_string_lossy();

let input = fs::read_to_string(&path).unwrap();
let deserializer = &mut serde_json::Deserializer::from_str(&input);
let result: Result<SourceUnit, _> = serde_path_to_error::deserialize(deserializer);
match result {
Err(e) => {
println!("... {path_str} fail: {e}");
panic!();
}
Ok(_) => {
println!("... {path_str} ok");
}
}
Ok(_) => {
println!("... {path_str} ok");
}
}
})
})
}
}
47 changes: 13 additions & 34 deletions crates/artifacts/solc/src/bytecode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,18 +66,11 @@ impl CompactBytecode {
///
/// Returns true if the bytecode object is fully linked, false otherwise
/// This is a noop if the bytecode object is already fully linked.
pub fn link(
&mut self,
file: impl AsRef<str>,
library: impl AsRef<str>,
address: Address,
) -> bool {
pub fn link(&mut self, file: &str, library: &str, address: Address) -> bool {
if !self.object.is_unlinked() {
return true;
}

let file = file.as_ref();
let library = library.as_ref();
if let Some((key, mut contracts)) = self.link_references.remove_entry(file) {
if contracts.remove(library).is_some() {
self.object.link(file, library, address);
Expand Down Expand Up @@ -148,8 +141,8 @@ impl Bytecode {
}

/// Same as `Bytecode::link` but with fully qualified name (`file.sol:Math`)
pub fn link_fully_qualified(&mut self, name: impl AsRef<str>, addr: Address) -> bool {
if let Some((file, lib)) = name.as_ref().split_once(':') {
pub fn link_fully_qualified(&mut self, name: &str, addr: Address) -> bool {
if let Some((file, lib)) = name.split_once(':') {
self.link(file, lib, addr)
} else {
false
Expand All @@ -161,18 +154,11 @@ impl Bytecode {
///
/// Returns true if the bytecode object is fully linked, false otherwise
/// This is a noop if the bytecode object is already fully linked.
pub fn link(
&mut self,
file: impl AsRef<str>,
library: impl AsRef<str>,
address: Address,
) -> bool {
pub fn link(&mut self, file: &str, library: &str, address: Address) -> bool {
if !self.object.is_unlinked() {
return true;
}

let file = file.as_ref();
let library = library.as_ref();
if let Some((key, mut contracts)) = self.link_references.remove_entry(file) {
if contracts.remove(library).is_some() {
self.object.link(file, library, address);
Expand All @@ -195,7 +181,7 @@ impl Bytecode {
T: AsRef<str>,
{
for (file, lib, addr) in libs.into_iter() {
if self.link(file, lib, addr) {
if self.link(file.as_ref(), lib.as_ref(), addr) {
return true;
}
}
Expand All @@ -209,7 +195,7 @@ impl Bytecode {
S: AsRef<str>,
{
for (name, addr) in libs.into_iter() {
if self.link_fully_qualified(name, addr) {
if self.link_fully_qualified(name.as_ref(), addr) {
return true;
}
}
Expand Down Expand Up @@ -316,9 +302,8 @@ impl BytecodeObject {
/// This will replace all occurrences of the library placeholder with the given address.
///
/// See also: <https://docs.soliditylang.org/en/develop/using-the-compiler.html#library-linking>
pub fn link_fully_qualified(&mut self, name: impl AsRef<str>, addr: Address) -> &mut Self {
pub fn link_fully_qualified(&mut self, name: &str, addr: Address) -> &mut Self {
if let Self::Unlinked(ref mut unlinked) = self {
let name = name.as_ref();
let place_holder = utils::library_hash_placeholder(name);
// the address as hex without prefix
let hex_addr = hex::encode(addr);
Expand All @@ -337,13 +322,8 @@ impl BytecodeObject {
/// Links using the `file` and `library` names as fully qualified name `<file>:<library>`.
///
/// See [`link_fully_qualified`](Self::link_fully_qualified).
pub fn link(
&mut self,
file: impl AsRef<str>,
library: impl AsRef<str>,
addr: Address,
) -> &mut Self {
self.link_fully_qualified(format!("{}:{}", file.as_ref(), library.as_ref()), addr)
pub fn link(&mut self, file: &str, library: &str, addr: Address) -> &mut Self {
self.link_fully_qualified(&format!("{file}:{library}"), addr)
}

/// Links the bytecode object with all provided `(file, lib, addr)`.
Expand All @@ -354,15 +334,14 @@ impl BytecodeObject {
T: AsRef<str>,
{
for (file, lib, addr) in libs.into_iter() {
self.link(file, lib, addr);
self.link(file.as_ref(), lib.as_ref(), addr);
}
self
}

/// Returns whether the bytecode contains a matching placeholder using the qualified name.
pub fn contains_fully_qualified_placeholder(&self, name: impl AsRef<str>) -> bool {
pub fn contains_fully_qualified_placeholder(&self, name: &str) -> bool {
if let Self::Unlinked(unlinked) = self {
let name = name.as_ref();
unlinked.contains(&utils::library_hash_placeholder(name))
|| unlinked.contains(&utils::library_fully_qualified_placeholder(name))
} else {
Expand All @@ -371,8 +350,8 @@ impl BytecodeObject {
}

/// Returns whether the bytecode contains a matching placeholder.
pub fn contains_placeholder(&self, file: impl AsRef<str>, library: impl AsRef<str>) -> bool {
self.contains_fully_qualified_placeholder(format!("{}:{}", file.as_ref(), library.as_ref()))
pub fn contains_placeholder(&self, file: &str, library: &str) -> bool {
self.contains_fully_qualified_placeholder(&format!("{file}:{library}"))
}
}

Expand Down
38 changes: 15 additions & 23 deletions crates/artifacts/solc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,15 +158,13 @@ impl SolcInput {

/// Sets the path of the source files to `root` adjoined to the existing path
#[must_use]
pub fn join_path(mut self, root: impl AsRef<Path>) -> Self {
let root = root.as_ref();
pub fn join_path(mut self, root: &Path) -> Self {
self.sources = self.sources.into_iter().map(|(path, s)| (root.join(path), s)).collect();
self
}

/// Removes the `base` path from all source files
pub fn strip_prefix(&mut self, base: impl AsRef<Path>) {
let base = base.as_ref();
pub fn strip_prefix(&mut self, base: &Path) {
self.sources = std::mem::take(&mut self.sources)
.into_iter()
.map(|(path, s)| (path.strip_prefix(base).map(Into::into).unwrap_or(path), s))
Expand Down Expand Up @@ -490,8 +488,7 @@ impl Settings {
self
}

pub fn strip_prefix(&mut self, base: impl AsRef<Path>) {
let base = base.as_ref();
pub fn strip_prefix(&mut self, base: &Path) {
self.remappings.iter_mut().for_each(|r| {
r.strip_prefix(base);
});
Expand Down Expand Up @@ -535,7 +532,7 @@ impl Settings {
}

/// Strips `base` from all paths
pub fn with_base_path(mut self, base: impl AsRef<Path>) -> Self {
pub fn with_base_path(mut self, base: &Path) -> Self {
self.strip_prefix(base);
self
}
Expand Down Expand Up @@ -624,8 +621,7 @@ impl Libraries {

/// Strips the given prefix from all library file paths to make them relative to the given
/// `base` argument
pub fn with_stripped_file_prefixes(mut self, base: impl AsRef<Path>) -> Self {
let base = base.as_ref();
pub fn with_stripped_file_prefixes(mut self, base: &Path) -> Self {
self.libs = self
.libs
.into_iter()
Expand Down Expand Up @@ -1510,16 +1506,14 @@ impl CompilerOutput {
}

/// Finds the _first_ contract with the given name
pub fn find(&self, contract: impl AsRef<str>) -> Option<CompactContractRef<'_>> {
let contract_name = contract.as_ref();
pub fn find(&self, contract_name: &str) -> Option<CompactContractRef<'_>> {
self.contracts_iter().find_map(|(name, contract)| {
(name == contract_name).then(|| CompactContractRef::from(contract))
})
}

/// Finds the first contract with the given name and removes it from the set
pub fn remove(&mut self, contract: impl AsRef<str>) -> Option<Contract> {
let contract_name = contract.as_ref();
pub fn remove(&mut self, contract_name: &str) -> Option<Contract> {
self.contracts.values_mut().find_map(|c| c.remove(contract_name))
}

Expand Down Expand Up @@ -1586,16 +1580,14 @@ impl OutputContracts {
}

/// Finds the _first_ contract with the given name
pub fn find(&self, contract: impl AsRef<str>) -> Option<CompactContractRef<'_>> {
let contract_name = contract.as_ref();
pub fn find(&self, contract_name: &str) -> Option<CompactContractRef<'_>> {
self.contracts_iter().find_map(|(name, contract)| {
(name == contract_name).then(|| CompactContractRef::from(contract))
})
}

/// Finds the first contract with the given name and removes it from the set
pub fn remove(&mut self, contract: impl AsRef<str>) -> Option<Contract> {
let contract_name = contract.as_ref();
pub fn remove(&mut self, contract_name: &str) -> Option<Contract> {
self.0.values_mut().find_map(|c| c.remove(contract_name))
}
}
Expand Down Expand Up @@ -1920,7 +1912,7 @@ mod tests {

#[test]
fn can_parse_compiler_output() {
let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/out");
let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/out");

for path in fs::read_dir(dir).unwrap() {
let path = path.unwrap().path();
Expand All @@ -1933,7 +1925,7 @@ mod tests {

#[test]
fn can_parse_compiler_input() {
let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/in");
let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/in");

for path in fs::read_dir(dir).unwrap() {
let path = path.unwrap().path();
Expand All @@ -1946,7 +1938,7 @@ mod tests {

#[test]
fn can_parse_standard_json_compiler_input() {
let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/in");
let dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/in");

for path in fs::read_dir(dir).unwrap() {
let path = path.unwrap().path();
Expand Down Expand Up @@ -2078,7 +2070,7 @@ mod tests {

let libs = Libraries::parse(&libraries[..])
.unwrap()
.with_stripped_file_prefixes("/global/root")
.with_stripped_file_prefixes("/global/root".as_ref())
.libs;

assert_eq!(
Expand Down Expand Up @@ -2208,8 +2200,8 @@ mod tests {
// <https://github.com/foundry-rs/foundry/issues/3012>
#[test]
fn can_parse_compiler_output_spells_0_6_12() {
let path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("../../../test-data/0.6.12-with-libs.json");
let path =
Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../test-data/0.6.12-with-libs.json");
let content = fs::read_to_string(path).unwrap();
let _output: CompilerOutput = serde_json::from_str(&content).unwrap();
}
Expand Down
Loading

0 comments on commit bc96471

Please sign in to comment.