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.

Sign up for GitHub

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

Jump to bottom

feat(sol-macro): improve error messages #345

Merged
merged 1 commit into from
Oct 8, 2023
Merged

feat(sol-macro): improve error messages #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions crates/sol-macro/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
6 changes: 6 additions & 0 deletions crates/sol-macro/src/expand/contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,19 @@ use syn::{ext::IdentExt, parse_quote, Attribute, Result};
///
/// ```ignore (pseudo-code)
/// pub mod #name {
/// #(#items)*
///
/// pub enum #{name}Calls {
/// ...
/// }
///
/// pub enum #{name}Errors {
/// ...
/// }
///
/// pub enum #{name}Events {
/// ...
/// }
/// }
/// ```
pub(super) fn expand(cx: &ExpCtxt<'_>, contract: &ItemContract) -> Result<TokenStream> {
Expand Down
123 changes: 64 additions & 59 deletions crates/sol-macro/src/expand/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pub fn expand(ast: File) -> Result<TokenStream> {
ExpCtxt::new(&ast).expand()
}

/// The expansion context.
struct ExpCtxt<'ast> {
all_items: Vec<&'ast Item>,
custom_types: HashMap<SolIdent, Type>,
Expand Down Expand Up @@ -61,25 +62,31 @@ impl<'ast> ExpCtxt<'ast> {
}

fn expand(mut self) -> Result<TokenStream> {
let mut abort = false;
let mut tokens = TokenStream::new();

if let Err(e) = self.parse_file_attributes() {
tokens.extend(e.into_compile_error());
}

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);
}
Expand Down Expand Up @@ -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()),
Expand All @@ -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<SolIdent, Type> =
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| {
Expand All @@ -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()
Expand All @@ -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";
);
}
}
}
Expand All @@ -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(())
}
}

Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -489,24 +496,22 @@ impl<'ast> ExpCtxt<'ast> {
where
I: IntoIterator<Item = &'a VariableDeclaration>,
{
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(())
}
}

Expand Down
5 changes: 3 additions & 2 deletions crates/sol-macro/src/expand/struct.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
});

Expand Down
18 changes: 9 additions & 9 deletions crates/sol-macro/src/expand/ty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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,
},

Expand All @@ -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()
Expand All @@ -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,
},

Expand All @@ -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()
Expand All @@ -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,
},

Expand Down
3 changes: 3 additions & 0 deletions crates/sol-macro/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down
Loading