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

fix: allow struct constants #2617

Merged
merged 25 commits into from
Mar 19, 2022
Merged

Conversation

tserg
Copy link
Collaborator

@tserg tserg commented Jan 23, 2022

What I did

Fix for #1430, extending on #2396.

The following code results in an error in v0.3.1:

# @version ^0.3.0

struct Foo:
    a: uint256
    b: uint256

CONST_BAR: constant(Foo) = Foo({a: 7, b: 2})
vyper.exceptions.StateAccessViolation: Value must be a literal
  contract "contracts/constantStruct1.vy", line 7:27 
       6
  ---> 7 CONST_BAR: constant(Foo) = Foo({a: 7, b: 2})
  ----------------------------------^
       8

[UPDATE]

After the above changes, attempting to return a struct constant or any of its member values resulted in an unhandled compiler error (see comments below) due to struct constants not being propagated during constant folding. I have made changes to handle constant propagation for folding of struct constants, and a minor change to handle constant struct members.

How I did it

Type checking for struct constants

  • Modify check_constant function in vyper/semantics/types/utils.py to handle structs by checking if each member is a literal value. For nested structs, make a recursive call.
  • Change visit_AnnAssign function of ModuleNodeVisitor class in vyper/semantics/validation/module.py to check node.value using check_constants (instead of check_literals), and to check for any reference to environment variables (which should not be accessible at compile time (?)).

Folding for struct constants

  • For constant top-level structs, replace the Name node with the entire Call node of the defined constant.
  • Where the constant to be replaced is a user-defined struct, loop through its members to also replace such references with the constant value of the respective struct member. This is done through a helper function _replace_struct_members, which takes in a path of parent attributes that should be present in a Name node that is to be replaced. For example, Foo.a.b where a is a struct will be represented as the following when used
`b`  (`Attribute` node)
 |---> `a` (`Attribute` node)
        |---> `Foo` (`Name` node)

but during construction we visit in the order Foo >> a >> b. Since the search for nodes to be replaced is premised on Name nodes (i.e. Foo), we need to store a path of parent attributes for all possible (plain and nested) struct members (including structs) in order to trace the path of parent attributes backwards.

  • Add an additional parameter parent_attributes_ids to replace_constant() helper function to supplement the id_ parameter, so as to enable additional filtering of nodes successively if they are referencing a struct member (plain or nested, including nested structs).
  • Handle Call AST node in _replace() helper function as this is the node type for structs.

How to verify it

See test cases.

Commit message

This commit enables constant structs, and constant struct members. 
It also introduces a new helper function named "check_kwargable",
that checks if a given node can be a kwarg to a function. This is 
meant to clarify the existing "check_constant" helper function, 
which actually checks whether a given node is not assignable.

The fix for constant structs is handled in the AST folding by
replacing references to a constant struct with the corresponding
Call AST node. The constant struct is then recursively checked
by the "check_constant" helper function to ensure all nested
structs and struct members are not assignable or a literal value.

This commit also involves a related change to the semantics 
validation of a function. Previously, both the validation of a 
function's kwargs and a module-level constant uses the same
"check_constant" function. While it previously conveniently 
covered both use cases of "not_assignable" and "is_kwargable",
this approach breaks for constant structs. Therefore, a new 
helper function "check_kwargable" has been added to separate
the use cases.

Resolves: #1430 
See also: #2396 

Description for the changelog

Allow constant structs.

Cute Animal Picture

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

@tserg
Copy link
Collaborator Author

tserg commented Jan 23, 2022

With the above changes, if I try to return a constant struct, I am now getting an unhandled error.

# @version ^0.3.0

struct Foo:
    a: uint256
    b: uint256

CONST_BAR: constant(Foo) = Foo({a: 7, b: 2})

@external
def baz() -> Foo:
    bar: Foo = Foo({a: 1, b: 2})
    return CONST_BAR
vyper.exceptions.TypeCheckFailure: Name node did not produce LLL. vyper.ast.nodes.Name:
     11     bar: Foo = Foo({a: 1, b: 2})  # Works fine!
---> 12     return CONST_BAR
-------------------^
     13

This is an unhandled internal compiler error. Please create an issue on Github to notify the developers.

@charles-cooper
Copy link
Member

With the above changes, if I try to return a constant struct, I am now getting an unhandled error.

# @version ^0.3.0

struct Foo:
    a: uint256
    b: uint256

CONST_BAR: constant(Foo) = Foo({a: 7, b: 2})

@external
def baz() -> Foo:
    bar: Foo = Foo({a: 1, b: 2})
    return CONST_BAR
vyper.exceptions.TypeCheckFailure: Name node did not produce LLL. vyper.ast.nodes.Name:
     11     bar: Foo = Foo({a: 1, b: 2})  # Works fine!
---> 12     return CONST_BAR
-------------------^
     13

This is an unhandled internal compiler error. Please create an issue on Github to notify the developers.

seems like the constant is not getting propagated during folding

@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2022

Codecov Report

Merging #2617 (bc5d026) into master (cfe26e1) will increase coverage by 0.41%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2617      +/-   ##
==========================================
+ Coverage   86.55%   86.96%   +0.41%     
==========================================
  Files          91       91              
  Lines        9852     9871      +19     
  Branches     2480     2478       -2     
==========================================
+ Hits         8527     8584      +57     
+ Misses        829      799      -30     
+ Partials      496      488       -8     
Impacted Files Coverage Δ
vyper/ast/folding.py 91.73% <76.19%> (-3.42%) ⬇️
vyper/semantics/types/utils.py 88.13% <88.88%> (-1.87%) ⬇️
vyper/semantics/types/function.py 84.92% <100.00%> (ø)
vyper/semantics/validation/module.py 86.97% <100.00%> (ø)
vyper/semantics/types/value/numeric.py 85.05% <0.00%> (-2.57%) ⬇️
vyper/codegen/core.py 83.26% <0.00%> (-1.13%) ⬇️
vyper/lll/compile_lll.py 94.18% <0.00%> (-0.35%) ⬇️
vyper/semantics/types/__init__.py 100.00% <0.00%> (ø)
vyper/semantics/types/abstract.py 100.00% <0.00%> (ø)
... and 10 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 cfe26e1...bc5d026. Read the comment docs.

@tserg tserg changed the title Fix type checking for struct constants fix: allow struct constants Jan 24, 2022
@tserg tserg marked this pull request as ready for review January 24, 2022 06:26
vyper/ast/folding.py Outdated Show resolved Hide resolved
vyper/ast/folding.py Outdated Show resolved Hide resolved
@tserg tserg marked this pull request as draft February 8, 2022 08:43
@tserg tserg marked this pull request as ready for review February 8, 2022 11:10
vyper/ast/folding.py Outdated Show resolved Hide resolved
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

nested structs don't work here

struct Foo:
    bar: Bar
struct Bar:
    baz: String[100]

da: constant(Foo) = Foo({bar: Bar({baz: "hello"})})

@external
@view
def get() -> Foo:
  return da
$ vyper --version && vyper tmp/bar.vy 
0.3.2+commit.f80aedb3

vyper.exceptions.StateAccessViolation: Value must be a literal
  contract "tmp/bar.vy", function "get", line 6:20 
       5
  ---> 6 da: constant(Foo) = Foo({bar: Bar({baz: "hello"})})
  ---------------------------^
       7

@tserg
Copy link
Collaborator Author

tserg commented Feb 16, 2022

nested structs don't work here

struct Foo:
    bar: Bar
struct Bar:
    baz: String[100]

da: constant(Foo) = Foo({bar: Bar({baz: "hello"})})

@external
@view
def get() -> Foo:
  return da
$ vyper --version && vyper tmp/bar.vy 
0.3.2+commit.f80aedb3

vyper.exceptions.StateAccessViolation: Value must be a literal
  contract "tmp/bar.vy", function "get", line 6:20 
       5
  ---> 6 da: constant(Foo) = Foo({bar: Bar({baz: "hello"})})
  ---------------------------^
       7

ah, will look into this

@tserg tserg marked this pull request as draft February 17, 2022 17:29
@tserg
Copy link
Collaborator Author

tserg commented Feb 18, 2022

Updated initial write-up

@tserg tserg marked this pull request as ready for review February 18, 2022 03:22
vyper/ast/folding.py Outdated Show resolved Hide resolved
vyper/ast/folding.py Outdated Show resolved Hide resolved
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

couple minor truthy checks need to be cleaned up but i think overall this is good

@tserg tserg marked this pull request as draft March 15, 2022 09:17
@tserg tserg force-pushed the fix/constant_structs branch 3 times, most recently from d3245bd to 8101c9c Compare March 17, 2022 01:30
@tserg tserg marked this pull request as ready for review March 17, 2022 01:58
@tserg tserg marked this pull request as draft March 18, 2022 05:04
@tserg tserg marked this pull request as ready for review March 18, 2022 07:24
raise StateAccessViolation("Value must be a literal", node.value)
if isinstance(node.value, vy_ast.Attribute):
# Check for environment variables
if node.value.get("value.id") in get_constant_vars():
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can remove this now that we separated the behavior of check_constant and check_kwargable

@charles-cooper charles-cooper merged commit 5b7ccbe into vyperlang:master Mar 19, 2022
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

4 participants