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

Require KWargs for struct initialization #260

Merged

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Feb 19, 2021

What was wrong?

Example:

struct House:
    vacant: bool
    price: u256

Currently, House can be instantiated as House(true, 1000000).

This means:

  1. Property assignment can easily get mixed up leading to bugs
  2. struct initialization is visually very close to regular function invocations

How was it fixed?

With this change it is required to be instantiated like House(vacant=true, price=1000000)

While working on this it got me thinking if this is something that should be handled on the parser stage instead but I don't think that would be straight forward because technically there is nothing the syntax is identical with method invocation syntax.

We could move this into the parser stage if we used a distinct syntax such as House(vacant: true, price: 100). But I'm personally ok to keep it as with the check being handled by the analyzer.

@cburgdorf cburgdorf force-pushed the christoph/feat/struct_invocation_kw_args branch from 9f2d8e8 to 8a309ba Compare February 19, 2021 10:07
@cburgdorf cburgdorf force-pushed the christoph/feat/struct_invocation_kw_args branch from 8a309ba to 5d92140 Compare February 19, 2021 10:16
@g-r-a-n-t
Copy link
Member

g-r-a-n-t commented Feb 20, 2021

Something I noticed while working on #189 is that initialization doesn't work much of the time, since the values are in an unpredictable order upon retrieval.

The following will fail to compile for me:

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

contract Foo:
    pub def create_house():
        self.my_house = House(1, 2, u8(5), false)

as the attribute types just so happen to be retrieved in the the order u256, u8, u256, bool.

I'll fix this in my PR and we can go ahead and merge this.

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

fixed. I just added another attribute for key order to the struct struct

@cburgdorf cburgdorf merged commit 13d74ef into ethereum:master Feb 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.

2 participants