Ep 748 simplify pub interface #209

reuvenpo · 2019-08-27T07:39:29Z

rewritten eng_wasm_derive::pub_interface

Enabled usage of the #[pub_interface] annotation on impl Struct items
as well as on trait items. When using the new form, the user may use whatever name they want
for the struct. When using the trait version the name of the struct/enum implementing
the methods was hardcoded as Contract, but is now customizable by passing the
implementor name as an argument to the macro.
(They can basically use any type they want now)
Added the following diagnostics to the macro invocation:
(when any of these is detected, the user will see a useful error message)
- The attribute was placed on an item which is not a trait or inherent impl
- The trait or impl methods use the self receiver
- Trait methods provided a default implementation (i understand that we
  wanted the users to define the implementations on the Contract struct)
- Trait method with any additional attribute
- Impl method with a #[no_mangle] attribute
- Impl method with visibility which isn't private or pub
  (we don't want them defining pub(crate) or pub(in something) because
  that can be misleading in this context)
- check that the constructor returns no value
- check that when applied to an impl-item, the constructor is marked as pub
Fixed issue around parsing return values from contract methods where their
amount was incorrectly calculated
When parsing of method inputs fails at runtime, the panic message now
mentions the type and argument that caused the failure.
If an unknown method is called, the panic message mentions the name of
that method.
Using the macro no longer requires writing use eng_wasm::*; in the using
module
restructured the module layout to make it easier to follow.
added unittests
documented a lot of things

The ERC20 example now generates the following code:
https://gist.github.com/rust-play/d9e375f7a587725be238ced968630d28
EDIT:
https://gist.github.com/rust-play/4a1da16589b738ee2f206d351dfd4c84

eng-wasm/derive/src/eth_contract.rs

reuvenpo · 2019-08-27T12:51:00Z

I'll point out that this code relies on the order of evaluation of function arguments, which is unspecified:
https://www.reddit.com/r/rust/comments/cw0kyd/is_the_order_of_evaluation_of_function_arguments/
but is never going to change from left-to-right:
https://internals.rust-lang.org/t/rust-expression-order-of-evaluation/2605/29
and will be documented as such at some point:
rust-lang/reference#248

EDIT: I removed this dependency anyways, it was fairly easy :)

moriaab · 2019-09-02T09:20:49Z

eng-wasm/derive/src/into_ident.rs

@@ -0,0 +1,9 @@
+pub trait IntoIdent {


Nice addition!

I just realised this is probably why it isn't made to be as easy to use as i made it:
https://docs.rs/quote/1.0.2/quote/macro.format_ident.html#panics

moriaab · 2019-09-02T09:33:09Z

eng-wasm/derive/src/lib.rs

@@ -1,333 +1,49 @@
-#![deny(unused_extern_crates)]


Nice cleaning of attributes and dependencies :-)

moriaab · 2019-09-02T12:05:14Z

eng-wasm/derive/src/pub_interface.rs

+use super::into_ident::IntoIdent;
+use parse_signatures::PubInterfaceSignatures;
+
+const DEFAULT_IMPLEMENTOR_NAME: &str = "Contract";


Excellent addition of definition and work with string constants here!

moriaab · 2019-09-03T14:09:55Z

eng-wasm/derive/src/pub_interface/parse_signatures.rs

@@ -0,0 +1,600 @@
+//! This module defines the parsing of input for the `#[pub_interface]` macro


parse_signatures.rs code sets high standards :) Excellent work!

eng-wasm/derive/src/pub_interface/parse_signatures.rs

eng-wasm/derive/src/pub_interface.rs

moriaab · 2019-09-04T07:32:27Z

eng-wasm/derive/src/pub_interface.rs

+                    // pwasm_abi/derive/src/lib.rs :: fn generate_eth_endpoint
+                    // which dictates how return values are serialised into the Sink.
+                    // This can be 0 is the return type is () which is correct.
+                    syn::ReturnType::Type(_, box syn::Type::Tuple(return_tuple)) => return_tuple.elems.len(),


Amazing catch and fix here!

eng-wasm/derive/src/eth_contract.rs

reuvenpo · 2019-08-27T10:02:01Z

eng-wasm/derive/.rustfmt.toml

@@ -0,0 +1 @@
+# override global formatting. i want clean and standard runstfmt.


Th project directory contains a fustfmt configuration file with a lot of unusual settings (e.g. line length of 135?). i believe we should use the standard rustfmt settings, so at least for this directory i override the current global settings. i propose to remove the project-wide rustfmt configuration and reformat the project with the standard formatter.

If we keep this, I need to add a newline here

reuvenpo · 2019-08-27T10:04:35Z

eng-wasm/derive/Cargo.toml

 [dependencies]
-eng-wasm = { version = "0.1.3", path = "../" }


Please note that this dependency is both unnecessary, and causes a future compatibility issue. I would like to create a dependency from eng-wasm to eng-wasm-derive so that we can reexport pub_interface from eng-wasm directly (and remove the requirement to depend on eng-wasm-derive directly. This can not happen without bumping the version of eng-wasm to 0.2 as it will not be compatible with older versions of eng-wasm-derive which depend on eng-wasm

reuvenpo · 2019-08-27T10:05:18Z

eng-wasm/derive/Cargo.toml

-[lib]
-proc-macro = true
+[dev-dependencies]
+syn = { version = "1.0", features = ["full", "extra-traits"] }


I use the extra-traits feature for printing and comparisons of AST objects

reuvenpo · 2019-08-27T10:07:36Z

eng-wasm/derive/src/into_ident.rs

@@ -0,0 +1,9 @@
+pub trait IntoIdent {


I think this is fairly self-explanatory, but if not i can add a docstring. i'm surprised this is not implemented upstream, so we can file a pull request with this to https://github.com/dtolnay/syn

eng-wasm/derive/src/pub_interface/parse_signatures.rs

eng-wasm/derive/src/lib.rs

reuvenpo · 2019-09-02T11:35:19Z

eng-wasm/derive/src/into_ident.rs

@@ -0,0 +1,9 @@
+pub trait IntoIdent {


I just realised this is probably why it isn't made to be as easy to use as i made it:
https://docs.rs/quote/1.0.2/quote/macro.format_ident.html#panics

eng-wasm/derive/src/pub_interface.rs

eng-wasm/derive/src/pub_interface/parse_signatures.rs

moriaab · 2019-09-12T13:27:23Z

eng-wasm/derive/src/pub_interface/parse_signatures.rs

+    }
+}
+
+/// This function checks if the function has a non-unit return type.


Good work! It is so easy to miss the distinction between empty tuple and no return type.

moriaab

This PR improves eng-wasm-derive crate significantly in all aspects. Well done!

* Enabled usage of the \#[pub_interface] annotation on `impl Struct` items as well as on trait items. When using the trait version the name of the struct/enum implementing the methods was (and still is) hardcoded as `Contract`. When using the new form, the user may use whatever name they want for the struct. It has no side-effects. (They can theoretically use any type they want now) * Added the following diagnostics to the macro invocation: (when any of these is detected, the user will see a useful error message) * The attribute was placed on an item which is not a trait or inherent impl * The trait or impl methods use the `self` receiver * Trait methods provided a default implementation (i understand that we wanted the users to define the implementations on the `Contract` struct) * Trait method with any additional attribute * Impl method with a \#[no_mangle] attribute * Impl method with visibility which isn't private or `pub` (we don't want them defining `pub(crate)` or `pub(in something)` because that can be misleading in this context) * Fixed issue around parsing return values from contract methods where their amount was incorrectly calculated * When parsing of method inputs fails at runtime, the panic message now mentions the type and argument that caused the failure. * If an unknown method is called, the panic message mentions the name of that method. * Using the contract no longer requires writing `use eng_wasm::*;` in the using module * restructured the module layout to make it easier to follow. * added unittests * documented a lot of things The ERC20 example now generates the following code: https://gist.github.com/rust-play/d9e375f7a587725be238ced968630d28

…mplementing Display on `eng-wasm-derive::pub_interface::parse_signatures::ParseError`

* added check that the constructor returns no value * added check that when applied to an impl-item, the constructor is marked as `pub`

… macro * If applied to a trait, the tokens are interpreted as the type that implements the trait (this affects code generation, which otherwise assumed the implementor was named `Contract`) * If applied to an impl, and there are extra `attr` tokens, an error is issued.

…n pub_interface Also slightly improved the generated code for dispatches to contract functions that do not take arguments.

… fn signature\n\npreviously they accidentally pointed to the missing pub token

… written back Previously the code was removed in case of error, which caused weird "missing function" errors after macro evaluation

…b_interface

moriaab · 2019-09-22T13:06:49Z

eng-wasm/derive/examples/trait-impl-rename.rs

+    fn expand(input: u32) -> u64;
+}
+
+impl MyContract for MyContractImplementation {


I would maybe add impl MyContract for another structure in addition, just to emphasize the point of the argument to pub_interface. But it is up to you.

reuvenpo commented Aug 27, 2019

View reviewed changes

eng-wasm/derive/src/eth_contract.rs Show resolved Hide resolved

moriaab closed this Sep 2, 2019

moriaab reopened this Sep 2, 2019

reuvenpo force-pushed the EP-748-simplify-pub_interface branch from cfee76b to ef068a1 Compare September 2, 2019 09:08

moriaab reviewed Sep 2, 2019

View reviewed changes

moriaab reviewed Sep 3, 2019

View reviewed changes

eng-wasm/derive/src/pub_interface/parse_signatures.rs Show resolved Hide resolved

moriaab reviewed Sep 4, 2019

View reviewed changes

eng-wasm/derive/src/pub_interface/parse_signatures.rs Outdated Show resolved Hide resolved

moriaab reviewed Sep 4, 2019

View reviewed changes

eng-wasm/derive/src/pub_interface.rs Show resolved Hide resolved

moriaab reviewed Sep 4, 2019

View reviewed changes

reuvenpo commented Sep 4, 2019

View reviewed changes

reuvenpo force-pushed the EP-748-simplify-pub_interface branch from 5766b3d to b3a0a9a Compare September 9, 2019 12:07

moriaab reviewed Sep 12, 2019

View reviewed changes

moriaab approved these changes Sep 13, 2019

View reviewed changes

reuvenpo added 10 commits September 18, 2019 14:35

reformatted eng-wasm/derive with rustfmt

8006f7f

reformatted eng-wasm with rustfmt

e0bfb8f

trivial fixups and typo fixes

3d07427

removed use of feature(box_patterns) from eng-wasm-derive

bab2194

changed a few uses of result.ok().unwrap() into result.unwrap()

b359a79

replaced usage of failure::Fail with parse_display::Display for i…

5f7f24b

…mplementing Display on `eng-wasm-derive::pub_interface::parse_signatures::ParseError`

added checks to pub_interface related to the contract constructor

922686f

* added check that the constructor returns no value * added check that when applied to an impl-item, the constructor is marked as `pub`

removed the dependency on the order of function argument evaluation i…

39cc487

…n pub_interface Also slightly improved the generated code for dispatches to contract functions that do not take arguments.

reuvenpo force-pushed the EP-748-simplify-pub_interface branch from b35216c to 39cc487 Compare September 18, 2019 11:42

reuvenpo added 2 commits September 19, 2019 11:28

added two basic examples to eng-wasm

dbd3b1f

fixed spans of type errors in inputs/outputs of secret contract methods

40cf488

reuvenpo added 4 commits September 19, 2019 11:47

errors about private contructors in secret contracts now point to the…

dffcb5b

… fn signature\n\npreviously they accidentally pointed to the missing pub token

added example for renaming of the implementor in eng-derive

bfebca7

When custom error happen in pub_interface, the original code is still…

797cc40

… written back Previously the code was removed in case of error, which caused weird "missing function" errors after macro evaluation

Merge remote-tracking branch 'origin/develop' into EP-748-simplify-pu…

e2a138e

…b_interface

moriaab reviewed Sep 22, 2019

View reviewed changes

reuvenpo merged commit cfe501e into develop Sep 22, 2019

Cashmaney deleted the EP-748-simplify-pub_interface branch December 5, 2019 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ep 748 simplify pub interface #209

Ep 748 simplify pub interface #209

reuvenpo commented Aug 27, 2019 •

edited

Loading

reuvenpo commented Aug 27, 2019 •

edited

Loading

moriaab Sep 2, 2019

reuvenpo Sep 2, 2019

moriaab Sep 2, 2019 •

edited

Loading

moriaab Sep 2, 2019

moriaab Sep 3, 2019

moriaab Sep 4, 2019

reuvenpo Aug 27, 2019

reuvenpo Aug 27, 2019

reuvenpo Aug 27, 2019

reuvenpo Aug 27, 2019

reuvenpo Sep 2, 2019

moriaab Sep 12, 2019

moriaab left a comment

moriaab Sep 22, 2019

		@@ -0,0 +1,600 @@
		//! This module defines the parsing of input for the `#[pub_interface]` macro

		@@ -0,0 +1 @@
		# override global formatting. i want clean and standard runstfmt.

		[dependencies]
		eng-wasm = { version = "0.1.3", path = "../" }

Ep 748 simplify pub interface #209

Ep 748 simplify pub interface #209

Conversation

reuvenpo commented Aug 27, 2019 • edited Loading

reuvenpo commented Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moriaab Sep 2, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moriaab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reuvenpo commented Aug 27, 2019 •

edited

Loading

reuvenpo commented Aug 27, 2019 •

edited

Loading

moriaab Sep 2, 2019 •

edited

Loading