Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failure to build 1.4.1 on existing projects that were using 1.3.2 #208

Open
leighmcculloch opened this issue Nov 8, 2024 · 4 comments
Open

Comments

@leighmcculloch
Copy link

leighmcculloch commented Nov 8, 2024

Similar to #203, it seems like the 1.4.1 releases are still making assumptions about items in the current space that weren't prior, which is a breaking change, and fails to build on projects who were dependent on 1.3.2.

Cargo.toml:

[package]
name = "pkg"
edition = "2021"

[dependencies]
soroban-sdk = { version = "=21.7.6" }

[dev-dependencies]
soroban-sdk = { version = "=21.7.6", features = ["testutils"] }

src/lib.rs:

#![no_std]

use soroban_sdk::contracttype;

#[contracttype]
#[derive(Clone, Debug, PartialEq)]
pub struct Struct {
    pub index: u64,
}

Run cargo test with 1.4.1:

$ cargo update artbirary --precise 1.4.1
$ cargo update derive_artbirary --precise 1.4.1
$ cargo test

Included in the output are the following build errors:

error[E0433]: failed to resolve: could not find `std` in the list of imported crates
 --> src/lib.rs:5:1
  |
5 | #[contracttype]
  | ^^^^^^^^^^^^^^^ could not find `std` in the list of imported crates
  |
  = note: this error originates in the derive macro `soroban_sdk::testutils::arbitrary::arbitrary::Arbitrary` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0425]: cannot find value `RECURSIVE_COUNT_ArbitraryStruct` in this scope
 --> src/lib.rs:5:1
  |
5 | #[contracttype]
  | ^^^^^^^^^^^^^^^ not found in this scope
  |
  = note: this error originates in the derive macro `soroban_sdk::testutils::arbitrary::arbitrary::Arbitrary` (in Nightly builds, run with -Z macro-backtrace for more info)

Re-run cargo test with 1.3.2 and it compiles:

$ cargo update artbirary --precise 1.3.2
$ cargo update derive_artbirary --precise 1.3.2
$ cargo test
@leighmcculloch leighmcculloch changed the title Failure to build 1.4.0/1.4.1 on existing projects that were using 1.3.2 Failure to build 1.4.1 on existing projects that were using 1.3.2 Nov 8, 2024
@fitzgen
Copy link
Member

fitzgen commented Nov 8, 2024

Can you provide a standalone and minimal test case that reproduces the issue? That would be helpful in diagnosing and fixing the bug.

@Manishearth
Copy link
Member

I suspect it's the use of std::thread_local in the recursion counter.

I think folks should just #[cfg_attr(feature = "std", derive(Arbitrary))] or something (or #[cfg(feature = "arbitrary)] extern crate std;). I don't think this crate ever guaranteed that the derive works in no_std. We potentially could try to but I don't see a clean way of doing so without losing the recursion guard.

@Manishearth
Copy link
Member

Recursion guard was added in #109

@leighmcculloch
Copy link
Author

leighmcculloch commented Nov 8, 2024

Up until the 1.4 releases we've been able to support arbitrary in no_std crates by extern importing std into the space like so:

soroban-sdk-macros/src/arbitrary.rs:

        const _: () = {
            // derive(Arbitrary) expects these two to be in scope
            use #path::testutils::arbitrary::std;
            use #path::testutils::arbitrary::arbitrary;

            #[derive(#path::testutils::arbitrary::arbitrary::Arbitrary)]
            #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd)]
            #vis #arbitrary_type_decl

            impl #path::testutils::arbitrary::SorobanArbitrary for #ident {
                type Prototype = #arbitrary_type_ident;
            }

            impl #path::TryFromVal<#path::Env, #arbitrary_type_ident> for #ident {
                type Error = #path::ConversionError;
                fn try_from_val(env: &#path::Env, v: &#arbitrary_type_ident) -> std::result::Result<Self, Self::Error> {
                    Ok(#arbitrary_ctor)
                }
            }
        };

Ref: https://github.com/stellar/rs-soroban-sdk/blob/0c65e9c9866905808a18ff97ec54be08da986bc3/soroban-sdk-macros/src/arbitrary.rs#L320-L339

soroban-sdk/src/testutils/arbitrary.rs:

// Used by `contracttype`
#[doc(hidden)]
pub use std;

Ref: https://github.com/stellar/rs-soroban-sdk/blob/0c65e9c9866905808a18ff97ec54be08da986bc3/soroban-sdk/src/testutils/arbitrary.rs#L177-L179

It seems something has changed where I guess the arbitrary derive is now creating a new scope, that isn't seeing the std we are including into the derive scope.

I think this basically breaks arbitrary for any no_std crate, where it worked before as long as the crate imported std into the scope being derived into.

github-merge-queue bot pushed a commit to stellar/rs-soroban-sdk that referenced this issue Nov 12, 2024
### What
Limit the versions of arbitrary/derive_arbitrary to 1.3.x.

### Why
Arbitrary introduced changes into the 1.4.x releases that no longer
compiles with the use of arbitrary in the soroban-sdk:
- rust-fuzz/arbitrary#203
- rust-fuzz/arbitrary#208
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants