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

Added reserved keywords #1741

Merged
merged 8 commits into from Nov 29, 2019
Merged

Added reserved keywords #1741

merged 8 commits into from Nov 29, 2019

Conversation

fubuloubu
Copy link
Member

@fubuloubu fubuloubu commented Nov 26, 2019

What I did

Added some more things to the internal reserved keyword list

Fixes: #1710

How I did it

grep-foo

How to verify it

Added all reserved keywords to test. Added test for function names.

Description for the changelog

N/A

Cute Animal Picture

baby giraffe

@codecov-io
Copy link

@codecov-io codecov-io commented Nov 26, 2019

Codecov Report

Merging #1741 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1741      +/-   ##
==========================================
+ Coverage    87.3%   87.32%   +0.02%     
==========================================
  Files          48       48              
  Lines        5709     5720      +11     
  Branches     1512     1512              
==========================================
+ Hits         4984     4995      +11     
  Misses        448      448              
  Partials      277      277
Impacted Files Coverage Δ
vyper/exceptions.py 91.3% <100%> (+1.14%) ⬆️
vyper/ast_utils.py 91.66% <100%> (+0.23%) ⬆️

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 fc744a3...d872f69. Read the comment docs.

vyper/utils.py Show resolved Hide resolved
@fubuloubu fubuloubu requested a review from charles-cooper Nov 27, 2019
@fubuloubu
Copy link
Member Author

@fubuloubu fubuloubu commented Nov 27, 2019

@charles-cooper 👍 (and unit tests pass) => can merge

vyper/utils.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@charles-cooper charles-cooper left a comment

can you add a test to validate that #1710 has been fixed? and then I think this is good to go

@fubuloubu fubuloubu mentioned this pull request Nov 27, 2019
@fubuloubu
Copy link
Member Author

@fubuloubu fubuloubu commented Nov 27, 2019

@charles-cooper went to go add tests, but ran into a ton of issues. And then I got into a refactoring rut, so #1746 came out. TL;DR is that we use inconsistent methods for representing constants

@fubuloubu
Copy link
Member Author

@fubuloubu fubuloubu commented Nov 27, 2019

Figured out my issue in a1a3310. Will add tests after rebasing from merge of #1746

@fubuloubu fubuloubu force-pushed the fubuloubu/reserved-keywords branch 3 times, most recently from 5d35c71 to eeac89b Compare Nov 28, 2019
@fubuloubu
Copy link
Member Author

@fubuloubu fubuloubu commented Nov 28, 2019

Found a bunch of uncaught SyntaxErrors and stuff!

@charles-cooper
Copy link
Collaborator

@charles-cooper charles-cooper commented Nov 28, 2019

Found a bunch of uncaught SyntaxErrors and stuff!

yay for code quality :)

@fubuloubu fubuloubu force-pushed the fubuloubu/reserved-keywords branch 2 times, most recently from 01bd2af to 5a9460d Compare Nov 28, 2019
@fubuloubu
Copy link
Member Author

@fubuloubu fubuloubu commented Nov 28, 2019

A bunch of force pushing for linting issues lol

Conversion from error using ast.parse()
"""
def __init__(self, syntax_error: SyntaxError, source_code: str):
item = types.SimpleNamespace() # TODO: Create an actual object for this
Copy link
Member Author

@fubuloubu fubuloubu Nov 28, 2019

Choose a reason for hiding this comment

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

TODO is here in case this object is more broadly useful (which I don't think it is)

Would also just be better if it were a static object

try:
py_ast = python_ast.parse(reformatted_code)
except SyntaxError as e:
# TODO: Ensure 1-to-1 match of source_code:reformatted_code SyntaxErrors
Copy link
Member Author

@fubuloubu fubuloubu Nov 28, 2019

Choose a reason for hiding this comment

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

TODO is here because theorhetically there is a scenario where this code reformatting could affect the location of offsets used to display an error. Would be best if the conversion was accounted for.

Copy link
Collaborator

@charles-cooper charles-cooper Nov 29, 2019

Choose a reason for hiding this comment

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

This is interesting because syntax error reporting actually happens on the reformatted code, but other error reporting can happen on the untransformed code. Look at for instance how the struct -> class preparsing rule affects error reporting:

# syntax error
$ vyper -f ir /dev/stdin <<EOF
struct Foo: aoijewofijwef098 \`\`
EOF
  File "<unknown>", line 1
    class Foo: aoijewofijwef098 ``
                                ^
SyntaxError: invalid syntax

During handling of the above exception, another exception occurred:

vyper.exceptions.PythonSyntaxException: line 1:29 SyntaxError: invalid syntax
---> 1 struct Foo: aoijewofijwef098 ``
------------------------------------^

# vs plain structure exception
$ vyper -f ir /dev/stdin <<EOF
struct Foo: aoijewofijwef098   
EOF

Error compiling: /dev/stdin
vyper.exceptions.StructureException: line 1:11 Structs can only contain variables
---> 1 struct Foo: aoijewofijwef098
------------------^
     2

Copy link
Member Author

@fubuloubu fubuloubu Nov 29, 2019

Choose a reason for hiding this comment

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

Does this add buggy behavior now?

Copy link
Collaborator

@charles-cooper charles-cooper Nov 29, 2019

Choose a reason for hiding this comment

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

no, the behavior is as buggy as before

Copy link
Member Author

@fubuloubu fubuloubu Nov 29, 2019

Choose a reason for hiding this comment

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

... Ship it!

vyper/utils.py Outdated Show resolved Hide resolved
vyper/utils.py Outdated Show resolved Hide resolved
vyper/utils.py Outdated Show resolved Hide resolved
fubuloubu and others added 7 commits Nov 29, 2019
- Changed filename to be more correct
- Added built-in functions and reserved keywords to tests
- Removed redundant testing
Note: RLPList in built-in functions was mixed case and caused an error
Co-Authored-By: Charles Cooper <cooper.charles.m@gmail.com>
@fubuloubu fubuloubu force-pushed the fubuloubu/reserved-keywords branch from 146c4f6 to c5160de Compare Nov 29, 2019
vyper/ast_utils.py Outdated Show resolved Hide resolved
Co-Authored-By: Charles Cooper <cooper.charles.m@gmail.com>
Copy link
Collaborator

@charles-cooper charles-cooper left a comment

I'm pretty happy with this. Thanks!

@charles-cooper charles-cooper merged commit deec045 into master Nov 29, 2019
4 checks passed
@fubuloubu fubuloubu deleted the fubuloubu/reserved-keywords branch Nov 29, 2019
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.

4 participants