Skip to content

Commit

Permalink
Keep testutils on/off state internal to the sdk (#1301)
Browse files Browse the repository at this point in the history 
### What
Change how testutils code is generated to make it rely only on the state
of the SDK testutils feature, and not influenced at all by the contract
crate's feature set or test cfg.

Today the SDK macros generate both non-testutils and testutils code and
it is up to the contract crate to enable or disable it. With this change
the SDK macros only generate testutils code if the SDK is compiled with
testutils.

### Why
Today both the SDK and the contract crate must be configured with the
testutils feature (or the test cfg) to enable the testutils that are
generated by the SDK macros.

The SDK has a testutils feature that enables the testutils functions and
types in the SDK.

The contract crate generated code also has code behind a testutils
feature, and the contract crate testutils feature (or test cfg) must be
enabled for them to function as well.

This largely works, but it results in an oddity in more complex
workspace setups that you can be using a testutils SDK with a
non-testutils compiled contract crate meaning that none of the types or
functions in that crate can be used with testutils functions.

This largely goes unnoticed because it is a bit of an edge case and
definitely not apparent in simpler setups. Even larger setups may never
notice this as an oddity if they always happen to configure their crates
appropriately.

This also goes unnoticed because when the test cfg is enabled (when
building for tests) we also enable all that testutils generated code.

This issue has become more apparent to me lately because I'm
experimenting with Soroban contracts inside Jupyter notebooks, as one
way that folks can experiment and learn Soroban.

The Rust runtime kernel for Jupyter notebooks doesn't currently support
enable features for code in the notebook so I end up with an odd
situation where testutils are enabled in the SDK, but not in the
contract.

After looking at how to solve this problem it occurred to me that the
way we implemented the generated testutils code is not ideal, even
independent of this problem.

Instead of implementing the generated code so it contains a new
testutils feature for the contract crate, we should have made all the
generated testutils code enabled/disabled based on the SDKs feature
flag.

The advantage of this is that in any workspace whenever you have
testutils enabled on the SDK, all contracts will also have testutils
enabled. Instead of having many levers to pull to get testutils enabled,
there is just one.

### Reviewing

I recommend reviewing with whitespace changes disabled, because a large
portion of the diff is the change to indents when some code was moved
inside if..else statements.

### Merging

This change _should_ be practically non-breaking. While technically it
is removing code from being generated when compiled with an SDK without
testutils enabled, most of the testutils code in generated code would be
unusable without it anyway.

It might be warranted to hold this change until v22, just for the
purpose of shipping it in a preview / rc and getting feedback before
committing to it. But I think the change is also pretty straightforward
and low risk.

I'd like to merge this in a minor release, but it warrants some
discussion.
  • Loading branch information
@leighmcculloch
leighmcculloch authored Jul 18, 2024
1 parent 1f10e9a commit cd541d0
Show file tree
Hide file tree
Showing 8 changed files with 440 additions and 415 deletions.
4 changes: 0 additions & 4 deletions soroban-sdk-macros/src/arbitrary.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,15 +312,11 @@ fn quote_arbitrary(
arbitrary_type_decl: TokenStream2,
arbitrary_ctor: TokenStream2,
) -> TokenStream2 {
if !cfg!(any(test, feature = "testutils")) {
return quote!();
}
quote! {
// This allows us to create a scope to import std and arbitrary, while
// also keeping everything from the current scope. This is better than a
// module because: modules inside functions have surprisingly
// inconsistent scoping rules and visibility management is harder.
#[cfg(any(test, feature = "testutils"))]
const _: () = {
// derive(Arbitrary) expects these two to be in scope
use #path::testutils::arbitrary::std;
Expand Down
353 changes: 189 additions & 164 deletions soroban-sdk-macros/src/derive_client.rs
View file

Large diffs are not rendered by default.

121 changes: 60 additions & 61 deletions soroban-sdk-macros/src/derive_enum.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,8 @@ pub fn derive_type_enum(
None
};

let arbitrary_tokens = crate::arbitrary::derive_arbitrary_enum(path, vis, enum_ident, data);

// Output.
quote! {
let mut output = quote! {
#spec_gen

impl #path::TryFromVal<#path::Env, #path::Val> for #enum_ident {
Expand Down Expand Up @@ -194,81 +192,82 @@ pub fn derive_type_enum(
}
}
}
};

#[cfg(any(test, feature = "testutils"))]
impl #path::TryFromVal<#path::Env, #path::xdr::ScVec> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from_val(env: &#path::Env, val: &#path::xdr::ScVec) -> Result<Self, #path::xdr::Error> {
use #path::xdr::Validate;
use #path::TryIntoVal;
// Additional output when testutils are enabled.
if cfg!(feature = "testutils") {
let arbitrary_tokens = crate::arbitrary::derive_arbitrary_enum(path, vis, enum_ident, data);
output.extend(quote! {
impl #path::TryFromVal<#path::Env, #path::xdr::ScVec> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from_val(env: &#path::Env, val: &#path::xdr::ScVec) -> Result<Self, #path::xdr::Error> {
use #path::xdr::Validate;
use #path::TryIntoVal;

let vec = val;
let mut iter = vec.iter();
let discriminant: #path::xdr::ScSymbol = iter.next().ok_or(#path::xdr::Error::Invalid)?.clone().try_into().map_err(|_| #path::xdr::Error::Invalid)?;
let discriminant_name: &str = &discriminant.to_utf8_string()?;
let vec = val;
let mut iter = vec.iter();
let discriminant: #path::xdr::ScSymbol = iter.next().ok_or(#path::xdr::Error::Invalid)?.clone().try_into().map_err(|_| #path::xdr::Error::Invalid)?;
let discriminant_name: &str = &discriminant.to_utf8_string()?;

Ok(match discriminant_name {
#(#try_from_xdrs,)*
_ => Err(#path::xdr::Error::Invalid)?,
})
Ok(match discriminant_name {
#(#try_from_xdrs,)*
_ => Err(#path::xdr::Error::Invalid)?,
})
}
}
}

#[cfg(any(test, feature = "testutils"))]
impl #path::TryFromVal<#path::Env, #path::xdr::ScVal> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from_val(env: &#path::Env, val: &#path::xdr::ScVal) -> Result<Self, #path::xdr::Error> {
if let #path::xdr::ScVal::Vec(Some(vec)) = val {
<_ as #path::TryFromVal<_, _>>::try_from_val(env, vec)
} else {
Err(#path::xdr::Error::Invalid)
impl #path::TryFromVal<#path::Env, #path::xdr::ScVal> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from_val(env: &#path::Env, val: &#path::xdr::ScVal) -> Result<Self, #path::xdr::Error> {
if let #path::xdr::ScVal::Vec(Some(vec)) = val {
<_ as #path::TryFromVal<_, _>>::try_from_val(env, vec)
} else {
Err(#path::xdr::Error::Invalid)
}
}
}
}

#[cfg(any(test, feature = "testutils"))]
impl TryFrom<&#enum_ident> for #path::xdr::ScVec {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: &#enum_ident) -> Result<Self, #path::xdr::Error> {
extern crate alloc;
Ok(match val {
#(#into_xdrs,)*
})
impl TryFrom<&#enum_ident> for #path::xdr::ScVec {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: &#enum_ident) -> Result<Self, #path::xdr::Error> {
extern crate alloc;
Ok(match val {
#(#into_xdrs,)*
})
}
}
}

