Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

Default values for TPayload objects change if instance is modified. #261

Open
mrterry opened this issue Nov 22, 2016 · 4 comments · May be fixed by #262
Open

Default values for TPayload objects change if instance is modified. #261

mrterry opened this issue Nov 22, 2016 · 4 comments · May be fixed by #262

Comments

@mrterry
Copy link

mrterry commented Nov 22, 2016

If you:

  • have TPayload object with a default value
  • make an instance with the default values
  • mutate that instance

all subsequent default instances have new defaults. The snippet below will reproduce

from thriftpy import thrift
class Person(thrift.TPayload):                                                  
    thrift_spec = {                                                             
        1: (thrift.TType.LIST, "phones", thrift.TType.I32),                     
    }                                                                           
    default_spec = [("phones", [])]

p = Person()
p.phones.append(3)

p2 = Person()
assert p2.phones == []  # FAIL.  default is now [3]

In Python, default values for function/methods are re-used across uses. http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

We can solve this by requiring that default values actually be callables eg

default_spec = [("phones", list)]

or systematically making copies of the default values eg

class AltPerson(object):                                                        
    default_spec = [                                                            
        ('phones', []),                                                         
    ]                                                                           
                                                                                
    def __init__(self, **kwargs):                                               
        for name, default_value in self.default_spec:                           
            if name in kwargs:                                                  
                value = kwargs[name]                                            
            else:                                                               
                value = copy.copy(default_value)                                
            setattr(self, name, value)

If you go with the first option (my preference), it becomes tricky to intentionally create TPayload objects with callable attributes. Eg if you wanted phones to be set to an object that is both indexable f[0] and callable f(). That said, if you're in that camp, you ought to know what you're doing and can do `Person(phones=
@mrterry mrterry linked a pull request Nov 23, 2016 that will close this issue
Open
@mrterry mrterry changed the title TPayload object have different defaults if modified. Default values for TPayload objects change if instance is modified. Nov 23, 2016
@lxyu
Copy link
Contributor

lxyu commented Nov 24, 2016

This changed how the generated __init__ func works.

Previously:

def __init__(self, name='Alice', number=None):

now

def __init__(self, *args, **kwargs):

And the information lost.

@mrterry
Copy link
Author

mrterry commented Nov 24, 2016 •
edited

Doing it this way actually retains more information. Previously you had the signature:

def __init__(self, phones=[])

You can't distinguish between Person() and Person(phones=[]). This is important because the original signature, with a mutable default argument is all kinds of bad, and requires the copy.copy(default_value) business.

Also, there are places in you test suite that create TPayload objects using positional arguments ie
Person(['phone1', 'phone2']) That's why I had to put in the *args bit. The logic in the PR exactly reproduces the behavior expected by the current test suite. I'm happy to add more tests to make this more explicit.

@pawl
Copy link

pawl commented Jan 28, 2017

Looks like this one can be closed because of #262

@mrterry
Copy link
Author

mrterry commented Jan 28, 2017

if that PR is accepted, it will resolve this bug. right now the PR is limbo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make copies of default values to TPayload objects mrterry/thriftpy
3 participants
@mrterry @lxyu @pawl