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

Make overloads support classmethod and staticmethod #5224

Merged
merged 6 commits into from Jun 16, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Jun 15, 2018

This pull request adds support for mixing classmethods, staticmethods, and overloads.

It does so by adding in some extra logic into the semantic analysis phase to detect when an overload is using classmethods or staticmethods and adds that information into OverloadedFuncDef. This allowed me to add is_class and is_static fields to FuncBase, the common base class for overloads and other function-related things.

This PR does not attempt to handle overloads + other arbitrary decorators.

Resolves #328 (which unblocks #2254?)

Michael0x2a added some commits Jun 15, 2018

Move 'is_class' and 'is_static' into FuncBase
This commit moves the `is_class` and `is_static` fields into FuncBase.
It also cleans up the list of flags so they don't repeat the
'is_property' entry, which is now present in `FUNCBASE_FLAGS`.

The high-level plan is to modify the `is_class` and `is_static` fields
in OverloadedFuncDef for use later in mypy.
Make semantic analysis phase record class/static methods with overloads
This commit adjusts the semantic analysis phase to detect and record
when an overload appears to be a classmethod or staticmethod.
Broaden class/static method checks to catch overloads
This commit modifies mypy to use the `is_static` and `is_class` fields
of OverloadedFuncDef as appropriate.

I found the code snippets to modify by asking PyCharm for all instances
of code using those two fields and modified the surrounding code as
appropriate.
Add support for overloaded classmethods in attrs/dataclasses
Both the attrs and dataclasses plugins manually patch classmethods -- we
do the same for overloads.
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! Here are few comments.

@@ -481,9 +490,9 @@ def set_line(self, target: Union[Context, int], column: Optional[int] = None) ->
self.variable.set_line(self.line, self.column)


FUNCITEM_FLAGS = [
FUNCITEM_FLAGS = FUNCBASE_FLAGS + [

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

This change adds is_property, is this intentional?

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 16, 2018

Author Collaborator

It is. I made this change partly on principle since is_property actually is a field of FuncBase -- it felt cleaner to just force all subclasses to preserve that field no matter what.

This change also doesn't actually change the serialized output in practice. FuncItem currently has only two subtypes: FuncDef and LambdaExpr. The former subclass previously explicitly set and serialized the is_property field so this change makes no difference there. The latter subclass never really uses is_property but also doesn't have any serialize/deserialize methods, which makes this change moot.

if stmt.impl is not None:
assert isinstance(stmt.impl, Decorator)
if isinstance(stmt.impl.func.type, CallableType):
stmt.impl.func.type.arg_types[0] = class_type

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

It's good that you also take care about plugins.

@@ -370,13 +370,20 @@ def __str__(self) -> str:
return 'ImportedName(%s)' % self.target_fullname


FUNCBASE_FLAGS = [
'is_property', 'is_class', 'is_static',
]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

Shouldn't we also update astdiff.py according to these flag reshuffling? This may break fine grained incremental mode (in some corner cases).

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 16, 2018

Author Collaborator

Ooh, good point -- I didn't even know that file existed.

I tried making the change + tried adding an test to one of the fine-grained incremental tests. (I'm pretty unfamiliar with fine-grained incremental stuff though, so let me know if I did it incorrectly.)

elif isinstance(item, FuncDef):
inner = item
else:
raise AssertionError()

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

Please add a message to all assertion errors. Typically we write assert False, "Impossible blah-blah-blah".

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 16, 2018

Author Collaborator

Done!

@attr.s
class A:
a: int
b: str

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

I would rather make these attr.ibs, to check that the cls signature below is generated correctly by the plugin.

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 16, 2018

Author Collaborator

Also done

def foo(cls, x): pass

class BadChild(Parent):
@overload # E: Signature of "foo" incompatible with supertype "Parent"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

Maybe add few more tests to check overriding an overloaded instance method, with overloaded class methods and vice versa?

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 16, 2018

Author Collaborator

Ah whoops, I completely forgot to handle this case. Fixed.

@@ -1290,8 +1290,8 @@ def check_override(self, override: FunctionLike, original: FunctionLike,
fail = True

if isinstance(original, CallableType) and isinstance(override, CallableType):
if (isinstance(original.definition, FuncItem) and
isinstance(override.definition, FuncItem)):
if (isinstance(original.definition, FuncBase) and

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 15, 2018

Collaborator

Note that this PR (in particular because of this change) may have conflicts with my type aliases refactoring. In turned out that the type_override (that I killed) was also used for some overload hacks.

This comment has been minimized.

@Michael0x2a

Michael0x2a Jun 16, 2018

Author Collaborator

I'm not sure I'm completely following -- is there now going to be some major difference between FuncItem and FuncBase?

(I also ended up streamlining this code to handle the "overriding a classmethod with an instance method" case more cleanly -- not sure if that change makes the potential conflict better or worse.)

@ilevkivskyi ilevkivskyi self-assigned this Jun 15, 2018

Respond to code review
This commit:

1. Updates astdiff.py and adds a case to one of the fine-grained
   dependency test files.

2. Adds some helper methods to FunctionLike.

3. Performs a few misc cleanups.
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! Here are few more comments. I just checked locally your changes work well with my refactoring.

return func.is_static
return False
raise AssertionError("Unexpected func type: {}".format(type(func)))

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 16, 2018

Collaborator

Could you please reformat this as assert False, "message"?

class FuncBase(Node):
"""Abstract base class for function-like nodes"""

__slots__ = ('type',
'unanalyzed_type',
'info',
'is_property',
'is_class',

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 16, 2018

Collaborator

Could you please add a comment # Uses @classmethod? as before (and probably also for is_static) below. Currently we have is_class, is_classmethod, and is_classmethod_class (the latter I would say is quite bad name), and I want to avoid potential confusions.

elif isinstance(defn.impl, FuncDef):
inner = defn.impl
else:
raise AssertionError()

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 16, 2018

Collaborator

Could you also please add an assertion message here, ad reformat like above via assert False?

@@ -173,7 +173,7 @@ def snapshot_definition(node: Optional[SymbolNode],
signature = snapshot_type(node.type)
else:
signature = snapshot_untyped_signature(node)
return ('Func', common, node.is_property, signature)
return ('Func', common, node.is_property, node.is_class, node.is_property, signature)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 16, 2018

Collaborator

is_property now appears twice in the list, did you mean is_static? Also maybe the isinstance above can use FuncBase?

@classmethod
def foo(cls, x: int) -> int: ...
@overload
@classmethod

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 16, 2018

Collaborator

One more suggestion: could you please add to your TODO list adding few tests to check that overloads work well with self-types (both instance, and class methods using cls: Type[T])?

main:3: error: Revealed type is 'builtins.int'
==
main:3: error: Revealed type is 'Any'
main:3: error: No overload variant of "foo" of "Wrapper" matches argument type "int"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Jun 16, 2018

Collaborator

Yes, this test looks good.

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 16, 2018

@ilevkivskyi -- ok, done. I went ahead and just added some self-type tests to this commit.

@ilevkivskyi ilevkivskyi merged commit 29889c8 into python:master Jun 16, 2018

2 checks passed

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

@Michael0x2a Michael0x2a deleted the Michael0x2a:make-overloads-support-classmethod-and-staticmethod branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.