Refactor type generation, remove code duplication #352

ascjones · 2021-12-03T16:32:34Z

Closes #328. Pure refactoring of the codegen to unify code generating structs and fields, the duplication of which has caused bugs such as: #327.

The main goal of this refactoring was to consolidate the generation of code which produces a set of fields, give some scale_info::Fields. This is required in several different places:

Generation of standalone structs from the type registry
Generation of enum variants, which themselves are represented as a sequence of scale_info::Fields.
Generation of call and event structs, extracted from the variants of the top level Call and Event types.

This refactoring moves most of the core code generation for types which consist of a set of fields to the CompositeDef type. This type is an "intermediate representation" of some set of fields for generation of a struct or an enum variant. A good place to start is to look at it's ToTokens implementation and the field_tokens method which performs the actual codegen of the fields. You will see that all logic to do with adding compact attributes and phantom data fields is consolidated here, where previously it was spread around in various places. See code removed in struct_def.rs and type_def.rs.

I hope that it also improves the modularity of the types codegen to make it easier to understand and maintain. I think there could be more improvements here but this is a step in the right direction.

jsdw · 2022-01-06T10:29:31Z

codegen/src/types/composite_def.rs

+pub struct CompositeDefFields {
+    named: bool,
+    fields: Vec<CompositeDefField>,
+}


I wonder whether a slightly better fitting type here would be something like:

enum CompositeDefFields { Named(Vec<(syn::Ident, CompositeDefField)>), Unnamed(Vec<CompositeDefField>) }

And then remove the name from CompositeDefField. You wouldn't have to check a boolean or handle the option in that case :)

Indeed, that is similar to the type as it currently is:

subxt/codegen/src/struct_def.rs

Lines 40 to 43 in 415e122

pub enum StructDefFields {

Named(Vec<(syn::Ident, TypePath)>),

Unnamed(Vec<TypePath>),

}

It's been a while since I did this but I remember getting into difficulties with this type across it's different usage contexts, hence the relaxing of the strict type.

I will look again at this to see whether it can be improved.

This change has now been applied, let me know if it looks good

jsdw

Barring a small suggestion this looks good to me!

dvdplm · 2022-01-20T12:03:10Z

codegen/src/types/composite_def.rs

+                }
+            }
+            CompositeDefKind::EnumVariant => {
+                let fields = self.fields.field_tokens(None, None, false);


I wonder if it would make sense to have separate field_tokens methods for Struct vs Enums, e.g. struct_tokens()/enum_tokens(), to make the signature a bit easier on the eyes here.

Done, let me know if that works now.

dvdplm · 2022-01-20T12:07:32Z

codegen/src/types/type_def_params.rs

+use std::collections::HashSet;
+
+#[derive(Clone, Debug, Default)]
+pub struct TypeDefParameters {


SOme docs would be good here. Maybe on on the module level as well.

codegen/src/types/type_def_params.rs

dvdplm · 2022-01-20T12:25:32Z

codegen/src/types/type_def_params.rs

+        if self.unused.is_empty() {
+            return None
+        }
+        let mut params = self.unused.iter().collect::<Vec<_>>();


Maybe let mut params = Vec::from_iter(self.unused);?

Hm, actually I have just changed this from HashSet to a BTreeSet for consistent ordering of params, so the conversion to Vec is no longer required.

That said I'm never 100% sure when to use the explicit form e.g. Vec::from_iter. Although probably better than the turbofish in this case, perhaps let mut params: Vec<_> = self.unused.iter().collect(); implicit form is most idiomatic?

dvdplm

LGTM, left a few nitpicks.

* codegen: fix compact unnamed fields * Fmt * codegen: move derives and struct_def to types * codegen: rename struct_def to composite_def.r * WIP: deduplicate struct def code * Fmt * WIP refactoring composite type codegen duplication * Fmt * Fix TokenStream import * Fix field_tokens ty_path parse error * Fix call struct generation * Refactor ty_path() * Optional derives and move CompactAs derive to composite_def * Fmt * Introduce CompositeDefFieldType * Restore default codec derives * Extract TypeDefParameters and TypeDefGen construction * Fmt * Reset codegen to master * Introduce CompositeDefFields * Introduce CompositeDefFields * Fix up errors * Fix Box field types * Fmt * Fix compact attribute * Handle no fields case * Handle no fields with trailing semi * Fix compact field detection * Fix generic phantom marker * Fmt * Fix generic type parm fields * Clippy * Fix up boxed call fields * Fmt * Add comments to composite_def.rs * Fix license headers * Restore Debug derive in tests * Fix empty struct codegen test * Use BTreeSet for type params for ordering * code review: fix comment * code review: refactor CompositeDefFields as enum * Fix empty named fields * Fix generation of call variant enum structs * Expand field_tokens into separate methods for struct and enum variants * Add TypeDefParameters docs * Fix doc link * Clippy redundant return

ascjones added 30 commits November 24, 2021 16:43

codegen: fix compact unnamed fields

4888900

Fmt

0763311

codegen: move derives and struct_def to types

84e96f6

codegen: rename struct_def to composite_def.r

8da8752

WIP: deduplicate struct def code

4037c1c

Fmt

c1ee8f5

Merge branch 'master' into aj-refactor-struct-codegen

4cd5fc1

WIP refactoring composite type codegen duplication

81a4530

Fmt

75592fe

Fix TokenStream import

d42eb12

Merge branch 'master' into aj-refactor-struct-codegen

143c1cb

Fix field_tokens ty_path parse error

a36e71c

Fix call struct generation

591d06d

Refactor ty_path()

8e99d1e

Optional derives and move CompactAs derive to composite_def

b6f32a3

Fmt

f5c5c1a

Introduce CompositeDefFieldType

7e4fac8

Restore default codec derives

83821e2

Extract TypeDefParameters and TypeDefGen construction

ac0469f

Fmt

a26d689

Reset codegen to master

d748672

Introduce CompositeDefFields

9e1cb04

Introduce CompositeDefFields

52048d5

Fix up errors

f0a2009

Merge branch 'master' into aj-refactor-struct-codegen

a48e3e4

Fix Box field types

bd2635c

Fmt

22d662f

Fix compact attribute

d849bac

Handle no fields case

1191c1c

Handle no fields with trailing semi

47f7532

Fmt

77a237a

dvdplm requested review from insipx and jsdw December 10, 2021 13:19

Add comments to composite_def.rs

66db15f

ascjones mentioned this pull request Dec 10, 2021

[track] Release subxt to crates.io #345

Closed

18 tasks

ascjones added 2 commits January 5, 2022 10:43

Merge branch 'master' into aj-refactor-struct-codegen

c7d6d12

Fix license headers

c25f4ab

jsdw reviewed Jan 6, 2022

View reviewed changes

jsdw approved these changes Jan 6, 2022

View reviewed changes

dvdplm reviewed Jan 20, 2022

View reviewed changes

codegen/src/types/type_def_params.rs Outdated Show resolved Hide resolved

dvdplm reviewed Jan 20, 2022

View reviewed changes

dvdplm approved these changes Jan 20, 2022

View reviewed changes

ascjones added 12 commits January 24, 2022 13:35

Merge branch 'master' into aj-refactor-struct-codegen

a0542b6

Restore Debug derive in tests

5dfc1cf

Fix empty struct codegen test

3128e62

Use BTreeSet for type params for ordering

c30571c

code review: fix comment

4717c13

code review: refactor CompositeDefFields as enum

da8e9af

Fix empty named fields

a2c4574

Fix generation of call variant enum structs

f61c405

Expand field_tokens into separate methods for struct and enum variants

e2fdb4c

Add TypeDefParameters docs

fc6c42f

Fix doc link

415dfb9

Clippy redundant return

2396417

ascjones merged commit b0004ea into master Jan 26, 2022

ascjones deleted the aj-refactor-struct-codegen branch January 26, 2022 09:18

fewensa mentioned this pull request Jan 27, 2022

Wrong struct name after #352 merged #410

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor type generation, remove code duplication #352

Refactor type generation, remove code duplication #352

ascjones commented Dec 3, 2021 •

edited

Loading

jsdw Jan 6, 2022

ascjones Jan 6, 2022 •

edited

Loading

ascjones Jan 25, 2022

jsdw left a comment

dvdplm Jan 20, 2022

ascjones Jan 25, 2022

dvdplm Jan 20, 2022

ascjones Jan 25, 2022

dvdplm Jan 20, 2022

ascjones Jan 24, 2022

dvdplm left a comment

	pub enum StructDefFields {
	Named(Vec<(syn::Ident, TypePath)>),
	Unnamed(Vec<TypePath>),
	}

Refactor type generation, remove code duplication #352

Refactor type generation, remove code duplication #352

Conversation

ascjones commented Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

ascjones Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

ascjones commented Dec 3, 2021 •

edited

Loading

ascjones Jan 6, 2022 •

edited

Loading