Tuple support. #352

g-r-a-n-t · 2021-03-31T20:32:05Z

To support tuples, we need the following lowerings:

1.) New struct definitions need to be added for each tuple that is used in the module.

For example, if (u256, bool) is used somewhere in a module, we must add the following struct definition to the module:

struct TUPLE_U256_BOOL:
  item0: u256
  item1: bool

2.) Tuple type descriptions need to be mapped to struct type descriptions.

my_tuple: (u256, bool) -> my_tuple: TUPLE_U256_BOOL

3.) Tuple constructions need to be mapped to struct constructions

(42, true) -> TUPLE_U256_BOOL(item0=42, item1=true)

bonus: Tuple deconstruction can be mapped to multiple assignments.

(foo, bar): (u256, bool) = my_tuple

->

foo: u256 = my_tuple.item0
bar: bool = my_tuple.item1

Included in this PR:

Lowerings 1, 2, and 3 mentioned above
Testing for lowered source files (added some coverage for aug assigns too)
Refactoring around "safe names". Basically, I'm trying to figure out how we should go about creating unique identifiers for generated code. In this PR, we're using the names provided by the SafeNames trait to to generate identifiers for the tuple structs. These names may conflict with user defined types, which is something that needs to be addressed.

One thing to note is that empty tuples are not lowered, since we don't have zero-sized structs. It's not a big deal, but it would be nice to have all tuple branches marked as unimplemented in the Yul codegen pass.

To-Do

Write an issue for tuple destructuring.
Write an issue for name collisions as mentioned in the 3rd bullet.
Add entry to the release notes (may forgo for trivial changes)
Clean up commit history

analyzer/src/lib.rs

analyzer/src/namespace/types.rs

compiler/src/lowering/mappers/module.rs

compiler/tests/lowering/main.rs

compiler/tests/stress.rs

codecov-commenter · 2021-04-23T01:09:05Z

Codecov Report

Merging #352 (71f32a1) into master (e64b98e) will increase coverage by 0.33%.
The diff coverage is 89.12%.

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
+ Coverage   82.80%   83.14%   +0.33%     
==========================================
  Files          62       64       +2     
  Lines        3862     3980     +118     
==========================================
+ Hits         3198     3309     +111     
- Misses        664      671       +7

Impacted Files	Coverage Δ
analyzer/src/traversal/declarations.rs	`96.66% <ø> (ø)`
analyzer/src/traversal/expressions.rs	`92.07% <ø> (ø)`
compiler/src/yul/operations/abi.rs	`94.02% <ø> (ø)`
compiler/src/yul/operations/data.rs	`100.00% <ø> (ø)`
compiler/src/yul/runtime/functions/abi.rs	`96.25% <ø> (ø)`
compiler/src/abi/elements.rs	`84.61% <60.00%> (-2.43%)`	⬇️
analyzer/src/namespace/types.rs	`87.22% <84.10%> (+1.09%)`	⬆️
analyzer/src/lib.rs	`77.43% <100.00%> (+1.78%)`	⬆️
analyzer/src/namespace/events.rs	`100.00% <100.00%> (ø)`
analyzer/src/namespace/scopes.rs	`95.43% <100.00%> (+0.02%)`	⬆️
... and 23 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 e64b98e...71f32a1. Read the comment docs.

compiler/src/lowering/mappers/contracts.rs

compiler/src/lowering/mappers/module.rs

compiler/src/lowering/names.rs

analyzer/src/lib.rs

analyzer/src/namespace/scopes.rs

analyzer/src/namespace/types.rs

g-r-a-n-t · 2021-04-27T20:36:22Z

analyzer/src/namespace/types.rs

@@ -82,30 +82,24 @@ pub struct AbiComponent {
 }


I grouped trait implementations together in this file, so there is some noise.

analyzer/src/namespace/types.rs

g-r-a-n-t · 2021-04-27T20:36:45Z

analyzer/src/namespace/types.rs


    /// The ABI type of a Fe type.
    fn abi_type(&self) -> AbiType;
 }

