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

Add support for structs #203

Merged
merged 3 commits into from
Feb 8, 2021
Merged

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Jan 25, 2021

What was wrong?

Fe should support structs to allow better user defined abstractions.

How was it fixed?

This PR lays the ground work for struct support as demonstrated with the following test.

struct House:
    price: u256
    size: u256
    vacant: bool

contract Foo:

    pub def bar() -> u256:
        building: House = House(300, 500, true)
        assert building.size == 500
        assert building.price == 300
        assert building.vacant

        building.vacant = false
        building.price = 1
        building.size = 2

        assert building.vacant == false
        assert building.price == 1
        assert building.size == 2
        return building.size

There are still a few open todos which were moved into dedicated issues (see todos)

To-Do

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #203 (9ac8ca6) into master (8b3e813) will decrease coverage by 0.21%.
The diff coverage is 90.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
- Coverage   94.26%   94.04%   -0.22%     
==========================================
  Files          51       53       +2     
  Lines        3487     3679     +192     
==========================================
+ Hits         3287     3460     +173     
- Misses        200      219      +19     
Impacted Files Coverage Δ
analyzer/src/namespace/operations.rs 84.84% <0.00%> (-2.66%) ⬇️
compiler/src/yul/mappers/expressions.rs 97.16% <ø> (ø)
compiler/src/yul/mappers/module.rs 75.00% <ø> (ø)
compiler/src/yul/runtime/functions/mod.rs 100.00% <ø> (ø)
parser/src/ast_traits.rs 80.64% <0.00%> (-5.57%) ⬇️
analyzer/src/namespace/types.rs 81.22% <65.38%> (-1.76%) ⬇️
analyzer/src/traversal/expressions.rs 94.88% <93.33%> (-0.28%) ⬇️
parser/src/parsers.rs 99.85% <96.96%> (-0.15%) ⬇️
analyzer/src/lib.rs 91.86% <100.00%> (+0.34%) ⬆️
analyzer/src/traversal/module.rs 100.00% <100.00%> (ø)
... and 7 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 8b3e813...9ac8ca6. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/feat/struct branch 4 times, most recently from 56f8ffb to 515c565 Compare January 29, 2021 10:32
@cburgdorf cburgdorf force-pushed the christoph/feat/struct branch 3 times, most recently from dc80649 to dca8f72 Compare February 3, 2021 10:24
@cburgdorf cburgdorf force-pushed the christoph/feat/struct branch 2 times, most recently from 883bf53 to b1af6a5 Compare February 4, 2021 15:55
compiler/src/yul/names.rs Outdated Show resolved Hide resolved
output/Foo/Foo_ir.yul Outdated Show resolved Hide resolved
parser/src/parsers.rs Outdated Show resolved Hide resolved
Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

Cool! Just one comment for now, will look over more thoroughly today.

compiler/src/yul/operations/calls.rs Outdated Show resolved Hide resolved
analyzer/src/namespace/types.rs Outdated Show resolved Hide resolved
let body = statement! { return_val := alloc(0) };
return function_definition! {
function [function_name]() -> return_val {
[body]
Copy link
Member

Choose a reason for hiding this comment

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

Could we just return 0 here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But what we return here is a pointer address if I return 0 doesn't this end up as being wrongfully interpreted as the pointer address where the (non-existent) struct is being stored. As far as I understand the pointer address 0 actually stores the highest available pointer so we would end up pointing there, no?

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand the pointer address 0 actually stores the highest available pointer so we would end up pointing there, no?

That's correct, but if it's an empty struct, we should be able to make the assumption that we'll never write or read to the address. Using alloc here performs a read and write on the 0x00 address that has no effect , since we're allocating 0 bytes. If anything, we would use the avail function to get the highest available memory pointer (which would return the same thing that alloc(0) does). But again, since this pointer is never used, we could just simplify things and return 0. If there's a situation where we write to an empty struct, then we have a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, that makes sense. I'll do the change and will put a comment on it to explain it.

@cburgdorf cburgdorf force-pushed the christoph/feat/struct branch 2 times, most recently from 37e2737 to 68a4024 Compare February 8, 2021 18:00
@cburgdorf cburgdorf merged commit 584f5ac into ethereum:master Feb 8, 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.

3 participants