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

[Bugfix] Invalid generation of types with >1 generic parameters #1023

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Jun 20, 2023

fixes #550

Investigated reason for the bug: In subxt, when calling resolve_field_type_path() to get the type of a generic, we were not taking into account multiple generics with different names (originial_name e.g. T, U, V, ...). The function just looked for the first type where the type (e.g. u8) matches (see: parent_type_params.iter().find(...)).

Fix: add an optional parameter original_name: Option<&str> to the type resolving function to also check the name of the generic argument.

Added a test to confirm the behavior is now as desired. The test matches the problem statement in #550.

@tadeohepperle tadeohepperle changed the title add original name match [Bugfix] Invalid generation of types with >1 generic parameters Jun 21, 2023
@tadeohepperle tadeohepperle marked this pull request as ready for review June 21, 2023 11:19
@tadeohepperle tadeohepperle requested a review from a team as a code owner June 21, 2023 11:19
@tadeohepperle tadeohepperle self-assigned this Jun 21, 2023
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.

Nice job! Also love the test :)

@@ -144,3 +145,75 @@ fn generic_types_overwrite_each_other() {
// We do _not_ expect this to exist, since a generic is present on the type:
assert!(!interface.contains("DuplicateType2"));
}

#[test]
fn more_than_1_generic_parameters_work() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

Looks good! Amazing PR!

use subxt_codegen::{CratePath, DerivesRegistry, RuntimeGenerator, TypeSubstitutes};
use syn::__private::quote;
Copy link
Member

Choose a reason for hiding this comment

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

hrm, does that compile? :)

It would be better to bring in quote in dependency tree as this might break between releases...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could do that, good idea.

@tadeohepperle tadeohepperle merged commit 8413c4d into master Jun 23, 2023
@tadeohepperle tadeohepperle deleted the tadeo-hepperle-invalid-generation-types-multiple-generic-parameters branch June 23, 2023 13:14
tadeohepperle added a commit that referenced this pull request Jul 4, 2023
* add original name match

* add unit test for generic params
@jsdw jsdw mentioned this pull request Jul 24, 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.

Invalid generation of types with >1 generic
4 participants