Defer subclass methods if superclass has not been analyzed by ilevkivskyi · Pull Request #5637 · python/mypy

ilevkivskyi · 2018-09-18T20:13:38Z

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.

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.

JukkaL · 2018-09-19T12:43:58Z


    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:


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.

JukkaL · 2018-09-19T12:52:15Z

            del self.globals

-    def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef],
+    def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator],


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.

JukkaL · 2018-09-19T12:52:47Z

        self.patches = []

-    def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef],
+    def refresh_partial(self, node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator],


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

JukkaL · 2018-09-19T12:53:03Z



-def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef]) -> None:
+def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator]) -> None:


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

JukkaL · 2018-09-19T12:55:01Z


 def target_from_node(module: str,
-                     node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr]
+                     node: Union[FuncDef, MypyFile, OverloadedFuncDef, LambdaExpr, Decorator]


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

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

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.

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.

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

Yes, I will remove both.

JukkaL · 2018-09-19T13:01:28Z

 DeferredNode = NamedTuple(
    'DeferredNode',
    [
        # In batch mode only FuncDef and LambdaExpr are supported


This comment seems out of date.

JukkaL · 2018-09-19T13:01:53Z


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

+DeferredNodeType = Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef, Decorator]


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.

JukkaL · 2018-09-19T13:14:58Z

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


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

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

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

JukkaL · 2018-09-19T13:19:42Z

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


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

ilevkivskyi · 2018-09-19T16:36:07Z

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

JukkaL · 2018-09-20T09:07:15Z

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


Typo: "on" -> "one"

JukkaL · 2018-09-20T09:15:30Z

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


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.

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

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

JukkaL · 2018-09-20T09:17:37Z

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

+DeferredNodeType = Union[FuncDef, LambdaExpr, OverloadedFuncDef, Decorator]
+FineDeferredNodeType = Union[FuncDef, MypyFile, OverloadedFuncDef]


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.

JukkaL · 2018-09-20T09:18:07Z


+# Same as above, but for fine-grained mode targets. Only top-level functions/methods
+# and module top levels are allowed as such.
+FineDeferredNode = NamedTuple(


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

Ivan Levkivskyi added 3 commits September 18, 2018 20:48

Defer subclass methods if superclass has not been analyzed

0760f27

Fix self-check

1fe32b5

Update docstring

b03279a

ilevkivskyi requested a review from JukkaL September 18, 2018 20:13

JukkaL reviewed Sep 19, 2018

View reviewed changes

Ivan Levkivskyi added 5 commits September 19, 2018 16:24

Add some tests

62a1556

Add some tests

8f6b4c9

Add even more tests (overriding a var)

002ca67

Separate fine and coarse grained deferred nodes

e11822b

Add some clarifying comments and docstrings

250931c

JukkaL reviewed Sep 20, 2018

View reviewed changes

Address CR

132b222

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

ilevkivskyi deleted the defer-subclass branch September 20, 2018 13:33

andersk mentioned this pull request Jul 26, 2024

Crash in version 1.11 lambda processing (regression since version 1.10) #17595

Closed



		def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef]) -> None:
		def strip_target(node: Union[MypyFile, FuncItem, OverloadedFuncDef, Decorator]) -> None:


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

		DeferredNodeType = Union[FuncDef, LambdaExpr, MypyFile, OverloadedFuncDef, Decorator]

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

Conversation

ilevkivskyi commented Sep 18, 2018

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Sep 19, 2018

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ilevkivskyi Sep 19, 2018 •

edited

Loading