Generate metadata for constructor return type #1460

SkymanOne · 2022-11-01T14:37:01Z

This is a follow up for #1446 and closes #1457

Robbepop · 2022-11-01T16:47:58Z

crates/ink/codegen/src/generator/metadata.rs

+    fn generate_constructor_return_type(ret_ty: Option<&syn::Type>) -> TokenStream2 {
+        /// Returns `true` if the given type is `Self`.
+        fn type_is_self_val(ty: &syn::Type) -> bool {
+            matches!(ty, syn::Type::Path(syn::TypePath {
+            qself: None,
+            path
+        }) if path.is_ident("Self"))
+        }
+
+        match ret_ty {
+            None => {
+                quote! {
+                    ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None)
+                }
+            }
+            Some(ty) => {
+                if type_is_self_val(ty) {
+                    return quote! { ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None)}
+                }
+                let type_spec = Self::generate_constructor_type_spec(ty);
+                let type_token = Self::replace_self_with_unit(ty);
+                quote! {
+                    if !::ink::is_result_type!(#type_token) {
+                        ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None)
+                    } else {
+                       ::ink::metadata::ReturnTypeSpec::new(#type_spec)
+                    }
+                }
+            }
+        }
+    }


This again does syntactical checks instead of using the Rust type system properly and will fail for various syntactic contexts. To do this properly you'd need to take a similar approach as in generate_trait_messages with Rust type system reflection such as

