Skip to content

Commit

Permalink
Remove need for Copy on error enums (#1292)
Browse files Browse the repository at this point in the history
### What
Remove need for Copy/Clone/etc on error enums and int enums.

### Why
Less compiler errors for folks just getting started.

The generated code currently requires that error enums impl the Copy
trait because they use a copy within. That copy is unnecessary and this
change removes it, removing the need for Copy to be derived on error
enums.

I was confident that the copy had been put there as an optimisation, but
when I looked into the code that was relying on it I found not
optimisation was in use. When I tried to add the optimisation, I
couldn't find a contract size difference.

A while ago we in #1293
added better error messages for needing copy, which was a good fix at
the time, I just think we can remove the need for this all together.

Close #630
  • Loading branch information
leighmcculloch authored Sep 27, 2024
1 parent 8fc9f53 commit 627e164
Show file tree
Hide file tree
Showing 8 changed files with 464 additions and 29 deletions.
9 changes: 6 additions & 3 deletions soroban-sdk-macros/src/derive_enum_int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ pub fn derive_type_enum_int(
// Output.
let mut output = quote! {
#spec_gen
const _: () = { fn assert_copy(v: #enum_ident) -> impl Copy { v } };

impl #path::TryFromVal<#path::Env, #path::Val> for #enum_ident {
type Error = #path::ConversionError;
Expand Down Expand Up @@ -144,15 +143,19 @@ pub fn derive_type_enum_int(
type Error = #path::xdr::Error;
#[inline(always)]
fn try_into(self) -> Result<#path::xdr::ScVal, #path::xdr::Error> {
Ok((*self as u32).into())
Ok(match self {
#(#try_intos,)*
})
}
}

impl TryInto<#path::xdr::ScVal> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_into(self) -> Result<#path::xdr::ScVal, #path::xdr::Error> {
Ok((self as u32).into())
Ok(match self {
#(#try_intos,)*
})
}
}

Expand Down
17 changes: 8 additions & 9 deletions soroban-sdk-macros/src/derive_error_enum_int.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ pub fn derive_type_error_enum_int(
// Output.
quote! {
#spec_gen
const _: () = { fn assert_copy(v: #enum_ident) -> impl Copy { v } };

impl TryFrom<#path::Error> for #enum_ident {
type Error = #path::Error;
Expand Down Expand Up @@ -121,16 +120,16 @@ pub fn derive_type_error_enum_int(
impl From<#enum_ident> for #path::Error {
#[inline(always)]
fn from(val: #enum_ident) -> #path::Error {
match val {
#(#into_errors,)*
}
<_ as From<&#enum_ident>>::from(&val)
}
}

impl From<&#enum_ident> for #path::Error {
#[inline(always)]
fn from(val: &#enum_ident) -> #path::Error {
<_ as From<#enum_ident>>::from(*val)
match val {
#(#into_errors,)*
}
}
}

Expand Down Expand Up @@ -159,16 +158,16 @@ pub fn derive_type_error_enum_int(
impl From<#enum_ident> for #path::InvokeError {
#[inline(always)]
fn from(val: #enum_ident) -> #path::InvokeError {
match val {
#(#into_invoke_errors,)*
}
<_ as From<&#enum_ident>>::from(&val)
}
}

impl From<&#enum_ident> for #path::InvokeError {
#[inline(always)]
fn from(val: &#enum_ident) -> #path::InvokeError {
<_ as From<#enum_ident>>::from(*val)
match val {
#(#into_invoke_errors,)*
}
}
}

Expand Down
1 change: 1 addition & 0 deletions soroban-sdk/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod contract_snapshot;
mod contract_store;
mod contract_timepoint;
mod contract_udt_enum;
mod contract_udt_enum_error;
mod contract_udt_option;
mod contract_udt_struct;
mod contract_udt_struct_tuple;
Expand Down
57 changes: 57 additions & 0 deletions soroban-sdk/src/tests/contract_udt_enum_error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
use crate::{self as soroban_sdk};
use soroban_sdk::{contract, contracterror, contractimpl, contracttype, Env};

#[contract]
pub struct Contract;

#[contracttype]
pub enum Flag {
A = 0,
B = 1,
}

#[contracterror]
pub enum Error {
AnError = 1,
}

#[contractimpl]
impl Contract {
pub fn f(flag: Flag) -> Result<(), Error> {
match flag {
Flag::A => Ok(()),
Flag::B => Err(Error::AnError),
}
}
}

// The assertions in the following tests intentionally don't use assert_eq, and
// unwraps the object using Rust's pattern matching, so that no derives like
// Debug, Eq, PartialEq, etc are required on the error enum type because this
// test serves to ensure that none of the generated code for the error type or
// the client that uses it is dependent on derives that may or may not be
// available.

#[test]
fn test_success() {
let env = Env::default();
let contract_id = env.register(Contract, ());
let client = ContractClient::new(&env, &contract_id);

// See comment above about why this assertion is a match.
let Ok(Ok(())) = client.try_f(&Flag::A) else {
panic!("unexpected value returned");
};
}

#[test]
fn test_error() {
let env = Env::default();
let contract_id = env.register(Contract, ());
let client = ContractClient::new(&env, &contract_id);

// See comment above about why this assertion is a match.
let Err(Ok(Error::AnError)) = client.try_f(&Flag::B) else {
panic!("unexpected value returned");
};
}
4 changes: 2 additions & 2 deletions soroban-sdk/src/tests/contractimport_with_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ pub struct Contract;

#[contractimpl]
impl Contract {
pub fn hello_with(env: Env, contract_id: Address, flag: u32) -> Symbol {
pub fn hello_with(env: Env, contract_id: Address, flag: errcontract::Flag) -> Symbol {
errcontract::Client::new(&env, &contract_id).hello(&flag)
}
}
Expand All @@ -27,6 +27,6 @@ fn test_functional() {
let contract_id = e.register(Contract, ());
let client = ContractClient::new(&e, &contract_id);

let z = client.hello_with(&err_contract_id, &0);
let z = client.hello_with(&err_contract_id, &errcontract::Flag::A);
assert!(z == symbol_short!("hello"));
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
{
"generators": {
"address": 1,
"nonce": 0
},
"auth": [
[],
[]
],
"ledger": {
"protocol_version": 22,
"sequence_number": 0,
"timestamp": 0,
"network_id": "0000000000000000000000000000000000000000000000000000000000000000",
"base_reserve": 0,
"min_persistent_entry_ttl": 4096,
"min_temp_entry_ttl": 16,
"max_entry_ttl": 6312000,
"ledger_entries": [
[
{
"contract_data": {
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_data": {
"ext": "v0",
"contract": "CAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAD2KM",
"key": "ledger_key_contract_instance",
"durability": "persistent",
"val": {
"contract_instance": {
"executable": {
"wasm": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
},
"storage": null
}
}
}
},
"ext": "v0"
},
4095
]
],
[
{
"contract_code": {
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
}
},
[
{
"last_modified_ledger_seq": 0,
"data": {
"contract_code": {
"ext": "v0",
"hash": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
"code": ""
}
},
"ext": "v0"
},
4095
]
]
]
},
"events": [
{
"event": {
"ext": "v0",
"contract_id": null,
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "fn_call"
},
{
"bytes": "0000000000000000000000000000000000000000000000000000000000000001"
},
{
"symbol": "__constructor"
}
],
"data": "void"
}
}
},
"failed_call": false
},
{
"event": {
"ext": "v0",
"contract_id": null,
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "fn_call"
},
{
"bytes": "0000000000000000000000000000000000000000000000000000000000000001"
},
{
"symbol": "f"
}
],
"data": {
"u32": 1
}
}
}
},
"failed_call": false
},
{
"event": {
"ext": "v0",
"contract_id": "0000000000000000000000000000000000000000000000000000000000000001",
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "fn_return"
},
{
"symbol": "f"
}
],
"data": {
"error": {
"contract": 1
}
}
}
}
},
"failed_call": true
},
{
"event": {
"ext": "v0",
"contract_id": "0000000000000000000000000000000000000000000000000000000000000001",
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "error"
},
{
"error": {
"contract": 1
}
}
],
"data": {
"string": "escalating Ok(ScErrorType::Contract) frame-exit to Err"
}
}
}
},
"failed_call": true
},
{
"event": {
"ext": "v0",
"contract_id": null,
"type_": "diagnostic",
"body": {
"v0": {
"topics": [
{
"symbol": "error"
},
{
"error": {
"contract": 1
}
}
],
"data": {
"vec": [
{
"string": "contract try_call failed"
},
{
"symbol": "f"
},
{
"vec": [
{
"u32": 1
}
]
}
]
}
}
}
},
"failed_call": false
}
]
}
Loading

0 comments on commit 627e164

Please sign in to comment.