From d78c7f191797d62e24a6f88035f386b83486ff09 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 17 Jun 2025 12:23:47 +0200 Subject: [PATCH 1/2] refactor: flatten bind_json.rs to one pass without state structs Refactored the bind_json implementation to eliminate the intermediate state structs (PreprocessedState, StructsState, ResolvedState) and flatten the logic into a single pass through the workflow. All logic now resides within methods on BindJsonArgs, making the code more straightforward while preserving the same functionality. --- crates/forge/src/cmd/bind_json.rs | 418 ++++++++++++++---------------- 1 file changed, 200 insertions(+), 218 deletions(-) diff --git a/crates/forge/src/cmd/bind_json.rs b/crates/forge/src/cmd/bind_json.rs index fc5ca28bccdf7..32214c74740b9 100644 --- a/crates/forge/src/cmd/bind_json.rs +++ b/crates/forge/src/cmd/bind_json.rs @@ -25,7 +25,7 @@ use solar_parse::{ use solar_sema::thread_local::ThreadLocal; use std::{ collections::{BTreeMap, BTreeSet, HashSet}, - fmt::{self, Write}, + fmt::Write, ops::ControlFlow, path::PathBuf, sync::Arc, @@ -48,32 +48,11 @@ pub struct BindJsonArgs { impl BindJsonArgs { pub fn run(self) -> Result<()> { - self.preprocess()?.find_structs()?.resolve_imports_and_aliases().write()?; - - Ok(()) - } - - /// In cases when user moves/renames/deletes structs, compiler will start failing because - /// generated bindings will be referencing non-existing structs or importing non-existing - /// files. - /// - /// Because of that, we need a little bit of preprocessing to make sure that bindings will still - /// be valid. - /// - /// The strategy is: - /// 1. Replace bindings file with an empty one to get rid of potentially invalid imports. - /// 2. Remove all function bodies to get rid of `serialize`/`deserialize` invocations. - /// 3. Remove all `immutable` attributes to avoid errors because of erased constructors - /// initializing them. - /// - /// After that we'll still have enough information for bindings but compilation should succeed - /// in most of the cases. - fn preprocess(self) -> Result { let config = self.load_config()?; let project = config.ephemeral_project()?; - let target_path = config.root.join(self.out.as_ref().unwrap_or(&config.bind_json.out)); + // Step 1: Read and preprocess sources let sources = project.paths.read_input_files()?; let graph = Graph::::resolve_sources(&project.paths, sources)?; @@ -92,6 +71,38 @@ impl BindJsonArgs { .max_by(|(v1, _, _), (v2, _, _)| v1.cmp(v2)) .unwrap(); + // Step 2: Preprocess sources to handle potentially invalid bindings + self.preprocess_sources(&mut sources)?; + + // Insert empty bindings file. + sources.insert(target_path.clone(), Source::new(JSON_BINDINGS_PLACEHOLDER)); + + // Step 3: Find structs and generate bindings + let structs_to_write = + self.find_and_resolve_structs(&config, &project, version, sources, &target_path)?; + + // Step 4: Write bindings + self.write_bindings(&structs_to_write, &target_path)?; + + Ok(()) + } + + /// In cases when user moves/renames/deletes structs, compiler will start failing because + /// generated bindings will be referencing non-existing structs or importing non-existing + /// files. + /// + /// Because of that, we need a little bit of preprocessing to make sure that bindings will still + /// be valid. + /// + /// The strategy is: + /// 1. Replace bindings file with an empty one to get rid of potentially invalid imports. + /// 2. Remove all function bodies to get rid of `serialize`/`deserialize` invocations. + /// 3. Remove all `immutable` attributes to avoid errors because of erased constructors + /// initializing them. + /// + /// After that we'll still have enough information for bindings but compilation should succeed + /// in most of the cases. + fn preprocess_sources(&self, sources: &mut Sources) -> Result<()> { let sess = Session::builder().with_stderr_emitter().build(); let result = sess.enter_parallel(|| -> solar_parse::interface::Result<()> { sources.0.par_iter_mut().try_for_each(|(path, source)| { @@ -115,154 +126,30 @@ impl BindJsonArgs { }) }); eyre::ensure!(result.is_ok(), "failed parsing"); - - // Insert empty bindings file. - sources.insert(target_path.clone(), Source::new(JSON_BINDINGS_PLACEHOLDER)); - - Ok(PreprocessedState { version, sources, target_path, project, config }) - } -} - -struct PreprocessorVisitor { - updates: Vec<(Span, &'static str)>, -} - -impl PreprocessorVisitor { - fn new() -> Self { - Self { updates: Vec::new() } - } - - fn update(mut self, sess: &Session, content: &mut String) { - if self.updates.is_empty() { - return; - } - - let sf = sess.source_map().lookup_source_file(self.updates[0].0.lo()); - let base = sf.start_pos.0; - - self.updates.sort_by_key(|(span, _)| span.lo()); - let mut shift = 0_i64; - for (span, new) in self.updates { - let lo = span.lo() - base; - let hi = span.hi() - base; - let start = ((lo.0 as i64) - shift) as usize; - let end = ((hi.0 as i64) - shift) as usize; - - content.replace_range(start..end, new); - shift += (end - start) as i64; - shift -= new.len() as i64; - } - } -} - -impl<'ast> Visit<'ast> for PreprocessorVisitor { - type BreakValue = solar_parse::interface::data_structures::Never; - - fn visit_item_function( - &mut self, - func: &'ast ast::ItemFunction<'ast>, - ) -> ControlFlow { - // Replace function bodies with a noop statement. - if let Some(block) = &func.body { - if !block.is_empty() { - let span = block.first().unwrap().span.to(block.last().unwrap().span); - let new_body = match func.kind { - FunctionKind::Modifier => "_;", - _ => "revert();", - }; - self.updates.push((span, new_body)); - } - } - - self.walk_item_function(func) - } - - fn visit_variable_definition( - &mut self, - var: &'ast ast::VariableDefinition<'ast>, - ) -> ControlFlow { - // Remove `immutable` attributes. - if let Some(VarMut::Immutable) = var.mutability { - self.updates.push((var.span, "")); - } - - self.walk_variable_definition(var) - } -} - -/// A single struct definition for which we need to generate bindings. -#[derive(Debug, Clone)] -struct StructToWrite { - /// Name of the struct definition. - name: String, - /// Name of the contract containing the struct definition. None if the struct is defined at the - /// file level. - contract_name: Option, - /// Import alias for the contract or struct, depending on whether the struct is imported - /// directly, or via a contract. - import_alias: Option, - /// Path to the file containing the struct definition. - path: PathBuf, - /// EIP712 schema for the struct. - schema: String, - /// Name of the struct definition used in function names and schema_* variables. - name_in_fns: String, -} - -impl StructToWrite { - /// Returns the name of the imported item. If struct is defined at the file level, returns the - /// struct name, otherwise returns the parent contract name. - fn struct_or_contract_name(&self) -> &str { - self.contract_name.as_deref().unwrap_or(&self.name) - } - - /// Same as [StructToWrite::struct_or_contract_name] but with alias applied. - fn struct_or_contract_name_with_alias(&self) -> &str { - self.import_alias.as_deref().unwrap_or(self.struct_or_contract_name()) - } - - /// Path which can be used to reference this struct in input/output parameters. Either - /// StructName or ParantName.StructName - fn full_path(&self) -> String { - if self.contract_name.is_some() { - format!("{}.{}", self.struct_or_contract_name_with_alias(), self.name) - } else { - self.struct_or_contract_name_with_alias().to_string() - } - } - - fn import_item(&self) -> String { - if let Some(alias) = &self.import_alias { - format!("{} as {}", self.struct_or_contract_name(), alias) - } else { - self.struct_or_contract_name().to_string() - } + Ok(()) } -} - -struct PreprocessedState { - version: Version, - sources: Sources, - target_path: PathBuf, - project: Project, - config: Config, -} - -impl PreprocessedState { - fn find_structs(self) -> Result { - let mut structs_to_write = Vec::new(); - let Self { version, sources, target_path, config, project } = self; + /// Find structs, resolve conflicts, and prepare them for writing + fn find_and_resolve_structs( + &self, + config: &Config, + project: &Project, + version: Version, + sources: Sources, + _target_path: &PathBuf, + ) -> Result> { let settings = config.solc_settings()?; - let include = config.bind_json.include; - let exclude = config.bind_json.exclude; - let root = config.root; + let include = &config.bind_json.include; + let exclude = &config.bind_json.exclude; + let root = &config.root; let input = SolcVersionedInput::build(sources, settings, SolcLanguage::Solidity, version); let mut sess = Session::builder().with_stderr_emitter().build(); sess.dcx = sess.dcx.set_flags(|flags| flags.track_diagnostics = false); + let mut structs_to_write = Vec::new(); + sess.enter_parallel(|| -> Result<()> { // Set up the parsing context with the project paths, without adding the source files let mut parsing_context = solar_pcx_from_solc_project(&sess, &project, &input, false); @@ -317,7 +204,6 @@ impl PreprocessedState { .unwrap_or_else(|_| path) .to_path_buf(), schema, - // will be filled later import_alias: None, name_in_fns: String::new(), @@ -331,31 +217,24 @@ impl PreprocessedState { eyre::ensure!(sess.dcx.has_errors().is_ok(), "errors occurred"); - Ok(StructsState { structs_to_write, target_path }) - } -} + // Resolve import aliases and function names + self.resolve_conflicts(&mut structs_to_write); -#[derive(Debug)] -struct StructsState { - structs_to_write: Vec, - target_path: PathBuf, -} + Ok(structs_to_write) + } -impl StructsState { - /// We manage 2 namespsaces for JSON bindings: + /// We manage 2 namespaces for JSON bindings: /// - Namespace of imported items. This includes imports of contracts containing structs and /// structs defined at the file level. /// - Namespace of struct names used in function names and schema_* variables. /// /// Both of those might contain conflicts, so we need to resolve them. - fn resolve_imports_and_aliases(self) -> ResolvedState { - let Self { mut structs_to_write, target_path } = self; - + fn resolve_conflicts(&self, structs_to_write: &mut Vec) { // firstly, we resolve imported names conflicts // construct mapping name -> paths from which items with such name are imported let mut names_to_paths = BTreeMap::new(); - for s in &structs_to_write { + for s in structs_to_write.iter() { names_to_paths .entry(s.struct_or_contract_name()) .or_insert_with(BTreeSet::new) @@ -367,8 +246,7 @@ impl StructsState { for (name, paths) in names_to_paths { if paths.len() <= 1 { - // no alias needed - continue + continue; // no alias needed } for (i, path) in paths.into_iter().enumerate() { @@ -379,7 +257,7 @@ impl StructsState { } } - for s in &mut structs_to_write { + for s in structs_to_write.iter_mut() { let name = s.struct_or_contract_name(); if aliases.contains_key(name) { s.import_alias = Some(aliases[name][&s.path].clone()); @@ -410,37 +288,19 @@ impl StructsState { for (s, fn_name) in structs_to_write.iter_mut().zip(fn_names.into_iter()) { s.name_in_fns = fn_name.unwrap_or(s.name.clone()); } - - ResolvedState { structs_to_write, target_path } } -} - -struct ResolvedState { - structs_to_write: Vec, - target_path: PathBuf, -} -impl ResolvedState { - fn write(self) -> Result { + /// Write the final bindings file + fn write_bindings( + &self, + structs_to_write: &[StructToWrite], + target_path: &PathBuf, + ) -> Result<()> { let mut result = String::new(); - self.write_imports(&mut result)?; - self.write_vm(&mut result); - self.write_library(&mut result)?; - - if let Some(parent) = self.target_path.parent() { - fs::create_dir_all(parent)?; - } - fs::write(&self.target_path, &result)?; - - sh_println!("Bindings written to {}", self.target_path.display())?; - - Ok(result) - } - fn write_imports(&self, result: &mut String) -> fmt::Result { + // Write imports let mut grouped_imports = BTreeMap::new(); - - for struct_to_write in &self.structs_to_write { + for struct_to_write in structs_to_write { let item = struct_to_write.import_item(); grouped_imports .entry(struct_to_write.path.as_path()) @@ -452,18 +312,15 @@ impl ResolvedState { for (path, names) in grouped_imports { writeln!( - result, + &mut result, "import {{{}}} from \"{}\";", names.iter().join(", "), path.to_slash_lossy() )?; } - Ok(()) - } - - /// Writes minimal VM interface to not depend on forge-std version - fn write_vm(&self, result: &mut String) { + // Write VM interface + // Writes minimal VM interface to not depend on forge-std version result.push_str(r#" interface Vm { function parseJsonTypeArray(string calldata json, string calldata key, string calldata typeDescription) external pure returns (bytes memory); @@ -473,9 +330,8 @@ interface Vm { function serializeJsonType(string calldata objectKey, string calldata valueKey, string calldata typeDescription, bytes memory value) external returns (string memory json); } "#); - } - fn write_library(&self, result: &mut String) -> fmt::Result { + // Write library result.push_str( r#" library JsonBindings { @@ -483,19 +339,20 @@ library JsonBindings { "#, ); + // write schema constants - for struct_to_write in &self.structs_to_write { + for struct_to_write in structs_to_write { writeln!( - result, + &mut result, " {}{} = \"{}\";", TYPE_BINDING_PREFIX, struct_to_write.name_in_fns, struct_to_write.schema )?; } // write serialization functions - for struct_to_write in &self.structs_to_write { + for struct_to_write in structs_to_write { write!( - result, + &mut result, r#" function serialize({path} memory value) internal pure returns (string memory) {{ return vm.serializeJsonType(schema_{name_in_fns}, abi.encode(value)); @@ -524,6 +381,131 @@ library JsonBindings { result.push_str("}\n"); + // Write to file + if let Some(parent) = target_path.parent() { + fs::create_dir_all(parent)?; + } + fs::write(target_path, &result)?; + + sh_println!("Bindings written to {}", target_path.display())?; + Ok(()) } } + +struct PreprocessorVisitor { + updates: Vec<(Span, &'static str)>, +} + +impl PreprocessorVisitor { + fn new() -> Self { + Self { updates: Vec::new() } + } + + fn update(mut self, sess: &Session, content: &mut String) { + if self.updates.is_empty() { + return; + } + + let sf = sess.source_map().lookup_source_file(self.updates[0].0.lo()); + let base = sf.start_pos.0; + + self.updates.sort_by_key(|(span, _)| span.lo()); + let mut shift = 0_i64; + for (span, new) in self.updates { + let lo = span.lo() - base; + let hi = span.hi() - base; + let start = ((lo.0 as i64) - shift) as usize; + let end = ((hi.0 as i64) - shift) as usize; + + content.replace_range(start..end, new); + shift += (end - start) as i64; + shift -= new.len() as i64; + } + } +} + +impl<'ast> Visit<'ast> for PreprocessorVisitor { + type BreakValue = solar_parse::interface::data_structures::Never; + + fn visit_item_function( + &mut self, + func: &'ast ast::ItemFunction<'ast>, + ) -> ControlFlow { + // Replace function bodies with a noop statement. + if let Some(block) = &func.body { + if !block.is_empty() { + let span = block.first().unwrap().span.to(block.last().unwrap().span); + let new_body = match func.kind { + FunctionKind::Modifier => "_;", + _ => "revert();", + }; + self.updates.push((span, new_body)); + } + } + + self.walk_item_function(func) + } + + fn visit_variable_definition( + &mut self, + var: &'ast ast::VariableDefinition<'ast>, + ) -> ControlFlow { + // Remove `immutable` attributes. + if let Some(VarMut::Immutable) = var.mutability { + self.updates.push((var.span, "")); + } + + self.walk_variable_definition(var) + } +} + +/// A single struct definition for which we need to generate bindings. +#[derive(Debug, Clone)] +struct StructToWrite { + /// Name of the struct definition. + name: String, + /// Name of the contract containing the struct definition. None if the struct is defined at the + /// file level. + contract_name: Option, + /// Import alias for the contract or struct, depending on whether the struct is imported + /// directly, or via a contract. + import_alias: Option, + /// Path to the file containing the struct definition. + path: PathBuf, + /// EIP712 schema for the struct. + schema: String, + /// Name of the struct definition used in function names and schema_* variables. + name_in_fns: String, +} + +impl StructToWrite { + /// Returns the name of the imported item. If struct is defined at the file level, returns the + /// struct name, otherwise returns the parent contract name. + fn struct_or_contract_name(&self) -> &str { + self.contract_name.as_deref().unwrap_or(&self.name) + } + + /// Same as [StructToWrite::struct_or_contract_name] but with alias applied. + fn struct_or_contract_name_with_alias(&self) -> &str { + self.import_alias.as_deref().unwrap_or(self.struct_or_contract_name()) + } + + /// Path which can be used to reference this struct in input/output parameters. Either + /// StructName or ParantName.StructName + fn full_path(&self) -> String { + if self.contract_name.is_some() { + format!("{}.{}", self.struct_or_contract_name_with_alias(), self.name) + } else { + self.struct_or_contract_name_with_alias().to_string() + } + } + + fn import_item(&self) -> String { + if let Some(alias) = &self.import_alias { + format!("{} as {}", self.struct_or_contract_name(), alias) + } else { + self.struct_or_contract_name().to_string() + } + } +} From 0987bcdc6ff5c98419c868cf8f0b171d61b6b0c5 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 17 Jun 2025 12:38:32 +0200 Subject: [PATCH 2/2] fix: address clippy warnings in bind_json refactor - Change &PathBuf to &Path in function parameter - Remove unnecessary borrows - Change &mut Vec to &mut slice - Add Path import --- crates/forge/src/cmd/bind_json.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/forge/src/cmd/bind_json.rs b/crates/forge/src/cmd/bind_json.rs index 32214c74740b9..4860fb7afa6d4 100644 --- a/crates/forge/src/cmd/bind_json.rs +++ b/crates/forge/src/cmd/bind_json.rs @@ -27,7 +27,7 @@ use std::{ collections::{BTreeMap, BTreeSet, HashSet}, fmt::Write, ops::ControlFlow, - path::PathBuf, + path::{Path, PathBuf}, sync::Arc, }; @@ -136,7 +136,7 @@ impl BindJsonArgs { project: &Project, version: Version, sources: Sources, - _target_path: &PathBuf, + _target_path: &Path, ) -> Result> { let settings = config.solc_settings()?; let include = &config.bind_json.include; @@ -152,7 +152,7 @@ impl BindJsonArgs { sess.enter_parallel(|| -> Result<()> { // Set up the parsing context with the project paths, without adding the source files - let mut parsing_context = solar_pcx_from_solc_project(&sess, &project, &input, false); + let mut parsing_context = solar_pcx_from_solc_project(&sess, project, &input, false); let mut target_files = HashSet::new(); for (path, source) in &input.input.sources { @@ -200,7 +200,7 @@ impl BindJsonArgs { .contract .map(|id| hir.contract(id).name.as_str().into()), path: path - .strip_prefix(&root) + .strip_prefix(root) .unwrap_or_else(|_| path) .to_path_buf(), schema, @@ -229,7 +229,7 @@ impl BindJsonArgs { /// - Namespace of struct names used in function names and schema_* variables. /// /// Both of those might contain conflicts, so we need to resolve them. - fn resolve_conflicts(&self, structs_to_write: &mut Vec) { + fn resolve_conflicts(&self, structs_to_write: &mut [StructToWrite]) { // firstly, we resolve imported names conflicts // construct mapping name -> paths from which items with such name are imported let mut names_to_paths = BTreeMap::new();