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

feat!: add DiscriminantValue to Definition::Enum::variants tuples #232

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

dj8yfo
Copy link
Collaborator

@dj8yfo dj8yfo commented Sep 25, 2023

resolves #230

@dj8yfo dj8yfo force-pushed the discriminant_num_schema branch 3 times, most recently from 79eb434 to 2c1aa85 Compare September 25, 2023 17:54
to be more congruent with `BorshSerialize`, `BorshDeserialize`
@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Sep 25, 2023

this commit was added to restrict possible values of discriminants to u8 range for BorshSchema to be congruent with what BorshSerialize, BorshDeserialize derives do.


with it:

    #[derive(BorshSchema)]
    #[borsh(use_discriminant = true)]
    enum XYa {
        A,
        B = discrim(),
        C,
        D,
        E = -100,
        F,
    }

    const fn discrim() -> isize {
        0x14
    }
//     |
// 277 |     #[derive(BorshSchema)]
//     |              ----------- expected due to this
// ...
// 281 |         B = discrim(),
//     |             ^^^^^^^^^ expected `u8`, found `isize`
//     |
// help: you can convert an `isize` to a `u8` and panic if the converted value doesn't fit
// 4  error[E0277]: the trait bound `u8: Neg` is not satisfied
//    --> borsh/tests/test_enum_discriminants.rs:284:13
//     |
// 284 |         E = -100,
//     |             ^^^^ the trait `Neg` is not implemented for `u8`
//     |
//     = help: the following other types implement trait `Neg`:

Without it:

    #[derive(BorshSchema)]
    #[borsh(use_discriminant = true)]
    enum XYa {
        A,
        B = discrim(),
        C,
        D,
        E = -100,
        F,
    }

    const fn discrim() -> isize {
        0x14
    }
//  1  error[E0308]: mismatched types
//     --> borsh/tests/test_enum_discriminants.rs:281:13
//      |
//  281 |         B = discrim(),
//      |             ^^^^^^^^^ expected `i64`, found `isize`
//      |
//  help: you can convert an `isize` to an `i64` and panic if the converted value doesn't fit

but

    #[derive(BorshSchema)]
    #[borsh(use_discriminant = true)]
    #[repr(i64)]
    enum XYa {
        A,
        B = discrim(),
        C,
        D,
        E = -100,
        F,
    }

    const fn discrim() -> i64 {
        0x14
    }
// OK

@dj8yfo dj8yfo marked this pull request as ready for review September 25, 2023 20:41
@dj8yfo dj8yfo requested a review from frol as a code owner September 25, 2023 20:41
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for solidifying it!

@dj8yfo dj8yfo merged commit 2a13d3a into master Sep 26, 2023
7 checks passed
@dj8yfo dj8yfo deleted the discriminant_num_schema branch September 26, 2023 17:12
@frol frol mentioned this pull request Sep 23, 2023
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

Successfully merging this pull request may close these issues.

Include discriminant number in BorshSchema::Enum::variants
2 participants