Refactor proc macros #164

rory-ocl · 2024-09-30T17:21:36Z

Description

This is a rather large refactor of the stylus proc macros with the main goal of improving maintainability, testability, and documentation of these macros. The following changes have been made:

Operate on AST nodes instead of raw tokens. This allows testing of smaller parts of the macros as unit tests. This also allows some composibility within the implementations.
Use the visitor pattern in several places to avoid confusing nesting of for-loops.
Separation of export-abi code using "extention" traits so that it is easy to reason about export-abi when needed, and otherwise ignore it.
Improved error reporting. The main change here is the ability to report multiple errors when compilation fails, allowing the user to see all errors at once. There is also some small changes with regards to consistency between the macros.
Unit tests for utilities and macro generation at the AST level. This includes a assert_ast_eq() function which can be used to compare two ASTs of the same type. If an error occurs, the ASTs are pretty printed and a diff is output so that the issue can be easily addressed.
Integration tests in the stylus-proc/tests directory. These allow compiled macros to be tested as working correctly. Additionally the stylus-proc/tests/fail directory will contain failing integration tests. These all fail to compile and include expected stderr outputs.
Modify code snippets within docstrings so that they are not ignored, and pass as part of the cargo test process.
A README.md which will contain information about developing and maintaining these proc macros.
Support for solidity.path.separator to rust::path::separator for custom structs within sol_interface! definitions.
Change the folder structure to have all macro implementations in a macros module. Easier to find the macro implementation you are looking for.
Docstrings within the macro implementations for developer reference.
Utilities for handing inner attributes including those which contain tokens which need to be parsed.

A little more work is going to be done before this is ready for merge, but it will be good to get this out for review since it is such a large set of changes.

Checklist

I have documented these changes where necessary.
I have read the DCO and ensured that these changes comply.
I assign this work under its open source licensing.

These tests are important before upgrading alloy to avoid misuing their API and causing errors.

This alloy version includes generic integer types, so instead of implementing those on our end, we special case the built-in integer sizes so they work with both primitive ints and alloy generics.

This is a rather large refactor of the stylus proc macros with the main goal of improving maintainability, testability, and documentation of these macros. The following changes have been made: - Operate on AST nodes instead of raw tokens. This allows testing of smaller parts of the macros as unit tests. This also allows some composibility within the implementations. - Use the visitor pattern in several places to avoid confusing nesting of for-loops. - Separation of export-abi code using "extention" traits so that it is easy to reason about export-abi when needed, and otherwise ignore it. - Improved error reporting. The main change here is the ability to report multiple errors when compilation fails, allowing the user to see all errors at once. There is also some small changes with regards to consistency between the macros. - Unit tests for utilities and macro generation at the AST level. This includes a `assert_ast_eq()` function which can be used to compare two ASTs of the same type. If an error occurs, the ASTs are pretty printed and a diff is output so that the issue can be easily addressed. - Integration tests in the `stylus-proc/tests` directory. These allow compiled macros to be tested as working correctly. Additionally the `stylus-proc/tests/fail` directory will contain failing integration tests. These all fail to compile and include expected stderr outputs. - Modify code snippets within docstrings so that they are not ignored, and pass as part of the `cargo test` process. - A `README.md` which will contain information about developing and maintaining these proc macros. - Support for `solidity.path.separator` to `rust::path::separator` for custom structs within `sol_interface!` definitions. - Change the folder structure to have all macro implementations in a `macros` module. Easier to find the macro implementation you are looking for. - Docstrings within the macro implementations for developer reference. - Utilities for handing inner attributes including those which contain tokens which need to be parsed. A little more work is going to be done before this is ready for merge, but it will be good to get this out for review since it is such a large set of changes.

chrisco512 · 2024-10-03T23:47:11Z

stylus-proc/src/consts.rs

+use proc_macro2::{Span, TokenStream};
+use quote::ToTokens;
+
+/// Name of the entrypoint funciton that is generated for struct-based contracts.


typo on function

chrisco512 · 2024-10-04T08:57:02Z

stylus-proc/src/imports.rs

+
+        use crate::imports::ConstPath;
+
+        pub const Address: ConstPath = ConstPath("stylus_sdk::alloy_sol_types::sol_data::Address");


This is cool, why stop at Address though?

Right now I just have the imports that were declared as constants before for easy interpolation. Could potentially add more in the future as needed.

chrisco512 · 2024-10-04T09:08:15Z

stylus-proc/src/macros/derive/abi_type.rs

+use syn::{parse_macro_input, parse_quote};
+
+use crate::imports::stylus_sdk::abi::AbiType;
+