let is_payable = quote! {{ <<::ink::reflect::TraitDefinitionRegistry<<#storage_ident as ::ink::reflect::ContractEnv>::Env> as #trait_path>::__ink_TraitInfo as ::ink::reflect::TraitMessageInfo<#local_id>>::PAYABLE }};

We do have the information for ink! messages and constructors as well. They are identified using their selectors which are always known at proc. macro compile time. Therefore you can access them in this scope just like in the linked example.

The metadata codegen takes place after the WASM blob compilation, hence at this stage all types have already been evaluated by compiler and we can work with syntax.

In fact, I generally do not do syntactical checks. I only check if the token is Self here because metadata generation takes place outside the storage definition, hence Self can not be evaluated there. Same logic applies for Result<Self, ...>.

However, if the return type is the explicit type of a contract or Result with explicit contract type for Ok variant, then I do the type evaluation, hence why we have TransformResult factory.
I can be sure that the Self is valid in this context is because the WASM build is already done by now.

I am not sure how message selectors are relevant here.

I think we may be able to refactor this to make it use the Rust type system, I'm looking into this now.

I've done an experiment with this in #1491, take a look and see what you think.

crates/metadata/src/specs.rs

SkymanOne · 2022-11-04T20:26:43Z

After few tweaks and additional tests here is the summary of what has been achieved:

The metadata is successfully generated for these edges cases:

Return type is Self then no return type is indicated in metadata
Return type is storage type then no return type is indicated in metadata too
Return type is alias to the storage type, same behaviour as above
Return type is Result, then metadata indicates that Ok is () and Err is the type specified by developer
Return type is alias to Result - same behaviour as above
Return type is alias to Result and Ok variant is a type alias to the contract type - same behaviour as above

xermicus

LGTM!

Stuffed some minor changes into a piggyback PR #1473, please have a look.

The metadata is successfully generated for these edges cases:
...

Is this explicitly tested for somewhere? In the metadata I see only one case (<Result<u8, ()> being tested for. Might be worthwhile to add some contract testcases.

Also don't forget about documentation (if not already done by the original PR).

crates/ink/codegen/src/generator/metadata.rs

crates/metadata/src/specs.rs

crates/ink/codegen/src/generator/metadata.rs

crates/metadata/src/specs.rs

crates/ink/codegen/src/generator/metadata.rs

* test changes * rename trait to ConstructorReturnSpec * adapt test

codecov-commenter · 2022-11-05T23:23:04Z

Codecov Report

Merging #1460 (4959e4c) into master (f2e799e) will decrease coverage by 6.61%.
The diff coverage is 42.00%.

❗ Current head 4959e4c differs from pull request most recent head e335081. Consider uploading reports for the commit e335081 to get more accurate results

@@            Coverage Diff             @@
##           master    #1460      +/-   ##
==========================================
- Coverage   71.04%   64.43%   -6.62%     
==========================================
  Files         201      201              
  Lines        6189     6236      +47     
==========================================
- Hits         4397     4018     -379     
- Misses       1792     2218     +426

Impacted Files	Coverage Δ
crates/ink/src/codegen/dispatch/execution.rs	`0.00% <0.00%> (ø)`
crates/metadata/src/lib.rs	`0.00% <ø> (ø)`
crates/ink/codegen/src/generator/metadata.rs	`82.55% <30.30%> (-14.87%)`	⬇️
crates/metadata/src/specs.rs	`89.64% <64.28%> (-1.50%)`	⬇️
crates/metadata/src/tests.rs	`100.00% <100.00%> (ø)`
crates/ink/ir/src/ir/contract.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/ink/ir/src/ir/ink_test.rs	`0.00% <0.00%> (-87.50%)`	⬇️
crates/ink/ir/src/ir/trait_def/mod.rs	`0.00% <0.00%> (-83.34%)`	⬇️
crates/ink/ir/src/ir/storage_item/config.rs	`0.00% <0.00%> (-73.34%)`	⬇️
crates/ink/ir/src/ir/storage_item/mod.rs	`0.00% <0.00%> (-72.35%)`	⬇️
... and 25 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

xermicus · 2022-11-07T10:25:30Z

Made some more minor changes: #1475

Also this PR fixes something, but the CI was passing prior/without the fix. Doesn't this mean that we should add the missing test case here as well?

* remove unecessary helper fn * fix some comments * rename var

crates/ink/codegen/src/generator/metadata.rs

Co-authored-by: Cyrill Leutwiler <cyrill@parity.io>

* Add test for metadata generator * nightly fmt

xermicus

Thanks!

This reverts commit 8269a3d.

ascjones

The resulting metadata looks to be correct, but the main problem is that at the moment the actually returned value will not be an encoded instance of the type in the metadata.

Also I think we can utilize the type system here as @Robbepop suggests, I am looking into this now.

ascjones · 2022-11-08T16:52:02Z

crates/ink/src/codegen/dispatch/execution.rs

@@ -68,7 +68,7 @@ where
                &Contract::KEY,
                contract,
            );
-            Ok(())
+            ink_env::return_value(ReturnFlags::default().set_reverted(false), &());


But what if the return type is Result<Self, _>, the value will never be returned. The client decoding will fail because it will expect 0x00 for Ok but will get empty bytes instead.

I suppose you have not been able to test this since there are no clients which are attempting to decode the return value?

One way to test it would be via the in-contract instantiation with CreateBuilder. E.g used in delegator.

ascjones · 2022-11-08T17:05:01Z

crates/ink/codegen/src/generator/metadata.rs

+
+    #[test]
+    fn constructor_return_type_works() {
+        let expected_no_ret_type_spec = ":: ink :: metadata :: ReturnTypeSpec :: new (:: core :: option :: Option :: None)";


It's not a great way to test it using strings like this. It's brittle and dependent upon the codegen impl.

If we can come up with a good solution with types + reflection this will be easier to test.

ascjones · 2022-11-08T17:07:10Z

crates/ink/codegen/src/generator/metadata.rs

+    fn generate_constructor_return_type(ret_ty: Option<&syn::Type>) -> TokenStream2 {
+        /// Returns `true` if the given type is `Self`.
+        fn type_is_self_val(ty: &syn::Type) -> bool {
+            matches!(ty, syn::Type::Path(syn::TypePath {
+            qself: None,
+            path
+        }) if path.is_ident("Self"))
+        }
+
+        match ret_ty {
+            None => {
+                quote! {
+                    ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None)
+                }
+            }
+            Some(ty) => {
+                if type_is_self_val(ty) {
+                    return quote! { ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None)}
+                }
+                let type_spec = Self::generate_constructor_type_spec(ty);
+                let type_token = Self::replace_self_with_unit(ty);
+                quote! {
+                    if !::ink::is_result_type!(#type_token) {
+                        ::ink::metadata::ReturnTypeSpec::new(::core::option::Option::None)
+                    } else {
+                       ::ink::metadata::ReturnTypeSpec::new(#type_spec)
+                    }
+                }
+            }
+        }
+    }


I think we may be able to refactor this to make it use the Rust type system, I'm looking into this now.

SkymanOne added 3 commits November 1, 2022 14:02

generates correct metadata for Self type

4be003b

generate metadata for Result type

68df576

generate metadata for Result<Self, ...>

ced904f

Robbepop reviewed Nov 1, 2022

View reviewed changes

SkymanOne requested a review from Robbepop November 1, 2022 17:58

docs and comments

1686a8a

SkymanOne marked this pull request as ready for review November 2, 2022 09:50

SkymanOne requested review from cmichi, ascjones and HCastano as code owners November 2, 2022 09:50

SkymanOne added 2 commits November 3, 2022 17:50

Merge branch 'master' into gn/constructor-metadata

42fac54

cargo fmt

7e4aed2

xermicus suggested changes Nov 3, 2022

View reviewed changes

crates/metadata/src/specs.rs Show resolved Hide resolved

SkymanOne added 4 commits November 4, 2022 14:24

fix the metadata tests

2912089

Merge branch 'master' into gn/constructor-metadata

bdd717f

makes compatible with new tests

32e99fa

rewrite return type transformation to support aliases

f5ec420

SkymanOne requested a review from xermicus November 4, 2022 20:21

xermicus suggested changes Nov 5, 2022

View reviewed changes

test changes (#1473)

4959e4c

* test changes * rename trait to ConstructorReturnSpec * adapt test

SkymanOne requested a review from xermicus November 6, 2022 00:42

remove unecessary helper fn (#1475)

e59821f

* remove unecessary helper fn * fix some comments * rename var

xermicus reviewed Nov 7, 2022

View reviewed changes

crates/ink/codegen/src/generator/metadata.rs Outdated Show resolved Hide resolved

Update crates/ink/codegen/src/generator/metadata.rs

e335081

Co-authored-by: Cyrill Leutwiler <cyrill@parity.io>

SkymanOne requested a review from xermicus November 7, 2022 13:10

Add test for metadata generator (#1477)

17795d0

* Add test for metadata generator * nightly fmt

ascjones mentioned this pull request Nov 7, 2022

ink! 4.0-beta tracking #1478

Closed

8 tasks

xermicus approved these changes Nov 7, 2022

View reviewed changes

SkymanOne merged commit 8269a3d into master Nov 7, 2022

SkymanOne deleted the gn/constructor-metadata branch November 7, 2022 17:20

SkymanOne added a commit that referenced this pull request Nov 7, 2022

Revert "Generate metadata for constructor return type (#1460)"

d8da988

This reverts commit 8269a3d.

ascjones reviewed Nov 8, 2022

View reviewed changes

This was referenced Nov 8, 2022

Fix and refactor fallible constructors #1483

Closed

Rework fallible constructors #1491

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate metadata for constructor return type #1460

Generate metadata for constructor return type #1460

SkymanOne commented Nov 1, 2022

Robbepop Nov 1, 2022

SkymanOne Nov 1, 2022 •

edited

Loading

ascjones Nov 8, 2022

ascjones Nov 10, 2022

SkymanOne commented Nov 4, 2022

xermicus left a comment •

edited

Loading

codecov-commenter commented Nov 5, 2022 •

edited

Loading

xermicus commented Nov 7, 2022

xermicus left a comment

ascjones left a comment

ascjones Nov 8, 2022 •

edited

Loading

ascjones Nov 8, 2022

ascjones Nov 8, 2022

Generate metadata for constructor return type #1460

Generate metadata for constructor return type #1460

Conversation

SkymanOne commented Nov 1, 2022

Robbepop Nov 1, 2022

Choose a reason for hiding this comment

SkymanOne Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

ascjones Nov 8, 2022

Choose a reason for hiding this comment

ascjones Nov 10, 2022

Choose a reason for hiding this comment

SkymanOne commented Nov 4, 2022

xermicus left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Nov 5, 2022 • edited Loading

Codecov Report

xermicus commented Nov 7, 2022

xermicus left a comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

ascjones Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

ascjones Nov 8, 2022

Choose a reason for hiding this comment

ascjones Nov 8, 2022

Choose a reason for hiding this comment

SkymanOne Nov 1, 2022 •

edited

Loading

xermicus left a comment •

edited

Loading

codecov-commenter commented Nov 5, 2022 •

edited

Loading

ascjones Nov 8, 2022 •

edited

Loading