From 627e1643cabb49ec2d2648937a4d2a5e5c2cab3b Mon Sep 17 00:00:00 2001 From: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com> Date: Fri, 27 Sep 2024 10:49:49 +1000 Subject: [PATCH] Remove need for Copy on error enums (#1292) ### 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 https://github.com/stellar/rs-soroban-sdk/pull/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 --- soroban-sdk-macros/src/derive_enum_int.rs | 9 +- .../src/derive_error_enum_int.rs | 17 +- soroban-sdk/src/tests.rs | 1 + .../src/tests/contract_udt_enum_error.rs | 57 +++++ .../src/tests/contractimport_with_error.rs | 4 +- .../contract_udt_enum_error/test_error.1.json | 216 ++++++++++++++++++ .../test_success.1.json | 148 ++++++++++++ tests/errors/src/lib.rs | 41 ++-- 8 files changed, 464 insertions(+), 29 deletions(-) create mode 100644 soroban-sdk/src/tests/contract_udt_enum_error.rs create mode 100644 soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_error.1.json create mode 100644 soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_success.1.json diff --git a/soroban-sdk-macros/src/derive_enum_int.rs b/soroban-sdk-macros/src/derive_enum_int.rs index 6cc326c16..27e68b01a 100644 --- a/soroban-sdk-macros/src/derive_enum_int.rs +++ b/soroban-sdk-macros/src/derive_enum_int.rs @@ -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; @@ -144,7 +143,9 @@ 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,)* + }) } } @@ -152,7 +153,9 @@ 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,)* + }) } } diff --git a/soroban-sdk-macros/src/derive_error_enum_int.rs b/soroban-sdk-macros/src/derive_error_enum_int.rs index bfa45f301..fd1dd9016 100644 --- a/soroban-sdk-macros/src/derive_error_enum_int.rs +++ b/soroban-sdk-macros/src/derive_error_enum_int.rs @@ -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; @@ -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,)* + } } } @@ -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,)* + } } } diff --git a/soroban-sdk/src/tests.rs b/soroban-sdk/src/tests.rs index 6cb330d43..468ee5111 100644 --- a/soroban-sdk/src/tests.rs +++ b/soroban-sdk/src/tests.rs @@ -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; diff --git a/soroban-sdk/src/tests/contract_udt_enum_error.rs b/soroban-sdk/src/tests/contract_udt_enum_error.rs new file mode 100644 index 000000000..fbe61034f --- /dev/null +++ b/soroban-sdk/src/tests/contract_udt_enum_error.rs @@ -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"); + }; +} diff --git a/soroban-sdk/src/tests/contractimport_with_error.rs b/soroban-sdk/src/tests/contractimport_with_error.rs index 13ab9e69b..fba3aa1f3 100644 --- a/soroban-sdk/src/tests/contractimport_with_error.rs +++ b/soroban-sdk/src/tests/contractimport_with_error.rs @@ -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) } } @@ -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")); } diff --git a/soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_error.1.json b/soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_error.1.json new file mode 100644 index 000000000..4d0c8fda5 --- /dev/null +++ b/soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_error.1.json @@ -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 + } + ] +} \ No newline at end of file diff --git a/soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_success.1.json b/soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_success.1.json new file mode 100644 index 000000000..17ed13020 --- /dev/null +++ b/soroban-sdk/test_snapshots/tests/contract_udt_enum_error/test_success.1.json @@ -0,0 +1,148 @@ +{ + "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": 0 + } + } + } + }, + "failed_call": false + }, + { + "event": { + "ext": "v0", + "contract_id": "0000000000000000000000000000000000000000000000000000000000000001", + "type_": "diagnostic", + "body": { + "v0": { + "topics": [ + { + "symbol": "fn_return" + }, + { + "symbol": "f" + } + ], + "data": "void" + } + } + }, + "failed_call": false + } + ] +} \ No newline at end of file diff --git a/tests/errors/src/lib.rs b/tests/errors/src/lib.rs index dd18ece77..a18757674 100644 --- a/tests/errors/src/lib.rs +++ b/tests/errors/src/lib.rs @@ -1,32 +1,43 @@ #![no_std] use soroban_sdk::{ - contract, contracterror, contractimpl, panic_with_error, symbol_short, Env, Symbol, + contract, contracterror, contractimpl, contracttype, panic_with_error, symbol_short, Env, + Symbol, }; #[contract] pub struct Contract; +#[contracttype] +#[derive(PartialEq)] +pub enum Flag { + A = 0, + B = 1, + C = 2, + D = 3, + E = 4, +} + #[contracterror] -#[derive(Copy, Clone, Debug, Eq, PartialEq)] +#[derive(Debug, Eq, PartialEq)] pub enum Error { AnError = 1, } #[contractimpl] impl Contract { - pub fn hello(env: Env, flag: u32) -> Result { + pub fn hello(env: Env, flag: Flag) -> Result { env.storage() .persistent() .set(&symbol_short!("persisted"), &true); - if flag == 0 { + if flag == Flag::A { Ok(symbol_short!("hello")) - } else if flag == 1 { + } else if flag == Flag::B { Err(Error::AnError) - } else if flag == 2 { + } else if flag == Flag::C { panic_with_error!(&env, Error::AnError) - } else if flag == 3 { + } else if flag == Flag::D { panic!("an error") - } else if flag == 4 { + } else if flag == Flag::E { panic_with_error!(&env, soroban_sdk::Error::from_contract_error(9)) } else { unimplemented!() @@ -46,7 +57,7 @@ impl Contract { mod test { use soroban_sdk::{symbol_short, xdr, Env, InvokeError}; - use crate::{Contract, ContractClient, Error}; + use crate::{Contract, ContractClient, Error, Flag}; #[test] fn hello_ok() { @@ -54,7 +65,7 @@ mod test { let contract_id = e.register(Contract, ()); let client = ContractClient::new(&e, &contract_id); - let res = client.hello(&0); + let res = client.hello(&Flag::A); assert_eq!(res, symbol_short!("hello")); assert!(client.persisted()); } @@ -65,7 +76,7 @@ mod test { let contract_id = e.register(Contract, ()); let client = ContractClient::new(&e, &contract_id); - let res = client.try_hello(&0); + let res = client.try_hello(&Flag::A); assert_eq!(res, Ok(Ok(symbol_short!("hello")))); assert!(client.persisted()); } @@ -76,7 +87,7 @@ mod test { let contract_id = e.register(Contract, ()); let client = ContractClient::new(&e, &contract_id); - let res = client.try_hello(&1); + let res = client.try_hello(&Flag::B); assert_eq!(res, Err(Ok(Error::AnError))); assert!(!client.persisted()); } @@ -87,7 +98,7 @@ mod test { let contract_id = e.register(Contract, ()); let client = ContractClient::new(&e, &contract_id); - let res = client.try_hello(&2); + let res = client.try_hello(&Flag::C); assert_eq!(res, Err(Ok(Error::AnError))); assert!(!client.persisted()); } @@ -98,7 +109,7 @@ mod test { let contract_id = e.register(Contract, ()); let client = ContractClient::new(&e, &contract_id); - let res = client.try_hello(&3); + let res = client.try_hello(&Flag::D); assert_eq!(res, Err(Err(InvokeError::Abort))); assert!(!client.persisted()); } @@ -109,7 +120,7 @@ mod test { let contract_id = e.register(Contract, ()); let client = ContractClient::new(&e, &contract_id); - let res = client.try_hello(&4); + let res = client.try_hello(&Flag::E); assert_eq!(res, Err(Err(InvokeError::Contract(9)))); assert!(!client.persisted()); }