Support for functions producing generic functions #3113

sixolet · 2017-04-03T04:56:37Z

Provides support for returning a generic Callable from a function, allowing you to write a function that produces a decorator, for example. This pattern is common:

def deco(x: int) -> Callable[[T], T]: ...

@deco(4)
def f(stuff): ...

Details on why this touches so much code:

Returning a generic Callable from a function requires binding type variables while we're traversing the type analysis phase of the check. Previously, all type variable binding was done from semanal.py. I refactored type variable tracking and binding into its own class, that's used by both semanal.py and typeanal.py to keep track of its type variables. I also, in the process, nixed the thing where we're mutating TypeVarExprs to bind them, instead keeping track of the bindings in the scope object I created. Seems more sustainable in a world where more than one class has to deal with typevar binding.

Merge checklist:

Depends on the following other PRs -- their code is included, it'll be excluded when they merge:
- Make TypeQuery more general, handling nonboolean queries. #3084
- Make it an error to use a class-attribute type var outside a type #3105
Still needs a few more tests
- Actually make and use a second-order decorator
- Multiple levels of generic function return
- Generic function where some tvars are bound by outer context and some are bound by the fn itself.

Previously, e9d28a0 fixed a crash when you tried to access a class-attribute type variable. The test in that commit involved assigning a variable the value of the typevar. It resulted in no crash, but rather treating the variable as being an instance of the type the typevar bound to, later, which is incorrect. Instead, this PR treats such an assignment as an error, and gives you the same message as when you try to alias a typevar directly. Also test a *correct* alias with the typevar access method in question -- it works.

Instead of TypeQuery always returning a boolean and having the strategy be an enum, the strategy is now a Callable describing how to combine partial results, and the two default strategies are plain old funcitons. To preserve the short-circuiting behavior of the previous code, this PR uses an exception. This is a pure refactor that I am using in my experimentation regarding fixing python#1551. It should result in exactly no change to current behavior. It's separable from the other things I'm experimenting with, so I'm filing it as a separate pull request now. It enables me to rewrite the code that pulls type variables out of types as a TypeQuery. Consider waiting to merge this PR until I have some code that uses it ready for review. Or merge it now, if you think it's a pleasant cleanup instead of an ugly complication. I'm of two minds on that particular question.

…with a scope object

sixolet

This needs a little more love before it's mergeable, and it depends on two other PRs currently in flight, but I wanted to get this up for comment if people like.

sixolet · 2017-04-03T04:58:32Z

mypy/checkmember.py

@@ -406,7 +406,9 @@ def analyze_class_attribute_access(itype: Instance,
        return AnyType()

    if isinstance(node.node, TypeVarExpr):


This is the part that depends on #3105 -- the TypeVarExpr is now only associated with its TypeVarDef by scope, not direct reference.

sixolet · 2017-04-03T04:59:31Z

mypy/types.py

@@ -1500,112 +1500,112 @@ def keywords_str(self, a: Iterable[Tuple[str, Type]]) -> str:
        ])




This business is #3084

sixolet · 2017-04-03T05:03:23Z

mypy/typeanal.py

-                               fallback=t.fallback or self.builtin_type('builtins.function'),
-                               variables=self.anal_var_defs(t.variables))
+    def visit_callable_type(self, t: CallableType, nested: bool = True) -> Type:
+        with self.tvar_scope_frame():


The above line is the punchline of this diff. Make a layer of type variable scope when analyzing a callable type, so it can bind its own type variables if it needs to.

Add a code comment with contents similar to the second sentence in the above comment?

sixolet · 2017-04-03T05:05:38Z

mypy/typeanal.py

+                    names.append(name)
+                    tvars.append(tvar_expr)
+        for name, tvar_expr in type.ret_type.accept(
+                TypeVariableQuery(self.lookup, self.tvar_scope, include_callables=False)):


Other punchline of this diff: when you're finding the type variables that a function has, don't include the tvars you only find in a Callable in the return type. Those belong to that Callable

Add code comment mentioning what you said above.

sixolet · 2017-04-12T20:27:58Z

Ok. Feels ready for review now.

ilevkivskyi · 2017-04-12T21:35:58Z

test-data/unit/semanal-errors.test

-T = TypeVar('T')
-class A(Generic[T]):
-    class B(Generic[T]): pass \
-          # E: Free type variable expected in Generic[...]


This appears in PEP 484 as an example of an error. I think it should still be prohibited.

Oh, yes, you're totally right. According to PEP484 you can neither make the inner class generic in the same tvar, nor use the outer tvar in the inner class. I'll adjust as necessary.

sixolet · 2017-04-14T05:09:20Z

Bah. The difference between the type variable scope you get from a class and the one you get from a function is complicated, and I suspect it causes bugs in static methods and class methods. I'll write up a test for what happens with those, to see if there are things that don't make sense.

sixolet · 2017-04-14T06:28:47Z

