Disallow use of `None`, disallow use of `del`, implemented `clear()` built-in function. #1106

jakerockland · 2018-11-28T18:43:24Z

- What I did

Implemented #539. Disallow use of None at the syntax level and implemented a built-in function clear().

- How I did it

Created special self-modifying statement function within stmt.py, updated tests that were previously using assignment to None, added comprehensive tests to test_clear.py.

- How to verify it

Added comprehensive tests, all that needs to be done is:

make test

- Description for the changelog

Disallow use of None, disallow use of del, implemented clear() built-in function.

- Cute Animal Picture

vyper/parser/stmt.py

jakerockland · 2018-11-29T08:51:25Z

@fubuloubu @jacqueswww Quick question: Is the description of handling the replacement for None within structs as described by @fubuloubu in #539 something that should be implemented with this PR?

If no, I believe that this PR is basically ready for review. There are some minor things we would have to either adjust with test_maps.py to remove the test that uses the None syntax, and some other minor clean up.

If yes, I have been doing a bit of diving into the solution for that but probably have a couple more questions 😅

fubuloubu · 2018-11-29T16:13:37Z

@jakerockland I don't think you should implement that suggestion as it's a little bit "magical". It should be a further suggestion explored in a different VIP.

fubuloubu · 2018-11-29T16:15:20Z

Also, correct me if I'm wrong, but I think del struct was decided to use reset(struct) instead to make it clearer the resulting type was not None as one would expect in Python

Fixed test name Basic implementation of reset() functionality Catch use of None at pre-parser stage Reset from both memory and storage Removed unnecessary usage of None in other tests and added more testing for usage of None Reworked approach to implementing reset function, updated tests Refactored, finished testing suite Make the linter happy Disallow use of None at a syntax level, implement build-in reset() function

jakerockland · 2018-11-29T18:30:00Z

@fubuloubu @jacqueswww Squashed my commits here, I believe this one is ready for review!

charles-cooper · 2018-11-29T19:33:34Z

vyper/parser/pre_parser.py

@@ -49,6 +52,9 @@ def pre_parse(code):
            # Prevent semi-colon line statements.
            elif (token.type, token.string) == (OP, ";"):
                raise StructureException("Semi-colon statements not allowed.", token.start)
+            # Prevent use of None literal


This blocks None at the tokenization stage, but a user might want to make a contract or function named None (weird, I know). I'm thinking this exception should be moved to a later stage - https://github.com/ethereum/vyper/blob/921bb80cf59c9be0067ed20e1a7b6ad0b59598b3/vyper/parser/expr.py#L147.

@charles-cooper Yeah so that's actually intentional haha. 😅I previously had this set up to block at a later stage, when checking assignments or comparisons. Refactored though because it just generally seems like something to prevent a user from doing even if they want to, because it is confusing given that Vyper is based mostly on Python's syntax and would allow for potentially very misleading code (where one thinks a custom type or custom struct named None is the Python None but its actually some custom thing called None.

I think the best thing in that case would be if we just add None as 'n reserved keyword?

@jacqueswww That makes sense, just pushed a commit that implements that approach instead.

charles-cooper · 2018-11-29T19:42:57Z

vyper/parser/stmt.py

@@ -235,7 +251,10 @@ def call(self):
        )
        if isinstance(self.stmt.func, ast.Name):
            if self.stmt.func.id in stmt_dispatch_table:
-                return stmt_dispatch_table[self.stmt.func.id](self.stmt, self.context)
+                if self.stmt.func.id == 'reset':


This seems interesting .. why not just put the implementation in the dispatch table?

@charles-cooper Doesn't work without serious refactoring given that the reset() function self-mutates its input stmt, which is different than all the other functions in the stmt_dispatch_table.

jakerockland · 2018-11-30T15:19:56Z

One open question here is if reset() should be named clear().

jakerockland · 2018-11-30T15:20:44Z

One open question here is if reset() should be named clear(), relevant for #1076 convo.

fubuloubu · 2018-11-30T15:22:58Z

Oh, I quite like clear()!

…ection of the docs, added more tests

jakerockland · 2018-12-03T15:47:23Z

@jacqueswww Updated this PR to disallow usage of del per our convo this morning.

… expression evaluation rather than in pre-parser, more tests

jakerockland · 2018-12-06T06:27:47Z

@jacqueswww Updated this branch to use the new struct syntax in testing and resolved merge conflicts.

jakerockland · 2018-12-07T06:49:11Z

@jacqueswww I'm a bit confused why Travis is failing with:

The command "python setup.py test" exited with 1.

It doesn't seem relevant to the changes made in this PR, guessing something with Travis config may have gotten borked?

jacqueswww · 2018-12-07T12:27:54Z

Strange indeed.

jakerockland · 2018-12-07T19:29:08Z

@jacqueswww Seems like it also isn't just an issue with this branch, #1125 seems to be failing for the same reason 😓

Is it possible to just trigger a re-build on Travis?

…ssignments, added aditional tests

jakerockland · 2018-12-12T01:40:32Z

@jacqueswww HAZAH! Travis seems to be happily working now and also updated to work with some upstream changes that have happened since last update.

jacqueswww

@jakerockland LGTM, please add changelog entry for clear here: https://github.com/ethereum/vyper/blob/master/docs/index.rst#compatibility-breaking-changelog

jacqueswww · 2018-12-12T09:46:47Z

vyper/utils.py

@@ -148,7 +148,7 @@ def in_bounds(cls, type_str, value):
    'pass', 'def', 'push', 'dup', 'swap', 'send', 'call',
    'selfdestruct', 'assert', 'stop', 'throw',
    'raise', 'init', '_init_', '___init___', '____init____',
-    'true', 'false', 'self', 'this', 'continue',
+    'true', 'false', 'self', 'this', 'continue', 'none',


Should clear become a reserved keyword?

Just pushed a PR that added clear as a reserved word and added a entry to changelog.

jakerockland mentioned this pull request Nov 28, 2018

None in viper is misleading #539

Closed

jakerockland commented Nov 28, 2018

View reviewed changes

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

jakerockland changed the title ~~WIP: Disallow assignment to None~~ Disallow assignment to None Nov 29, 2018

jakerockland changed the title ~~Disallow assignment to None~~ Disallow use ofNone, implemented reset() built-in function. Nov 29, 2018

jakerockland changed the title ~~Disallow use ofNone, implemented reset() built-in function.~~ Disallow use of None, implemented reset() built-in function. Nov 29, 2018

jakerockland force-pushed the no-none branch from 80de5ad to 2a26431 Compare November 29, 2018 18:29

charles-cooper reviewed Nov 29, 2018

View reviewed changes

Renamed reset() to clear(), added description to built-in functions s…

6753ce6

…ection of the docs, added more tests

jakerockland changed the title ~~Disallow use of None, implemented reset() built-in function.~~ Disallow use of None, implemented clear() built-in function. Nov 30, 2018

Remove usage of del function, refactor tests

34abab0

jakerockland changed the title ~~Disallow use of None, implemented clear() built-in function.~~ Disallow use of None, disallow use of del, implemented clear() built-in function. Dec 3, 2018

jakerockland added 5 commits December 3, 2018 08:53

Resolved merge conflict

84cad14

Make None a resevered word, throw errors in statements evaluation and…

e453b01

… expression evaluation rather than in pre-parser, more tests

Make the linter happy

e8b4a4f

Resolved merge conflict

7b2eebd

Update clear() tests to use new struct syntax

7e76e89

Merge branch 'master' of github.com:ethereum/vyper into no-none

1f447a2

jakerockland added 2 commits December 11, 2018 08:01

Merge branch 'master' of github.com:ethereum/vyper into no-none

31b2397

Fixed upstream test cases using None, disallow using None in struct a…

d974624

…ssignments, added aditional tests

jacqueswww approved these changes Dec 12, 2018

View reviewed changes

jacqueswww reviewed Dec 12, 2018

View reviewed changes

Made clear a reserved word and added a entry to the changelog

e72fae4

jacqueswww merged commit 17a4f43 into vyperlang:master Dec 13, 2018

nrryuya mentioned this pull request Jan 22, 2019

Remove the description of None from docs #1212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow use of `None`, disallow use of `del`, implemented `clear()` built-in function. #1106

Disallow use of `None`, disallow use of `del`, implemented `clear()` built-in function. #1106

jakerockland commented Nov 28, 2018 •

edited

Loading

jakerockland commented Nov 29, 2018

fubuloubu commented Nov 29, 2018

fubuloubu commented Nov 29, 2018

jakerockland commented Nov 29, 2018

charles-cooper Nov 29, 2018 •

edited

Loading

jakerockland Nov 29, 2018

jacqueswww Dec 4, 2018

jakerockland Dec 4, 2018

charles-cooper Nov 29, 2018

jakerockland Nov 29, 2018

jakerockland commented Nov 30, 2018

jakerockland commented Nov 30, 2018

fubuloubu commented Nov 30, 2018

jakerockland commented Dec 3, 2018

jakerockland commented Dec 6, 2018

jakerockland commented Dec 7, 2018

jacqueswww commented Dec 7, 2018

jakerockland commented Dec 7, 2018 •

edited

Loading

jakerockland commented Dec 12, 2018

jacqueswww left a comment

jacqueswww Dec 12, 2018

fubuloubu Dec 12, 2018

jakerockland Dec 12, 2018

Disallow use of None, disallow use of del, implemented clear() built-in function. #1106

Disallow use of None, disallow use of del, implemented clear() built-in function. #1106

Conversation

jakerockland commented Nov 28, 2018 • edited Loading

- What I did

- How I did it

- How to verify it

- Description for the changelog

- Cute Animal Picture

jakerockland commented Nov 29, 2018

fubuloubu commented Nov 29, 2018

fubuloubu commented Nov 29, 2018

jakerockland commented Nov 29, 2018

charles-cooper Nov 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakerockland commented Nov 30, 2018

jakerockland commented Nov 30, 2018

fubuloubu commented Nov 30, 2018

jakerockland commented Dec 3, 2018

jakerockland commented Dec 6, 2018

jakerockland commented Dec 7, 2018

jacqueswww commented Dec 7, 2018

jakerockland commented Dec 7, 2018 • edited Loading

jakerockland commented Dec 12, 2018

jacqueswww left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Disallow use of `None`, disallow use of `del`, implemented `clear()` built-in function. #1106

Disallow use of `None`, disallow use of `del`, implemented `clear()` built-in function. #1106

jakerockland commented Nov 28, 2018 •

edited

Loading

charles-cooper Nov 29, 2018 •

edited

Loading

jakerockland commented Dec 7, 2018 •

edited

Loading