feat: immutable variables #2466

skellet0r · 2021-10-01T16:47:41Z

This is a very³ basic attempt at enabling immutable variables

What I did

Attempt to add a new keyword immutable to vyper, which enables immutable variables.

Immutable variables act like constants, except they are set at runtime instead of compile-time. At construction, the value of the immutable is copied to the end of the runtime code returned. Runtime code later accesses the immutable value via codecopy, and any attempts to modify the variable outside of the constructor raises an error at compile time.

Fixes: #1164

Immutables can be any base type + user defined structs + lists

How I did it

tl;dr - Piggy backed off charles work in other PRs and mainly added keywords all over the place

Basically, a majority of the edits involved adding the keyword is_immutable to a bunch of the primitives and definition classes. Just going through the class inheritence until stuff compiled.

The more important changes I think are in the expr.py, parser.py, and module.py files.

`module.py`

Here I modified the visit_AnnAssign function to allow the immutable keyword, simply adding an additional branch, and doing other validation.

Here is also where I went through the class inheritance of certain types to allow for values to be defined as immutable.

`expr.py`

Here I modified the parse_Name function. Essentially there are two cases to look out for when it comes to immutables.

The constructor
runtime code

In the constructor, immutables are ... well very mutable, so they're simply regular variables, and as such we allocate memory for them and return a pointer to where they are in memory (not an internal variable as those get deallocated).

Outside of the constructor however, we return the pointer to where the immutable is stored in the runtime code. Each immutable is appended to the runtime code, and their offset can be determined by taking the sum of all immutables allocated space, subtracting the offset from the runtime code, and then subtracting that from the actual runtime code size. codesize - immutable_space + offset

This part actually takes advantage of the new code location introduced in ccb167d.

`parser.py`

