Fix Result type generation for user-defined error enums named "Error"#1709
Fix Result type generation for user-defined error enums named "Error"#1709willemneal wants to merge 2 commits intostellar:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where user-defined error enums named "Error" were incorrectly mapped to the built-in soroban_sdk::Error type instead of being treated as user-defined types (UDTs). The fix ensures that all error enums in contract return types are properly represented as UDTs in the contract specification.
Changes:
- Removed special-case handling of "Error" as a built-in type in type mapping
- Added comprehensive test coverage for Result types with user-defined error enums
- Verified the fix works for both "Error" and "MyError" custom error types
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| soroban-sdk-macros/src/map_type.rs | Removed line that mapped "Error" to built-in error type; added unit tests verifying "Error" and "MyError" map to UDTs |
| tests/add_u64/src/lib.rs | Added two error enums and safe_add functions to test Result type generation |
| tests/import_contract/src/lib.rs | Added Error enum and functions that use imported contract's error types |
| tests/add_u64/test_snapshots/test/test_safe_add.1.json | Added ledger snapshot for new test case |
| soroban-spec-rust/src/lib.rs | Added integration tests verifying correct Result type generation for user-defined errors |
|
I used this as a test for Claude at finding and fixing the bug I found. I provided the minimal example and it found and fixed it very quickly. And now it being reviewed by another AI which had no comments feels like a new era. @leighmcculloch My only question is whether there would ever be the case that the |
|
Can you open an issue to go into detail about the problem to make sure we fully capture this bug, it's scope of impact, before we focus on the solution? We can do some analysis of contracts on mainnet to understand how changing the existing behaviour might break existing contracts, or if there's any impact today that we might need to think differently about. I suspect this fix is right though when we come back to it. Side note, this might be resolved by planned work around improving type identities (#1570). |
Yes I think so. Error is an SDK type, it's a valid type to appear at a contract boundary. Whether it is frequently used is a good question though, it may not be. |
|
Although the Error type captures a much larger range of errors than what a contract fn could return, so there's an argument for not supporting it, or finding some other way to capture that use case. 🤔 |
Remove the special case in `map_type` that mapped the type name "Error" to
`ScSpecTypeDef::Error`. User-defined error enums are now always treated as
UDTs (user-defined types), regardless of their name.
When a contract defined an error enum named `Error` and used it in a
`Result<T, Error>` return type, the generated client code incorrectly
produced `Result<T, soroban_sdk::Error>` instead of `Result<T, Error>`.
This happened because `map_type.rs` had a special case that mapped any type
named "Error" to `ScSpecTypeDef::Error` (the built-in error type), even when
it was a user-defined error enum. Error enums with other names like `MyError`
worked correctly because they fell through to the UDT handling.
The fix removes this special case so that all user-defined types, including
those named "Error", are treated as UDTs and generate correct client code.
Example - before the fix:
```rust
// Contract defines:
pub enum Error { Overflow = 1 }
pub fn safe_add(a: u64, b: u64) -> Result<u64, Error>
// Generated client had:
fn try_safe_add(...) -> Result<..., Result<soroban_sdk::Error, ...>>
```
Example - after the fix:
```rust
// Generated client now correctly has:
fn try_safe_add(...) -> Result<..., Result<Error, ...>>
```
N/A
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
willemneal
force-pushed
the
fix/error_generation
branch
from
b5bc732 to
db61291
Compare
Fix Result type generation for user-defined error enums named "Error"
What
Remove the special case in
map_typethat mapped the type name "Error" toScSpecTypeDef::Error. User-defined error enums are now always treated asUDTs (user-defined types), regardless of their name.
Why
When a contract defined an error enum named
Errorand used it in aResult<T, Error>return type, the generated client code incorrectlyproduced
Result<T, soroban_sdk::Error>instead ofResult<T, Error>.This happened because
map_type.rshad a special case that mapped any typenamed "Error" to
ScSpecTypeDef::Error(the built-in error type), even whenit was a user-defined error enum. Error enums with other names like
MyErrorworked correctly because they fell through to the UDT handling.
The fix removes this special case so that all user-defined types, including
those named "Error", are treated as UDTs and generate correct client code.
Example - before the fix:
Example - after the fix:
Close #1710
Known limitations
N/A
Co-Authored-By: Claude Opus 4.5 noreply@anthropic.com