Follow-up: SpreadLayout #995

Robbepop · 2021-10-31T12:11:36Z

Implements

Partially implements

Support Result<Self, E> returning ink! constructors #988

Note: Only partial since fallible ink! constructors remain artificially forbidden as of now. Another follow-up PR might change that soon.

Doesn't implement, but related

Automatically implement SpreadAllocate for ink! storage struct if all fields implement SpreadAllocate #991

All in all this heavily simplifies working with the new SpreadLayout traits in ink! smart contracts. As a consequence we can further simplify #979 after merging this PR.

Note that this commit does not yet allow to write fallible ink! constructors.

xgreenx · 2021-10-31T13:32:56Z

crates/lang/codegen/src/generator/storage.rs

+            let field_span = field.span();
+            let field_ty = &field.ty;
+            quote_spanned!(field_span=>
+                #field_ty: ::ink_storage::traits::SpreadAllocate


I think you will have a compilation error in case of two fields with the same type

Since trivial_bounds Rust feature is still unstable we have to remove this auto generated trait impl anyways unfortunately. Otherwise I guess it would be fine.

xgreenx · 2021-10-31T13:42:56Z

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

-    if config.dynamic_storage_alloc {
-        alloc::finalize();
+    let result = ManuallyDrop::new(private::Seal(f()));
+    match result.as_result() {


Does it fix the issue #985? (if you apply this change for execute_message)

I think I now know what causes failure of result check. It is explained here:
https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md#limitations

Message return values are strictly more powerful than constructor return values therefore this cannot directly applied to fix the bug with messages. However, I am sure we can still fix the result bug with another technique.

This is due to the fact that trivial_bounds feature is not yet stable. Otherwise this automatically generated implementation would be totally fine. We can re-add this feature once Rust adds trivial trait bounds.

codecov-commenter · 2021-10-31T21:06:27Z

Codecov Report

Merging #995 (bd84d77) into master (6f3d7c0) will increase coverage by 16.42%.
The diff coverage is 51.51%.

❗ Current head bd84d77 differs from pull request most recent head 6368f2b. Consider uploading reports for the commit 6368f2b to get more accurate results

@@             Coverage Diff             @@
##           master     #995       +/-   ##
===========================================
+ Coverage   62.53%   78.96%   +16.42%     
===========================================
  Files         244      245        +1     
  Lines        9220     9204       -16     
===========================================
+ Hits         5766     7268     +1502     
+ Misses       3454     1936     -1518

Impacted Files	Coverage Δ
crates/lang/codegen/src/generator/dispatch.rs	`100.00% <ø> (+100.00%)`	⬆️
crates/lang/codegen/src/generator/storage.rs	`100.00% <ø> (+100.00%)`	⬆️
crates/lang/src/codegen/dispatch/execution.rs	`0.00% <0.00%> (ø)`
crates/storage/src/traits/mod.rs	`100.00% <ø> (+17.39%)`	⬆️
crates/storage/derive/src/spread_allocate.rs	`93.33% <93.33%> (ø)`
crates/primitives/src/key.rs	`93.42% <100.00%> (+0.17%)`	⬆️
crates/env/src/backend.rs	`0.00% <0.00%> (-80.65%)`	⬇️
crates/lang/src/result_info.rs	`100.00% <0.00%> (ø)`
crates/env/src/call/selector.rs	`100.00% <0.00%> (ø)`
crates/env/src/call/call_builder.rs	`0.00% <0.00%> (ø)`
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f3d7c0...6368f2b. Read the comment docs.

HCastano

There's a couple of things I haven't looked at yet, but for the most part I think it's looking good 🦾

HCastano · 2021-11-05T17:16:49Z

crates/storage/derive/src/spread_allocate.rs

+fn derive_struct(s: synstructure::Structure) -> TokenStream2 {
+    assert!(
+        s.variants().len() == 1,
+        "can only operate on structs or enums with 1 variant"


Suggested change

"can only operate on structs or enums with 1 variant"

"can only operate on structs"

HCastano · 2021-11-05T17:17:01Z

crates/storage/derive/src/spread_allocate.rs

+    }
+}
+
+/// Derives `ink_storage`'s `SpreadAllocate` trait for the given `struct` or `enum`.


Suggested change

/// Derives `ink_storage`'s `SpreadAllocate` trait for the given `struct` or `enum`.

/// Derives `ink_storage`'s `SpreadAllocate` trait for the given `struct`.

HCastano · 2021-11-05T17:17:13Z

crates/storage/derive/src/lib.rs

+    ///
+    /// # Note
+    ///
+    /// As of now `enum` types are not supported!


Why aren't they supported yet?

SpreadAllocate is very similar to the Default trait. Both do default initialization of some kind. There is no trivial way to figure out the default variant of an enum.

Would it be possible to implement SpreadAllocate for enums that implement the Default trait?

e.g. something like:

#[derive(Debug)] pub enum Color { Red, Blue, Green, NoColor } impl Default for Color { fn default() -> Self { Color::NoColor } }

I have a contract that has been making use of enums but I'm no longer able to compile it due to the new initialize_contract concept:

354 | ink_lang::codegen::initialize_contract(|contract| { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `SpreadAllocate` is not implemented for `Prosopo` | note: required by a bound in `initialize_contract` --> /home/user/.cargo/git/checkouts/ink-e2bdf0affead2725/4391ae5/crates/lang/src/codegen/dispatch/execution.rs:142:33 | 142 | Contract: ContractRootKey + SpreadAllocate, | ^^^^^^^^^^^^^^ required by this bound in `initialize_contract`

Do you have any other suggestions?

It would definitely make sense to allow derive impls for enums with Default impl. Not sure how to implement this properly though. Maybe we could add a Self: Default where bound to the generated derive impl if the derive item is an enum:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=21d9e01c332a660d9e5ab22634250f1a

I modified ink to allow me to pass an enum with Default as per your suggestion. I'm now able to use the SpreadAllocate derive function on my structs that contain enums.

#[derive(Debug, SpreadLayout, SpreadAllocate)] #[cfg_attr(feature = "std", derive(scale_info::TypeInfo, StorageLayout))] pub struct ProviderAccountMap(Mapping<AccountId, Provider>); //Provider contains a multi value enum impl ProviderAccountMap { fn insert(&mut self, key: AccountId, value: Provider) { self.0.insert(key, &value); } fn get(&self, key: &AccountId) -> Option<Provider> { return self.0.get(key); } }

Unfortunately, I wasn't able to generalise this for T as scale_info::TypeInfo is not available within ink in no_std mode. I was trying to do something like this:

pub struct AccountMap<T>(Mapping<AccountId, T>) where T: scale::Encode + scale::Decode + SpreadLayout + PackedLayout + scale_info::TypeInfo; impl<T> AccountMap<T> where T: PackedLayout + scale::Encode + scale::Decode + scale::EncodeLike + scale_info::TypeInfo { fn insert(&mut self, key: AccountId, value: T) { self.0.insert(key, &value); } fn get(&self, key: &AccountId) -> Option<T> { return self.0.get(key); } }

Is this possible within the smart contract or am I wasting my time?

Could you please open an issue in order to resolve this?
I find it very unpleasant to talk about this feature request in a merged PR discussion.
Thank you! :)

Done: #1042

HCastano · 2021-11-05T17:26:05Z

crates/storage/derive/src/tests/spread_allocate.rs

+            struct NamedFields {
+                a: i32,
+                b: [u8; 32],
+                d: Box<i32>,


Suggested change

d: Box<i32>,

c: Box<i32>,

😄

HCastano · 2021-11-05T17:26:22Z

crates/storage/derive/src/tests/spread_allocate.rs

+                        NamedFields {
+                            a: <i32 as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),
+                            b: <[u8; 32] as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),
+                            d: <Box<i32> as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),


Suggested change

d: <Box<i32> as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),

c: <Box<i32> as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),

HCastano · 2021-11-05T17:38:58Z

crates/storage/derive/src/tests/spread_allocate.rs

+// The `synstructure` crate currently expands to code that has a single
+// `panic` inside an `if` block. We have to allow the following `clippy`
+// lint until the fixing PR has been merged:
+// PR: https://github.com/mystor/synstructure/pull/50


This should have a ink! issue attached to it, otherwise we'll forget to fix it once the PR gets merged

I opened #1009.

HCastano · 2021-11-05T17:58:41Z

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

+    /// Since it is private it can only be used from within this crate.
+    /// We need this type in order to properly seal the `ConstructorReturnType`
+    /// trait from unwanted external trait implementations.
+    #[repr(transparent)]


Why is this needed?

I dont think it really is. However I tend to add transparent to generic thin wrappers to indicate their use.

So in this case you're using it as an indicator that people shouldn't be using the Seal per say, but instead should be using the T?

I'd argue that the naming and docs are enough, but I see what you're getting at

Seal is private so they cannot use it. It is more of a dev documentation to never ever make it public.

HCastano · 2021-11-08T14:44:33Z

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

-    push_spread_root::<S>(&storage, &root_key);
-    if config.dynamic_storage_alloc {
-        alloc::finalize();
+    let result = ManuallyDrop::new(private::Seal(f()));


Why don't we want this getting dropped here? From what I can tell f isn't used in the codegen after being passed through to here

We dont want and dont need the otherwise automated Drop call.

HCastano · 2021-11-08T15:47:37Z

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

+/// Only `()` and `Result<(), E>` are allowed contract initializer return types.
+/// For `WrapReturnType<C>` where `C` is the contract type the trait converts
+/// `()` into `C` and `Result<(), E>` into `Result<C, E>`.
+pub trait InitializerReturnType<C>: private::Sealed {


Since there are only two basic cases here, I don't know if I would go ahead with a new trait for this. I'd just do the wrapping in-line

How would this look like?

* implement const_fn Key::new constructor * add ContractRootKey trait to ink_lang crate * use qualified path * re-export ContractRootKey trait * automatically implement ContractRootKey for ink! contracts * implement #[derive(SpreadAllocate)] macro & tests * re-export #[derive(SpreadAllocate)] macro from ink_storage::traits * automatically implement SpreadAllocate for the ink! storage struct * extend execute_constructor to work with fallible ink! constructors Note that this commit does not yet allow to write fallible ink! constructors. * add initialize_contract utility method * re-export initialize_contract utility method * adjust ink! codegen for recent changes with execute_constructor * fix clippy warning * fix hunspell dictionary * add missing docs to codegen types * fix UI test * remove automatic SpreadAllocate implementation This is due to the fact that trivial_bounds feature is not yet stable. Otherwise this automatically generated implementation would be totally fine. We can re-add this feature once Rust adds trivial trait bounds. * Satisfy OCD in test * Remove mention of `enum`s from `derive_struct()` * Remove outdated comment about Clippy lint * RustFmt Co-authored-by: Hernando Castano <[email protected]>

Robbepop added 12 commits October 31, 2021 13:08

implement const_fn Key::new constructor

b192f55

add ContractRootKey trait to ink_lang crate

dc374ac

use qualified path

b78d849

re-export ContractRootKey trait

147b5e2

automatically implement ContractRootKey for ink! contracts

e1c8a7e

implement #[derive(SpreadAllocate)] macro & tests

96d6b0c

re-export #[derive(SpreadAllocate)] macro from ink_storage::traits

00372bb

automatically implement SpreadAllocate for the ink! storage struct

82e93ad

extend execute_constructor to work with fallible ink! constructors

8f626ce

Note that this commit does not yet allow to write fallible ink! constructors.

add initialize_contract utility method

1dad2b1

re-export initialize_contract utility method

6156806

adjust ink! codegen for recent changes with execute_constructor

5160b17

Robbepop requested review from HCastano, ascjones and cmichi October 31, 2021 12:13

Robbepop added 4 commits October 31, 2021 13:18

fix clippy warning

ee53370

fix hunspell dictionary

a292116

add missing docs to codegen types

c3999ca

fix UI test

ce48ebc

xgreenx reviewed Oct 31, 2021

View reviewed changes

remove automatic SpreadAllocate implementation

bd84d77

This is due to the fact that trivial_bounds feature is not yet stable. Otherwise this automatically generated implementation would be totally fine. We can re-add this feature once Rust adds trivial trait bounds.

HCastano mentioned this pull request Nov 4, 2021

Simple Mapping type improvements #979

Merged

HCastano reviewed Nov 5, 2021

View reviewed changes

HCastano approved these changes Nov 8, 2021

View reviewed changes

HCastano mentioned this pull request Nov 9, 2021

Remove allow(clippy::if_then_panic) from spread allocate tests #1009

Closed

HCastano added 4 commits November 9, 2021 16:16

Merge branch 'master' into rf-implement-pr992

276df24

Satisfy OCD in test

499befc

Remove mention of enums from derive_struct()

5c20590

Remove outdated comment about Clippy lint

6368f2b

RustFmt

7fed332

HCastano merged commit c7cc828 into master Nov 9, 2021

HCastano deleted the rf-implement-pr992 branch November 9, 2021 16:10

HCastano mentioned this pull request Nov 9, 2021

Simple Mapping Storage Primitive #946

Merged

forgetso mentioned this pull request Nov 24, 2021

Allow enum to implement SpreadAllocate #1042

Closed

This was referenced Nov 24, 2021

Add derive macro for SpreadAllocate trait #993

Closed

Implement utility function for spread allocate contract initialization #992

Closed

Automatically implement SpreadAllocate for ink! storage struct if all fields implement SpreadAllocate #991

Closed

	"can only operate on structs or enums with 1 variant"
	"can only operate on structs"

	/// Derives `ink_storage`'s `SpreadAllocate` trait for the given `struct` or `enum`.
	/// Derives `ink_storage`'s `SpreadAllocate` trait for the given `struct`.

	d: <Box<i32> as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),
	c: <Box<i32> as ::ink_storage::traits::SpreadAllocate>::allocate_spread(__key_ptr),

Follow-up: SpreadLayout #995

Follow-up: SpreadLayout #995

Uh oh!

Conversation

Robbepop commented Oct 31, 2021 • edited by HCastano Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HCastano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

forgetso Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Robbepop Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Robbepop commented Oct 31, 2021 •

edited by HCastano

Loading

codecov-commenter commented Oct 31, 2021 •

edited

Loading

forgetso Nov 23, 2021 •

edited

Loading

Robbepop Nov 24, 2021 •

edited

Loading