+/// Names that can be used to build identifiers.
+pub trait SafeNames {


analyzer/src/traversal/expressions.rs

compiler/src/lowering/mappers/expressions.rs

parser/tests/test_parser.rs

compiler/tests/lowering/main.rs

cburgdorf

Heroic effort! ⚔️ 👷
Left a few comments inline but just tiny suggestions. The most important thing I think is opening issues for a few things left and right.

cburgdorf · 2021-04-28T08:30:36Z

analyzer/src/lib.rs

@@ -297,6 +318,8 @@ impl Context {
            contracts: HashMap::new(),
            calls: HashMap::new(),
            events: HashMap::new(),
+            type_descs: HashMap::new(),
+            module: None,


Did you consider using ModuleAttributes instead of Option<ModuleAttributes> and using ModuleAttributes::new() which would in turn initialize with an empty HashMap and BTreeSet. This would feel slightly more symmetric with the rest of the Context and eliminate checking for Some(_) down the road. But it may make other things trickier so I'm just curious if you considered it and deliberately went for this design.

This is what I went for initially, but then realized fe::Module isn't wrapped in a node and therefore doesn't have a nodeId.

analyzer/src/traversal/expressions.rs

cburgdorf · 2021-04-28T08:46:36Z

analyzer/src/traversal/expressions.rs

@@ -670,6 +711,19 @@ fn expr_call_value_attribute(
                        Location::Memory,
                    ))
                }
+                Type::Tuple(tuple) => {
+                    if value_attributes.final_location() != Location::Memory {
+                        todo!("encode tuple from storage")


Would be good to create an issue for this.

compiler/src/lowering/mappers/module.rs

cburgdorf · 2021-04-28T09:08:10Z

compiler/src/lowering/mappers/types.rs

+    let typ = context.get_type_desc(&desc).expect("missing attributes");
+
+    match typ {
+        Type::Tuple(tuple) if !tuple.is_empty() => {


I think we should allow to define zero sized structs probably just as struct Something. That's struct followed by the name and then not followed by a :. Rust has support for empty structs, too so we are in good company.

Not saying that has to be in this PR so. But worth creating an issue for.

This is what I was thinking too 👍

cburgdorf · 2021-04-28T09:11:41Z

compiler/tests/fixtures/demos/uniswap.fe

-            num2=self.reserve1,
-            num3=self.block_timestamp_last
-        )
+    pub def get_reserves() -> (u256, u256, u256):


Super cool to see it in action and leading to this code cleanup 👍

cburgdorf · 2021-04-28T09:17:17Z

compiler/tests/fixtures/stress/tuple_stress.fe

@@ -0,0 +1,43 @@
+contract Foo:
+    my_sto_tuple: (u256, i32)


There's this todo!("encode tuple from storage") in the analyzer...what exactly is it doesn't yet work with tuples in storage yet. Is it that we can not yet move an entire tuple from storage to memory?

Right now someone would need to do self.my_sto_tuple.to_mem().abi_encode() instead of self.my_sto_tuple.abi_encode(). It's a low priority todo imo, we might not event want to support encoding values from storage.

cburgdorf · 2021-04-28T09:24:02Z

compiler/tests/lowering/fixtures/base_tuple_lowered.fe

@@ -0,0 +1,68 @@
+struct tuple_u256_bool:


Regarding the naming collisions. We could suffix all of them with an uuid and call it a day (e.g. tuple_u256_bool_ce6819ea_e0a1_4af2_92f5_7d3be21437d1). Alternatively I guess we could try to generate the most human readable name possible (so, tuple_u256_bool) unless we detect that the user has also used the same name in which case we just append something like _1. In any case, we should create an issue for that as it should have a high priority.

Btw, super cool that we have these lowering tests now!

I'd prefer to have a solution that does not involve checking to see if a name is already take. Something to discuss in the issue once it's written..

cburgdorf · 2021-04-28T09:25:55Z

compiler/tests/lowering/fixtures/base_tuple_lowered.fe

+
+    pub def bing():
+        foo: tuple_address_address_u16_i32_bool = tuple_address_address_u16_i32_bool(
+            item0=address(0),


Random fact, C# also exposes tuple items as item0, item1 etc (just with a capitalized I) so we are in good company.

Nice! Correct decision for sure 😆

g-r-a-n-t force-pushed the tuple-support branch 2 times, most recently from 6a6bb93 to 3fe10c6 Compare April 22, 2021 19:05

g-r-a-n-t commented Apr 22, 2021

View reviewed changes

g-r-a-n-t marked this pull request as draft April 24, 2021 17:32

g-r-a-n-t force-pushed the tuple-support branch from 0466977 to e8a3b47 Compare April 26, 2021 21:25