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: Forward generics to the interface #264

Merged

Conversation

jawoznia
Copy link
Collaborator

@jawoznia jawoznia commented Nov 21, 2023

Part of #263

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (ef1da32) 86.56% compared to head (b84a48f) 87.25%.

Files Patch % Lines
sylvia-derive/src/multitest.rs 94.59% 4 Missing ⚠️
sylvia-derive/src/interfaces.rs 50.00% 3 Missing ⚠️
sylvia-derive/src/parser.rs 85.71% 3 Missing ⚠️
sylvia-derive/src/input.rs 96.77% 1 Missing ⚠️
sylvia-derive/src/message.rs 97.50% 1 Missing ⚠️
Additional details and impacted files
@@                     Coverage Diff                      @@
##           feat/generics_forwarding     #264      +/-   ##
============================================================
+ Coverage                     86.56%   87.25%   +0.68%     
============================================================
  Files                            25       25              
  Lines                          1742     1812      +70     
============================================================
+ Hits                           1508     1581      +73     
+ Misses                          234      231       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jawoznia jawoznia force-pushed the generics_in_interface_implementation branch 2 times, most recently from 41dfe82 to 9ec276d Compare November 22, 2023 10:05
@jawoznia jawoznia marked this pull request as ready for review November 22, 2023 10:05
@jawoznia jawoznia requested a review from hashedone November 22, 2023 10:05
@jawoznia jawoznia changed the title fix: Allow generic return type in contract queries feat: Forward generics to the interface Nov 22, 2023
@jawoznia jawoznia force-pushed the generics_in_interface_implementation branch 2 times, most recently from 97d37e1 to 90fd2d2 Compare November 22, 2023 13:04
Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

ATM, I don't like examples. Limitations are unacceptable really. If you have no idea how to work them around, I can take a closer look at implementation, but I didn't really use much time there as the API doesn't work for me.

//
// Sylvia will fail to recognize generic used if their path is different.
// F.e. if we this query would return `SvCustomMsg` and we would pass
// `sylvia::types::SvCustomMsg` to the `Generic` trait paths would not match.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand it, and I don't like this limitation. It should work as in Rust. Could you explain what does it exactly mean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is because when we check if generic type is used in context of given type it compares their full path.
In such case Msg, types::Msg and sylvia::types::Msg are three different types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't find this explains everything. types::Msg and sylvia::types::Msg are obviously not generic parameters - they are coming from a module, so you just should assume they exist and let Rust resolve it. Msg is either a generic type or a type available in global scope. Still, its Rust that would handle figuring out the exact type, but for additional codegen you should consider a type being maybe a generic argument only if it has no path besides the ident. If that is the case, you should be able to see the generic parameters of your definition block. If the ident you face is defined as generic, it is generic. Otherwise, it is not generic, and you don't make any further assumptions about that. I need help understanding how scoping makes any deal here. Could you give some very particular examples of what is possible and what is not? And I am not asking about a description of an example, but an example itself - I think some misunderstanding is happening here. We can a call about this if you want as it seems to be not trivial.

Ok(Response::new())
}

// Sylvia will fail if single type is used to match against two different generics
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, that means that I can't have both generics being monomorphized to String? That cannot be the case, and using particular solutions is not an excuse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These comments are actually related to the generic_contract where we pass the concrete types to the generics.
Comment landed here when I copied the examples.


// Sylvia will fail if single type is used to match against two different generics
// It's because we have to map unique generics used as they can be used multiple times.
// If for some reason like here one type would be used in place of two generics either full
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are plenty of reasons for that, and we cannot make people using aliases for every generic String.

Copy link
Collaborator Author

@jawoznia jawoznia Nov 23, 2023

Choose a reason for hiding this comment

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

Ye, now that I think of it it doesn't make sense.

I didn't want to initially change this behavior, but now as I look at the algorithm we basically take the list of generic parameters and traverse the signatures of methods. There is no risk of fake duplication as the iterated collection are passed generics.

I will change this.

//
// Sylvia will fail to recognize generic used if their path is different.
// F.e. if we this query would return `SvCustomMsg` and we would pass
// `sylvia::types::SvCustomMsg` to the `Generic` trait paths would not match.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again - I don't understand it, but it scares me.

Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

Before going further I need to exactly understand those limitations. Let's setup a call about that.

Copy link
Collaborator

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

Mostly good. Unresolved comments to be handled under #263

@jawoznia jawoznia merged commit 7ac645e into feat/generics_forwarding Nov 30, 2023
7 checks passed
@jawoznia jawoznia deleted the generics_in_interface_implementation branch November 30, 2023 14:27
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.

2 participants