feat[lang]: allow DynArrays of length 0 - #5234
Conversation
Gas ChangesNo changes detected. Summary
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c7cda67d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
|
@Sporarum tests seem to fail under venom |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06d3ae26ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not 0 <= length < 2**256: | ||
| raise InvalidType("Array length is invalid") |
There was a problem hiding this comment.
With the default legacy backend, a valid expression such as return [] if cond else [] from a function returning DynArray[uint256, 0] (or assigning that expression to such a local) reaches codegen and raises CompilerPanic: cannot dereference non-pointer type. Both arms now have the accepted zero-capacity type and remain ~empty non-pointers, so the ternary retains them without materializing memory and ABI encoding later tries to load their length. Simple return [] and the Venom backend compile successfully, making this a legacy-codegen failure for the newly enabled type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Unrelated to the current changes, see #5199
|
|
||
| # assert rounds <= round_bound | ||
| if rounds != rounds_bound: | ||
| if rounds != rounds_bound or rounds_bound.value == 0: |
There was a problem hiding this comment.
the new clause in the condition needs a detailed explanation for why rounds_bound.value == 0 requires the code generated below
charles-cooper
changed the title
DynArrays of length 0
charles-cooper
left a comment
charles-cooper
left a comment
There was a problem hiding this comment.
lgtm. mergeable pending 2nd pair of eyes from @HodanPlodky
|
It turned out to be simpler to refactor the code than to explain the existing one, see: 0b52720 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b652989d47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(rounds.value, int): | ||
| assert isinstance(rounds_bound.value, int) | ||
| assert 0 <= rounds.value <= rounds_bound.value |
There was a problem hiding this comment.
For documented direct VyperIR inputs, a literal rounds value is not otherwise constrained by IRnode.from_list; for example, repeat(i, 0, 1, 0, body) was previously compiled with the documented runtime rounds <= rounds_bound assertion and therefore reverted when executed. This new assertion instead aborts compilation for any literal count above its bound (and for negative literals), so invalid untrusted/count-derived IR can no longer be compiled into a safely reverting contract. Keep the runtime check for out-of-range literals or reject them with a user-facing IR validation error before lowering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@charles-cooper @HodanPlodky do we care about direct VyperIR inputs ?
(by care I mean preserve backwards compat)
There was a problem hiding this comment.
I tried it and it fails even on master in vyper/codegen/ir_node.py. There is a check repeat without 0 bound so why would this be a change against the current version, what am I missing?
There was a problem hiding this comment.
no i think the bot comment is being too pedantic
HodanPlodky
left a comment
HodanPlodky
left a comment
There was a problem hiding this comment.
This one test just seems bit odd to me but otherwise looks good
|
|
||
|
|
||
| @pytest.mark.xfail(raises=InvalidOperation) | ||
| def test_index_all_empty_lists_variable_index(get_contract, tx_failed): |
There was a problem hiding this comment.
are we planning to support this in future, since the return of the single of DynArray[Never, 0] will be handled correctly?
And also
@external
def foo(i: uint256) -> DynArray[uint256, 5]:
return [[], [1]][i]
would work as I would expect so what is the blocking this one to compile?
There was a problem hiding this comment.
are we planning to support this in future, since the return of the single of DynArray[Never, 0] will be handled correctly?
I think we should
@external def foo(i: uint256) -> DynArray[uint256, 5]: return [[], [1]][i]would work as I would expect so what is the blocking this one to compile?
It does work as you would expect (0 -> [], 1 -> [1], 2+ -> reverts)
The issue is that we use the element type's abi to compile even empty lists
With [[], [1]] we infer type SArrayT(DArrayT(uint256, 1), 2) for the whole expression, so [] is compiled as a DArrayT(uint256, 2) (or , 0], doesn't matter)
But with [[], []] we infer the type to SArrayT(DArrayT(BottomT, 0), 2), so we don't know how to compile the [] elements
This can be fixed in two ways:
- For nodes of type
DArrayT[T, 0], don't fetch the abi-encoding of the elements, just output an empty (discussed in private with @harkal as a possibility) - Annotate nodes with the expected type, and not the infered type, requires/part of Simplify Typer Internals #5017
And I think we should do both (but outside of scope for this PR)
pcaversaccio
left a comment
pcaversaccio
left a comment
There was a problem hiding this comment.
hmmm, if you use empty it doesn't compile:
@external
@pure
def foo(x: DynArray[uint256, empty(uint256)]) -> DynArray[uint256, empty(uint256)]:
return x
separate issue, having to do with compile-time elaboration of |
do we have an open issue on this? i found my old one here: #3480 |
|
i think you could create a new issue -- that one is not about availability of empty in the type system |
|
5 participants
What I did
Allow
DynArrays of length 0, including in user-written annotationsChange the type of
[]toDynArray[Never, 0](wasDynArray[Never, 1])Note: Does not allow
Bytes[0]andString[0](a different PR can do that).How I did it
Change bounds check on lengths for DynArrays from
>to>=How to verify it
pytest, also see new testsCommit message
Description for the changelog
Allow
DynArray[T, 0]type (which is only inhabited by[])Cute Animal Picture