#[cfg(any(test, feature = "testutils"))]
impl TryFrom<#enum_ident> for #path::xdr::ScVec {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: #enum_ident) -> Result<Self, #path::xdr::Error> {
(&val).try_into()
impl TryFrom<#enum_ident> for #path::xdr::ScVec {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: #enum_ident) -> Result<Self, #path::xdr::Error> {
(&val).try_into()
}
}
}

#[cfg(any(test, feature = "testutils"))]
impl TryFrom<&#enum_ident> for #path::xdr::ScVal {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: &#enum_ident) -> Result<Self, #path::xdr::Error> {
Ok(#path::xdr::ScVal::Vec(Some(val.try_into()?)))
impl TryFrom<&#enum_ident> for #path::xdr::ScVal {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: &#enum_ident) -> Result<Self, #path::xdr::Error> {
Ok(#path::xdr::ScVal::Vec(Some(val.try_into()?)))
}
}
}

#[cfg(any(test, feature = "testutils"))]
impl TryFrom<#enum_ident> for #path::xdr::ScVal {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: #enum_ident) -> Result<Self, #path::xdr::Error> {
(&val).try_into()
impl TryFrom<#enum_ident> for #path::xdr::ScVal {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from(val: #enum_ident) -> Result<Self, #path::xdr::Error> {
(&val).try_into()
}
}
}

#arbitrary_tokens
#arbitrary_tokens
});
}
output
}

struct VariantTokens {
Expand Down
65 changes: 34 additions & 31 deletions soroban-sdk-macros/src/derive_enum_int.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,8 @@ pub fn derive_type_enum_int(
None
};

let arbitrary_tokens = crate::arbitrary::derive_arbitrary_enum_int(path, vis, enum_ident, data);

// Output.
quote! {
let mut output = quote! {
#spec_gen
const _: () = { fn assert_copy(v: #enum_ident) -> impl Copy { v } };

Expand All @@ -120,41 +118,46 @@ pub fn derive_type_enum_int(
})
}
}
};

#[cfg(any(test, feature = "testutils"))]
impl #path::TryFromVal<#path::Env, #path::xdr::ScVal> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from_val(env: &#path::Env, val: &#path::xdr::ScVal) -> Result<Self, #path::xdr::Error> {
if let #path::xdr::ScVal::U32(discriminant) = val {
Ok(match *discriminant {
#(#try_froms,)*
_ => Err(#path::xdr::Error::Invalid)?,
})
} else {
Err(#path::xdr::Error::Invalid)
// Additional output when testutils are enabled.
if cfg!(feature = "testutils") {
let arbitrary_tokens =
crate::arbitrary::derive_arbitrary_enum_int(path, vis, enum_ident, data);
output.extend(quote! {
impl #path::TryFromVal<#path::Env, #path::xdr::ScVal> for #enum_ident {
type Error = #path::xdr::Error;
#[inline(always)]
fn try_from_val(env: &#path::Env, val: &#path::xdr::ScVal) -> Result<Self, #path::xdr::Error> {
if let #path::xdr::ScVal::U32(discriminant) = val {
Ok(match *discriminant {
#(#try_froms,)*
_ => Err(#path::xdr::Error::Invalid)?,
})
} else {
Err(#path::xdr::Error::Invalid)
}
}
}
}

#[cfg(any(test, feature = "testutils"))]
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())
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())
}
}
}

#[cfg(any(test, feature = "testutils"))]
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())
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())
}
}
}

#arbitrary_tokens
#arbitrary_tokens
});
}
output
}
1 change: 0 additions & 1 deletion soroban-sdk-macros/src/derive_fn.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ pub fn derive_contract_function_registration_ctor<'a>(
let ctor_ident = format_ident!("__{ty_str}_{trait_str}_{methods_hash}_ctor");

quote! {
#[cfg(any(test, feature = "testutils"))]
#[doc(hidden)]
#[#crate_path::reexports_for_macros::ctor::ctor]
fn #ctor_ident() {
Expand Down
Loading

0 comments on commit cd541d0

Please sign in to comment.