fix: correctly resolve duplicate struct names in json abi #690
Conversation
There was a problem hiding this comment.
cool, I believe this works and is easy to follow.
can we add another test case for foundry-rs/foundry#8444
imo we can ignore this edge case import {Struct as Struct2}
ptal @DaniPopes
| /// various structs with such name have. | ||
| #[derive(Debug, Default, Clone)] | ||
| struct Collector<'a> { | ||
| unique_struct_components: BTreeMap<&'a str, BTreeSet<Vec<&'a str>>>, |
There was a problem hiding this comment.
perhaps worth using smallvec here for the Vec because we only expect a few projections?
| .map(move |(idx, components)| { | ||
| let components = | ||
| components.into_iter().map(ToString::to_string).collect::<Vec<_>>(); | ||
| (components, format!("{name}{idx}")) |
There was a problem hiding this comment.
should we start at 0 here or at 1?
There was a problem hiding this comment.
function overloads start with 0, so I've just made it similar
just noticed that functions are named as function_0, function_1, should we include underscore here as well?
| /// A [crate::Visitor] implementation that collects a mapping from struct name to set of components | ||
| /// various structs with such name have. |
There was a problem hiding this comment.
ah okay, I understand how this works, first we traverse the abi and collect unique names and their paths
There was a problem hiding this comment.
I don't really like renaming because this doesn't tackle the underlying problem that namespaces are not handled in sol!, e.g. this only fixes json input
I have a branch open where I make namespaced output work, but got stuck on weird compile errors https://github.com/alloy-rs/core/tree/dani/to-sol-contract-specifiers
| /// structs. | ||
| /// | ||
| /// Namespace for global struct names is managed and in cases of duplicates structs are renamed. | ||
| pub fn rename_internal_structs(&mut self) { |
There was a problem hiding this comment.
I don't think any of this should be exposed publicly, it should only be a sol! implentation detail
yeah, such approach indeed is more flexible current branch compiles fine for me, do you have more fixes for sol! as wip? from diff and a quick test it seems that updated |
|
It fails to compile in tests in crates/sol-types/tests/macros/sol/json.rs, but expanding the output and compiling it separately is fine It only handles the ToSol side, expanding libraries, but namespaces are not handled in the sol! internal representation, so function signatures are wrong Probably best to just continue that branch |
fyi moving |
|
one of the issues seems to be a limitation of function-level modules e.g. this code doesn't work fn test_fn() {
pub mod inner1 {
pub struct InnerStruct {}
}
pub mod inner2 {
use super::*;
fn some_fn() {
inner1::InnerStruct {};
}
}
}and it seems that there's no way to reach other modules defined inside of a function from inside of another module |
Motivation
Closes #660
Solution
I've added second abi preprocessing method
JsonAbi::rename_internal_structs, which goes over ABI, resolves structs that have a same name but different components sets, and renames them into<struct name>0,<struct name>1, etcIt should be also possible to key them by
contractinstead, however, there are some edge cases related to aliases with such approach. E.g.import {Struct as Struct1} from "..."; import {Struct as Struct2} from "...";will result in two different structs referenced asStructin ABI. Same is possible withLib.Structif differentLibs are aliased.This edge case is kind of weird, so I am open to changing impl if we consider performance improvements worth it.
Also not sure if we want this to happen on the level of
JsonAbi::to_solinstead of only forsol!used codePR Checklist