-
Notifications
You must be signed in to change notification settings - Fork 30
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
Derive TypeInfo for own types #72
base: master
Are you sure you want to change the base?
Conversation
Derive TypeInfo for all of scale-infos types
derive/src/lib.rs
Outdated
:: #scale_info ::meta_type::<#ty_ident>() | ||
#scale_info::meta_type::<#ty_ident>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as before for all non-scale-info
crates this should still prepend ::
to signal root crate path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't compile with the ::
and I don't know why tbh, but I think that when deriving for scale-info
itself, and we have extern crate self as _scale_info;
, then ::_scale_info::path::to::something
doesn't work. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extern crate self as _scale_info;
is not required since Rust edition 2018 and I don't think we should support Rust edition 2015 tbh because it actually resolves many problems we are facing otherwise such as this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So either this crate should produce: crate::
paths or whatever crate alias a dependency uses for scale-info
prepended with ::
. To be honest I am not sure how counterintuitive it is to use the derive macros for the crate that exposes them. I see a gain but issues like these let me feel it might be better to simply provide manual implementations for the few types in the re-exporting crate. Happy to learn a better approaches for this problem. In ink! we also use manual implementations for our derive macros in the root crates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more concrete: While the proc-macro-crate
crate is actually useful in these cases it makes use of file I/O while expanding a proc. macro. There are discussions about encapsulating proc. macros in stricter environments that would break proc. macros such as these. Generally tooling such as rust-analyzer
has good reasons why proc. macros that are "impure" should not be relied on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest I am not sure how counterintuitive it is to use the derive macros for the crate that exposes them.
How do you mean "counterintuitive"? I take it you mean it's forcing things a bit?
I think it's a good litmus test for the crate, especially since the type zoo in scale-info
is fairly advanced and gives us a "free test suite" of sorts. The amount of manual implementations we'd have to maintain is pretty large so if we can derive most of them that's a win imo.
I am concerned by the increased complexity this brings. I will see what I can do to make things a wee bit less messy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am concerned by the increased complexity this brings. I will see what I can do to make things a wee bit less messy.
I think this commit is makes things much cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proc. macros bring overhead with them. So some people prefer using crates but not their provided proc. macros which is why we find derive
crate feature on some crates to opt-out. It is just nice to be able to separate concerns between the crate that provides definitions and the crate that provides the proc. macro implementations. As soon as the crate providing definitions makes use of the proc. macro you no longer have the separation and a direct dependency instead. I personally am not against this but overhead can be serious in some cases.
The biggest problem is that derive macros that can be used today in their parent crate require this type of "hack" that is encapsulated by the proc-macro-crate
crate currently. Using this "hack" in any proc. macro turns the proc. macro impure. For example it can no longer enjoy acceleration by the techniques behind the watt
crate which some people believe to be the future compilation model for all proc. macros since it is super fast and more secure than today's proc. macros.
Don't get me wrong: I see the tradeoffs and see some of the pros of doing this. But I still consider this feature to be taking in some amount of technical depth into the project in order to have the anticipated feature implementation for which we will probably have to pay back in the future. Technical depth is death to all progress so I always am careful how much of it I accumulate in projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The use of
proc-macro-crate
predates this PR so the technical debt is already there; - This PR does not introduce a mandatory dependence on
scale-info-derive
: it's opt-in through a feature (as it should be, couldn't agree with you more);
I think we should have a conversation about the use of proc-macro-crate
; you make a really good point about it making the crate "impure" and we should hash out the trade-offs involved. But I'm not sure this PR is the right place for that?
Prepend `::` to paths when not deriving for `scale-info`. Alias self as `scale-info` Prefer unwrap_or_else to expect Add todo about broken features (break trybuild tests)
Move scale-info crate name construction to own function
@Robbepop @ascjones Do you have any thoughtsabout what to do about Lines 54 to 67 in 5a165b1
|
Hmmm it is a tricky one. Do we actually need to derive for it? It is only really used as generic parameter in a raw function signature type so maybe |
|
This seems to be stable. Is there a good reason to keep this open and eventually merge it or should we not pursue it further? |
I think we should merge it. It's not super useful in itself, but does provide a sanity check. Happy to update it; won't make a fuss if @ascjones feel it's not needed any longer. :) |
It's absolutely something we will need, it just wasn't on the critical path for substrate integration so I have neglected looking at it. Happy to give this a review once it is updated with |
We'd need to make extra sure that before/after semantics are equal with respect to the newly derived traits. |
Use case for this would be generating equivalent types in another language e.g. https://github.com/polkadot-js/api/blob/58cc197c890881db5a604a4ad98c2446a16e8631/packages/types/src/interfaces/scaleInfo/types.ts. |
@@ -40,6 +40,7 @@ jobs: | |||
- name: check-features | |||
run: | | |||
cargo check --no-default-features --features bit-vec | |||
cargo check --no-default-features --features dogfood |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep this optional crate feature? What purpose does it have?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good question. I fail to see the reason to keep this as a feature. @ascjones you ok making it non-optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw Giles's PR with this stuff in and fwiw I think it doesn't need to be behind a feature flag.
derive/src/lib.rs
Outdated
// Err(e) => return Err(syn::Error::new(Span::call_site(), e)), | ||
// }; | ||
// Ok(crate_ident) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't entirely sure which version of this function to use so went with master. If that's fine we can delete this commented out version.
Have just brought this up to date as realised I might need this to decode metadata in the browser in rust. |
* support for frame-metadata to have TypeInfo
Builds upon/supersedes #21
Derive
TypeInfo
for all ofscale-info
s own types.