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

None in viper is misleading #539

Closed
yzhang90 opened this issue Dec 3, 2017 · 17 comments
Closed

None in viper is misleading #539

yzhang90 opened this issue Dec 3, 2017 · 17 comments
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting

Comments

@yzhang90
Copy link

yzhang90 commented Dec 3, 2017

What's your issue about?

NullType value None in viper is misleading.
For base type, None is converted to 0, which means:

a:num
a = None
if a == 0:
   ...        ## this branch will be taken at runtime.

If a list is assigned None, the list will get initialized to zeroes.
If a byte array is assigned None, it is equivalent to an empty string ( but compiled codes are different).
These behaviors are quite misleading.

How can it be fixed?

Since the actual behavior is nothing more than giving a variable default value, I suggest removing the None keyword from the syntax.

Cute Animal Picture

image

@fubuloubu
Copy link
Member

This is important to avoid subtle user errors from expecting None to work the same way it does in Python e.g. as a separate value from 0

Also has the side benefit of simplifying Formal Verification techniques

@jacqueswww
Copy link
Contributor

+1 from my side to remove a None to avoid confusion about what values can be expected for each type.

@DavidKnott
Copy link
Contributor

+1 I don't see any point in having None and as long as 0 == None it seems confusing and repetitive.

@DavidKnott DavidKnott added the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Dec 4, 2017
@DavidKnott
Copy link
Contributor

DavidKnott commented Dec 11, 2017

@fubuloubu @jacqueswww @yzhang90 For the initialization / removing of large arrays / mappings I see None as being useful, here's an example:

code = """
x: num[100]

@public
def set_x() -> num[100]:
    for i in range(100):
        self.x[i] = 1
    return self.x

@public
def unset_x():
    # Now:
    self.x = None 
    # Proposed solution:
    self.x = [0] * 100

@public
def get_x() -> num[100]:
    return self.x
"""
    c = get_contract(code)
    assert c.set_x() == [1] * 100
    assert c.get_x() == [1] * 100
    c.unset_x()
    assert c.get_x() == [0] * 100

As Viper is right now we'd have to empty x manually if we didn't have None. Though I still think None is unclear, what do you thinking of changing None to [0] * 100 in the case of the example above?

@fubuloubu
Copy link
Member

[0] * 100 is clearer than None

But maybe a built-in like setall(self.x, 0) could make that even clearer, as it matches what happens behind the scenes. The proposed array-like syntax is a little like magic to the uninitiated

@yzhang90
Copy link
Author

@fubuloubu How about struct? or other compound structures? I think reset(self.x) would be better. The semantics of reset(arg) would be:

  1. evaluate the arg, which is a memory address addr.
  2. calculate the size of the arg type size.
  3. set addr to addr+size to zero.
    You can not reset to other values.

The remaining question is whether reset(map) is allowed.

@fubuloubu
Copy link
Member

reset(type) is a nice syntax, I think we can roll with that.

For a mapping, as per issue #570 it's not really clear what to do. I think reset(test[id]) is much clearer than del test[id] in regards to what it's doing.

A little bit of documentation around reset and setting to Null/Zero values would tidy this up nicely.

@charles-cooper
Copy link
Member

@jacqueswww
Copy link
Contributor

jacqueswww commented Apr 9, 2018

Initial values that are zeros, with no zeroing functions.

decimal = 0.0
int128 = 0
bytes = ""
map = {each member = 0}
list = [0, 0, 0] 
struct = ?

Remove None as a type.

@fubuloubu
Copy link
Member

Well, for a struct, we can just say all values inside are assumed to be their corresponding zero values,
if not set. What the real issue concerns is how we manage comparisions and assigments.


Idea: for struct we can implement a filtering syntax for comparisions and assignments, something like:

myStruct: {
    a: int128,
    b: bytes32
}

# Assignments
myStruct = { a: 2, b: 0b1000_0001 } # Sets both a and b
myStruct = { a: 2 } # b is assumed to be null, or 0b0
myStruct = { } # both a and b are assumed null (0 ad 0b0)

# Comparisions
assert myStruct == { a: 2, b: 0b1000_0001 } # Checks myStruct == { a: 2, b: 0b1000_0001 }
assert myStruct == { a: 2 } # checks myStruct == { a: 2, b: 0b0 }
assert myStruct == { } # checks myStruct == { a: 0, b: 0b0 }

Might be a little bit magical, but I think it's interesting and it solves the problem of not using None only for structs, as well as how do you work with larger structs in a more efficient way.

@jakerockland
Copy link
Contributor

Starting a crack at this, will push a WIP later this evening.

@jacqueswww
Copy link
Contributor

@jakerockland just check if this is still valid, I have faint memory of partially removing some parts of this.

@jakerockland
Copy link
Contributor

@jacqueswww could you elaborate a bit on what you mean by "this" when you say to check if this is still valid 😅? For context I just pushed my initial change to a WIP at #1106, I believe this appropriately catches assignment to None as disallowed, but still need to work on an initial implementation of reset().

@jacqueswww
Copy link
Contributor

That looks good. 👍

@jacqueswww
Copy link
Contributor

go right ahead, I think I was confused that this was already implemented.

@nrryuya
Copy link
Contributor

nrryuya commented Jan 22, 2019

None isn't supported anymore (#1106) so we can close this?

@jacqueswww
Copy link
Contributor

Yup! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting
Projects
None yet
Development

No branches or pull requests

7 participants