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: derive * for recursive structures #178

Merged
merged 10 commits into from
Jul 13, 2023

Conversation

dj8yfo
Copy link
Collaborator

@dj8yfo dj8yfo commented Jul 5, 2023

Resolves #7

@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch 3 times, most recently from 22c3430 to 8c53a8d Compare July 5, 2023 19:28
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch 3 times, most recently from c776a5e to 782a85f Compare July 6, 2023 11:38
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch from 782a85f to 57d8f4f Compare July 6, 2023 12:17
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch from 02235b8 to 0c16f65 Compare July 6, 2023 17:25
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch 2 times, most recently from ce536d1 to f88a3a8 Compare July 6, 2023 19:11
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch 2 times, most recently from a5c34c9 to a369169 Compare July 7, 2023 11:42
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch from 2e6d815 to 40f45d1 Compare July 7, 2023 13:04
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch 13 times, most recently from fe7a701 to a1be496 Compare July 10, 2023 19:17
@dj8yfo dj8yfo marked this pull request as ready for review July 10, 2023 19:23
borsh-derive/src/lib.rs Outdated Show resolved Hide resolved
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch 2 times, most recently from 8a912cd to 32b6ae7 Compare July 11, 2023 11:16
@dj8yfo dj8yfo force-pushed the derive_recursive_structures branch from 4c2a078 to 9d445e9 Compare July 11, 2023 18:57
field: T::Associated,
another: V,
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

serde's FindTyParams code isn't perfect.
In commented example it does the right thing.
In a case, which is synonymous:

struct Parametrized<T, V> where T: TraitName {

    field: <T as TraitName>::Associated,
    another: V,     
}

it produces following expansion

impl<T, V> borsh::ser::BorshSerialize for Parametrized<T, V>
where
    T: TraitName,
    T: borsh::ser::BorshSerialize,
    V: borsh::ser::BorshSerialize,
{
...

which is an error

error[E0277]: the trait bound `<T as TraitName>::Associated: BorshSerialize` is not satisfied
  --> src/main.rs:28:10
   |
28 | #[derive(BorshSerialize)]
   |          ^^^^^^^^^^^^^^ the trait `BorshSerialize` is not implemented for `<T as TraitName>::Associated`
   |
   = note: this error originates in the derive macro `BorshSerialize` (in Nightly builds, run with -Z macro-backtrace for more info)
                                                                                                                                                                                                                                                                            
For more information about this error, try `rustc --explain E0277`.

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.

This is sic!

P.S. I feel we should have killed borsh-schema earlier, so you would not need to spend so much effort implementing it...


#[cfg_attr(
feature = "force_exhaustive_checks",
deny(non_exhaustive_omitted_patterns)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this should be the default configuration for the module of even crate, and explicitly opted-out (allowed) whenever necessary. What do you think?

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.

i think that it's so in selene's code (where i borrowed it from) due to convention to specify exact nightly features that are going to be used
(probably because some combinations of them can interact/bug in weird ways, and it's hard to test all possible combinations in nightly; like there's more or less only one stable compiler for a version, and there're infinite number of nightly compilers ) ).
My suggestion, is when feature(non_exhaustive_omitted_patterns_lint) is stabilized, then all of #[cfg_attr(...)] relevant to it can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the attribute is removed now, then it becomes less apparent, that the feature is not stable yet.

@frol frol merged commit dc1b0aa into near:master Jul 13, 2023
6 checks passed
@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Jul 14, 2023

This is sic!

P.S. I feel we should have killed borsh-schema earlier, so you would not need to spend so much effort implementing it...

Well, at least it has some unique quirks, that are more interesting when tried to be implemented than copy-and-paste of functionality from serde )

@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.

Failed to derive borsh ser/de in a recursive data structure
2 participants