Skip to content

fix(sol-macro): derive builtin traits on overloaded event/error enums#1118

Open
pjdurden wants to merge 3 commits into
alloy-rs:mainfrom
pjdurden:fix/overloaded-event-enum-derives
Open

fix(sol-macro): derive builtin traits on overloaded event/error enums#1118
pjdurden wants to merge 3 commits into
alloy-rs:mainfrom
pjdurden:fix/overloaded-event-enum-derives

Conversation

@pjdurden

@pjdurden pjdurden commented Jun 1, 2026

Copy link
Copy Markdown

Fixes alloy-rs/alloy#3856

The generated *Events / *Errors enum decided whether the #[sol(all_derives)]
builtin traits (Debug, PartialEq, Eq, Hash) could be derived by resolving
each variant's type name against the item table. Overloaded items get synthetic
_N-suffixed names (Swap_0, Swap_1, …) which don't resolve as items, so the
check hit the conservative false arm and silently dropped the derives — e.g. the
Uniswap V3 Pool (two Swap events) produced a PoolEvents enum that didn't impl
the std traits.

Compute derivability from the underlying items' parameter types (available when
building ExpandData) rather than from the synthetic variant type names. The
functions enum is unaffected (contract-level all_derives is intentionally not
propagated there).

Added a regression test with overloaded events under #[sol(all_derives)].

The `*Events` / `*Errors` enum derive computation resolved each variant's
type name against the item table to decide whether the `#[sol(all_derives)]`
builtin traits (Debug, PartialEq, Eq, Hash) could be derived. Overloaded
items get synthetic `_N`-suffixed names (e.g. `Swap_0`, `Swap_1`) that don't
resolve as items, so the check fell through to the conservative `false` arm
and silently dropped the derives.

Compute derivability from the underlying items' parameter types (available
when building `ExpandData`) instead of from the synthetic variant type names.

Fixes alloy-rs/alloy#3856
Copilot AI review requested due to automatic review settings June 1, 2026 04:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds support for #[sol(all_derives)] on generated *Calls / *Errors / *Events enums even when variants use overloaded/synthetic names that can’t be resolved for trait-derivability checks.

Changes:

  • Add enum_derives helper to apply extra_derives / all_derives to generated enums.
  • Precompute can_derive_builtin from underlying parameter types for calls/errors/events expansions.
  • Add a regression test for overloaded events ensuring the generated *Events enum derives the expected builtin traits.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
crates/sol-types/tests/derives.rs Adds regression test covering overloaded events + #[sol(all_derives)] on generated events enum.
crates/sol-macro-expander/src/expand/mod.rs Introduces enum_derives helper to derive traits on generated enums using precomputed derivability.
crates/sol-macro-expander/src/expand/contract.rs Computes can_derive_builtin for sets of calls/errors/events and uses enum_derives for enum generation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/sol-macro-expander/src/expand/mod.rs
@pjdurden

pjdurden commented Jun 13, 2026

Copy link
Copy Markdown
Author

Hi @mattsse , @DaniPopes Let me know if anything is required from my side for this.

@github-project-automation github-project-automation Bot moved this to Reviewed in Alloy Jun 13, 2026
@DaniPopes DaniPopes enabled auto-merge (squash) June 13, 2026 16:07
@DaniPopes DaniPopes disabled auto-merge June 13, 2026 16:08
@satyakwok

Copy link
Copy Markdown
Contributor

Nice — computing from the underlying param types is the cleaner direction here.

@pjdurden

Copy link
Copy Markdown
Author

Thanks @satyakwok. Resolving the synthetic _N names back to their items was the other route I tried, but derivability only really depends on the variant param types, so computing from those skips the item-table lookup entirely and falls out the same way for the non-overloaded enums too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewed

Development

Successfully merging this pull request may close these issues.

[Bug] Missing derive all for events

4 participants