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

Allow multi-variable declaration for tuple assignment to use explicit types. #3520

Closed
chriseth opened this issue Feb 15, 2018 · 43 comments
Closed
Assignees

Comments

@chriseth
Copy link
Contributor

With the deprecation of var, it is not possible anymore to easily assign the result of a function to local variables. We should allow types to be specified in such assignments:

(unit a, uint b, SomeStruct c) = f();
@redsquirrel
Copy link
Contributor

Are the surrounding parentheses necessary?

@chriseth
Copy link
Contributor Author

Not sure, we always had them to make the tuples explicit.

@chriseth
Copy link
Contributor Author

chriseth commented Feb 15, 2018

Also it might be confusing for people coming from C or JavaScript (because there, , is almost like ; and thus the assignment in the above example would only apply to c).

@axic
Copy link
Member

axic commented Feb 15, 2018

With the deprecation of var, it is not possible anymore to easily assign the result of a function to local variables.

This is not true, it is only true in conjunction with the disallow-uninitalised-storage-pointers and only affects types/variables residing int storage.

Probably a better example would be one using such a case.

@chriseth
Copy link
Contributor Author

I would say that

uint a;
uint b;
uint c;
(a, b, c) = f();

already counts as "not easy".

@redsquirrel
Copy link
Contributor

redsquirrel commented Feb 15, 2018

Also it might be confusing for people coming from C or JavaScript

Yeah, I'm coming from Ruby where this is valid:

a, b = [1, 2]

@data-harmonization
Copy link

This post was exactly what I was looking for! In all the Ethereum examples, the "var" keyword is used when you want to assign values to a tuple function (e.g. var (x,y) = f() ). Since "var" is now deprecated, how should a multi variable assignment be done?

@data-harmonization
Copy link

