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

Defer subclass methods if superclass has not been analyzed #5637

Merged
merged 9 commits into from Sep 20, 2018

Conversation

Projects
None yet
2 participants
@ilevkivskyi
Collaborator

ilevkivskyi commented Sep 18, 2018

Fixes #5560
Fixes #5548

Half of the PR is updating various function signatures to also accept Decorator. I still prohibit Decorator as a target in fine-grained mode, but I think there should be no harm to defer it in normal mode.

@ilevkivskyi ilevkivskyi requested a review from JukkaL Sep 18, 2018

@JukkaL

Thanks for working on this issue -- when users hit this issue, it's very confusing. It's totally unclear why mypy is complaining and how to fix the problem.

Left detailed comments since deferred nodes are a very tricky part of mypy. Once you are updating this code it's a good opportunity to add some more documentation.

def check_method_override_for_base_with_name(
self, defn: Union[FuncBase, Decorator], name: str, base: TypeInfo) -> None:
self, defn: Union[FuncBase, Decorator], name: str, base: TypeInfo) -> bool:

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Document the return type.

It might be better to use concrete types in the union type, at least if it would let us remove the assert below.

@@ -317,7 +317,7 @@ def visit_file(self, file_node: MypyFile, fnam: str, options: Options,
del self.cur_mod_node
del self.globals
def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef],
def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator],

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

We should never get a Decorator here, since this is only used in fine-grained mode? We should either remove Decorator from the union (preferable) or assert that node is not a Decorator. I think that it would be better to have the assert as early as possible, i.e. in mypy.server.update.

@@ -73,7 +73,7 @@ def visit_file(self, file_node: MypyFile, fnam: str, options: Options,
del self.cur_mod_node
self.patches = []
def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef],
def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator],

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Similar to the comment related to mypy.semanal.py.

@@ -52,7 +52,7 @@
from mypy.typestate import TypeState
def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef]) -> None:
def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator]) -> None:

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Similar to the comment related to mypy.senanal.py.

@@ -1058,7 +1058,7 @@ def is_verbose(manager: BuildManager) -> bool:
def target_from_node(module: str,
node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr]
node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr, Decorator]

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Again, could we prevent this function from being called with a Decorator argument earlier on?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 19, 2018

Collaborator

I can remove Decorator from all these unions, but this will require a couple of asserts.

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

I think that it's better to have the caller ensure that the function is called with the correct types. The annotation as it is now is arguably misleading.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 19, 2018

Collaborator

It is arguably not more misleading than it was before, it already accepts LambdaExpr, but then fails with assert False on it (as well as the other functions above, they accept LambdaExpr while they shouldn't). Anyway, I will try to clean this up.

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Ah, good point! In that case maybe LambdaExpr should also be removed from the annotation?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 19, 2018

Collaborator

Yes, I will remove both.

@@ -72,14 +72,15 @@
DEFAULT_LAST_PASS = 1 # type: Final # Pass numbers start at 0
DeferredNodeType = Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef, Decorator]
# A node which is postponed to be processed during the next pass.
# This is used for both batch mode and fine-grained incremental mode.
DeferredNode = NamedTuple(
'DeferredNode',
[
# In batch mode only FuncDef and LambdaExpr are supported

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

This comment seems out of date.

@@ -72,14 +72,15 @@
DEFAULT_LAST_PASS = 1 # type: Final # Pass numbers start at 0
DeferredNodeType = Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef, Decorator]

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Add comment about which node types can be deferred in fine-grained vs. normal modes.

Also mention that nested functions can't be deferred -- only top-level functions and methods of classes not defined within a function can be deferred.

type_name = self.errors.type_name[-1]
else:
type_name = None
# Shouldn't we freeze the entire scope?

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

This comment is kind of vague. Maybe this should be a TODO comment?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 19, 2018

Collaborator

I am not sure what was the intent. This comment is two years old, maybe just remove it?

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Maybe reword it like this: "We don't freeze the entire scope since only top-level functions and methods can be deferred. Only module/class level scope information is needed. Module-level scope information is preserved in the TypeChecker instance." (I think that this is correct.)

Show resolved Hide resolved mypy/checker.py
@@ -1257,11 +1258,16 @@ def check_method_override(self, defn: Union[FuncBase, Decorator]) -> None:
"""Check if function definition is compatible with base classes."""

This comment has been minimized.

@JukkaL

JukkaL Sep 19, 2018

Collaborator

Update docstring to mention that this may defer the method if a signature is not available in a base class.

@ilevkivskyi

This comment has been minimized.

Collaborator

ilevkivskyi commented Sep 19, 2018

@JukkaL Thanks for review! I addressed the comments. I decided to have a separate FineDeferredNode named tuple (since named tuples can't be generic). This way we can avoid all asserts and extra items in unions in arguments.

I also added a bunch of tests combining overloads, decorators, variables, double deferrals, and super calls (also with various combinations of these).

@JukkaL

Thanks for the cleanups! Looks better. I left a few more comments. Feel free to merge once you've addressed them.

def check_method_override(self, defn: Union[FuncDef, OverloadedFuncDef, Decorator]) -> None:
"""Check if function definition is compatible with base classes.
This may defer the method if a signature is not available in at least on base class.

This comment has been minimized.

@JukkaL

JukkaL Sep 20, 2018

Collaborator

Typo: "on" -> "one"

def deco(f: Callable[..., T]) -> Callable[..., Tuple[T, int]]: ...
[out]
[case testCyclicDecoratorSuperDeferred]

This comment has been minimized.

@JukkaL

JukkaL Sep 20, 2018

Collaborator

Did you verify that these test cases trigger deferral? Using from ... import can be used to make it more explicit in which order modules are processed by mypy.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 20, 2018

Collaborator

I actually checked only first two tests I added, I will also double-check rest just in case.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Sep 20, 2018

Collaborator

They all get deferred expected number of times (1, 2, or 3), I switched to from ... import in half of the tests anyway.

@@ -72,19 +73,31 @@
DEFAULT_LAST_PASS = 1 # type: Final # Pass numbers start at 0
DeferredNodeType = Union[FuncDef, LambdaExpr, OverloadedFuncDef, Decorator]
FineDeferredNodeType = Union[FuncDef, MypyFile, OverloadedFuncDef]

This comment has been minimized.

@JukkaL

JukkaL Sep 20, 2018

Collaborator

Style nit: I'd prefer to call this FineGrainedDeferredNodeType or something.

We don't use "fine" as a shorted version of "fine-grained" elsewhere and it can be confusing.

('context_type_name', Optional[str]), # Name of the surrounding class (for error messages)
('active_typeinfo', Optional[TypeInfo]), # And its TypeInfo (for semantic analysis
# self type handling)
])
# Same as above, but for fine-grained mode targets. Only top-level functions/methods
# and module top levels are allowed as such.
FineDeferredNode = NamedTuple(

This comment has been minimized.

@JukkaL

JukkaL Sep 20, 2018

Collaborator

Similar to above, I'd prefer to name this FineGrainedDeferredNode.

Ivan Levkivskyi

@ilevkivskyi ilevkivskyi merged commit b41bb66 into python:master Sep 20, 2018

2 checks passed

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

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:defer-subclass branch Sep 20, 2018

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