Move tuple and list expr collection to lowering pass; support lowering of nested tuples #459

sbillig · 2021-06-20T00:56:27Z

What was wrong?

Another salsa-inspired change. The analyzer collects a bunch of stuff for the later passes, but those passes can collect the stuff just as easily.

How was it fixed?

Added a couple context structs to the lowering pass, and removed tuples_used and list_expressions stuff from the analyzer.

While I was mucking about, I added lowering support for nested tuples, because I can't stop myself from sneaking unrelated stuff into a PR. Generated tuple struct names now include a trailing underscore to differentiate between different tuple nestings. I initially made this a special case for nested tuples, but the special casing didn't seem as aesthetically pleasing to me. Conceptually, the generated tuple struct name replaces each paren and comma with an underscore, and each item type name with its lowered type name.

((A, B), C, D) // tuple_tuple_A_B__C_D_
((A, B, C), D) // tuple_tuple_A_B_C__D_
((A, B, C, D),) // tuple_tuple_A_B_C_D__

Also tuples in type aliases weren't being lowered; eg type T = (u8, u8); they are now. I called this a feature, because I guess tuple type aliases are new functionality.

To-Do

a bit o' cleanup
rebase on master when Move ast::ModuleStmt and ContractStmt variants into separate structs #458 is merged
add type alias fix to release notes
add test for lowering of nested tuples

codecov-commenter · 2021-06-20T20:15:11Z

Codecov Report

Merging #459 (2719d3e) into master (0ab0f5d) will decrease coverage by 0.14%.
The diff coverage is 95.35%.

❗ Current head 2719d3e differs from pull request most recent head 6807996. Consider uploading reports for the commit 6807996 to get more accurate results

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   87.33%   87.18%   -0.15%     
==========================================
  Files          72       73       +1     
  Lines        4975     5011      +36     
==========================================
+ Hits         4345     4369      +24     
- Misses        630      642      +12

Impacted Files	Coverage Δ
analyzer/src/traversal/expressions.rs	`91.30% <ø> (-0.09%)`	⬇️
analyzer/src/traversal/types.rs	`100.00% <ø> (ø)`
parser/src/node.rs	`58.82% <ø> (ø)`
analyzer/src/namespace/types.rs	`82.73% <62.50%> (-0.34%)`	⬇️
compiler/src/lowering/mappers/module.rs	`83.33% <78.78%> (-12.32%)`	⬇️
compiler/src/yul/mappers/module.rs	`86.66% <87.50%> (ø)`
parser/src/ast.rs	`48.88% <90.90%> (ø)`
parser/src/grammar/module.rs	`86.25% <93.33%> (ø)`
compiler/src/yul/mappers/contracts.rs	`97.14% <95.83%> (ø)`
analyzer/src/traversal/contracts.rs	`97.46% <96.66%> (ø)`
... and 38 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 0ab0f5d...6807996. Read the comment docs.

cburgdorf

Looks good and makes sense. Left a few comments inline.

cburgdorf · 2021-06-21T09:16:10Z

.github/workflows/main.yml

@@ -7,7 +7,6 @@ on:
      - v*

  pull_request:
-    branches: [master]


Was this intended? What does it do?

Yeah; that's in a separate commit. It enables gh actions for all PRs, regardless of the target branch. Sometimes I open a PR that depends on another that's pending review/merge; so I push the branch of the first to the main repo, and have the second PR target that branch (temporarily) so that the changes of the first don't show in the second's diff. (This one depends on #458)

Ah, makes sense 👍

cburgdorf · 2021-06-21T09:17:01Z

analyzer/src/context.rs

@@ -53,8 +53,6 @@ pub struct ContractAttributes {
    /// Events that have been defined by the user.
    pub events: Vec<EventDef>,
    /// List expressions that the contract uses
-    pub list_expressions: BTreeSet<Array>,
-    /// Static strings that the contract defines


Looks like the wrong comment was removed. (Should have removed the comment from line 55

cburgdorf · 2021-06-21T09:56:43Z

compiler/Cargo.toml

@@ -28,6 +28,7 @@ maplit = "1.0.2"
 once_cell = "1.5.2"
 vec1 = "1.8.0"
 num-bigint = "0.3.1"
+indexmap = "1.6.2"


cburgdorf · 2021-06-21T10:07:10Z

tests/fixtures/lowering/base_tuple_lowered.fe

@@ -1,52 +1,52 @@
-struct tuple_u256_bool:
+struct __Tuple0:


Did you consider alterations to the current naming scheme that would preserve the readability better? E.g. we could encode the opening and closing parenthesis with some character like an extra _.

((A, B), C, D) // _tuple__tuple_A_B__C_D_ ((A, B, C), D) // _tuple__tuple_A_B_C__D_

Yeah, I was thinking that that's less readable in the case of a couple nestings, but it's better in the simpler/common cases, so I'll switch it back.

yes, I guess having to differentiate between two easy to confuse tuples will be rather rare so maybe it's better to keep it readable for the common cases.

It's now

(A, B) => tuple_A_B_ (A, (B, C), D) => tuple_A_tuple_B_C__D_

ie, replace parens and commas with underscores, and prefix with "tuple"

Looks good 👍

cburgdorf · 2021-06-21T10:10:21Z

tests/src/lowering.rs

+    //  nested structs aren't supported yet. we should move the
+    //  not-yet-implemented error to the compiler.
+    //
+    // fe_analyzer::analyze(&actual_lowered_ast, src_id)


Probably worth creating an issue for it and putting it on the active milestone unless you have that work already in progress anyway.

Opened #461

Also adds support lowering of tuples nested within other types

sbillig marked this pull request as ready for review June 20, 2021 19:32

sbillig force-pushed the lowering-context branch from 2b0b629 to c8ad714 Compare June 20, 2021 19:34

sbillig force-pushed the lowering-context branch 3 times, most recently from ce066af to d6db2d5 Compare June 21, 2021 06:18

sbillig requested a review from cburgdorf June 21, 2021 06:21

cburgdorf approved these changes Jun 21, 2021

View reviewed changes

sbillig mentioned this pull request Jun 21, 2021

Move (some) not-yet-implemented errors to later stages #461

Open

sbillig changed the title ~~Move tuple and list expr collection to lowering pass~~ Move tuple and list expr collection to lowering pass; support lowering of nested tuples Jun 21, 2021

sbillig force-pushed the lowering-context branch 2 times, most recently from 52be986 to 768df08 Compare June 22, 2021 19:27

sbillig added 3 commits June 22, 2021 12:41

Move tuple and list expr collection from analyzer to lowering

ae25b3a

Also adds support lowering of tuples nested within other types

github: run workflows on PRs to non-master branches

bad7cc0

Lower tuples in type aliases

6807996

sbillig force-pushed the lowering-context branch from 768df08 to 6807996 Compare June 22, 2021 19:42

sbillig changed the base branch from ast-item-structs to master June 22, 2021 19:42

sbillig mentioned this pull request Jun 22, 2021

Add support for nested tuples #427

Closed

sbillig merged commit b0caf12 into ethereum:master Jun 22, 2021

sbillig deleted the lowering-context branch June 22, 2021 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move tuple and list expr collection to lowering pass; support lowering of nested tuples #459

Move tuple and list expr collection to lowering pass; support lowering of nested tuples #459

sbillig commented Jun 20, 2021 •

edited

Loading

codecov-commenter commented Jun 20, 2021 •

edited

Loading

cburgdorf left a comment

cburgdorf Jun 21, 2021

sbillig Jun 21, 2021

cburgdorf Jun 22, 2021

cburgdorf Jun 21, 2021

cburgdorf Jun 21, 2021

cburgdorf Jun 21, 2021

sbillig Jun 21, 2021

cburgdorf Jun 22, 2021

sbillig Jun 22, 2021

cburgdorf Jun 22, 2021

cburgdorf Jun 21, 2021

sbillig Jun 22, 2021

Move tuple and list expr collection to lowering pass; support lowering of nested tuples #459

Move tuple and list expr collection to lowering pass; support lowering of nested tuples #459

Conversation

sbillig commented Jun 20, 2021 • edited Loading

What was wrong?

How was it fixed?

To-Do

codecov-commenter commented Jun 20, 2021 • edited Loading

Codecov Report

cburgdorf left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbillig commented Jun 20, 2021 •

edited

Loading

codecov-commenter commented Jun 20, 2021 •

edited

Loading