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

Move reserved keywords check into the type checker #2286

Merged

Conversation

iamdefinitelyahuman
Copy link
Contributor

What I did

Move reserved keyword checks into the type checker.

How I did it

The logic was already partially being handled within type checking: shadowing of builtins and environment variables would raise upon attempted assignment. However, reserved keywords and opcodes weren't caught until later when generating the GlobalContext object.

I've moved the reserved keywords list into namespace and am now applying the check on all new assignments as they're added to the Namespace object. For object members (values in events, structs, interfaces) I added a validate_identifier method that's slightly looser: it doesn't raise on collisions between non-builtins, so that e.g MyStruct.foo and self.foo won't collide.

How to verify it

Run the tests. I had to modify a few tests in order to make this work, because the exception being raised in some places has changed.

Cute Animal Picture

image

@codecov-io
Copy link

codecov-io commented Jan 28, 2021

Codecov Report

Merging #2286 (43d8305) into master (69f0402) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2286      +/-   ##
==========================================
+ Coverage   85.45%   85.47%   +0.01%     
==========================================
  Files          84       84              
  Lines        8583     8580       -3     
  Branches     2076     2079       +3     
==========================================
- Hits         7335     7334       -1     
+ Misses        747      746       -1     
+ Partials      501      500       -1     
Impacted Files Coverage Δ
vyper/parser/context.py 95.29% <ø> (-0.17%) ⬇️
vyper/parser/expr.py 77.77% <ø> (-0.05%) ⬇️
vyper/parser/global_context.py 77.77% <ø> (ø)
vyper/context/namespace.py 96.87% <100.00%> (+0.57%) ⬆️
vyper/context/types/meta/event.py 77.58% <100.00%> (+1.22%) ⬆️
vyper/context/types/meta/interface.py 81.48% <100.00%> (+0.34%) ⬆️
vyper/context/types/meta/struct.py 80.00% <100.00%> (+0.89%) ⬆️
vyper/signatures/event_signature.py 94.44% <100.00%> (-0.43%) ⬇️
vyper/signatures/function_signature.py 77.16% <100.00%> (-0.56%) ⬇️
vyper/types/types.py 82.41% <100.00%> (-0.09%) ⬇️

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 69f0402...43d8305. Read the comment docs.



# Cannot be used for variable or member naming
RESERVED_KEYWORDS = {
Copy link
Member

Choose a reason for hiding this comment

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

it would be lovely if we can somehow pull this from our stuff dyanmically, instead of statically maintaining this alongside changes to the compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely worth implementing.

@iamdefinitelyahuman iamdefinitelyahuman merged commit 021c17c into vyperlang:master Jan 28, 2021
@iamdefinitelyahuman iamdefinitelyahuman deleted the refactor-reserved branch January 28, 2021 20:39
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.

3 participants