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

Full support for empty tuples #276

Merged
merged 1 commit into from Mar 3, 2021

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Mar 1, 2021

What was wrong?

Even though functions do always implicitly return empy tuples it isn't yet supported to actually use the empty tuple syntax (()) in code.

E.g. the following doesn't work:

  pub def explicit_return():
    return ()

This doesn't work either:

  pub def explicit_return() -> ():
    pass

With this change, all of these are treated equal:

contract Foo:

  pub def explicit_return_a1():
    return

  pub def explicit_return_a2():
    return ()

  pub def explicit_return_b1() ->():
    return

  pub def explicit_return_b2() ->():
    return ()

  pub def implicit_a1():
    pass

  pub def implicit_a2() ->():
    pass

How was it fixed?

  1. Taught the Parser how to parse tuples via the tuple_type function
  2. Added tests for tuple parsing
  3. Added support for (empty!) tuples in the analyzer and mapper pass
  4. Taught the mapper to treat return () as if there is no return at all (leave)
  5. A few more tests

To-Do

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

  • Clean up commit history

typ: type_desc(type_defs, &return_type.node)?,
}]
match type_desc(type_defs, &return_type.node)? {
// Should there be a different ABI for `pub def foo() -> ():` and `pub def foo():`?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@g-r-a-n-t I'm unsure about this. On the one hand, it would feel weird if pub def foo() -> (): and pub def foo(): resulted in different ABI types. On the other hand, forcing pub def foo() -> (): to become pub def foo():` would prevent people from matching a given ABI spec they are trying to implement. Or is this a non-issue because of something that I might be missing?

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to treat pub def foo() -> (): the same as pub def foo():, since both technically return an empty tuple. It might even make sense to emit a warning in the case of pub def foo() -> ():.

The ABI spec does say that you can have an empty tuple..

(T1,...,Tk) for k >= 0 and any types T1, …, Tk

So there is room for this sort of thing in the ABI, I'm just very skeptical of there being an instance where putting an empty tuple in the json ABI makes sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm just very skeptical of there being an instance where putting an empty tuple in the json ABI makes sense

So am I but let's say there's a project that defines some interface IStrategy with a method pub def do_something(val: Whatever) -> (). The way I understand it, one would not be able to write a strategy in Fe that produces an ABI that matches that ABI signature because the Fe compiler would always normalize it to pub def do_something(val: Whatever):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll merge this as is. If this turns out to become a problem we can address it later.

@cburgdorf cburgdorf merged commit 27e3e7a into ethereum:master Mar 3, 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.

None yet

2 participants