Following up, here are the things that don't make sense: #3172

I can include a fix in this PR, but maybe I might prefer to make it in a smaller follow-on PR. This one is quite long enough already.

gvanrossum · 2017-04-14T14:16:12Z

Separate PRs, please.

… into TypeVarScope This allows us to be more consistent about which type variables we allow and which we don't.

gvanrossum · 2017-04-19T17:33:56Z

FWIW I've ran this against our internal codebase and found no issues.

gvanrossum · 2017-04-19T22:42:59Z

I'm hoping that @JukkaL can do the detailed review though.

JukkaL

Looks good -- thanks for implementing this! A have just a few random nits, other than these this is ready to merge.

JukkaL · 2017-04-20T12:22:23Z

mypy/tvar_scope.py

+        """Initializer for TypeVarScope
+
+        Parameters:
+        parent: the outer scope for this scope


Style nit: indent the parameter descriptions like this (for better consistency):

Args: parent: the outer scope for this cope ...

JukkaL · 2017-04-20T12:25:56Z

mypy/tvar_scope.py

+    def get_function_scope(self) -> Optional['TypeVarScope']:
+        """Get the nearest parent that's a function scope, not a class scope"""
+        it = self
+        while it.is_class_scope:


Should the check be while it is not None and ...?

Yes, that's safer. We have an invariant that the root is an empty function scope, with the shape of the code now, but that could change.

JukkaL · 2017-04-20T12:28:01Z

mypy/tvar_scope.py

+
+    def class_frame(self) -> 'TypeVarScope':
+        """A new scope frame for binding a class. Prohibits *this* class's tvars"""
+        return TypeVarScope(self.get_function_scope(), True, self)


Should we assert that the current scope is a class scope? If not, should we still prohibit the definitions in the current scope even if it's a function scope?

You are always prohibited from re-binding an already-bound type variable. When you define a class in a function, this doesn't prohibit anything more -- your class type variables are also going to be prohibited by dint of being in your own parent.

JukkaL · 2017-04-20T12:34:57Z

mypy/typeanal.py

-                               fallback=t.fallback or self.builtin_type('builtins.function'),
-                               variables=self.anal_var_defs(t.variables))
+    def visit_callable_type(self, t: CallableType, nested: bool = True) -> Type:
+        with self.tvar_scope_frame():


Add a code comment with contents similar to the second sentence in the above comment?

JukkaL · 2017-04-20T12:56:15Z

mypy/typeanal.py

+        return ret.accept(self)
+
+    @contextmanager
+    def tvar_scope_frame(self) -> Generator:


Use Iterator[None] as the return type to avoid implicit Any types?

JukkaL · 2017-04-20T12:58:21Z

mypy/typeanal.py

+                    names.append(name)
+                    tvars.append(tvar_expr)
+        for name, tvar_expr in type.ret_type.accept(
+                TypeVariableQuery(self.lookup, self.tvar_scope, include_callables=False)):


Add code comment mentioning what you said above.

JukkaL · 2017-04-20T12:59:38Z

mypy/typeanal.py

+
+    def bind_function_type_variables(self,
+                                     fun_type: CallableType, defn: Context) -> List[TypeVarDef]:
+        """Find the type variables of the function type and bind them in our tvar_scope.


Style nit: move the trailing """ to the end of the first docstring line, for consistency.

JukkaL · 2017-04-20T13:12:44Z

mypy/typeanal.py

+        if node and node.kind == TVAR and (
+                self.include_bound or self.scope.get_binding(node) is None):
+            assert isinstance(node.node, TypeVarExpr)
+            return[(name, node.node)]


Style nit: missing space after return.

JukkaL · 2017-04-20T13:14:44Z

mypy/typeanal.py

+                 scope: 'TypeVarScope',
+                 *,
+                 include_callables: bool = True,
+                 include_bound: bool = False) -> None:


I was confused by this argument initially. What about renaming it to include_bound_tvars, for example? 'Bound' is also used to refer to type variable bounds.

sixolet

Comments from being on the phone with Jukka

sixolet · 2017-04-20T18:13:29Z

I think everything addressed now!

sixolet added 28 commits April 2, 2017 16:23

Fix the error message to handle more cases

74e9262

oops accidentally changed a file

5470c77

Typo fix

12def8b

more checking things in tests

52782fe

Change error message

0184595

Change error message

757a6b8

Remove redundant empty check

3b8bc23

Some types in the visitor were missing recursion

82da335

Add ellipsis type handler to TypeQuery

9f12644

Typechecks, but gives spurious error. Need to move variable binding?

8cf4fa6

Small change: Move tvar id generation nearer type analysis

2634277

Even nearer

30dba10

Replace the concept of BOUND_TVAR and UNBOUND_TVAR as mutative flags …

7b4f9f2

…with a scope object

Mid debugging gotta push now

daf9592

Checkpoint. Most tests pass, classvar type aliases not yet

826b9cf

forgot this file

4141f94

Fix aliasing to not bind type vars within a callable while aliasing

58e3a2a

removed some now-unused code

c061a09

my own code review, first pass

603013d

Use type var query instead of another query fn

c6fec63

Tighten code

f46d52a

fix some types

ea21337

Use same type alias throughout

80d62a7

Make semanal tests pass. Delete one I think is wrong

eaf8c0d

Generator types for generators

5009a2b

Lint and cleanups

7c64464

sixolet commented Apr 3, 2017

View reviewed changes

sixolet mentioned this pull request Apr 3, 2017

Function returning a generic function #1551

Closed

sixolet added 2 commits April 11, 2017 18:20

Merge upstream

9d5c630

Merge lint

4683d70

sixolet mentioned this pull request Apr 12, 2017

Make TypeQuery more general, handling nonboolean queries. #3084

Merged

sixolet added 2 commits April 12, 2017 12:59

Merged master; adapted code to use better TypeQuery

cbb3cb0

More tests for nested and multi-type-var situations

da0d936

sixolet changed the title ~~Support for functions producing generic functions (WIP)~~ Support for functions producing generic functions Apr 12, 2017

ilevkivskyi reviewed Apr 12, 2017

View reviewed changes

Fixed type variable scoping rules to conform to PEP484 better

edbecb0

This was referenced Apr 14, 2017

Type variables bound by a generic class make no sense in static or class methods. #3172

Open

Generic inner classes sees type variable from outer class #3144

Open

sixolet added 2 commits April 17, 2017 16:23

Move the typevars that are prohibited due to being bound by the class…

21d8b13

… into TypeVarScope This allows us to be more consistent about which type variables we allow and which we don't.

More doc

214aebc

JukkaL reviewed Apr 20, 2017

View reviewed changes

sixolet added 2 commits April 20, 2017 09:57

Jukka comments

5405646

one more

a25e926

sixolet commented Apr 20, 2017

View reviewed changes

sixolet mentioned this pull request Apr 20, 2017

Better callable: Callable[[Arg('x', int), VarArg(str)], int] now a thing you can do #2607

Merged

sixolet added 2 commits April 20, 2017 10:47

get imports right

4aaab6c

Merge branch 'master' into second-order-decorator

fd153ad

JukkaL merged commit cc24f5b into python:master Apr 21, 2017

gvanrossum mentioned this pull request Apr 21, 2017

Making a decorator which preserves function signature #1927

Closed

WillAyd mentioned this pull request Jul 15, 2019

Support Decorators Maintaining Signatures of Generic Functions pandas-dev/pandas#27404

Closed

abhinavk99 mentioned this pull request Nov 18, 2019

Bench swaps and facade models (fixes #71) fantasy-anime-league/fantasy-anime-league#90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for functions producing generic functions #3113

Support for functions producing generic functions #3113

sixolet commented Apr 3, 2017 •

edited

Loading

sixolet left a comment

sixolet Apr 3, 2017

sixolet Apr 3, 2017

sixolet Apr 3, 2017

JukkaL Apr 20, 2017

sixolet Apr 3, 2017

JukkaL Apr 20, 2017

sixolet commented Apr 12, 2017

ilevkivskyi Apr 12, 2017

sixolet Apr 12, 2017

sixolet commented Apr 14, 2017

sixolet commented Apr 14, 2017

gvanrossum commented Apr 14, 2017

gvanrossum commented Apr 19, 2017

gvanrossum commented Apr 19, 2017

JukkaL left a comment

JukkaL Apr 20, 2017

JukkaL Apr 20, 2017

sixolet Apr 20, 2017

JukkaL Apr 20, 2017

sixolet Apr 20, 2017

JukkaL Apr 20, 2017

JukkaL Apr 20, 2017

JukkaL Apr 20, 2017

JukkaL Apr 20, 2017

JukkaL Apr 20, 2017

JukkaL Apr 20, 2017

sixolet left a comment

sixolet commented Apr 20, 2017

		@@ -406,7 +406,9 @@ def analyze_class_attribute_access(itype: Instance,
		return AnyType()

		if isinstance(node.node, TypeVarExpr):

		@@ -1500,112 +1500,112 @@ def keywords_str(self, a: Iterable[Tuple[str, Type]]) -> str:
		])

Support for functions producing generic functions #3113

Support for functions producing generic functions #3113

Conversation

sixolet commented Apr 3, 2017 • edited Loading

sixolet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixolet commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixolet commented Apr 14, 2017

sixolet commented Apr 14, 2017

gvanrossum commented Apr 14, 2017

gvanrossum commented Apr 19, 2017

gvanrossum commented Apr 19, 2017

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sixolet left a comment

Choose a reason for hiding this comment

sixolet commented Apr 20, 2017

sixolet commented Apr 3, 2017 •

edited

Loading