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

allow __getattr__ in nested scopes; support module-level __getattr__ #4958

Merged
merged 7 commits into from May 20, 2018

Conversation

Projects
None yet
3 participants
@JelleZijlstra
Collaborator

JelleZijlstra commented Apr 24, 2018

Fixes #4950 and implements PEP 562 for mypy.

@JelleZijlstra JelleZijlstra requested a review from ilevkivskyi Apr 24, 2018

self.msg.fail('__getattr__ is not valid at the module level outside a stub file',
context)
return
# __getattr__ is fine at the module level as of Python 3.7 (PEP 562). We could

This comment has been minimized.

@ethanhs

ethanhs Apr 24, 2018

Collaborator

Hm, I think this could actually bite people more than you'd think. People already get confused about why __getattr__ works in stubs and not in runtime modules without mypy allowing it in non-stubs. Also code that supports both 3.7 and below probably wouldn't be able to use __getattr__, I can't think of a non-hacky way in which it would work at runtime...

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Apr 24, 2018

Collaborator

I think it is actually fine. We don't warn about __setattr__ being unsupported etc.

This comment has been minimized.

@ethanhs

ethanhs Apr 24, 2018

Collaborator

Okay, we can always add it in later if it does confuse people.

@ilevkivskyi

Thanks! Looks good, just one idea how to cover one more use case.

self.msg.fail('__getattr__ is not valid at the module level outside a stub file',
context)
return
# __getattr__ is fine at the module level as of Python 3.7 (PEP 562). We could

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Apr 24, 2018

Collaborator

I think it is actually fine. We don't warn about __setattr__ being unsupported etc.

def __getattr__() -> None: # no error because not directly inside a class
pass
return __getattr__

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Apr 24, 2018

Collaborator

I would add two more tests like this:

def make_getattr_good() -> Callable[[str], int]: ...
__getattr__ = make_getattr_good()  # OK

def make_getattr_bad() -> Callable[[], int]: ...
__getattr__ = make_getattr_bad()  # E: Invalid signature...

Because this may be one of typical use cases for PEP 572. This will likely require to also call check_getattr_method from visit_assignment_statement (not only from function definition like now).

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Apr 24, 2018

Collaborator

Good point. I'll also check for from other_module import __getattr__.

This comment has been minimized.

@JelleZijlstra

JelleZijlstra Apr 25, 2018

Collaborator

And another one I realized I don't cover in the code I just pushed: from other_module import other_name as __getattr__.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented May 3, 2018

@JelleZijlstra Is this ready for review or do you want to work more on this? (Note the tests fail and there is a merge conflict now.)

@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 3, 2018

I got a bit caught up supporting more use cases and couldn't get something non-essential to work. I'll take another look soon to get a minimal version working.

JelleZijlstra added some commits May 17, 2018

@JelleZijlstra

This comment has been minimized.

Collaborator

JelleZijlstra commented May 18, 2018

@ilevkivskyi this is ready for another review now.

@ilevkivskyi

Thanks! Looks very good, just three small comments.

@@ -1483,6 +1486,19 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
else:
lvalue_type, index_lvalue, inferred = self.check_lvalue(lvalue)
if isinstance(lvalue, NameExpr):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 18, 2018

Collaborator

I think we should have and lvalue.name in '__getattr__', '__getattribute__', '__setattr__', otherwise this will change the logic of all assignments. Also a short comment about why this is needed will be helpful.

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 19, 2018

Collaborator

Rearranged the code a bit. I think the only logic change is that we may call accept on the rvalue more than once, which is probably fine because errors get deduplicated somewhere.

Relatedly, the way this method is structured makes it hard to verify that we actually call accept on the rvalue at all times. It might be worth it to refactor the code to just call self.expr_checker.accept(rvalue) once at the top.

# check for a module level __getattr__
if module and not node and module.is_stub and '__getattr__' in module.names:
# If it is still not resolved, check for a module level __getattr__
if (module and not node and (module.is_stub or self.options.python_version >= (3, 7))

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 18, 2018

Collaborator

We don't have this version check in checker.py and you added a good comment why. I think we also don't need it here

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 19, 2018

Collaborator

I think that's different actually. In checker.py we don't show an error if the user uses module-level __getattr__ before 3.7, because it's harmless (it does nothing, but also doesn't break anything). Here though we're essentially implementing the equivalent of the runtime attribute lookup on modules, so we should only look at __getattr__ in 3.7 or later.

@@ -2715,18 +2715,23 @@ def visit_member_expr(self, expr: MemberExpr) -> None:
expr.kind = n.kind
expr.fullname = n.fullname
expr.node = n.node
elif file is not None and file.is_stub and '__getattr__' in file.names:
elif (file is not None and (file.is_stub or self.options.python_version >= (3, 7))

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 18, 2018

Collaborator

The above comment also applies here.

@ilevkivskyi

OK, this is good to merge now, feel free to merge after fixing one minor comment.

else:
signature = self.expr_checker.accept(rvalue)
if signature:
name = lvalue.node.name()

This comment has been minimized.

@ilevkivskyi

ilevkivskyi May 20, 2018

Collaborator

This line is now redundant, the name is already assigned above.

This comment has been minimized.

@JelleZijlstra

JelleZijlstra May 20, 2018

Collaborator

Fixed, thanks for the review.

@JelleZijlstra JelleZijlstra merged commit da519e6 into python:master May 20, 2018

2 checks passed

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

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:getattr branch May 20, 2018

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