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: bounds for ser/de derive and schema_params for schema derive attributes #180

Merged
merged 9 commits into from
Jul 22, 2023

Conversation

dj8yfo
Copy link
Collaborator

@dj8yfo dj8yfo commented Jul 14, 2023

This is logical continuation/conclusion of #178 .

Pr adds possibility to override bounds for BorshSerialize and BorshDeserialize in order to allow not to keep
bounds on type parameters in struct definition itself and fix complex cases, when derive hasn't figured
the right bounds on type parameters automatically, while still not having to implement the traits manually.

/// `T: PartialOrd` is injected into derived trait implementation via field bound to avoid having this restriction on
/// the struct itself
#[cfg(hash_collections)]
#[derive(BorshSerialize)]
struct C1<T, U> {
    a: String,
    #[borsh(bound(serialize = 
        "T: borsh::ser::BorshSerialize + PartialOrd,
         U: borsh::ser::BorshSerialize"))]
    b: HashMap<T, U>,
}

/// `T: PartialOrd + Hash + Eq` is injected into derived trait implementation via field bound to avoid having this restriction on
/// the struct itself
#[allow(unused)]
#[cfg(hash_collections)]
#[derive(BorshDeserialize)]
struct C2<T, U> {
    a: String,
    #[borsh(bound(
        deserialize = 
        "T: PartialOrd + Hash + Eq + borsh::de::BorshDeserialize,
         U: borsh::de::BorshDeserialize"
    ))]
    b: HashMap<T, U>,
}

/// implicit derived `core::default::Default` bounds on `K` and `V` are dropped by empty bound
/// specified, as `HashMap` has its own `Default` implementation
#[cfg(hash_collections)]
#[derive(BorshDeserialize)]
struct G3<K, V, U>(
    #[borsh_skip]
    #[borsh(bound(deserialize = ""))]
    HashMap<K, V>,
    U,
);
// derive here figures the bound erroneously as `T: borsh::ser::BorshSerialize`
#[derive(BorshSerialize)]
struct ParametrizedWronDerive<T, V>
where
    T: TraitName,
{
    #[borsh(bound(serialize = "<T as TraitName>::Associated: borsh::ser::BorshSerialize"))]
    field: <T as TraitName>::Associated,
    another: V,
}

Pr also adds #[borsh(schema(params = ...))] attribute, which may be populated by a list of entries. order_param => override_type.
Such an entry instructs BorshSchema derive to:

  1. add override_type to types, bounded by borsh::BorshSchema in implementation block.
  2. add <override_type>::declaration() to parameters vector in fn declaration() method of BorshSchema trait that is being derived.
  3. the order_param is required to establish the same order in parameters vector (2.) as that of type parameters in generics of type, that BorshSchema is derived for.
  4. entries, specified for a field, together replace whatever would've been derived automatically for 1. and 2. .
    // derive here figures the bound erroneously as `T: borsh::BorshSchema`
    // `#[borsh(schema(params = "..."))]` attribute replaces it with <T as TraitName>::Associated: borsh::BorshSchema`
    #[derive(borsh::BorshSchema)]
    struct Parametrized<V, T>
    where
        T: TraitName,
    {
        #[borsh(schema(params = "T => <T as TraitName>::Associated"))]
        field: <T as TraitName>::Associated,
        another: V,
    }

@dj8yfo dj8yfo force-pushed the bound_and_schema_params_attributes branch 2 times, most recently from 7377ea2 to 1f1a8f4 Compare July 14, 2023 12:29
C: borsh::BorshSchema,
{
fn declaration() -> borsh::schema::Declaration {
let params = borsh::__private::maybestd::vec![< C > ::declaration()];
let params = borsh::__private::maybestd::vec![
< U > ::declaration(), < C > ::declaration()
Copy link
Collaborator Author

@dj8yfo dj8yfo Jul 14, 2023

Choose a reason for hiding this comment

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

the order has been changed to follow not alphabetic order of types transcription, but
the original order of generics declaration in this commit (as compared to master the pr is being merged on)

dj8yf0μl added 5 commits July 14, 2023 16:03
…structs

[✓] cleaning of traits of generics for inner structs
├── [✓] remove cleaning of associated bounds on params in impl block
└── [✓] implement filtering of where predicates used for not-skipped params (they were removed altogether previously)
@dj8yfo dj8yfo force-pushed the bound_and_schema_params_attributes branch from e778d2f to f9ff26e Compare July 17, 2023 11:17
@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Jul 17, 2023

Despite work in this commit on filtering relevant WherePredicate-s,
lifetimes in generics aren't filtered according to their actual usage in enum variants, so following example doesn't work:

fn test_enum_with_lifetimes() {
    
    #[allow(unused)]
    #[derive(borsh::BorshSchema)]
    enum EnumParametrized<'a, 'b, T, V>
    {
        A {
            x: BTreeMap<u16, &'a T>,
        },
        B(&'b V, u16),
    }
}
error[E0392]: parameter `'b` is never used
   --> borsh/tests/test_schema_enums.rs:359:31
    |
359 |     enum EnumParametrized<'a, 'b, T, V>
    |                               ^^ unused parameter
    |
    = help: consider removing `'b`, referring to it in a field, or using a marker such as `PhantomData`
                                                                                                                                                                                                                      
error[E0392]: parameter `'a` is never used
   --> borsh/tests/test_schema_enums.rs:359:27
    |
359 |     enum EnumParametrized<'a, 'b, T, V>
    |                           ^^ unused parameter
    |
    = help: consider removing `'a`, referring to it in a field, or using a marker such as `PhantomData`                                                                                                                                                                                                                      

One way to amend this might be to add yet one more attribute to hint relevant generics to derive:

#[test]
fn test_enum_with_lifetimes() {
    
    #[allow(unused)]
    #[derive(borsh::BorshSchema)]
    enum EnumParametrized<'a, 'b, T, V>
    {
        #[borsh(schema(variant_generics = "'a, T"))]
        A {
            x: BTreeMap<u16, &'a T>,
        },
        #[borsh(schema(variant_generics = "'b, V"))]
        B(&'b V, u16),
    }
}

Leaving it as is as of now (not adding borsh(schema(variant_generics = "..."))) as borsh-v0.11.0 isn't able
to handle this case with matching lifetimes against variants either.

 1  error[E0392]: parameter `'b` is never used
    --> borsh/tests/test_schema_enums.rs:218:31
 2  error[E0392]: parameter `'a` is never used
    --> borsh/tests/test_schema_enums.rs:218:27

@dj8yfo dj8yfo force-pushed the bound_and_schema_params_attributes branch 7 times, most recently from 5aff41e to c1d6383 Compare July 17, 2023 18:03
@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Jul 17, 2023

@iho the following files have been made identical, these contain some functionality shared between borsh-derive-internal and borsh-schema-derive-internal crates. When the unsplit of borsh-derive repo is done, only one copy of each file from borsh-derive-internal is expected to stay.

  • unified files:
    • ./borsh-derive-internal/src/generics.rs <=> ./borsh-schema-derive-internal/src/generics.rs
    • ./borsh-derive-internal/src/attribute_helpers.rs => (cut out tests in order not to duplicate snapshots) ./borsh-schema-derive-internal/src/attribute_helpers.rs
    • ./borsh-derive-internal/src/attribute_helpers/parsing_helpers.rs <=> ./borsh-schema-derive-internal/src/attribute_helpers/parsing_helpers.rs

@dj8yfo dj8yfo force-pushed the bound_and_schema_params_attributes branch 6 times, most recently from d90216f to 8368ed1 Compare July 18, 2023 12:40
@dj8yfo dj8yfo marked this pull request as ready for review July 18, 2023 12:47
@dj8yfo dj8yfo requested a review from frol as a code owner July 18, 2023 12:47
@dj8yfo dj8yfo force-pushed the bound_and_schema_params_attributes branch 3 times, most recently from be69381 to 34c77a8 Compare July 20, 2023 07:41
@dj8yfo dj8yfo force-pushed the bound_and_schema_params_attributes branch from 34c77a8 to 6fca9f0 Compare July 20, 2023 11:28
@frol frol merged commit 0959bee into near:master Jul 22, 2023
7 checks passed
@dj8yfo dj8yfo mentioned this pull request Aug 2, 2023
@frol frol mentioned this pull request Aug 9, 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.

2 participants