This is really nice!

chrisco512 · 2024-10-04T09:41:29Z

stylus-proc/src/macros/derive/solidity_error/mod.rs

+        self.match_arms.push(parse_quote! {
+            #self_name::#name(e) => stylus_sdk::call::MethodError::encode(e),
+        });
+        self._ext.add_variant(field);


This is throwing odd warnings. I guess since the SolidityErrorExtension trait is implemented for () you can just remove the self._extpart?

What is meaning of

impl SolidityErrorExtension for () { type Ast = Nothing; fn add_variant(&mut self, _field: syn::Field) {} fn codegen(_err: &DeriveSolidityError<Self>) -> Self::Ast { Nothing } }

?

the warning I get from clippy:

passing a unit value to a function
for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unit_arg
#[warn(clippy::unit_arg)] on by defaultclippyClick for full compiler diagnostic
mod.rs(56, 9): move the expression in front of the call and replace it with the unit literal (): self._ext; ().add_variant(field)

So the extension here (and in public macro), is designed to do nothing by default. That's why the default impl uses a () and returns Nothing. When the export-abi feature is enabled the Extension type alias points to a type that actually provides the logic. This just avoids having #[cfg(feature = "export-abi")] in a bunch of different places

chrisco512 · 2024-10-09T00:36:12Z

stylus-sdk/src/abi/mod.rs

+{
+    T::SolType::abi_encode(value)
+}
+


We have encode and encode_params, and we've got decode_params, should we also add a decode wrapper?

I'm not quite sure yet, it wasn't needed for these changes but I do want to take a broader look at how we handle abi encoding/decoding and revisit then.

chrisco512 · 2024-10-09T00:51:41Z

stylus-sdk/src/abi/mod.rs

+    let encoded = encode_params(&value);
+    assert_eq!(encoded, buffer.as_ref());
+    // TODO: debug this assertion for test encode_decode_bytes_tuple()
+    // assert_eq!(abi::encoded_size(&value), encoded.len());


This is failing on the encode_decode_bytes_tuple test from abi/bytes.rs. Is there a bug here?

Not that I am aware of. I haven't had a chance to dig into this but bytes have been working correctly. I'll revisit this with some other abi encoding/decoding changes mentioned above.

joshuacolvin0

LGTM

rorytrent and others added 7 commits September 16, 2024 17:06

Add some encoding/decoding tests

9ef0b17

These tests are important before upgrading alloy to avoid misuing their API and causing errors.

Upgrade alloy to latest 0.8.3

10c9c86

Upgrade alloy to 0.8.3

49b2953

This alloy version includes generic integer types, so instead of implementing those on our end, we special case the built-in integer sizes so they work with both primitive ints and alloy generics.

Update proc macro dependencies

f5820f2

use correct type for solidity type info

cc0efe5

add additional proc macro tests

bc9d654

rory-ocl marked this pull request as ready for review October 1, 2024 16:39

chrisco512 reviewed Oct 9, 2024

View reviewed changes

chrisco512 previously approved these changes Oct 9, 2024

View reviewed changes

Minor fixes for proc macro refactor

bf077d6

rory-ocl dismissed chrisco512’s stale review via bf077d6 October 10, 2024 16:42

joshuacolvin0 approved these changes Oct 10, 2024

View reviewed changes

rory-ocl merged commit fa990e4 into develop Oct 10, 2024
3 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor proc macros #164

Refactor proc macros #164

rory-ocl commented Sep 30, 2024 •

edited

Loading

chrisco512 Oct 3, 2024

chrisco512 Oct 4, 2024

rory-ocl Oct 9, 2024

chrisco512 Oct 4, 2024

chrisco512 Oct 4, 2024

chrisco512 Oct 9, 2024

rory-ocl Oct 10, 2024

chrisco512 Oct 9, 2024

rory-ocl Oct 10, 2024

chrisco512 Oct 9, 2024

rory-ocl Oct 10, 2024

joshuacolvin0 left a comment


		use crate::imports::ConstPath;

		pub const Address: ConstPath = ConstPath("stylus_sdk::alloy_sol_types::sol_data::Address");

		use syn::{parse_macro_input, parse_quote};

		use crate::imports::stylus_sdk::abi::AbiType;

Refactor proc macros #164

Refactor proc macros #164

Conversation

rory-ocl commented Sep 30, 2024 • edited Loading

Description

Checklist

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

Choose a reason for hiding this comment

joshuacolvin0 left a comment

Choose a reason for hiding this comment

rory-ocl commented Sep 30, 2024 •

edited

Loading