Here I modify the constructor code, After generating the runtime LLL, we create the constructor, which is a simple
[return, 0, ['lll', runtime_code, 0] LLLnode. I modified this, so that if any immutables are defined we instead copy the runtime code to a spot in front of the last immutable defined (this is to prevent mangling of the immutables in memory), and then append the immutables to the runtime code at their appropriate offsets, and then return the runtime code + the appended immutables.

How to verify it

Eventually the following should be able to compile and function correctly: ✔️

# define the immutable variable
VALUE: immutable(uint256)


@external
def __init__(_value: uint256):
    # assign a value to the immutable
    VALUE = _value


@view
@external
def get_value() -> uint256:
    # get the value assigned at construction
    return VALUE

Tests include:

Syntax checks
Base type definitions (set + get)
Multiple immutable definitions (set + get)
User defined Struct defintion (set + get)
List defintiion (set + get)

Description for the changelog

Enable the definition of immutable state variables which must be set within the constructor

Cute Animal Picture

fubuloubu · 2021-10-01T16:54:52Z

In general, love this idea. Would you say this is a solution for #1164 though? I think it is (not just "related")

codecov-commenter · 2021-10-01T16:59:08Z

Codecov Report

Merging #2466 (9bd3b45) into master (55ec927) will increase coverage by 0.04%.
The diff coverage is 91.81%.

@@            Coverage Diff             @@
##           master    #2466      +/-   ##
==========================================
+ Coverage   85.61%   85.65%   +0.04%     
==========================================
  Files          90       90              
  Lines        8946     9023      +77     
  Branches     2050     2069      +19     
==========================================
+ Hits         7659     7729      +70     
- Misses        809      815       +6     
- Partials      478      479       +1

Impacted Files	Coverage Δ
vyper/semantics/namespace.py	`96.87% <ø> (ø)`
vyper/semantics/types/function.py	`84.61% <ø> (ø)`
vyper/semantics/types/value/address.py	`91.66% <ø> (ø)`
vyper/old_codegen/types/types.py	`81.18% <20.00%> (-1.79%)`	⬇️
vyper/semantics/validation/module.py	`87.76% <82.35%> (-0.24%)`	⬇️
vyper/old_codegen/expr.py	`80.47% <91.66%> (+0.47%)`	⬆️
vyper/semantics/types/bases.py	`84.15% <95.45%> (+0.91%)`	⬆️
vyper/ast/signatures/function_signature.py	`87.70% <100.00%> (+0.30%)`	⬆️
vyper/compiler/utils.py	`100.00% <100.00%> (ø)`
vyper/lll/compile_lll.py	`94.95% <100.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ec927...9bd3b45. Read the comment docs.

skellet0r · 2021-10-03T05:20:02Z

In general, love this idea. Would you say this is a solution for #1164 though? I think it is (not just "related")

Yeah definitely agree, I should've marked it as such in the beginning.

Also, I rebased off of #2447 to benefit from some of the work charles did, so when that is merged into master, I can rebase again to clear out the commit clutter. Cherry-picking through would've been a pain, rebasing was easier.

TODO:

testing

lgtm-com · 2021-10-03T05:35:10Z

This pull request introduces 2 alerts when merging 73b4da3 into 030bde6 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fubuloubu

@charles-cooper to comment on the approach and where it fits in with the 0.3.x refactor

tests/parser/features/test_immutable.py

skellet0r · 2021-10-04T08:10:56Z

~~NOTE: This was rebased on top of PR #2468 to take advantage of the work done to make_setter~~

tests/parser/features/test_immutable.py

charles-cooper · 2021-10-04T17:09:26Z

@skellet0r there are a bunch of formatting related changes in here. Could you change your formatter/linter so that the PR is cleaner?

skellet0r · 2021-10-04T17:13:13Z

@skellet0r there are a bunch of formatting related changes in here. Could you change your formatter/linter so that the PR is cleaner?

I'll turn off pre-commit 😢, it's using the defaults in pyproject.toml + setup.cfg though

tests/parser/syntax/test_immutables.py

fubuloubu · 2021-10-04T19:20:46Z

@skellet0r there are a bunch of formatting related changes in here. Could you change your formatter/linter so that the PR is cleaner?

Can we do this fixup as a separate PR? Seems like things are out of sync

skellet0r · 2021-10-04T19:36:41Z

Can we do this fixup as a separate PR? Seems like things are out of sync

@fubuloubu yeh, a PITA, but definitely doable, would just require redoing the PR with pre-commit turned off.

skellet0r · 2021-10-04T22:34:44Z

i'm such a dummy, was going to manually go through this PRs history and pick apart the formatting changes ....
The smart thing to do which i did was just issue a new chore/lint PR, which when this is rebased off of that, will clean up all those formatting diffs. #2472

@fubuloubu @charles-cooper

charles-cooper

Generally looking good!

I left some comments in the code. I think the biggest change to the approach overall is (ab)using the asm symbol resolver to statically get the locations of the immutables in the data section at compile time.

vyper/old_codegen/expr.py

vyper/old_codegen/parser.py

vyper/old_codegen/types/types.py

vyper/old_codegen/parser.py

vyper/old_codegen/expr.py

charles-cooper · 2021-10-06T01:02:45Z

vyper/old_codegen/expr.py

+                )
+                offset = self.expr._metadata["type"].position.offset
+                return LLLnode.from_list(
+                    ["sub", "codesize", immutable_section_size - offset],


This is interesting. I wonder if we can get the location of the data section into the runtime code during deploy time.

(Thinking through the approach, it may not be worth it, but if you have some clever ideas here I am all ears.)

Here's an idea: our assembly-to-evm compilation phase has a step which resolves symbols to locations. Since the locations of immutables are statically known (no matter what value they are set to), we can insert symbols into the LLL which directly precede blank space. Basically, _sym_foo doesn't actually take any space in the code. So the runtime data section itself would have something like

_sym_datasection_start BLANK _sym_immutable_uint256 JUMPDEST... <32 JUMPDESTs> _sym_immutable_bytes6 JUMPDEST... <32+6 jumpdests>

Then your code for copying items to LLL would stay the same (it would just overwrite all the jumpdests), but we would know the locations statically.

OK let's add a TODO here for now: TODO: resolve code offsets for immutables at compile time

vyper/old_codegen/parser.py

vyper/semantics/validation/module.py

vyper/old_codegen/expr.py

Changed across codebase to prevent future naming confusion when the `immutable` keyword is added.

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

Instead when parsing the immutable modify the variable record in the global ctx

skellet0r · 2021-11-12T07:26:25Z

rebased + fixed mypy error

Next steps: should we implement data section in this PR (and importantly how ... ) or implement data section in separate PR (which may for review and testing)

vyper/old_codegen/global_context.py

vyper/semantics/types/indexable/sequence.py

vyper/semantics/types/bases.py

charles-cooper · 2021-11-12T16:53:55Z

vyper/old_codegen/expr.py

+                )
+                offset = self.expr._metadata["type"].position.offset
+                return LLLnode.from_list(
+                    ["sub", "codesize", immutable_section_size - offset],


OK let's add a TODO here for now: TODO: resolve code offsets for immutables at compile time

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>

… time

fubuloubu · 2021-11-12T22:04:20Z

rebased + fixed mypy error

Next steps: should we implement data section in this PR (and importantly how ... ) or implement data section in separate PR (which may for review and testing)

If it is easy to separate out these two changes, I would. they both seem like major changes

charles-cooper · 2021-11-12T23:15:20Z

rebased + fixed mypy error
Next steps: should we implement data section in this PR (and importantly how ... ) or implement data section in separate PR (which may for review and testing)

If it is easy to separate out these two changes, I would. they both seem like major changes

Just want to point out here that this PR does technically introduce a data section - just the structure is mostly implicit. A goal of a future PR would be to refactor the API for accessing and using it.

vyper/old_codegen/parser.py

skellet0r force-pushed the feat/immutable branch from 003e0cf to 73b4da3 Compare October 3, 2021 05:15

skellet0r force-pushed the feat/immutable branch from 73b4da3 to dc6a50c Compare October 3, 2021 06:00

fubuloubu reviewed Oct 4, 2021

View reviewed changes

tests/parser/features/test_immutable.py Outdated Show resolved Hide resolved

tests/parser/features/test_immutable.py Outdated Show resolved Hide resolved

skellet0r force-pushed the feat/immutable branch from 621269b to a1f33b7 Compare October 4, 2021 05:59

skellet0r changed the title ~~WIP: immutable variables~~ feat: immutable variables Oct 4, 2021

skellet0r marked this pull request as ready for review October 4, 2021 08:09

fubuloubu reviewed Oct 4, 2021

View reviewed changes

tests/parser/features/test_immutable.py Outdated Show resolved Hide resolved

charles-cooper reviewed Oct 4, 2021

View reviewed changes

tests/parser/syntax/test_immutables.py Outdated Show resolved Hide resolved

skellet0r force-pushed the feat/immutable branch from 71b18a5 to 861ac3e Compare October 4, 2021 19:40

skellet0r force-pushed the feat/immutable branch 2 times, most recently from 42861f5 to 1c5d062 Compare October 5, 2021 19:45

charles-cooper requested changes Oct 6, 2021

View reviewed changes

skellet0r force-pushed the feat/immutable branch 2 times, most recently from 8cf26db to b7e353e Compare October 21, 2021 17:12

charles-cooper mentioned this pull request Oct 22, 2021

optimization: don't store bytestring length when it is statically known #2505

Open

skellet0r added 5 commits November 12, 2021 07:10

chore: replace is_immutable -> is_constant

f4f3ea8

Changed across codebase to prevent future naming confusion when the `immutable` keyword is added.

feat: add immutable as a reserved keyword

b06886e

feat: handle immutable defs in ModuleNodeVisitor

d967cf2

fix: add is_immutable kwarg to get_type_from_annotation

9a407b6

feat: add is_immutable keyword to primitive classes

ba27726

skellet0r and others added 11 commits November 12, 2021 07:10

chore: modify test parametrization

ec13acf

Co-authored-by: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com>

docs: add immutable usage docs

b95bef8

fix: allocate memory of immutable in constructor

14b2edb

fix: disallow multiple assignments to immutable

10c73e4

test: multiple assignments blocked

42981ab

fix: set immutable data location to CODE

3e4de6e

fix: verify immutable is given a single argument

48d4691

fix: if stmt use bool type as condition

b0fe010

fix: make immutable data size a cached prop on global ctx

b60ae2e

fix: swap 'lll' arguments, offset is first code is second

9d04be1

fix: remove _metadata dict on LLLnode for immutables

99217d9

Instead when parsing the immutable modify the variable record in the global ctx

skellet0r force-pushed the feat/immutable branch from b7e353e to 99217d9 Compare November 12, 2021 07:22

fix: mypy typing error

7c01275

charles-cooper reviewed Nov 12, 2021

View reviewed changes

vyper/old_codegen/global_context.py Outdated Show resolved Hide resolved

charles-cooper reviewed Nov 12, 2021

View reviewed changes

vyper/semantics/types/indexable/sequence.py Outdated Show resolved Hide resolved

charles-cooper reviewed Nov 12, 2021

View reviewed changes

skellet0r and others added 3 commits November 12, 2021 18:16

Update vyper/semantics/types/indexable/sequence.py

b55dfe1

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>

Update vyper/semantics/types/bases.py

502bb6a

Co-authored-by: Charles Cooper <cooper.charles.m@gmail.com>

fix: use cached property from vyper.utils

90c8594

skellet0r force-pushed the feat/immutable branch from 417e6aa to 713a7bd Compare November 12, 2021 18:30

chore: leave todo, come back and resolve immutable offsets at compile…

713a7bd

… time

charles-cooper approved these changes Nov 12, 2021

View reviewed changes

charles-cooper reviewed Nov 12, 2021

View reviewed changes

vyper/old_codegen/parser.py Outdated Show resolved Hide resolved

update a comment about lll macro

9bd3b45

charles-cooper enabled auto-merge (squash) November 13, 2021 00:21

charles-cooper merged commit 017ef0e into vyperlang:master Nov 13, 2021

skellet0r deleted the feat/immutable branch November 13, 2021 03:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: immutable variables #2466

feat: immutable variables #2466

skellet0r commented Oct 1, 2021 •

edited

Loading

fubuloubu commented Oct 1, 2021

codecov-commenter commented Oct 1, 2021 •

edited

Loading

skellet0r commented Oct 3, 2021 •

edited

Loading

lgtm-com bot commented Oct 3, 2021

fubuloubu left a comment

skellet0r commented Oct 4, 2021 •

edited

Loading

charles-cooper commented Oct 4, 2021

skellet0r commented Oct 4, 2021

fubuloubu commented Oct 4, 2021

skellet0r commented Oct 4, 2021

skellet0r commented Oct 4, 2021

charles-cooper left a comment

charles-cooper Oct 6, 2021

charles-cooper Oct 6, 2021

charles-cooper Oct 6, 2021

charles-cooper Nov 12, 2021

skellet0r commented Nov 12, 2021

charles-cooper Nov 12, 2021

fubuloubu commented Nov 12, 2021

charles-cooper commented Nov 12, 2021

feat: immutable variables #2466

feat: immutable variables #2466

Conversation

skellet0r commented Oct 1, 2021 • edited Loading

What I did

How I did it

module.py

expr.py

parser.py

How to verify it

Description for the changelog

Cute Animal Picture

fubuloubu commented Oct 1, 2021

codecov-commenter commented Oct 1, 2021 • edited Loading

Codecov Report

skellet0r commented Oct 3, 2021 • edited Loading

lgtm-com bot commented Oct 3, 2021

fubuloubu left a comment

Choose a reason for hiding this comment

skellet0r commented Oct 4, 2021 • edited Loading

charles-cooper commented Oct 4, 2021

skellet0r commented Oct 4, 2021

fubuloubu commented Oct 4, 2021

skellet0r commented Oct 4, 2021

skellet0r commented Oct 4, 2021

charles-cooper left a comment

Choose a reason for hiding this comment

charles-cooper Oct 6, 2021

Choose a reason for hiding this comment

charles-cooper Oct 6, 2021

Choose a reason for hiding this comment

charles-cooper Oct 6, 2021

Choose a reason for hiding this comment

charles-cooper Nov 12, 2021

Choose a reason for hiding this comment

skellet0r commented Nov 12, 2021

charles-cooper Nov 12, 2021

Choose a reason for hiding this comment

fubuloubu commented Nov 12, 2021

charles-cooper commented Nov 12, 2021

skellet0r commented Oct 1, 2021 •

edited

Loading

`module.py`

`expr.py`

`parser.py`

codecov-commenter commented Oct 1, 2021 •

edited

Loading

skellet0r commented Oct 3, 2021 •

edited

Loading

skellet0r commented Oct 4, 2021 •

edited

Loading