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

Add __setattr__ support #3451

Merged
merged 6 commits into from May 29, 2017

Conversation

Projects
None yet
3 participants
@ethanhs
Collaborator

ethanhs commented May 25, 2017

This is my attempt at solving #521, which allows the setting of arbitrary attributes, restricted by the typing of values in __setattr__. I did not do the same signature checking as __getattr__, as @JelleZijlstra pointed out, the check would be done based on object's method.

I want to add some more in the docs about this, as I think it deserves to be well documented, I just wasn't sure where to put it.

Also the wiki should be updated if this is merged.

@ilevkivskyi

Thanks!

I have few comments.

Show outdated Hide outdated test-data/unit/check-classes.test
b.at = '3'
integer = b.at
[out]
main:13: error: Incompatible types in assignment (expression has type "str", variable has type "int")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Could you please use inline errors with # E: syntax?

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Could you please use inline errors with # E: syntax?

Show outdated Hide outdated docs/source/cheat_sheet.rst
def __setattr__(self, name, value):
# type: (str, int) -> None
...
a.foo = bar() # works if bar() returns int, fails otherwise

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Maybe just use an integer like 42 instead of bar() here and in Python 3 version?

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Maybe just use an integer like 42 instead of bar() here and in Python 3 version?

This comment has been minimized.

@ethanhs

ethanhs May 25, 2017

Collaborator

Fair point, and perhaps a str with a comment about it failing.

@ethanhs

ethanhs May 25, 2017

Collaborator

Fair point, and perhaps a str with a comment about it failing.

Show outdated Hide outdated mypy/checkmember.py
typ = map_instance_to_supertype(itype, setattr_func.info)
setattr_type = expand_type_by_instance(bound_type, typ)
if isinstance(setattr_type, CallableType):
return setattr_type.arg_types[-1]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Should we have some requirement that __setattr__ and __getattr__ types are somehow compatible? In either case this decision should be explicitly documented with some motivation.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Should we have some requirement that __setattr__ and __getattr__ types are somehow compatible? In either case this decision should be explicitly documented with some motivation.

This comment has been minimized.

@ethanhs

ethanhs May 25, 2017

Collaborator

This is something I considered, but since we don't know what __setattr__ does in some edge cases, I think it is safer to not assume that the types are the same. Consider a class which takes str attributes, and in __setattr__ converts it to an int before binding to itself.

But I don't know what the best decision here is.

@ethanhs

ethanhs May 25, 2017

Collaborator

This is something I considered, but since we don't know what __setattr__ does in some edge cases, I think it is safer to not assume that the types are the same. Consider a class which takes str attributes, and in __setattr__ converts it to an int before binding to itself.

But I don't know what the best decision here is.

[case testSetAttr]
from typing import Union
class A:
def __setattr__(self, name: str, value: Any) -> None: ...

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Two more ideas for tests:

  • presence of both __setattr__ and normal attributes
  • __setattr__ in superclass
@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Two more ideas for tests:

  • presence of both __setattr__ and normal attributes
  • __setattr__ in superclass

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

A few others:

  • __setattr__ is defined, but not a callable
  • __setattr__ is defined but takes the wrong arguments
@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

A few others:

  • __setattr__ is defined, but not a callable
  • __setattr__ is defined but takes the wrong arguments

This comment has been minimized.

@ethanhs

ethanhs May 25, 2017

Collaborator

@ilevkivskyi could you explain 'presence of both setattr and normal attributes'
You mean something like:

class Ex:
    def __setattr__(self, name: str, value: int) -> None:...
    test = '42'  # type: str
e = Ex()
e.test = 'hello'
@ethanhs

ethanhs May 25, 2017

Collaborator

@ilevkivskyi could you explain 'presence of both setattr and normal attributes'
You mean something like:

class Ex:
    def __setattr__(self, name: str, value: int) -> None:...
    test = '42'  # type: str
e = Ex()
e.test = 'hello'

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Yes, something like this (plus e.whatever = 5)

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Yes, something like this (plus e.whatever = 5)

Show outdated Hide outdated docs/source/cheat_sheet.rst
@@ -149,6 +149,15 @@ When you're puzzled or when things are complicated
reveal_type(c) # -> error: Revealed type is 'builtins.list[builtins.str]'
print(c) # -> [4] the object is not cast
# if you want dynamic attributes on your class, have it override __setattr__ in a stub

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

And also __getattr__, depending on our decision on that matter.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

And also __getattr__, depending on our decision on that matter.

This comment has been minimized.

@ethanhs

ethanhs May 25, 2017

Collaborator

Right, perhaps I should make that more clear and which one is for which case.

@ethanhs

ethanhs May 25, 2017

Collaborator

Right, perhaps I should make that more clear and which one is for which case.

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

Looks like you put it in the py3 but not the py2 cheat sheet. It should be the same in both.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

Looks like you put it in the py3 but not the py2 cheat sheet. It should be the same in both.

Show outdated Hide outdated mypy/checkmember.py
typ = map_instance_to_supertype(itype, setattr_func.info)
setattr_type = expand_type_by_instance(bound_type, typ)
if isinstance(setattr_type, CallableType):
return setattr_type.arg_types[-1]

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

Is this going to crash if __setattr__ is defined to take no arguments?

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

Is this going to crash if __setattr__ is defined to take no arguments?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Is this going to crash if __setattr__ is defined to take no arguments?

Good catch!

