Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Derive TypeInfo for own types #72
Changes from 1 commit
84b3bb4
c2f0f06
b4ba154
b1d26c2
22fb440
00aae34
5a165b1
9ac6306
39f8459
7db6778
687c757
7f4fae7
1d8ff8c
5c22f19
58dc3d2
e73c061
03bf034
c7bfb52
89d4499
14055e1
c7c19fc
d3259c5
54deaf4
5fd7525
2d7530d
ba2e94a
3420903
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 forscale-info
itself, and we haveextern 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 forscale-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 asrust-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.
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 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 thewatt
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.
proc-macro-crate
predates this PR so the technical debt is already there;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?