Don't copy attributes to call builders #1130

xgreenx · 2022-02-09T22:32:18Z

The current codegen copy all attributes from the original method. But that attributes were initially added for the original function and may require some restrictions on the struct that uses these attributes.

For example, OpenBrush provides #[brush::modifiers] attribute that inserts some code before the function.
In the case of when_paused modifier, the struct should implement the PausableStorage trait. That contract derives that trait but auto-generated call builders don't derive it(and they can't because it is complicated for them). And it will cause a compilation error because call builders will try to use that modifier.

The idea of that PR is to not insert attributes from the original method for auto-generated call builders.

Call builders are used only for cross-contract communication so they don't participate in the code logic of the contract and they don't participate in the metadata generation(ABI). So attributes like #[doc] is not important.

BUT, maybe someone wants to apply some functional attributes like clippy or allow and that change will break it. So maybe we also need to think about how ink! can provide customization of attributes(which attributes should be passed to auto-generated structures and which not).

paritytech-cicd-pr · 2022-02-09T23:06:16Z

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 0.17.0-86510f8 and comparing them to ink! master:

	Total Optimized Size	Total Used Gas
`accumulator`	1.44 K
`adder`	2.94 K
`contract-terminate`	1.31 K	215_704
`contract-transfer`	7.98 K	15_704
`delegator`	7.63 K	51_343
`dns`	10.20 K	47_112
`erc1155`	27.50 K	94_224
`erc20`	9.80 K	47_112
`erc721`	13.70 K	125_632
`flipper`	1.76 K	15_704
`incrementer`	1.64 K	15_704
`multisig`	26.79 K	103_580
`proxy`	3.81 K	32_243
`rand-extension`	5.12 K	15_704
`subber`	2.96 K
`trait-erc20`	10.10 K	47_112
`trait-flipper`	1.53 K	15_704
`trait-incrementer`	1.62 K	31_408

Link to the run | Last update: Thu Feb 10 00:06:15 CET 2022

HCastano · 2022-02-10T04:29:52Z

So just from your PR description the change seems okay. However, I tried to build the pausable contract you linked and it built fine.

Can you explain how to reproduce the compilation error you're talking about?

xgreenx · 2022-02-10T07:48:00Z

The pausable contract in the master is using ink rc-6=) Call builders were introduced in rc7.

We started the migration to rc8 and at the moment we are using our own version of the ink(while we are waiting for the merging of PRs). But we already fixed that issue so you can't reproduce it publickly.

The error is next:

error[E0277]: the trait bound `my_pausable::_::CallBuilder: brush::contracts::pausable::PausableStorage` is not satisfied
  --> lib.rs:23:28
   |
23 |         #[brush::modifiers(when_not_paused)]
   |                            ^^^^^^^^^^^^^^^ the trait `brush::contracts::pausable::PausableStorage` is not implemented for `my_pausable::_::CallBuilder`
   |
note: required by a bound in `brush::contracts::pausable::when_not_paused`
  --> /Users/green/External/4ire.labs/openbrush-contracts/contracts/security/pausable/mod.rs:40:8
   |
40 |     T: PausableStorage,
   |        ^^^^^^^^^^^^^^^ required by this bound in `brush::contracts::pausable::when_not_paused`

error[E0271]: type mismatch resolving `for<'r> <[[email protected]:23:9: 23:45] as FnOnce<(&'r mut my_pausable::_::CallBuilder,)>>::Output == Result<_, _>`
  --> lib.rs:23:28
   |
23 |         #[brush::modifiers(when_not_paused)]
   |                            ^^^^^^^^^^^^^^^ expected enum `Result`, found struct `ink_env::call::CallBuilder`
   |
   = note: expected enum `Result<_, _>`
            found struct `ink_env::call::CallBuilder<DefaultEnvironment, Set<ink_env::AccountId>, Unset<u64>, Unset<u128>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<Result<(), brush::contracts::pausable::PausableError>>>>`
note: required by a bound in `brush::contracts::pausable::when_not_paused`
  --> /Users/green/External/4ire.labs/openbrush-contracts/contracts/security/pausable/mod.rs:41:26
   |
41 |     F: FnOnce(&mut T) -> Result<R, E>,
   |                          ^^^^^^^^^^^^ required by this bound in `brush::contracts::pausable::when_not_paused`

error[E0308]: mismatched types
  --> lib.rs:23:9
   |
23 |         #[brush::modifiers(when_not_paused)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `ink_env::call::CallBuilder`, found enum `Result`
24 |         pub fn flip(&mut self) -> Result<(), PausableError> {
   |                                   ------------------------- expected `ink_env::call::CallBuilder<DefaultEnvironment, Set<ink_env::AccountId>, Unset<u64>, Unset<u128>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<Result<(), brush::contracts::pausable::PausableError>>>>` because of return type
   |
   = note: expected struct `ink_env::call::CallBuilder<DefaultEnvironment, Set<ink_env::AccountId>, Unset<u64>, Unset<u128>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<Result<(), brush::contracts::pausable::PausableError>>>>`
                found enum `Result<_, _>`
   = note: this error originates in the attribute macro `brush::modifiers` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `MyFlipperRef: brush::contracts::pausable::PausableStorage` is not satisfied
  --> lib.rs:23:28
   |
23 |         #[brush::modifiers(when_not_paused)]
   |                            ^^^^^^^^^^^^^^^ the trait `brush::contracts::pausable::PausableStorage` is not implemented for `MyFlipperRef`
   |
note: required by a bound in `brush::contracts::pausable::when_not_paused`
  --> /Users/green/External/4ire.labs/openbrush-contracts/contracts/security/pausable/mod.rs:40:8
   |
40 |     T: PausableStorage,
   |        ^^^^^^^^^^^^^^^ required by this bound in `brush::contracts::pausable::when_not_paused`

codecov-commenter · 2022-02-13T02:50:51Z

Codecov Report

Merging #1130 (9ceb297) into master (1188ee7) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1130      +/-   ##
==========================================
+ Coverage   78.80%   78.83%   +0.02%     
==========================================
  Files         252      252              
  Lines        9397     9391       -6     
==========================================
- Hits         7405     7403       -2     
+ Misses       1992     1988       -4

Impacted Files	Coverage Δ
...odegen/src/generator/as_dependency/call_builder.rs	`100.00% <ø> (ø)`
...odegen/src/generator/as_dependency/contract_ref.rs	`100.00% <ø> (ø)`
...ng/codegen/src/generator/trait_def/call_builder.rs	`100.00% <ø> (ø)`
.../codegen/src/generator/trait_def/call_forwarder.rs	`100.00% <ø> (ø)`
...ates/storage/src/collections/hashmap/fuzz_tests.rs	`100.00% <0.00%> (+4.25%)`	⬆️

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 1188ee7...9ceb297. Read the comment docs.

ascjones

LGTM

cmichi · 2022-02-17T16:46:22Z

@xgreenx I'm not so sure about this PR and would like to ask for some more clarification.

The idea of that PR is to not insert attributes from the original method for auto-generated call builders.

Call builders are used only for cross-contract communication so they don't participate in the code logic of the contract and they don't participate in the metadata generation(ABI). So attributes like #[doc] is not important.

If I run cargo doc --document-private-items on your branch with flipper the doc comments/attributes for e.g. the methods disappear.

Generally I feel like the current behavior is what I as a developer would expect and it's also closer to Rust than to have those attributes disappear. I can imagine some clippy use cases (e.g. complex types) where it's justified to have those attributes be retained.

Can you explain the use case you have some more? Maybe we find another solution. Maybe you can elaborate how you mean this:

For example, OpenBrush provides #[brush::modifiers] attribute that inserts some code before the function.

xgreenx · 2022-02-17T18:09:46Z

If I run cargo doc --document-private-items on your branch with flipper the doc comments/attributes for e.g. the methods disappear.

Yes, it was an idea of the change to remove all attributes=D As I mentioned it is not important for generated structures like builders. But I agree, maybe clippy or other utility attributes can be useful to propagate. As a solution, we can filter attributes and add only allowed ones(like doc, clippy). Or we need to provide an ability to mark those attributes that should be not propagated.

Can you explain the use case you have some more? Maybe we find another solution. Maybe you can elaborate how you mean this:

We implemented modifiers in OpenBrush like we describe here. The idea is that modifier's code is integrated into the method. For example usage of modifiers when_paused requires the contract to implement the PausableStorage trait. The problem is that CallBuilder is not implementing that trait.

cmichi · 2022-02-18T09:41:45Z

As a solution, we can filter attributes […]
[…] provide an ability to mark those attributes that should be not propagated.

This is the solution I would prefer. So to have attributes like clippy be propagated by default, with the possibility to filter out ones which shouldn't be forwarded.

xgreenx · 2022-02-18T09:52:22Z

Then we need to decide how it can be marked for filtering=)

We can remove all attributes by default, only keeping attributes that contains substring from the whitelist: ["ink", "doc", "clippy", "allow", "deny"](Maybe you know other useful attributes?)

If the user wants to keep the attribute that is not from the whitelist, he can use the #[ink(keep(...))] attribute proc-macro. During expanding of that macro it will remove ink(keep( + ) part and will only keep #[...].

cmichi · 2022-02-18T15:11:31Z

We can remove all attributes by default, only keeping attributes that contains substring from the whitelist: ["ink", "doc", "clippy", "allow", "deny"](Maybe you know other useful attributes?)

Let's do the other way around, so have attributes propagated by default and if one needs them filtered out to annotate this case explicitly. Would this still work for your use case then?

I would prefer this way since it comes with way less surprises, you should be able to hide this from an OpenBrush developer and people who use clippy & co. won't run into unexpected behavior.

cmichi · 2022-02-18T15:30:39Z

@xgreenx How about changing this PR so that the syntax #[ink::contract(filter_attr = "foo, bar")] would be supported? Filtering in that case would mean that the attributes are forwarded until explicitly specified in the filter.

xgreenx · 2022-02-18T15:30:45Z

Let's do the other way around, so have attributes propagated by default and if one needs them filtered out to annotate this case explicitly. Would this still work for your use case then?

That means that users of OpenBrush should explicitly use modifiers #[ink(skip(modifiers(...)))] so it is why I prefer my variant=D

I would prefer this way since it comes with way less surprises, you should be able to hide this from an OpenBrush developer and people who use clippy & co. won't run into unexpected behavior.

We can't create a workaround for the problem with attributes without explicit specification of #[ink(skip(...))] because #[ink::contract] macro expands before #[modifiers(...)].

I think in most cases developers will add attributes that are related to the contract itself, not to builders. So it is why I think in most cases better remove not expected attributes for builders.

xgreenx · 2022-02-18T16:00:21Z

How about changing this PR so that the syntax #[ink::contract(filter_attr = "foo, bar")] would be supported? Filtering in that case would mean that the attributes are forwarded until explicitly specified in the filter.

It means that we need to introduce the same syntaxis for #[ink::trait_definition]. Also, may exist cases where we want to have a case #[ink(skip(...))] only for one method in the contract -> we need support that syntaxis too.

It is hard to imagine for me when we need to pass attributes to builders except clippy, doc, allow and deny. I think simpler will introduction of #[ink(keep(...))] and maybe extend in the future whitelist(it is a simple modification).

cmichi · 2022-02-21T07:49:00Z

It is hard to imagine for me when we need to pass attributes to builders except clippy, doc, allow and deny.

@xgreenx Here is the list of built-in attributes: https://doc.rust-lang.org/reference/attributes.html#built-in-attributes-index, so those would need to be whitelisted as well, right?

Couldn't your #[brush::modifiers] attribute insert an ink! attribute #[ink(filter_attr = "…")]? This would then be hidden to the user, but achieve the same effect?

xgreenx · 2022-02-21T08:10:35Z

@xgreenx Here is the list of built-in attributes: https://doc.rust-lang.org/reference/attributes.html#built-in-attributes-index, so those would need to be whitelisted as well, right?

Not all=) We are talking about the contract's public methods so only attributes: Diagnostics, Documentation, Conditional compilation.

