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

feat: implement dynamic arrays #2556

Merged
merged 100 commits into from
Jan 6, 2022

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Dec 4, 2021

What I did

This commit implements Solidity-ABI-compatible dynamic arrays in vyper.

Dynamic arrays have the type DynArray[<type>, <maxlen>]. They are
implemented similarly to bytestrings, with a single length word followed
by the data, <len> <data...>.

How I did it

Threaded through typechecker, added abi encoder/decoder and added the relevant functionality in codegen

How to verify it

See tests

Description for the changelog

add dynamic arrays to vyper

Cute Animal Picture

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

@charles-cooper charles-cooper force-pushed the dynamic_arrays branch 3 times, most recently from ce872df to 2eba8e0 Compare December 6, 2021 20:46
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 6, 2021

This pull request introduces 1 alert when merging 2eba8e0 into fac2152 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 7, 2021

This pull request introduces 2 alerts when merging bea42c4 into fac2152 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 7, 2021

This pull request introduces 2 alerts when merging b1c93ec into fac2152 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 13, 2021

This pull request introduces 1 alert when merging 8bdbcdd into 654b21d - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 13, 2021

This pull request introduces 1 alert when merging 04fb957 into 654b21d - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 14, 2021

This pull request introduces 2 alerts when merging 3fa97c2 into 654b21d - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 14, 2021

This pull request introduces 3 alerts and fixes 2 when merging b398f15 into 654b21d - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import
  • 1 for Syntax error

fixed alerts:

  • 1 for `__eq__` not overridden when adding attributes
  • 1 for Use of the return value of a procedure

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Dec 20, 2021

This pull request introduces 4 alerts when merging c33e36d into c843b68 - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times
  • 1 for Unused local variable
  • 1 for Unused import

@charles-cooper charles-cooper changed the title [WIP] dynamic arrays feat: implement dynamic arrays Jan 3, 2022
@charles-cooper charles-cooper marked this pull request as ready for review January 3, 2022 19:34
@@ -74,11 +74,12 @@ event_def: _EVENT_DECL NAME ":" ( event_body | _PASS )

// Types
array_def: (NAME | array_def) "[" (DEC_NUMBER | NAME) "]"
tuple_def: "(" ( NAME | array_def | tuple_def ) ( "," ( NAME | array_def | tuple_def ) )* [","] ")"
dyn_array_def: "DynArray" "[" (NAME | array_def | dyn_array_def) "," (DEC_NUMBER | NAME) "]"
tuple_def: "(" ( NAME | array_def | dyn_array_def | tuple_def ) ( "," ( NAME | array_def | dyn_array_def | tuple_def ) )* [","] ")"
Copy link
Member

Choose a reason for hiding this comment

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

Tuple of DynArrays shouldn't cause any issues right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Theoretically no, we should add it to list of things to QA

vyper/old_codegen/lll_node.py Show resolved Hide resolved
Comment on lines +189 to +192
_check(repeat_count is None or repeat_count.valency == 1, repeat_count)
_check(counter_ptr.valency == 1, counter_ptr)
_check(start.valency == 1, start)
_check(body.valency == 0, body)
Copy link
Member

Choose a reason for hiding this comment

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

alright so maybe saves a little code here

vyper/semantics/validation/annotation.py Show resolved Hide resolved
vyper/semantics/validation/utils.py Show resolved Hide resolved
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Jan 6, 2022

This pull request introduces 1 alert when merging dc05585 into 4385110 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@@ -20,7 +20,11 @@
from vyper.semantics.namespace import get_namespace
from vyper.semantics.types.abstract import IntegerAbstractType
from vyper.semantics.types.bases import BaseTypeDefinition
from vyper.semantics.types.indexable.sequence import ArrayDefinition, TupleDefinition
from vyper.semantics.types.indexable.sequence import (
ArrayDefinition,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be StaticArrayDefinition to match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be, I didn't want to change the existing type name

Copy link
Member Author

Choose a reason for hiding this comment

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

This commit implements Solidity-ABI-compatible dynamic arrays in vyper.

Dynamic arrays have the type `DynArray[<type>, <maxlen>]`. They are
implemented similarly to bytestrings, with a single length word followed
by the data, <len> <data...>.

It also adds some code to clarify between the sizes of objects
when they are in memory vs in storage.

This commit also refactors a lot of code dealing with encoding and byte
copying. Byte copying is cheaper now since previously the logic was
```
for i in range(maxlen // 32):
    if i > len:
        break
    sstore(dst + i, mload(src + 32*i))
```

This was necessary because the `repeat` opcode in LLL requires a max
bound. Now, we have repeat(len, maxlen), which still enforces the max
bound but breaks once it hits the minimum of `len` and `maxlen`. Now the
byte copier loop has the logic of:

```
for i in range( min(len, maxlen) ):
    sstore(dst + i, mload(src + 32*i))
```

It also adds a method to the LLLnode API, `cache_when_complex`. This
abstracts the logic for checking when an LLL expression needs to be
cached using a `with` expression. This was used to simplify the ABI
encoder and several routines related to copying (including
`make_setter`).
@charles-cooper charles-cooper merged commit ef1e6e4 into vyperlang:master Jan 6, 2022
@charles-cooper charles-cooper deleted the dynamic_arrays branch January 6, 2022 22:18
charles-cooper added a commit that referenced this pull request Feb 13, 2022
this fixes an array index clamp bug introduced in #2556 where negative
numbers are not clamped properly  
                                  
it also                           
                                  
- streamlines logic by taking advantage of downstream optimizations
- blocks user indexing into `empty` arrays
- cleans up optimizer code        
- adds various `slice` tests (in preparation for rewrite)
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

3 participants