vyper icon indicating copy to clipboard operation
vyper copied to clipboard

feat: use kwargs for struct instantiation

Open tserg opened this issue 1 year ago • 5 comments

What I did

closes: https://github.com/vyperlang/vyper/issues/2381

Change the instantiation of a struct from a dictionary to a callable with kwargs.

How I did it

Update the analysis and codegen, as well as grammar.

How to verify it

See tests

Commit message

feat: use kwargs for struct instantiation

This PR changes the instantiation of a struct from a dictionary to a 
callable with kwargs.

Description for the changelog

Use kwargs for struct instantiation.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

tserg avatar Feb 13 '24 14:02 tserg

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1fc819c) 84.99% compared to head (a777514) 85.00%. Report is 2 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3777   +/-   ##
=======================================
  Coverage   84.99%   85.00%           
=======================================
  Files          92       92           
  Lines       13717    13731   +14     
  Branches     3079     3079           
=======================================
+ Hits        11659    11672   +13     
- Misses       1570     1572    +2     
+ Partials      488      487    -1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 13 '24 14:02 codecov-commenter

btw isn't this tracked in a VIP somewhere? please link

fubuloubu avatar Feb 14 '24 18:02 fubuloubu

btw isn't this tracked in a VIP somewhere? please link

Yes, I have updated the top-level post with the link to the VIP.

tserg avatar Feb 15 '24 02:02 tserg

btw isn't this tracked in a VIP somewhere? please link

Yes, I have updated the top-level post with the link to the VIP.

if you use closes: #X it will link and close the VIP as completed when this is merged

fubuloubu avatar Feb 15 '24 16:02 fubuloubu

btw isn't this tracked in a VIP somewhere? please link

Yes, I have updated the top-level post with the link to the VIP.

if you use closes: #X it will link and close the VIP as completed when this is merged

I was under the impression that this PR partially addressed the issue, as there was a separate discussion about having default values, but then I noticed your last comment. Thanks anyway!

tserg avatar Feb 16 '24 03:02 tserg

I'm not sure whether this is intended behaviour or simply a regression tbh but if I use now the same variable name for the kwarg, it will fail:

image

struct Batch:
    target: address
    allow_failure: bool
    call_data: Bytes[max_value(uint16)]


struct Result:
    success: bool
    return_data: Bytes[max_value(uint8)]


@external
def multicall(data: DynArray[Batch, max_value(uint8)]) -> DynArray[Result, max_value(uint8)]:
    results: DynArray[Result, max_value(uint8)] = []
    return_data: Bytes[max_value(uint8)] = b""
    success: bool = empty(bool)
    for batch: Batch in data:
        if (batch.allow_failure == False):
            return_data = raw_call(batch.target, batch.call_data, max_outsize=255)
            success = True
            results.append(Result({success: success, return_data: return_data})) # This will pass
            results.append(Result({success=success, return_data=return_data})) # This will raise
        else:
            success, return_data = \
                raw_call(batch.target, batch.call_data, max_outsize=255, revert_on_failure=False)
            results.append(Result({success: success, return_data: return_data})) # This will pass
            results.append(Result({success=success, return_data=return_data})) # This will raise
    return results

Since the intention was to make it backward-compatible, using e.g. success=success should also compile IMO.

pcaversaccio avatar Feb 20 '24 07:02 pcaversaccio

results.append(Result({success=success, return_data=return_data})) # This will raise

The curly brackets are dropped in the new syntax, so this should compile:

results.append(Result(success=success, return_data=return_data))

tserg avatar Feb 20 '24 07:02 tserg

results.append(Result({success=success, return_data=return_data})) # This will raise

The curly brackets are dropped in the new syntax, so this should compile:

results.append(Result(success=success, return_data=return_data))

Ah interesting, that indeed compiles, thx! Added this new feature to snekmate modules branch: https://github.com/pcaversaccio/snekmate/pull/207/commits/0c3a20308ef46cfe51cef4912704faa608bea712. Vey nitpick: we should always follow PEP-8 where it states:

Don’t use spaces around the = sign when used to indicate a keyword argument

The two following tests don't follow this:

  • https://github.com/vyperlang/vyper/pull/3777/files#diff-cb5228f23a8a9317e7de9bc3780d62cdfd69bb2655261be60e55143a874615f4
  • https://github.com/vyperlang/vyper/pull/3777/files#diff-51af4b3b600a938c59a20303d4a83f82f12746942d85c99bc7284bad5abfdc43

pcaversaccio avatar Feb 20 '24 07:02 pcaversaccio

Also, I think it would make sense to update the docs here with the new syntax.

pcaversaccio avatar Feb 20 '24 08:02 pcaversaccio

Also, I think it would make sense to update the docs here with the new syntax.

Good point, I will follow up on this and the tests. Thanks!

tserg avatar Feb 20 '24 08:02 tserg