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

Fix annotation of decorated methods, nested methods, nested classes #91

Merged
merged 4 commits into from
Oct 29, 2019

Conversation

msullivan
Copy link
Contributor

The determination of the name of a function is currently done by
looking at the function node and its grandparent node (for the class),
and nothing else. This misses the class name for decorated methods as
well as producing nonsensible results for nested classes and functions
(though none of the frontends handle those right yet either).

Fix this by doing a full traversal up the tree and including all
functions and classes.

The broken decorator behavior was also sort of accidentally causing
the class name to be left off for staticmethods and classmethods
(which the collector does also), so we need to keep supporting that
behavior.

The determination of the name of a function is currently done by
looking at the function node and its grandparent node (for the class),
and nothing else. This misses the class name for decorated methods as
well as producing nonsensible results for nested classes and functions
(though none of the frontends handle those right yet either).

Fix this by doing a full traversal up the tree and including all
functions and classes.

The broken decorator behavior was also sort of accidentally causing
the class name to be left off for staticmethods and classmethods
(which the collector does also), so we need to keep supporting that
behavior.
Copy link

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, being able to infer types of decorated methods should help significantly. I have a few questions and one issue with a test case.

a = """\
class C:
@staticmethod
def nop(cls, a):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't have a cls argument.

class C:
@staticmethod
def nop(cls, a):
# type: (int) -> int
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature if incorrect for a static method.

b = """\
@dec
def foo():
# type: () -> int
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the decorator does not preserve the signature?

What happens to argument types?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the pyannotate fixer. Its job is to just insert type annotations that are handed to it. That's a question for whatever the frontend producing the annotation is.

def get_funcname(name, node):
# type: (Leaf, Node) -> Text
def get_funcname(node):
# type: (Optional[Node]) -> Text
"""Get function name by the following rules:

- function -> function_name
- instance method -> ClassName.function_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe update the docstring?

return funcname
components = [] # type: List[str]
while node:
if node.type in (syms.classdef, syms.funcdef):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this do something about async defs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need to, since async_funcddef contains a funcdef, and that's what gets passed in. I'll add a test, since this means that "async methods" are one of the other things fixed by this.

@msullivan msullivan merged commit b7f96ca into master Oct 29, 2019
@msullivan msullivan deleted the method-decorator branch October 29, 2019 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants