Add informational field type name #30

ascjones · 2020-11-24T16:25:43Z

Given a complex type which incorporates a field with a type alias, we currently lose the information about the use of a type alias. e.g.

type Balance = u128

#[derive(TypeInfo)]
struct S { b: Balance }

This currently results in the field type being resolved to u128 in the type registry, and we have no way to distinguish between a plain u128 and an aliased u128, which we may want to treat differently, e.g. some custom decoding or display logic for that type.

This PR adds the optional type_name property to Field, and updates the derive macro to extract the field type names e,g, in the above example the underlying type is u128 but the type name is Balance.

Note that this will add a type_name even if it is non-aliased, e.g. a: u32 will still have the type display name "u32" as part of the field definition. Even though this is redundant, and it is possible to remove non aliased display names (I implemented then removed it), I have left them in for the sake of simplicity and flexibility, leaving it up to consumers to do the lookup to determine whether an alias is being used or not.

This is required for use-ink/cargo-contract#79 in order for custom encoding/decoding for aliased Balance types.

# Conflicts: # test_suite/tests/derive.rs

dvdplm

Two main concerns (really: nitpicks) are:

the API is a bit less ergonomic to use after this
not a fan of the display_name name; could it be just alias?

Overall great stuff though.

derive/src/lib.rs

src/build.rs

src/ty/fields.rs

src/build.rs

test_suite/tests/derive.rs

dvdplm · 2020-11-24T18:31:07Z

test_suite/tests/derive.rs

+
+#[test]
+fn fields_with_type_alias() {
+	type BoolAlias = bool;


Unrelated to this PR, but do you think we could have this work at some point?

#[derive(TypeInfo)] type BoolAlias = bool;

It's a good idea, it may well be worth experimenting with. This would provide extra information about what aliases are available and may increase the accuracy of the heuristic identifying alias usage.

Ultimately it would also have the same limitation in that there is still no guarantee to identify when a given alias is being used, because the registry is using the underlying type id i.e. any::TypeId::of<BoolAlias>() == any::TypeId::of<bool>().

Never thought about this use case. I am not sure it goes too far but making it optional would probably be okay. The question is how to integrate it properly into the current scheme if we actually want ot.

# Conflicts: # derive/src/lib.rs # src/build.rs # src/ty/fields.rs # test_suite/tests/derive.rs # test_suite/tests/json.rs

This will display any valid field type name as specified in the source code.

ascjones · 2020-11-25T17:12:43Z

Updated so it now contains the raw string which is a result of stringify(#ty) where ty is a syn::Type. Let me know what you think @Robbepop.

dvdplm

I'll give it a proper review, but this real good. :)

dvdplm · 2020-11-25T21:26:39Z

test_suite/tests/json.rs

-                    { "type": 2 },
-                    { "type": 4, "displayName": ["bool"] },
+                    { "type": 1, "typeName": "i32" },
+                    { "type": 2, "typeName": "[u8 ; 32]" },


This is nice. :)

Note sure we need displayName or typeName (why rename btw?) for types such as [u8; 32] because its primary use case is still to counterfeit type aliases and besides that I cannot think of any other serious use case. Also I am unsure about turning the namespace (array of strings) into a single string since that kinda dictates how 3rd party tools handle namespaces or simply removes namespaces which are kinda important to differentiate between types.

displayName or typeName (why rename btw?)

To make it more general to imply we are not just dealing with paths/aliases now, but whatever type name is in that position.

Note sure we need displayName or typeName (why rename btw?) for types such as [u8; 32]

Indeed it is redundant because you can just look it up on the underlying type. We can easily filter those out, but in this iteration I have decided to always take the type name in the source for simplicity's sake, at the cost of some redundancy.

besides that I cannot think of any other serious use case

I believe we might need this initially if we want to be able to convert to the existing substrate metadata format, though of course it may be possible without, needs some investigation.

Also I am unsure about turning the namespace (array of strings) into a single string since that kinda dictates how 3rd party tools handle namespaces or simply removes namespaces which are kinda important to differentiate between types.

3rd party tools should not be relying on any namespace in the type/display name because it will only be there if the user decides to fully qualify their type e.g. struct S { field: foo::bar::Foo }, if they just use foo::bar::Foo then the display/type name will be Foo.

By not using the Path type anymore we are just giving an extra indication that this field does not provide any guarantees as to the content of that field other than it is the name of a valid Rust type in the context of the .rs file where it is defined.

Robbepop

already commented

Robbepop · 2020-11-26T18:34:33Z

derive/src/lib.rs

+    input
+        .replace(" ::", "::")
+        .replace(":: ", "::")
+        .replace(" ,", ",")
+        .replace(" ;", ";")
+        .replace(" [", "[")
+        .replace("[ ", "[")
+        .replace(" ]", "]")
+        .replace(" (", "(")
+        .replace("( ", "(")
+        .replace(" )", ")")
+        .replace(" <", "<")
+        .replace("< ", "<")
+        .replace(" >", ">")


dq: why wouldn't .replace(" ", ""); work here? Is space ever significant?

yes, e.g. something like &'a mut dyn Foo would become &'amutdynFoo.

I'd love to claim credit for this beauty but I stole it from https://github.com/paritytech/substrate/blob/master/frame/support/procedural/tools/src/lib.rs#L96

Robbepop · 2020-11-27T17:14:45Z

The only thing that bogs me with this approach that we no longer really support overlapping aliases by replacing the namespace with plain strings. E.g.:

mod a {
    pub type Foo = i32;
}
mod b {
    pub type Foo = i64; // could also be just i32
}
fn do_stuff(foo1: a::Foo, foo2: b::Foo) { ... }

We probably would prefer having aliases here but we could no longer differentiate between them really with them being just strings.
I can imagine this to be commonly used in the Substrate codebase.

ascjones · 2020-11-27T17:18:39Z

You can still differentiate between the two though, instead of ["a", "Foo"] and ["b", "Foo"] you'd just have "a::Foo" and "b::Foo".

Robbepop · 2020-11-27T17:20:21Z

You can still differentiate between the two though, instead of ["a", "Foo"] and ["b", "Foo"] you'd just have "a::Foo" and "b::Foo".

But then how is "a::Foo" string in the metadata an improvement over the more generic ["a", "Foo"] array especially in case of our targeted cross-language ecosystem where not every supported language has the same Rust-y syntax for namespaces?

ascjones · 2020-11-27T17:36:51Z

For the case of a plain TypePath like that it's not an improvement. a structured Path is clearly better. This was a way to get access to the unstructured type name for the field for any possible type without having to define a bunch of types to map to all possible variants of syn::Type. It's crude but KISS, and it provides enough information to do what I want for my use case.

However if we want something a bit more sophisticated it will just require a bit more thought.

As an aside, I think it's useful information anyway. We probably don't want to model type in source code being &mut <<T as MyTrait>::Type as MyOtherTrait>::Type, all you care about is that it eventually resolves to u64, good to know what the source definition was.

Robbepop · 2020-11-27T21:46:36Z

For the case of a plain TypePath like that it's not an improvement. a structured Path is clearly better. This was a way to get access to the unstructured type name for the field for any possible type without having to define a bunch of types to map to all possible variants of syn::Type. It's crude but KISS, and it provides enough information to do what I want for my use case.

However if we want something a bit more sophisticated it will just require a bit more thought.

As an aside, I think it's useful information anyway. We probably don't want to model type in source code being &mut <<T as MyTrait>::Type as MyOtherTrait>::Type, all you care about is that it eventually resolves to u64, good to know what the source definition was.

ah okay I see.

ascjones added 6 commits November 23, 2020 17:22

Add display name to field, tests failing

40d3ccb

Fix up some tests with display names

b6086c3

Fix up derive tests

105571c

Add display names to json tests

7c0fdf7

Merge branch 'master' into aj-field-type-display-name

f6101b0

# Conflicts: # test_suite/tests/derive.rs

Fix up recursive types test

b6e83cb

ascjones requested a review from dvdplm November 24, 2020 16:26

ascjones added 2 commits November 24, 2020 16:27

Fmt

2f23241

Fix no_std tests

1284f14

dvdplm reviewed Nov 24, 2020

View reviewed changes

ascjones added 3 commits November 25, 2020 10:23

Merge branch 'master' into aj-field-type-display-name

ce70ea9

# Conflicts: # derive/src/lib.rs # src/build.rs # src/ty/fields.rs # test_suite/tests/derive.rs # test_suite/tests/json.rs

Rename done to finalize

7262111

Always provide type name

08eabe2

This will display any valid field type name as specified in the source code.

ascjones changed the title ~~Add field type display name to capture aliases~~ Add field type name Nov 25, 2020

ascjones changed the title ~~Add field type name~~ Add informational field type name Nov 25, 2020

ascjones requested a review from Robbepop November 25, 2020 17:10

dvdplm reviewed Nov 25, 2020

View reviewed changes

Robbepop reviewed Nov 26, 2020

View reviewed changes

Clean up whitespace from type names

c9873ce

Robbepop reviewed Nov 26, 2020

View reviewed changes

Robbepop approved these changes Nov 27, 2020

View reviewed changes

dvdplm approved these changes Nov 30, 2020

View reviewed changes

Add field comment

47ccdb0

ascjones merged commit 7d6a837 into master Nov 30, 2020

ascjones deleted the aj-field-type-display-name branch November 30, 2020 09:52

ascjones mentioned this pull request Dec 7, 2020

Substrate compatibility #36

Closed

8 tasks

ascjones mentioned this pull request Jan 27, 2021

Update codec to 2.0 and release new version (before primitive-types release a new version for codec 2.0) #55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add informational field type name #30

Add informational field type name #30

ascjones commented Nov 24, 2020 •

edited

Loading

dvdplm left a comment

dvdplm Nov 24, 2020

ascjones Nov 25, 2020

Robbepop Nov 26, 2020

ascjones commented Nov 25, 2020

dvdplm left a comment

dvdplm Nov 25, 2020

Robbepop Nov 26, 2020

ascjones Nov 26, 2020

Robbepop left a comment

Robbepop Nov 26, 2020

dvdplm Nov 26, 2020

Robbepop Nov 27, 2020 •

edited

Loading

dvdplm Nov 27, 2020

ascjones Nov 27, 2020

Robbepop commented Nov 27, 2020 •

edited

Loading

ascjones commented Nov 27, 2020

Robbepop commented Nov 27, 2020

ascjones commented Nov 27, 2020

Robbepop commented Nov 27, 2020

Add informational field type name #30

Add informational field type name #30

Conversation

ascjones commented Nov 24, 2020 • edited Loading

dvdplm 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

ascjones commented Nov 25, 2020

dvdplm 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

Robbepop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop Nov 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robbepop commented Nov 27, 2020 • edited Loading

ascjones commented Nov 27, 2020

Robbepop commented Nov 27, 2020

ascjones commented Nov 27, 2020

Robbepop commented Nov 27, 2020

ascjones commented Nov 24, 2020 •

edited

Loading

Robbepop Nov 27, 2020 •

edited

Loading

Robbepop commented Nov 27, 2020 •

edited

Loading