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

Defer subclass methods if superclass has not been analyzed #5637

Merged
merged 9 commits into from
Sep 20, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 33 additions & 20 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,15 @@

DEFAULT_LAST_PASS = 1 # type: Final # Pass numbers start at 0

DeferredNodeType = Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef, Decorator]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


# 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment seems out of date.

('node', Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef]),
('node', DeferredNodeType),
('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)
Expand Down Expand Up @@ -300,7 +301,7 @@ def check_second_pass(self, todo: Optional[List[DeferredNode]] = None) -> bool:
else:
assert not self.deferred_nodes
self.deferred_nodes = []
done = set() # type: Set[Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef]]
done = set() # type: Set[DeferredNodeType]
for node, type_name, active_typeinfo in todo:
if node in done:
continue
Expand All @@ -314,10 +315,7 @@ def check_second_pass(self, todo: Optional[List[DeferredNode]] = None) -> bool:
self.tscope.leave()
return True

def check_partial(self, node: Union[FuncDef,
LambdaExpr,
MypyFile,
OverloadedFuncDef]) -> None:
def check_partial(self, node: DeferredNodeType) -> None:
if isinstance(node, MypyFile):
self.check_top_level(node)
else:
Expand All @@ -338,20 +336,23 @@ def check_top_level(self, node: MypyFile) -> None:
assert not self.current_node_deferred
# TODO: Handle __all__

def defer_node(self, node: DeferredNodeType, enclosing_class: Optional[TypeInfo]) -> None:
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved
if self.errors.type_name:
type_name = self.errors.type_name[-1]
else:
type_name = None
# Shouldn't we freeze the entire scope?
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

self.deferred_nodes.append(DeferredNode(node, type_name, enclosing_class))

def handle_cannot_determine_type(self, name: str, context: Context) -> None:
node = self.scope.top_non_lambda_function()
if self.pass_num < self.last_pass and isinstance(node, FuncDef):
# Don't report an error yet. Just defer. Note that we don't defer
# lambdas because they are coupled to the surrounding function
# through the binder and the inferred type of the lambda, so it
# would get messy.
if self.errors.type_name:
type_name = self.errors.type_name[-1]
else:
type_name = None
# Shouldn't we freeze the entire scope?
enclosing_class = self.scope.enclosing_class()
self.deferred_nodes.append(DeferredNode(node, type_name, enclosing_class))
self.defer_node(node, enclosing_class)
# Set a marker so that we won't infer additional types in this
# function. Any inferred types could be bogus, because there's at
# least one type that we don't know.
Expand Down Expand Up @@ -1257,11 +1258,16 @@ def check_method_override(self, defn: Union[FuncBase, Decorator]) -> None:
"""Check if function definition is compatible with base classes."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

# Check against definitions in base classes.
for base in defn.info.mro[1:]:
self.check_method_or_accessor_override_for_base(defn, base)
if self.check_method_or_accessor_override_for_base(defn, base):
return

def check_method_or_accessor_override_for_base(self, defn: Union[FuncBase, Decorator],
base: TypeInfo) -> None:
"""Check if method definition is compatible with a base class."""
base: TypeInfo) -> bool:
"""Check if method definition is compatible with a base class.

Return True if the node was deferred because one of the corresponding
superclass nodes is not ready.
"""
if base:
name = defn.name()
base_attr = base.names.get(name)
Expand All @@ -1277,19 +1283,21 @@ def check_method_or_accessor_override_for_base(self, defn: Union[FuncBase, Decor
if name not in ('__init__', '__new__', '__init_subclass__'):
# Check method override
# (__init__, __new__, __init_subclass__ are special).
self.check_method_override_for_base_with_name(defn, name, base)
if self.check_method_override_for_base_with_name(defn, name, base):
return True
if name in nodes.inplace_operator_methods:
# Figure out the name of the corresponding operator method.
method = '__' + name[3:]
# An inplace operator method such as __iadd__ might not be
# always introduced safely if a base class defined __add__.
# TODO can't come up with an example where this is
# necessary; now it's "just in case"
self.check_method_override_for_base_with_name(defn, method,
base)
return self.check_method_override_for_base_with_name(defn, method,
base)
return False

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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

base_attr = base.names.get(name)
if base_attr:
# The name of the method is defined in the base class.
Expand Down Expand Up @@ -1317,7 +1325,11 @@ def check_method_override_for_base_with_name(
original_type = base_attr.type
original_node = base_attr.node
if original_type is None:
if isinstance(original_node, FuncBase):
if self.pass_num < self.last_pass:
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved
assert isinstance(defn, (FuncDef, OverloadedFuncDef, Decorator))
self.defer_node(defn, defn.info)
return True
elif isinstance(original_node, FuncBase):
original_type = self.function_type(original_node)
elif isinstance(original_node, Decorator):
original_type = self.function_type(original_node.func)
Expand Down Expand Up @@ -1359,6 +1371,7 @@ def check_method_override_for_base_with_name(
else:
self.msg.signature_incompatible_with_supertype(
defn.name(), name, base.name(), context)
return False

def get_op_other_domain(self, tp: FunctionLike) -> Optional[Type]:
if isinstance(tp, CallableType):
Expand Down
2 changes: 1 addition & 1 deletion mypy/semanal.py
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

patches: List[Tuple[int, Callable[[], None]]]) -> None:
"""Refresh a stale target in fine-grained incremental mode."""
self.patches = patches
Expand Down
2 changes: 1 addition & 1 deletion mypy/semanal_pass3.py
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

patches: List[Tuple[int, Callable[[], None]]]) -> None:
"""Refresh a stale target in fine-grained incremental mode."""
self.options = self.sem.options
Expand Down
2 changes: 1 addition & 1 deletion mypy/server/aststrip.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

"""Reset a fine-grained incremental target to state after semantic analysis pass 1.

NOTE: Currently we opportunistically only reset changes that are known to otherwise
Expand Down
5 changes: 3 additions & 2 deletions mypy/server/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@ilevkivskyi ilevkivskyi Sep 19, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will remove both.

) -> Optional[str]:
"""Return the target name corresponding to a deferred node.

Expand All @@ -1079,4 +1079,5 @@ def target_from_node(module: str,
else:
return '%s.%s' % (module, node.name())
else:
assert False, "Lambda expressions can't be deferred in fine-grained incremental mode"
assert False, "Lambda expressions and decorators can't be deferred in" \
" fine-grained incremental mode"
36 changes: 36 additions & 0 deletions test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -5117,3 +5117,39 @@ class C:
def x(self) -> int: pass
[builtins fixtures/property.pyi]
[out]

[case testCyclicDecorator]
import b
[file a.py]
import b
import c

class A(b.B):
@c.deco
def meth(self) -> int: ...
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved
[file b.py]
import a
import c

class B:
@c.deco
def meth(self) -> int: ...
[file c.py]
from typing import TypeVar, Tuple, Callable
T = TypeVar('T')
def deco(f: Callable[..., T]) -> Callable[..., Tuple[T, int]]: ...
[out]

[case testCyclicOverride]
import a
[file b.py]
import a
class Sub(a.Base):
def x(self) -> int: pass
ilevkivskyi marked this conversation as resolved.
Show resolved Hide resolved

[file a.py]
import b
class Base:
def __init__(self):
self.x = 1
[out]