From 2168ae3a8456f93de526df24c514b39363760cf1 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Sun, 8 Oct 2023 09:56:07 +0200 Subject: [PATCH] feat(sol-macro): improve error messages (#345) --- crates/sol-macro/Cargo.toml | 1 + crates/sol-macro/src/expand/contract.rs | 6 + crates/sol-macro/src/expand/mod.rs | 123 +++++++++++---------- crates/sol-macro/src/expand/struct.rs | 5 +- crates/sol-macro/src/expand/ty.rs | 18 +-- crates/sol-macro/src/lib.rs | 3 + crates/sol-types/tests/sol.rs | 49 ++++++++ crates/sol-types/tests/ui/overloads.stderr | 40 +++---- crates/syn-solidity/src/ident/path.rs | 9 -- crates/syn-solidity/src/item/contract.rs | 6 +- crates/syn-solidity/src/item/function.rs | 1 + 11 files changed, 161 insertions(+), 100 deletions(-) diff --git a/crates/sol-macro/Cargo.toml b/crates/sol-macro/Cargo.toml index 5ad4c1b03b..04f710bd19 100644 --- a/crates/sol-macro/Cargo.toml +++ b/crates/sol-macro/Cargo.toml @@ -35,6 +35,7 @@ hex.workspace = true # json alloy-json-abi = { workspace = true, optional = true } serde_json = { workspace = true, optional = true } +proc-macro-error = "1.0.4" [features] json = ["dep:alloy-json-abi", "dep:serde_json"] diff --git a/crates/sol-macro/src/expand/contract.rs b/crates/sol-macro/src/expand/contract.rs index 6e457df8f2..6965bacff6 100644 --- a/crates/sol-macro/src/expand/contract.rs +++ b/crates/sol-macro/src/expand/contract.rs @@ -12,6 +12,8 @@ use syn::{ext::IdentExt, parse_quote, Attribute, Result}; /// /// ```ignore (pseudo-code) /// pub mod #name { +/// #(#items)* +/// /// pub enum #{name}Calls { /// ... /// } @@ -19,6 +21,10 @@ use syn::{ext::IdentExt, parse_quote, Attribute, Result}; /// pub enum #{name}Errors { /// ... /// } +/// +/// pub enum #{name}Events { +/// ... +/// } /// } /// ``` pub(super) fn expand(cx: &ExpCtxt<'_>, contract: &ItemContract) -> Result { diff --git a/crates/sol-macro/src/expand/mod.rs b/crates/sol-macro/src/expand/mod.rs index a3041d42a2..2abf1cdd4d 100644 --- a/crates/sol-macro/src/expand/mod.rs +++ b/crates/sol-macro/src/expand/mod.rs @@ -34,6 +34,7 @@ pub fn expand(ast: File) -> Result { ExpCtxt::new(&ast).expand() } +/// The expansion context. struct ExpCtxt<'ast> { all_items: Vec<&'ast Item>, custom_types: HashMap, @@ -61,6 +62,7 @@ impl<'ast> ExpCtxt<'ast> { } fn expand(mut self) -> Result { + let mut abort = false; let mut tokens = TokenStream::new(); if let Err(e) = self.parse_file_attributes() { @@ -68,18 +70,23 @@ impl<'ast> ExpCtxt<'ast> { } self.visit_file(self.ast); + if self.all_items.len() > 1 { - self.resolve_custom_types()?; - self.mk_overloads_map()?; + self.resolve_custom_types(); + if self.mk_overloads_map().is_err() { + abort = true; + } + } + + if abort { + return Ok(tokens) } for item in &self.ast.items { + // TODO: Dummy items let t = match self.expand_item(item) { Ok(t) => t, - Err(e) => { - // TODO: Dummy items - e.into_compile_error() - } + Err(e) => e.into_compile_error(), }; tokens.extend(t); } @@ -118,6 +125,7 @@ impl<'ast> ExpCtxt<'ast> { map.reserve(self.all_items.len()); for &item in &self.all_items { let (name, ty) = match item { + Item::Contract(c) => (&c.name, c.as_type()), Item::Enum(e) => (&e.name, e.as_type()), Item::Struct(s) => (&s.name, s.as_type()), Item::Udt(u) => (&u.name, u.ty.clone()), @@ -128,14 +136,13 @@ impl<'ast> ExpCtxt<'ast> { self.custom_types = map; } - fn resolve_custom_types(&mut self) -> Result<()> { + fn resolve_custom_types(&mut self) { self.mk_types_map(); // you won't get me this time, borrow checker // SAFETY: no data races, we don't modify the map while we're iterating // I think this is safe anyway let map_ref: &mut HashMap = unsafe { &mut *(&mut self.custom_types as *mut _) }; - let map = &self.custom_types; for ty in map_ref.values_mut() { let mut i = 0; ty.visit_mut(|ty| { @@ -146,25 +153,25 @@ impl<'ast> ExpCtxt<'ast> { let Type::Custom(name) = &*ty else { unreachable!() }; - let Some(resolved) = map.get(name.last_tmp()) else { + let Some(resolved) = self.try_custom_type(name) else { return }; ty.clone_from(resolved); i += 1; }); if i >= RESOLVE_LIMIT { - let msg = "\ - failed to resolve types.\n\ - This is likely due to an infinitely recursive type definition.\n\ - If you believe this is a bug, please file an issue at \ - https://github.com/alloy-rs/core/issues/new/choose"; - return Err(Error::new(ty.span(), msg)) + abort!( + ty.span(), + "failed to resolve types.\n\ + This is likely due to an infinitely recursive type definition.\n\ + If you believe this is a bug, please file an issue at \ + https://github.com/alloy-rs/core/issues/new/choose" + ); } } - Ok(()) } - fn mk_overloads_map(&mut self) -> Result<()> { + fn mk_overloads_map(&mut self) -> std::result::Result<(), ()> { let all_orig_names: Vec<_> = self .overloaded_items .values() @@ -173,25 +180,21 @@ impl<'ast> ExpCtxt<'ast> { .collect(); let mut overloads_map = std::mem::take(&mut self.overloads); - // report all errors at the end - let mut errors = Vec::new(); + let mut failed = false; for functions in self.overloaded_items.values().filter(|fs| fs.len() >= 2) { // check for same parameters for (i, &a) in functions.iter().enumerate() { for &b in functions.iter().skip(i + 1) { if a.eq_by_types(b) { - let msg = format!( + failed = true; + emit_error!( + a.span(), "{} with same name and parameter types defined twice", - a.desc() - ); - let mut err = syn::Error::new(a.span(), msg); + a.desc(); - let msg = "other declaration is here"; - let note = syn::Error::new(b.span(), msg); - - err.combine(note); - errors.push(err); + note = b.span() => "other declaration is here"; + ); } } } @@ -202,27 +205,27 @@ impl<'ast> ExpCtxt<'ast> { }; let new_name = format!("{old_name}_{i}"); if let Some(other) = all_orig_names.iter().find(|x| x.0 == new_name) { - let msg = format!( + failed = true; + emit_error!( + old_name.span(), "{} `{old_name}` is overloaded, \ but the generated name `{new_name}` is already in use", - item.desc() - ); - let mut err = syn::Error::new(old_name.span(), msg); + item.desc(); - let msg = "other declaration is here"; - let note = syn::Error::new(other.span(), msg); - - err.combine(note); - errors.push(err); + note = other.span() => "other declaration is here"; + ) } overloads_map.insert(item.signature(self), new_name); } } - utils::combine_errors(errors).map(|()| { - self.overloads = overloads_map; - }) + if failed { + return Err(()) + } + + self.overloads = overloads_map; + Ok(()) } } @@ -310,28 +313,32 @@ impl<'a> OverloadedItem<'a> { // utils impl<'ast> ExpCtxt<'ast> { #[allow(dead_code)] - fn get_item(&self, name: &SolPath) -> &Item { - match self.try_get_item(name) { + fn item(&self, name: &SolPath) -> &Item { + match self.try_item(name) { Some(item) => item, - None => panic!("unresolved item: {name}"), + None => abort!(name.span(), "unresolved item: {}", name), } } - fn try_get_item(&self, name: &SolPath) -> Option<&Item> { - let name = name.last_tmp(); + fn try_item(&self, name: &SolPath) -> Option<&Item> { + let name = name.last(); self.all_items .iter() - .find(|item| item.name() == Some(name)) .copied() + .find(|item| item.name() == Some(name)) } fn custom_type(&self, name: &SolPath) -> &Type { - match self.custom_types.get(name.last_tmp()) { + match self.try_custom_type(name) { Some(item) => item, - None => panic!("unresolved item: {name}"), + None => abort!(name.span(), "unresolved custom type: {}", name), } } + fn try_custom_type(&self, name: &SolPath) -> Option<&Type> { + self.custom_types.get(name.last()) + } + /// Returns the name of the function, adjusted for overloads. fn function_name(&self, function: &ItemFunction) -> SolIdent { self.overloaded_name(function.into()) @@ -489,24 +496,22 @@ impl<'ast> ExpCtxt<'ast> { where I: IntoIterator, { - let mut errors = Vec::new(); + let mut errored = false; for param in params { param.ty.visit(|ty| { if let Type::Custom(name) = ty { - if !self.custom_types.contains_key(name.last_tmp()) { - let e = syn::Error::new(name.span(), "unresolved type"); - errors.push(e); + if !self.custom_types.contains_key(name.last()) { + let note = (!errored).then(|| { + errored = true; + "Custom types must be declared inside of the same scope they are referenced in,\n\ + or \"imported\" as a UDT with `type ... is (...);`" + }); + emit_error!(name.span(), "unresolved type"; help =? note); } } }); } - utils::combine_errors(errors).map_err(|mut e| { - let note = - "Custom types must be declared inside of the same scope they are referenced in,\n\ - or \"imported\" as a UDT with `type ... is (...);`"; - e.combine(Error::new(Span::call_site(), note)); - e - }) + Ok(()) } } diff --git a/crates/sol-macro/src/expand/struct.rs b/crates/sol-macro/src/expand/struct.rs index 0e3273276e..7424d5b4d6 100644 --- a/crates/sol-macro/src/expand/struct.rs +++ b/crates/sol-macro/src/expand/struct.rs @@ -183,13 +183,14 @@ fn expand_encode_type_fns( let mut fields = fields.clone(); fields.visit_types_mut(|ty| { let Type::Custom(name) = ty else { return }; - match cx.try_get_item(name) { + match cx.try_item(name) { // keep as custom Some(Item::Struct(_)) | None => {} // convert to underlying + Some(Item::Contract(_)) => *ty = Type::Address(ty.span(), None), Some(Item::Enum(_)) => *ty = Type::Uint(ty.span(), NonZeroU16::new(8)), Some(Item::Udt(udt)) => *ty = udt.ty.clone(), - Some(item) => panic!("Invalid type in struct field: {item:?}"), + Some(item) => abort!(item.span(), "Invalid type in struct field: {:?}", item), } }); diff --git a/crates/sol-macro/src/expand/ty.rs b/crates/sol-macro/src/expand/ty.rs index 5fe1719381..cf4cf2379b 100644 --- a/crates/sol-macro/src/expand/ty.rs +++ b/crates/sol-macro/src/expand/ty.rs @@ -248,8 +248,8 @@ pub(super) fn type_base_data_size(cx: &ExpCtxt<'_>, ty: &Type) -> usize { .map(|ty| type_base_data_size(cx, ty)) .sum(), - Type::Custom(name) => match cx.try_get_item(name) { - Some(Item::Enum(_)) => 32, + Type::Custom(name) => match cx.try_item(name) { + Some(Item::Contract(_)) | Some(Item::Enum(_)) => 32, Some(Item::Error(error)) => error .parameters .types() @@ -266,7 +266,7 @@ pub(super) fn type_base_data_size(cx: &ExpCtxt<'_>, ty: &Type) -> usize { .map(|ty| type_base_data_size(cx, ty)) .sum(), Some(Item::Udt(udt)) => type_base_data_size(cx, &udt.ty), - Some(item) => panic!("Invalid item in param list: {item:?}"), + Some(item) => abort!(item.span(), "Invalid type in struct field: {:?}", item), None => 0, }, @@ -293,8 +293,8 @@ pub(super) fn can_derive_default(cx: &ExpCtxt<'_>, ty: &Type) -> bool { } } - Type::Custom(name) => match cx.try_get_item(name) { - Some(Item::Enum(_)) => false, + Type::Custom(name) => match cx.try_item(name) { + Some(Item::Contract(_)) | Some(Item::Enum(_)) => false, Some(Item::Error(error)) => error .parameters .types() @@ -307,7 +307,7 @@ pub(super) fn can_derive_default(cx: &ExpCtxt<'_>, ty: &Type) -> bool { strukt.fields.types().all(|ty| can_derive_default(cx, ty)) } Some(Item::Udt(udt)) => can_derive_default(cx, &udt.ty), - Some(item) => panic!("Invalid item in param list: {item:?}"), + Some(item) => abort!(item.span(), "Invalid type in struct field: {:?}", item), _ => false, }, @@ -331,8 +331,8 @@ pub(super) fn can_derive_builtin_traits(cx: &ExpCtxt<'_>, ty: &Type) -> bool { } } - Type::Custom(name) => match cx.try_get_item(name) { - Some(Item::Enum(_)) => true, + Type::Custom(name) => match cx.try_item(name) { + Some(Item::Contract(_)) | Some(Item::Enum(_)) => true, Some(Item::Error(error)) => error .parameters .types() @@ -346,7 +346,7 @@ pub(super) fn can_derive_builtin_traits(cx: &ExpCtxt<'_>, ty: &Type) -> bool { .types() .all(|ty| can_derive_builtin_traits(cx, ty)), Some(Item::Udt(udt)) => can_derive_builtin_traits(cx, &udt.ty), - Some(item) => panic!("Invalid item in param list: {item:?}"), + Some(item) => abort!(item.span(), "Invalid type in struct field: {:?}", item), _ => false, }, diff --git a/crates/sol-macro/src/lib.rs b/crates/sol-macro/src/lib.rs index 26817440bb..29178960d9 100644 --- a/crates/sol-macro/src/lib.rs +++ b/crates/sol-macro/src/lib.rs @@ -22,6 +22,8 @@ #![deny(unused_must_use, rust_2018_idioms)] #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] +#[macro_use] +extern crate proc_macro_error; extern crate syn_solidity as ast; use proc_macro::TokenStream; @@ -192,6 +194,7 @@ mod utils; #[doc = include_str!("../doctests/json.rs")] /// ``` #[proc_macro] +#[proc_macro_error] pub fn sol(input: TokenStream) -> TokenStream { parse_macro_input!(input as input::SolInput) .expand() diff --git a/crates/sol-types/tests/sol.rs b/crates/sol-types/tests/sol.rs index 261f1f76f2..3c9d541d4c 100644 --- a/crates/sol-types/tests/sol.rs +++ b/crates/sol-types/tests/sol.rs @@ -430,6 +430,55 @@ fn enum_field_of_struct() { }; } +// TODO +// https://github.com/alloy-rs/core/issues/343 +#[test] +#[cfg(TODO)] +fn rust_keywords() { + sol! { + function mod(address impl) returns (bool is, bool fn); + } +} + +// TODO +#[test] +#[cfg(TODO)] +fn contract_type() { + sol! { + interface IERC20 {} + function func(IERC20 addr); + struct Struct { + IERC20 addr; + } + } +} + +// TODO: make commented out code work +#[test] +fn paths_resolution_basic() { + sol! { + // library OrderRFQLib { + struct OrderRFQ { + uint256 info; + address makerAsset; + address takerAsset; + address maker; + address allowedSender; + uint256 makingAmount; + uint256 takingAmount; + } + // } + + function fillOrderRFQ( + /*OrderRFQLib.*/OrderRFQ memory order, + bytes calldata signature, + uint256 flagsAndAmount + ) external payable returns(uint256, uint256, bytes32) { + return fillOrderRFQTo(order, signature, flagsAndAmount, msg.sender); + } + } +} + #[test] #[cfg(feature = "json")] fn abigen_json_large_array() { diff --git a/crates/sol-types/tests/ui/overloads.stderr b/crates/sol-types/tests/ui/overloads.stderr index 8fae12d9e0..80117b3d19 100644 --- a/crates/sol-types/tests/ui/overloads.stderr +++ b/crates/sol-types/tests/ui/overloads.stderr @@ -3,8 +3,8 @@ error: function `overloadTaken` is overloaded, but the generated name `overloadT | 15 | function overloadTaken(); | ^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:17:18 | 17 | function overloadTaken_0(); @@ -15,8 +15,8 @@ error: function `overloadTaken` is overloaded, but the generated name `overloadT | 16 | function overloadTaken(uint256); | ^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:18:18 | 18 | function overloadTaken_1(); @@ -27,8 +27,8 @@ error: function with same name and parameter types defined twice | 23 | function sameOverload(); | ^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:24:18 | 24 | function sameOverload(); @@ -39,8 +39,8 @@ error: function with same name and parameter types defined twice | 28 | function sameTysOverload1(uint256[]memory a); | ^^^^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:29:18 | 29 | function sameTysOverload1(uint256[]storage b); @@ -51,8 +51,8 @@ error: function with same name and parameter types defined twice | 33 | function sameTysOverload2(string memory,string storage); | ^^^^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:34:18 | 34 | function sameTysOverload2(string storage b,string calldata); @@ -63,8 +63,8 @@ error: event `overloadTaken` is overloaded, but the generated name `overloadTake | 50 | event overloadTaken(); | ^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:52:15 | 52 | event overloadTaken_0(); @@ -75,8 +75,8 @@ error: event `overloadTaken` is overloaded, but the generated name `overloadTake | 51 | event overloadTaken(uint256); | ^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:53:15 | 53 | event overloadTaken_1(); @@ -87,8 +87,8 @@ error: event with same name and parameter types defined twice | 58 | event sameOverload(); | ^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:59:15 | 59 | event sameOverload(); @@ -99,8 +99,8 @@ error: event with same name and parameter types defined twice | 63 | event sameTysOverload1(uint256[] a); | ^^^^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:64:15 | 64 | event sameTysOverload1(uint256[] b); @@ -111,8 +111,8 @@ error: event with same name and parameter types defined twice | 68 | event sameTysOverload2(string, string); | ^^^^^^^^^^^^^^^^ - -error: other declaration is here + | +note: other declaration is here --> tests/ui/overloads.rs:69:15 | 69 | event sameTysOverload2(string, string); diff --git a/crates/syn-solidity/src/ident/path.rs b/crates/syn-solidity/src/ident/path.rs index 5842a4a533..98d8e1cc91 100644 --- a/crates/syn-solidity/src/ident/path.rs +++ b/crates/syn-solidity/src/ident/path.rs @@ -119,15 +119,6 @@ impl SolPath { self.0.last().unwrap() } - // TODO: paths resolution - #[track_caller] - pub fn last_tmp(&self) -> &SolIdent { - if self.len() > 1 { - todo!("path resolution") - } - self.last() - } - pub fn last_mut(&mut self) -> &mut SolIdent { self.0.last_mut().unwrap() } diff --git a/crates/syn-solidity/src/item/contract.rs b/crates/syn-solidity/src/item/contract.rs index adb82e8119..ceec9b6039 100644 --- a/crates/syn-solidity/src/item/contract.rs +++ b/crates/syn-solidity/src/item/contract.rs @@ -1,4 +1,4 @@ -use crate::{kw, utils::DebugPunctuated, Item, Modifier, SolIdent, Spanned}; +use crate::{kw, utils::DebugPunctuated, Item, Modifier, SolIdent, Spanned, Type}; use proc_macro2::Span; use std::{cmp::Ordering, fmt}; use syn::{ @@ -101,6 +101,10 @@ impl Spanned for ItemContract { } impl ItemContract { + pub fn as_type(&self) -> Type { + Type::Address(self.span(), None) + } + /// Returns true if `self` is an abstract contract. pub fn is_abstract_contract(&self) -> bool { self.kind.is_abstract_contract() diff --git a/crates/syn-solidity/src/item/function.rs b/crates/syn-solidity/src/item/function.rs index c1ea0d61cd..a9b2010339 100644 --- a/crates/syn-solidity/src/item/function.rs +++ b/crates/syn-solidity/src/item/function.rs @@ -209,6 +209,7 @@ impl ItemFunction { /// /// Panics if the function has no name. This is the case when `kind` is not /// `Function`. + #[track_caller] pub fn name(&self) -> &SolIdent { match &self.name { Some(name) => name,