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: allow constant and immutable variables to be declared public #3024

Merged
merged 18 commits into from
Sep 9, 2022

Conversation

benber86
Copy link
Contributor

@benber86 benber86 commented Aug 4, 2022

What I did

I gave a first try to #2903 after encountering the same issue and discussing it on Discord. This will make it possible to declare immutable and constant variables as public.

This is still a WIP as:

  • When creating public getters, I'm lumping the immutable variables along with other state variables as attributes of self, but this should probably be done separately.
  • The public getter for constants currently just returns a value, so this will not work for lists or mappings.
  • This only allows for the declaration of public(immutable(x)) or public(constant(x)) not immutable(public(x)) or constant(public(x))

How I did it

  • Check if public nodes have immutable or constant children to set is_immutable or is_constant accordingly
  • Update the getter creation function to handle immutable and constant variables
  • Add logic to handle immutable public variables in codegen

How to verify it

  • Run tests/parser/syntax/test_public.py

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

image

(
i.func.id
for i in self.annotation.get_children(
Call, filters={"func.id": {"immutable", "constant"}}
Copy link
Member

Choose a reason for hiding this comment

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

this is weird. you might be able to have some weird combination like public(immutable(constant(...)))

@@ -260,6 +260,12 @@ def parse_Attribute(self):
elif isinstance(self.expr.value, vy_ast.Name) and self.expr.value.id == "self":
type_ = self.expr._metadata["type"]
var = self.context.globals[self.expr.attr]
# handle public(immutable) variables
Copy link
Member

Choose a reason for hiding this comment

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

once the expansion is fixed for immutables, i don't think there will be any need for any codegen changes

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #3024 (fab45f2) into master (cb41608) will increase coverage by 0.04%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##           master    #3024      +/-   ##
==========================================
+ Coverage   88.30%   88.34%   +0.04%     
==========================================
  Files          97       98       +1     
  Lines       10941    11021      +80     
  Branches     2587     2605      +18     
==========================================
+ Hits         9661     9737      +76     
- Misses        831      833       +2     
- Partials      449      451       +2     
Impacted Files Coverage Δ
vyper/semantics/types/utils.py 90.22% <ø> (ø)
vyper/semantics/types/function.py 87.16% <50.00%> (+0.04%) ⬆️
vyper/codegen/global_context.py 74.07% <75.00%> (-0.72%) ⬇️
vyper/ast/expansion.py 95.00% <100.00%> (+1.06%) ⬆️
vyper/ast/folding.py 92.62% <100.00%> (ø)
vyper/ast/nodes.py 93.22% <100.00%> (+0.04%) ⬆️
vyper/semantics/types/user/interface.py 83.19% <100.00%> (ø)
vyper/semantics/validation/data_positions.py 91.39% <100.00%> (ø)
vyper/semantics/validation/module.py 85.49% <100.00%> (-0.37%) ⬇️
vyper/__init__.py 58.82% <0.00%> (-17.65%) ⬇️
... and 14 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@charles-cooper
Copy link
Member

by the way, i think that only handling public(immutable(...)) and not immutable(public(...)) is a feature, not a bug

@benber86 benber86 force-pushed the immutable_public branch 2 times, most recently from 512113d to 6ecc427 Compare August 31, 2022 13:31
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.

this is looking pretty good. it looks like there is a test case failing which expects public() to throw an ArgumentException but it fails with an index OOB instead.

vyper/ast/nodes.py Outdated Show resolved Hide resolved
@@ -1290,6 +1291,14 @@ def __init__(self, *args, **kwargs):
elif call_name == "public":
# declaring a public variable
self.is_public = True
# handle the cases where a constant or immutable variable is public
if not len(self.annotation.args):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not len(self.annotation.args):
# do the same thing as `validate_call_args`
# (can't be imported due to cyclic dependency)
if len(self.annotation.args) != 1:

Copy link
Member

@charles-cooper charles-cooper Sep 2, 2022

Choose a reason for hiding this comment

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

also can you remove the corresponding dead code in vyper/semantics/validation/module.py?

@jarivers082461
Copy link

Nice

@charles-cooper
Copy link
Member

@benber86 i cleaned up the logic for extracting the public/immutable/constant annotations a little bit

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Sep 8, 2022

This pull request introduces 2 alerts when merging 062ffb8 into 915d430 - view on LGTM.com

new alerts:

  • 1 for Unused local variable
  • 1 for Unused import

@charles-cooper charles-cooper enabled auto-merge (squash) September 9, 2022 05:37
@charles-cooper charles-cooper merged commit be2b7f4 into vyperlang:master Sep 9, 2022
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Needs dynamic tests showing the constant is made available as a public method

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.

5 participants