Couldn't your #[brush::modifiers] attribute insert an ink! attribute #[ink(filter_attr = "…")]? This would then be hidden to the user, but achieve the same effect?

#[brush::modifiers] is an attribute for methods, it is expanded after #[ink::contract] so it can't insert any ink! related stuff before ink! expanding by itself=(

cmichi · 2022-02-21T09:12:17Z

Ok, so having discussed this, from my pov it's fine if you implement whitelisting + users having to opt-in if they want to keep the propagation. I think this then is still a better solution than how the PR is currently approved (removing the attributes altogether).

changes requested

xgreenx · 2022-02-21T09:17:07Z

I will work on it=) Are you okay with syntaxis #[ink(keep(...))]?

cmichi · 2022-02-21T09:22:20Z

I will work on it=) Are you okay with syntaxis #[ink(keep(...))]?

Cool! I would put it on the top level of the contract as #[ink::contract(keep_attr = "foo, bar")].

xgreenx · 2022-02-21T09:27:07Z

Cool! I would put it on the top level of the contract as #[ink::contract(keep_attr = "foo, bar")].

Okay, the same for #[ink::trait_definition(keep_attr = "foo, bar")]?

cmichi · 2022-02-21T09:30:33Z

Cool! I would put it on the top level of the contract as #[ink::contract(keep_attr = "foo, bar")].

Okay, the same for #[ink::trait_definition(keep_attr = "foo, bar")]?

Yup, fine with me!

xgreenx · 2022-02-23T15:41:58Z

Created a new pull request #1145

Don't copy attributes to call builders.

9ceb297

xgreenx requested review from Robbepop, cmichi, ascjones and HCastano as code owners February 9, 2022 22:32

xgreenx mentioned this pull request Feb 17, 2022

Release ink! 3.0.0-rc9 #1135

Merged

ascjones previously approved these changes Feb 17, 2022

View reviewed changes

Merge branch 'paritytech:master' into bugfix/attributes_call_builders

657e973

xgreenx mentioned this pull request Feb 23, 2022

Added keep_attr to #[ink::contract] and #[ink::trait_definition] #1145

Merged

xgreenx closed this Feb 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't copy attributes to call builders #1130

Don't copy attributes to call builders #1130

xgreenx commented Feb 9, 2022

paritytech-cicd-pr commented Feb 9, 2022

HCastano commented Feb 10, 2022

xgreenx commented Feb 10, 2022

codecov-commenter commented Feb 13, 2022 •

edited

Loading

ascjones left a comment

cmichi commented Feb 17, 2022 •

edited

Loading

xgreenx commented Feb 17, 2022

cmichi commented Feb 18, 2022 •

edited

Loading

xgreenx commented Feb 18, 2022

cmichi commented Feb 18, 2022

cmichi commented Feb 18, 2022

xgreenx commented Feb 18, 2022 •

edited

Loading

xgreenx commented Feb 18, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 21, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 21, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 21, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 23, 2022

Don't copy attributes to call builders #1130

Don't copy attributes to call builders #1130

Conversation

xgreenx commented Feb 9, 2022

paritytech-cicd-pr commented Feb 9, 2022

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

HCastano commented Feb 10, 2022

xgreenx commented Feb 10, 2022

codecov-commenter commented Feb 13, 2022 • edited Loading

Codecov Report

ascjones left a comment

Choose a reason for hiding this comment

cmichi commented Feb 17, 2022 • edited Loading

xgreenx commented Feb 17, 2022

cmichi commented Feb 18, 2022 • edited Loading

xgreenx commented Feb 18, 2022

cmichi commented Feb 18, 2022

cmichi commented Feb 18, 2022

xgreenx commented Feb 18, 2022 • edited Loading

xgreenx commented Feb 18, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 21, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 21, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 21, 2022

cmichi commented Feb 21, 2022

xgreenx commented Feb 23, 2022

codecov-commenter commented Feb 13, 2022 •

edited

Loading

cmichi commented Feb 17, 2022 •

edited

Loading

cmichi commented Feb 18, 2022 •

edited

Loading

xgreenx commented Feb 18, 2022 •

edited

Loading