feat[lang]: use keyword arguments for struct instantiation

[tserg]

What I did

closes: #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 keyword arguments for struct instantiation

This commit changes struct instantiation from taking a single dict
argument to taking keyword arguments.

For backwards compatibility (and to ease users into v0.4.0), the old
syntax is still supported, but a warning will be emitted.

Description for the changelog

Use kwargs for struct instantiation.

Cute Animal Picture

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fubuloubu]

love this change

[fubuloubu]

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

[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.

[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

[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

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!

[charles-cooper]

this is looking good. we should add a warning if somebody tries to use the old way, instead of failing. i don't think we should break compatibility for at least a few releases, so people have time to ease into it.

i think we can keep support for the old way by adding an AST transformation, when Calls are constructed you can check if the argument is a dict, and transform it.

[pcaversaccio]

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:

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.

[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))

[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))

Ah interesting, that indeed compiles, thx! Added this new feature to snekmate modules branch: pcaversaccio/snekmate@0c3a203. 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]

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

[tserg]

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!