I just found the solution (credits to: https://ethereum.stackexchange.com/questions/21348/get-the-return-of-a-function-with-multiple-returns)

  uint256 c;
  uint256 d;
  (c, d) = f();

The following is also supported in case you're only interested in d :

  uint256 d;
  ( , d) = f();

@ekpyron ekpyron self-assigned this Apr 4, 2018
@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2018

To be clear:

The grammer of a tuple expression should consequently be changed from

TupleExpression = '(' ( Expression? ( ',' Expression? )* )? ')' | '[' ( Expression ( ',' Expression )* )? ']'

to

TupleExpression = '(' ( (Expression, VariableDeclaration)? ( ',' (Expression, VariableDeclaration)? )* )? ')' | '[' ( Expression ( ',' Expression )* )? ']'

?

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2018

No, probably not. I guess it makes more sense to change VariableDefinition, resp VariableDeclaration to support tuple types instead, since tuples of variable declarations are no expressions...

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2018

So changing

VariableDefinition = ('var' IdentifierList | VariableDeclaration) ( '=' Expression )?

to

VariableDefinition = ('var' IdentifierList | VariableDeclaration | VariableDeclarationList) ( '=' Expression )?
VariableDeclarationList = '(' ( VariableDeclaration? ',' )* VariableDeclaration? ')'

is probably more like what we want?

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2018

Should the assignment be required for multi-variable declarations or should

(uint a, uint b, SomeStruct c);

be valid as well?

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2018

If the assignment should be required, that would probably mean

VariableDefinition = ( ( ('var' IdentifierList | VariableDeclaration) ( '=' Expression )? ) | (VariableDeclarationList '=' Expression) )
VariableDeclarationList = '(' ( VariableDeclaration? ',' )* VariableDeclaration? ')'

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2018

A potential option would be:

SimpleStatement = VariableDefinition | TupleDefinition | ExpressionStatement
VariableDeclarationList = '(' ( VariableDeclaration? ',' )* VariableDeclaration? ')'
TupleDefinition = VariableDeclarationList '=' Expression

@axic just said we should postpone and further discuss this, though.

@chriseth
Copy link
Contributor Author

chriseth commented Apr 4, 2018

I would say that assignment is required, otherwise it is just weird, even if this adds a special case.

@axic
Copy link
Member

axic commented Apr 4, 2018

I am not fully convinced this is a problem to be solved.

One benefit of not having it is the visual clarity that new variables are declared, whereas in a tuple declaration they would be less visible.

@axic
Copy link
Member

axic commented Apr 4, 2018

It is kind of the same discussion whether we should have multiple variable declarations on the same line or not. Currently we do not support that.

@pirapira
Copy link
Member

@ekpyron I think (int a, int b) = X; can appear without (int a, int b). The first is desired just because X is a tuple.

@chriseth
Copy link
Contributor Author

I think singleton tuples and tuple types are out of scope for this discussion. They are currently both possible, the latter is just not nameable. We certainly should disallow catch-all tuple components, but that is also addressed in a different issue.

While (uint, uint, uint) (a, b, c) looks good from a language theoretic viewpoint, I don't think it is a usability improvement over (uint a, uint b, uint c), as it has almost the same drawback and creates a new concept that needs to be learnt by users (they already know (uint a, uint b, uint c) from function headers).

@pirapira
Copy link
Member

And further in the future, substitute (uint a, uint b, uint c) = f() into g(uint a, uint b, uint c) and obtain g f().

@axic
Copy link
Member

axic commented Apr 10, 2018

Can we have some actual examples where this feature is badly needed?

@ekpyron
Copy link
Member

ekpyron commented Apr 10, 2018

@chriseth I'm not sure whether (uint a, uint b, uint c) is less of a new concept than (uint, uint, uint) (a,b,c)...
To me function headers are quite different - function f() returns (uint a, uint b, uint c) { (a,b,c) = (1,2,3); still requires to mention (a,b,c) again, whereas function f() returns (uint a, uint b, uint c) { return (1,2,3); } is the same as function f() returns (uint, uint, uint) { return (1,2,3); }...

However (uint, uint, uint) (a,b,c) = ... is practically the same as var (a,b,c) = ..., just with explcit types for safety...

@ekpyron
Copy link
Member

ekpyron commented Apr 10, 2018

@axic Ultimately it's a convenience feature, so I don't think there's cases in which it is really badly needed... However, I think it may be worth considering, whether promoting tuple types to first-class types that can actually be named rather than only having them somewhat implicitly in assignments and function returns, wouldn't be cleaner in general...

@chriseth
Copy link
Contributor Author

Yeah, it's probably true that this is only badly needed in cases where the types cannot be named anyway.

@chriseth
Copy link
Contributor Author

Ah right, if one of the variables is a storage pointer, it might be very hard to refactor the warning away, and it might require multiple storage reads.

@chriseth
Copy link
Contributor Author

If we introduce tuple types, I would probably prefer somethnig like uint*uint*uint (a, b,c) = ... - but we should check how other languages did this.

In general, I still think that most solidity users would find (uint a, uint b, uint c) = ... more natural, but we can make a survey.

@axic
Copy link
Member

axic commented Apr 10, 2018

I think somewhat relevant is the discussion on how restricted variable declarations are at the moment: they do not allow multiple declarations (with or without value assignment) in the same statement.

If those are like that, why is tuple declaration an exception?

I do not mean any of this is good or bad, but worth looking at the bigger picture.

@chriseth
Copy link
Contributor Author

I don't see the difference between a tuple declaration and the declaration of multiple variables in the same statement. You can declare multiple variables in the same statement, you just need to add parentheses.

@axic
Copy link
Member

axic commented Apr 10, 2018

You can declare multiple variables in the same statement, you just need to add parentheses.

What do you mean by that? Do you have an example?

@chriseth
Copy link
Contributor Author

Of course you need var because I was too lazy to change the parser accordingly back in the days: var (a, b, c, d).

@axic
Copy link
Member

axic commented Apr 10, 2018

Since var is deprecated, would you say that it should be made possible without var?

@chriseth
Copy link
Contributor Author

Yes, exactly. It should be possible to assign function return values to newly declared variables because a variable declaration with assignment looks safer (and probably is) than without.

@axic
Copy link
Member

axic commented Apr 10, 2018

Not talking about function return values, but actual variable declarations.

@chriseth
Copy link
Contributor Author

Yes, the declaration of multiple variables in the same statement without assigning values is redundant and can be removed.

@pirapira
Copy link
Member

pirapira commented Apr 10, 2018

For me this feature is a plus, because I can follow the single assignment style.

Without this feature, I have to write

<declarations of a and b>

<around here a and b contain the default value that's never needed (BAD)>

(a, b) = f();

With this feature, I can write

(uint a, uint b) = f();

So, each of a and b receives one value ever.

@axic
Copy link
Member

axic commented Apr 10, 2018

Expanded the only case zeppelin does multi return values into the options discussed above:

contract C {
  function tokenGrant(address _holder, uint256 _grantId)
    constant
    returns (
      address granter,
      uint256 value,
      uint256 vested,
      uint64 start,
      uint64 cliff,
      uint64 vesting,
      bool revokable,
      bool burnsOnRevoke
    )
  {
  }

  function f() {
    address granter;
    uint value;
    uint vested;
    uint start;
    uint cliff;
    uint vesting;
    bool revokable;
    bool burnsOnRevoke;

    // note: implicit conversion of start/cliff/vesting from 64 to 256bit
    (granter, value, vested, start, cliff, vesting, revokable, burnsOnRevoke) = tokenGrant(0, 0);
  }

  function g() {
    // note: implicit conversion of start/cliff/vesting from 64 to 256bit
    (address granter, uint value, uint vested, uint start, uint cliff, uint vesting, bool revokable, bool burnsOnRevoke) = tokenGrant(0, 0);
  }

  function h() {
    // note: implicit conversion of start/cliff/vesting from 64 to 256bit
    address granter, uint value, uint vested, uint start, uint cliff, uint vesting, bool revokable, bool burnsOnRevoke = tokenGrant(0, 0);
  }

  function i() {
    // note: implicit conversion of start/cliff/vesting from 64 to 256bit
    (address, uint, uint, uint, uint, uint, bool, bool) (granter, value, vested, start, cliff, vesting, revokable, burnsOnRevoke) = tokenGrant(0, 0);
  }
}

@chriseth
Copy link
Contributor Author

Most of the use-cases will probably cease being use-cases when we have support for structs...

@chriseth
Copy link
Contributor Author

Discussion: let's go with the original proposal for now and try to make breaking releases more often.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants