Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a unit type #406

Merged
merged 1 commit into from May 20, 2021
Merged

Added a unit type #406

merged 1 commit into from May 20, 2021

Conversation

g-r-a-n-t
Copy link
Member

@g-r-a-n-t g-r-a-n-t commented May 12, 2021

What was wrong?

closes #293
closes #407

We had two separate representations of empty tuples in the Yul output. One being 0x0 and the other being a function call with no return. This led to the issue discovered in #293.

How was it fixed?

1.) Enabled empty struct definitions using pass (also did events and contracts).
2.) Lowered empty tuples to a struct type named empty_tuple.
3.) Removed all tuple mappings in the Yul codegen stage.

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@g-r-a-n-t
Copy link
Member Author

decided Im going to hold off on this and wait for zero-sized structs so we can full implement tuple lowering

@g-r-a-n-t g-r-a-n-t changed the title Consistent empty tuples in IR Lowering of empty tuples May 14, 2021
analyzer/src/context.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/expressions.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/types.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/types.rs Outdated Show resolved Hide resolved
compiler/src/yul/constructor.rs Show resolved Hide resolved
compiler/src/yul/mappers/functions.rs Outdated Show resolved Hide resolved
parser/src/grammar/types.rs Outdated Show resolved Hide resolved
parser/tests/cases/parse_ast.rs Show resolved Hide resolved
test.fe Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2021

Codecov Report

Merging #406 (0ab2a1f) into master (934ffb5) will increase coverage by 0.12%.
The diff coverage is 89.58%.

❗ Current head 0ab2a1f differs from pull request most recent head 862d6f1. Consider uploading reports for the commit 862d6f1 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
+ Coverage   86.07%   86.20%   +0.12%     
==========================================
  Files          67       67              
  Lines        4008     4110     +102     
==========================================
+ Hits         3450     3543      +93     
- Misses        558      567       +9     
Impacted Files Coverage Δ
analyzer/src/operations.rs 83.58% <0.00%> (-1.27%) ⬇️
compiler/src/yul/mappers/expressions.rs 88.67% <50.00%> (+0.21%) ⬆️
analyzer/src/namespace/types.rs 85.05% <62.50%> (-2.59%) ⬇️
analyzer/src/context.rs 92.30% <66.66%> (+0.32%) ⬆️
compiler/src/lowering/mappers/functions.rs 98.91% <96.15%> (-1.09%) ⬇️
analyzer/src/traversal/expressions.rs 92.71% <100.00%> (+0.40%) ⬆️
analyzer/src/traversal/functions.rs 99.15% <100.00%> (+0.37%) ⬆️
compiler/src/abi/elements.rs 84.61% <100.00%> (ø)
compiler/src/lowering/mappers/expressions.rs 100.00% <100.00%> (ø)
compiler/src/lowering/mappers/types.rs 100.00% <100.00%> (ø)
... and 14 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 934ffb5...862d6f1. Read the comment docs.

compiler/src/lowering/mappers/functions.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/functions.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/functions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/functions.rs Outdated Show resolved Hide resolved
compiler/src/yul/mappers/functions.rs Outdated Show resolved Hide resolved
@g-r-a-n-t g-r-a-n-t force-pushed the agroce-293 branch 2 times, most recently from dc722a0 to a844a30 Compare May 17, 2021 22:51
analyzer/src/context.rs Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/functions.rs Outdated Show resolved Hide resolved
@sbillig
Copy link
Collaborator

sbillig commented May 18, 2021

@g-r-a-n-t It's maybe a bit late, but what if we introduce a Unit type (https://doc.rust-lang.org/std/primitive.unit.html) which just happens to resemble a tuple with no elements, and disallow empty tuples? We could use https://crates.io/crates/nonempty or https://crates.io/crates/vec1 to enforce the non-emptiness of the tuple elements vec.

That seems like a more aesthetically pleasing solution than lowering to an empty struct with a special name.

@g-r-a-n-t
Copy link
Member Author

@sbillig Yeah, I think that makes sense. The is_empty_tuple functions are also something that I'm not a big fan of.

I'd be happy to try it out in this PR.

analyzer/src/namespace/types.rs Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
analyzer/src/traversal/expressions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/functions.rs Show resolved Hide resolved
compiler/src/yul/constructor.rs Outdated Show resolved Hide resolved
compiler/src/yul/runtime/abi_dispatcher.rs Show resolved Hide resolved
@g-r-a-n-t g-r-a-n-t changed the title Lowering of empty tuples Added a unit type May 19, 2021
@g-r-a-n-t
Copy link
Member Author

Added a unit type @sbillig, this is ready for a review.

Copy link
Collaborator

@sbillig sbillig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

newsfragments/406.internal.md Outdated Show resolved Hide resolved
@g-r-a-n-t g-r-a-n-t merged commit 3d1a9da into ethereum:master May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow pass statement on contract level Yul compilation failed: Call or assignment expected
3 participants