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

fix(derive): require Default trait bound for the skipped field type, rather than for its type parameters #340

Closed
wants to merge 6 commits into from

Conversation

DanikVitek
Copy link

Fixes #339

@DanikVitek
Copy link
Author

Now that I think about it, it seems, that the same change could be made for the requirements of BorshDeserialze/BorshSerialize traits themselves. Meaning, that these traits should be required not for the type parameters, but for the field types. Because, once again, some types might implement these traits manually and not need the trait bounds for the generic types. Akin PhantomData<T>

@dj8yfo
Copy link
Collaborator

dj8yfo commented Feb 10, 2025

change could be made for the requirements of BorshDeserialze/BorshSerialize traits themselves. Meaning, that these traits should be required not for the type parameters, but for the field types.

It used to be so one day in borsh, but then derive for recursive types didn't work

@dj8yfo
Copy link
Collaborator

dj8yfo commented Feb 10, 2025

I'm declining this now until we finish with #337, which is a pretty big chunk all by itself.
We can reopen this and finish this pr when #337 is merged+released.

Current approach (copied from serde) with bounds on type parameters is simple to reason about, as it's final (you see the final bounds on actual type parameters, there's no complex logic and assumtions of what compiler might infer for OpaqueType<T, V> : Trait). Escape hatch with bounds has been provided and anyone who might've had problems with it have probably already resolved it with bounds, so it's not like they depend on this patch right now.

Though i don't foresee any problems doing it for BorshDeserialize + Default combination, but a little more thought has to be given to potential problematic cases, if they can arise.

Please adjust pr #337 to not depend on this in the case if you've already done otherwise.

@dj8yfo dj8yfo closed this Feb 10, 2025
@dj8yfo
Copy link
Collaborator

dj8yfo commented Feb 10, 2025

The pr didn't have a docs update on how the derived bounds would behave now, and it clearly lacked a primer of various non-default cases how the compiler will behave, e.g. when OpaqueStruct<T, V> requires some bounds (not Default trait) in its Default implementation.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Feb 11, 2025

@DanikVitek i've thought about this a bit. I guess the optimal implementation would be to:

  1. still place Default bounds on type parameters themselves, not OpaqueFieldType<T1, T2, T3>. This way it would be still easy to see in expansion and to figure out the actual bounds on input type parameters without diving into code of OpaqueFieldType and possibly other types in a chain -> OpaqueType2 -> OpaqueType3 (i mean, if everyone followed this pattern of deferring figuring out bounds to compiler).
  2. add mentioned std collections to exceptions, where no Default bounds will be added to their type parameters: HashMap, Vec, Option, BTreeMap. This would be a syntactic distinction, so anyone reusing these names for their non-std types would encounter the same behaviour. Add documentation about these exceptions to Bounds anchor of BorshDeserialize rustdoc page.
  3. current behaviour is consistent with what compiler derives for new types, so at least there will be no skew here:
#[derive(Default)]
struct A<X, Y, Z> {
    x: X,
    y: Y,
    z: Z,
}
#[automatically_derived]
impl<
    X: ::core::default::Default,
    Y: ::core::default::Default,
    Z: ::core::default::Default,
> ::core::default::Default for A<X, Y, Z> {
    #[inline]
    fn default() -> A<X, Y, Z> {
        A {
            x: ::core::default::Default::default(),
            y: ::core::default::Default::default(),
            z: ::core::default::Default::default(),
        }
    }
}

@DanikVitek
Copy link
Author

But then, in case if the type is not standard and is just similarly-named, the bound escape hatch should still work.

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.

#[borsh(skip)] should ask for Default implementation on the field's type, rather than its type parameters
2 participants