@ilevkivskyi

ilevkivskyi May 25, 2017

Collaborator

Is this going to crash if __setattr__ is defined to take no arguments?

Good catch!

This comment has been minimized.

@ethanhs

ethanhs May 25, 2017

Collaborator

Ah, yes. I fixed this by checking len of the args.

@ethanhs

ethanhs May 25, 2017

Collaborator

Ah, yes. I fixed this by checking len of the args.

Show outdated Hide outdated mypy/checkmember.py
else:
setattr_func = info.get_method('__setattr__')
if setattr_func and setattr_func.info.fullname() != 'builtins.object':
setattr_f = function_type(setattr_func, builtin_type('builtins.function'))

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

It's confusing to have separate variables called setattr_func and setattr_f. Perhaps the first one should be setattr_method.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

It's confusing to have separate variables called setattr_func and setattr_f. Perhaps the first one should be setattr_method.

@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs May 25, 2017

Collaborator

I believe I resolved all of the points reviewed. If I missed something or there is something else needed, lmk.

Collaborator

ethanhs commented May 25, 2017

I believe I resolved all of the points reviewed. If I missed something or there is something else needed, lmk.

Show outdated Hide outdated test-data/unit/check-classes.test
class B:
def __setattr__(self, name: str, value: int) -> None: ...
def __getattr__(self, name: str) -> str: ...
integer = None # type: int

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

why not just integer = 0? that will work with strict-optional too

@JelleZijlstra

JelleZijlstra May 25, 2017

Collaborator

why not just integer = 0? that will work with strict-optional too

This comment has been minimized.

@ethanhs

ethanhs May 25, 2017

Collaborator

Done.

@ethanhs

ethanhs May 25, 2017

Collaborator

Done.

@ilevkivskyi

Thanks! Just few very minor comments. Also please update PY2 cheat-sheet as proposed by @JelleZijlstra.

Show outdated Hide outdated docs/source/cheat_sheet_py3.rst
a.foo = 42 # works
a.bar = 'Ex-parrot' # fails type checking
f = None # type: int
b = None # type: str

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

Why do you need these two lines with None assignments?

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

Why do you need these two lines with None assignments?

This comment has been minimized.

@ethanhs

ethanhs May 28, 2017

Collaborator

I believe I was going to add more examples, but I don't think its needed. Removed.

@ethanhs

ethanhs May 28, 2017

Collaborator

I believe I was going to add more examples, but I don't think its needed. Removed.

Show outdated Hide outdated test-data/unit/check-classes.test
s.success = 4
s.fail = 'fail' # E: Incompatible types in assignment (expression has type "str", variable has type "int")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

One empty line will be enough here.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

One empty line will be enough here.

Show outdated Hide outdated test-data/unit/check-classes.test
class B:
def __setattr__(self, name, value: int): ...
b = B()
b.fail = 5

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

I think the attribute name fail in a test that passes is misleading.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

I think the attribute name fail in a test that passes is misleading.

Show outdated Hide outdated test-data/unit/check-classes.test
c = C()
c.check = 13
[case test testAttributes]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

This is probably too generic test case name. Maybe testGetAttrAndSetattr? Also there is an extra test word.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

This is probably too generic test case name. Maybe testGetAttrAndSetattr? Also there is an extra test word.

Show outdated Hide outdated test-data/unit/check-classes.test
b.at = '3' # E: Incompatible types in assignment (expression has type "str", variable has type "int")
integer = b.at # E: Incompatible types in assignment (expression has type "str", variable has type "int")

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

Again, remove the second added empty line.

@ilevkivskyi

ilevkivskyi May 28, 2017

Collaborator

Again, remove the second added empty line.

@ilevkivskyi

Thanks! This (finally :-) looks good to me!

@ilevkivskyi ilevkivskyi merged commit 46e41d9 into python:master May 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ethanhs

This comment has been minimized.

Show comment
Hide comment
@ethanhs

ethanhs May 29, 2017

Collaborator

Thank you! Sorry it took so long :^) next time I'll know more about what is needed!

Collaborator

ethanhs commented May 29, 2017

Thank you! Sorry it took so long :^) next time I'll know more about what is needed!

carljm added a commit to carljm/mypy that referenced this pull request May 30, 2017

Merge branch 'master' into module-alias
* master: (23 commits)
  Make return type of open() more precise (#3477)
  Add test cases that delete a file during incremental checking (#3461)
  Parse each format-string component separately (#3390)
  Don't warn about returning Any if it is a proper subtype of the return type (#3473)
  Add __setattr__ support (#3451)
  Remove bundled lib-typing (#3337)
  Move version of extensions to post-release (#3348)
  Fix None slice bounds with strict-optional (#3445)
  Allow NewType subclassing NewType. (#3465)
  Add console scripts (#3074)
  Fix 'variance' label.
  Change label for variance section to just 'variance' (#3429)
  Better error message for invalid package names passed to mypy (#3447)
  Fix last character cut in html-report if file does not end with newline (#3466)
  Print pytest output as it happens (#3463)
  Add mypy roadmap (#3460)
  Add flag to avoid interpreting arguments with a default of None as Optional (#3248)
  Add type checking plugin support for functions (#3299)
  Mismatch of inferred type and return type note (#3428)
  Sync typeshed (#3449)
  ...

@ilevkivskyi ilevkivskyi referenced this pull request Jun 10, 2017

Closed

Support __setattr__ #521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment