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

[proc macros]: support generic type params #436

Merged
merged 34 commits into from
Aug 27, 2021
Merged

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Aug 20, 2021

So this the first step for support generic params on the trait in the proc macros

For now I have ignored the JSON Map support as it currently not is used that widely used at least by us (substrate etc) and looks quite annoying to fix the serde impls

In addition I added that our proc macros adds the required traits bounds under the hood, it's quite annoying the repeat those everywhere as user of the API. I think this should be it...

Substrate companion: paritytech/substrate@dp-jsonrpsee-integration-2...na-jsonrpsee-proc-macros

Nice to have:

  • Support JSON Object/Map

examples/proc_macro.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 requested review from dvdplm and jsdw August 20, 2021 11:09
@niklasad1 niklasad1 marked this pull request as ready for review August 20, 2021 15:32
examples/proc_macro.rs Outdated Show resolved Hide resolved
@niklasad1 niklasad1 changed the title [WIP]: proc macros support generic type params [proc macros]: support generic type params Aug 23, 2021
examples/proc_macro.rs Outdated Show resolved Hide resolved
examples/proc_macro.rs Outdated Show resolved Hide resolved
fn subscribe_storage(&self, mut sink: SubscriptionSink, keys: Option<Vec<u8>>) {
sink.send(&keys.unwrap_or_default()).unwrap();
fn subscribe_storage(&self, mut sink: SubscriptionSink, _keys: Option<Vec<ExampleStorageKey>>) {
sink.send(&vec![[0; 32]]).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe produce a few more items here?

examples/proc_macro.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
Comment on lines 128 to 132
fn visit_return_type(&mut self, return_type: &'ast syn::ReturnType) {
self.visiting_return_type = true;
visit::visit_return_type(self, return_type);
self.visiting_return_type = false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here, I have a hard time understanding what visit::visit_return_type does and why we need the true/false stuff. Some docs would be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have "borrowed" this code from jsonrpc and I think it's to make it possible to know whether it's a return type or param when visiting the Ident in fn visit_ident

proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/new/render_client.rs Outdated Show resolved Hide resolved

let where_clause = server_generate_where_clause(&self.trait_def);

// NOTE(niklasad1): empty where clause is valid rust syntax.
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL too! (I do like how Rust syntax is generally very amenable to code gen)

@niklasad1 niklasad1 changed the title [proc macros]: support generic type params WIP [proc macros]: support generic type params Aug 24, 2021
niklasad1 and others added 3 commits August 25, 2021 09:54
Co-authored-by: David <dvdplm@gmail.com>
Co-authored-by: David <dvdplm@gmail.com>
@jsdw jsdw requested a review from maciejhirsz August 25, 2021 11:45
fn subscribe_storage(&self, mut sink: SubscriptionSink, keys: Option<Vec<u8>>) {
sink.send(&keys.unwrap_or_default()).unwrap();
fn subscribe_storage(&self, mut sink: SubscriptionSink, _keys: Option<Vec<ExampleStorageKey>>) {
sink.send(&vec![[0; 32]]).unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

technically, we don't have any compile time check that actually only Vec<Hash> can sent over the sink because send has T: Serialize bound.... not sure if we want ensure that at compile-time

@niklasad1 niklasad1 changed the title WIP [proc macros]: support generic type params [proc macros]: support generic type params Aug 25, 2021
let mut sub = client.subscribe_storage(None).await.unwrap();
assert_eq!(Some(vec![]), sub.next().await.unwrap());
let mut sub: Subscription<Vec<ExampleHash>> =
RpcClient::<ExampleHash, ExampleStorageKey>::subscribe_storage(&client, None).await.unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

annoying to have to add these type hints but I think the issue for that is that we implement the client trait for T and these are not known in this cause at least not ExampleStorageKey ^^

@niklasad1
Copy link
Member Author

niklasad1 commented Aug 26, 2021

@dvdplm @jsdw @maciejhirsz can you have another look at this?

I think it should ready.

proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/visitor.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/render_client.rs Outdated Show resolved Hide resolved
proc-macros/src/render_server.rs Outdated Show resolved Hide resolved
niklasad1 and others added 5 commits August 27, 2021 09:33
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, left a minor nitpick on the docs.

proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
Co-authored-by: David <dvdplm@gmail.com>
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
proc-macros/src/helpers.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

The code looks good (especially for proc macro code)! Especially as I'm not really familiar with this codebase yet, some docs explaining what the aims of the visitors are would be good, but yup, I'm happy with this!

@niklasad1 niklasad1 merged commit 1045c78 into master Aug 27, 2021
@niklasad1 niklasad1 deleted the na-proc-macros-generics branch August 27, 2021 12:15
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.

3 participants