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

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

Merged
merged 34 commits into from
May 2, 2017

Conversation

sixolet
Copy link
Collaborator

@sixolet sixolet commented Dec 25, 2016

Implements an experimental feature to allow Callable to have any kind of signature an actual function definition does.

This should enable better typing of callbacks &c.

Initial discussion: python/typing#239
Proposal, v. similar to this impl: python/typing#264
Relevant typeshed PR: python/typeshed#793

@@ -287,17 +287,6 @@ def g(*x): # type: (X) -> Y
file:1: error: Inconsistent use of '*' in function signature
file:3: error: Inconsistent use of '*' in function signature

[case testCommentFunctionAnnotationVarArgMispatch2]
Copy link
Collaborator Author

@sixolet sixolet Dec 29, 2016

Choose a reason for hiding this comment

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

Removed this test because I made **kwargs happening before *args an error no matter what, as is having more than one *args.

>>> def foo(**kwargs, *args):
  File "<stdin>", line 1
    def foo(**kwargs, *args):
                    ^
SyntaxError: invalid syntax


T = TypeVar('T')


def TypedDict(typename: str, fields: Dict[str, Type[T]]) -> Type[dict]: pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All these names are up for discussion

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 3, 2017

Excellent, this addresses a frequent user feature request! I did a quick pass and have one correctness issue and a small style issue.

First, this implementation doesn't consider imports, so a few things don't work as expected. This shouldn't be hard to fix, though.

There's no error for this code which doesn't import StarArg:

from typing import Callable

def f(x: Callable[[StarArg(int)], str]) -> None: ...  # expected error: StarArg not defined

This also doesn't work (and this is an idiom that some users might want to use, so that StartArg won't be imported through from m import *):

from mypy_extensions import StarArg as _StarArg
from typing import Callable

def f(x: Callable[[_StarArg(int)], str]) -> None: ...

Finally, using a non-from import doesn't work:

import mypy_extensions as ext
from typing import Callable

def f(x: Callable[[ext.StarArg(int)], str]) -> None: ...

You can fix all of these by doing the analysis of special arguments in two stages. During parsing, accept anything like id(args) and x.y.id(args) in argument lists, similar to how we already handle UnboundType. For example, call these UnboundArgument. During semantic analysis (typeanal.py) we'd convert the [x.y.]id to a fully-qualified name such as mypy_extensions.StarArg, and we'd use that to detect the argument kind and process the arguments. If the name fails to resolve, we'd generate an error.

The style issue is the order of arguments for things like DefaultArg. If the type would be the first argument, all the argument helpers would have type as the first argument, which would arguably be a bit more consistent. Also, defining a positional-only default argument would be simpler:

Callable[[DefaultArg(int)], str]

vs.

Callable[[DefaultArg(None, int)], str]

However, elsewhere the type comes after the name (NamedTuple is an example) so I'm not entirely convinced that this would be better.

@gvanrossum
Copy link
Member

I wonder if I could convince you to rename StarArg to VarArg? IMO that would increase the similarity to KwArg. It seems weird to have *args named after its form and *kwds named after its function.

Otherwise I have nothing to add to Jukka's feedback.

@ambv
Copy link
Contributor

ambv commented Jan 12, 2017

I love the feature but can't help but lament the syntax. It gets very unfriendly and unreadable real fast. When we were discussing the Callable when PEP 484 was first drafted, Guido argued that "Callable" for typing purposes is really "Callback", and callbacks are usually very simple in their signatures. Hence this anonymous callback type in typing.

For expressing any possible signature, it seems to me we'd be silly not to reuse Python's own syntax for expressing function signatures with types.

Before we commit to StarArg, VarArg, et al., I'd like to ask how much more complex would it be to enable the following instead:

def StartResponse(
    status: str, headers: List[Tuple[str, str]], exc_info: _exc_info = None,
) -> Callable[[Union[bytes, str]], None]: ...

def WSGIApplication(
    environ: Dict[str, str], start_response: StartResponse,
) -> Iterable[Union[bytes, str]]: ...

See how I used StartResponse as a "type" of callable? And I could later use WSGIApplication in the same vein. I would put those fake function stubs in my code to be able to use them as "types" but that's no different than creating a type alias or importing Dict from typing. If runtime behavior would be altered by the presence of an additional callable in the module, I could always do:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    def StartResponse(
        status: str, headers: List[Tuple[str, str]], exc_info: _exc_info = None,
    ) -> Callable[[Union[bytes, str]], None]: ...

def WSGIApplication(
    environ: Dict[str, str], start_response: "StartResponse",
) -> Iterable[Union[bytes, str]]: ...

It seems to me such representation provides radically better readability, and this was and still is one of the main reasons we have type hints.

(Too see the same example with just callables, look how unwieldy it got in python/typeshed#825)

@sixolet
Copy link
Collaborator Author

sixolet commented Jan 12, 2017 via email

@gvanrossum
Copy link
Member

Note that we've discussed this before, e.g. python/typing#239. The conclusion was that there are still some serious problems with using def syntax for this purpose.

That said, I think even once this PR is merged we could still iterate if we find that this is used a lot (personally I expect that the general form is still going to be pretty uncommon).

@rowillia
Copy link
Contributor

@ambv @sixolet It would be really nice if what is currently StarArgs became a TypedDict or there was some interoperability between the two. Effectively both are structured dictionaries. piggy-backing off of TypedDict would make it easier to type check banging together a **kwargs at runtime.

@rowillia
Copy link
Contributor

Note we had discussed this in #2003 as well. I still would love to be able to inherit keyword arguments from another function and expressing that as a TypedDict would be a really slick way to do that.

@gvanrossum
Copy link
Member

@rowillia Can you write up a detailed proposal in the typing tracker?

@sixolet
Copy link
Collaborator Author

sixolet commented Feb 13, 2017

@rowillia I've been looking at the prospect of typing **kwargs-kind arguments with typed dicts. There are a few relevant questions:

  1. Keeping parity with function definitions. You'd need a way to type a **kwargs with a typed-dict right in there in the arguments of a function def -- that doesn't confuse it with every value of the kwargs dict being required to be the typed dict. This bears some relation to the variadic type variable Expand operation -- if we can convince ourselves they're similar enough, perhaps we can use the same syntax.
  2. Then you'd also need a way to get the same semantics in Callable. This is actually easier -- you could have KwArg take keyword dict_type argument or something that'd differentiate it from the type argument. In any case, it's easily expandable from the syntax in this PR.
  3. Runtime value. It seems to me that like NamedTuple or its typing equivalent, TypedDict is in fact a runtime type. Please correct me if I'm wrong -- but the actual runtime type when the program was running in Python wouldn't be TypedDict but rather a plain old kwargs dictionary. I am not sure I like that.

@gvanrossum
Copy link
Member

@sixolet Actually the runtime type corresponding to TypedDict is always plain dict -- that's how it is designed. I'm a bit confused by the problem you're trying to solve there: if I can specify the kwds as a TypedDict I could just as well pass actual keyword arguments...

@gvanrossum
Copy link
Member

@sixolet Maybe you can rebase this too? (Or merge in origin/master, that should work too and might retain more review context.)

@@ -93,5 +95,35 @@ class Point2D(TypedDict):
"""


def Arg(typ=Any, name=None):
"""A normal positional argument"""
return typ
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 why it makes sense to just return typ here and elsewhere in this file.

@@ -43,6 +54,29 @@ def expr_to_unanalyzed_type(expr: Expression) -> Type:
return base
else:
raise TypeTranslationError()
elif isinstance(expr, CallExpr):
if not isinstance(expr.callee, NameExpr):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also support MemberExpr.

Copy link
Collaborator Author

@sixolet sixolet left a comment

Choose a reason for hiding this comment

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

Comments from being on the phone with Jukka

@@ -43,6 +54,29 @@ def expr_to_unanalyzed_type(expr: Expression) -> Type:
return base
else:
raise TypeTranslationError()
elif isinstance(expr, CallExpr):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Support MemberExpr

value = k.value
if k.arg == "name":
name = self._extract_str(value)
elif k.arg == "typ":
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use type as keyword arg name instead.

if k.arg == "name":
name = self._extract_str(value)
elif k.arg == "typ":
typ = self.visit(value)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make an error if we re-set name or type

def translate_argument_list(self, l: Sequence[ast3.AST]) -> TypeList:
return TypeList([self.visit(e) for e in l], line=self.line)

def _extract_str(self, n: ast3.expr) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rename this to be like _extract_argument_name

Figure out handling unicode in python2:

Arg(int, u'eggs')

mypy/fixup.py Outdated
@@ -11,7 +11,7 @@
from mypy.types import (
CallableType, EllipsisType, Instance, Overloaded, TupleType, TypedDictType,
TypeList, TypeVarType, UnboundType, UnionType, TypeVisitor,
TypeType
TypeType, CallableArgument,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove from imports

mypy/types.py Outdated
@@ -517,6 +552,7 @@ class CallableType(FunctionLike):
arg_names = None # type: List[str] # None if not a keyword argument
min_args = 0 # Minimum number of arguments; derived from arg_kinds
is_var_arg = False # Is it a varargs function? derived from arg_kinds
is_kw_arg = False
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unused. Remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not actually unused.

mypy/types.py Outdated
@@ -1288,6 +1328,9 @@ def visit_unbound_type(self, t: UnboundType) -> Type:
def visit_type_list(self, t: TypeList) -> Type:
return t

def visit_callable_argument(self, t: CallableArgument) -> Type:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have a recursive call here.

mypy/types.py Outdated
@@ -1288,6 +1328,9 @@ def visit_unbound_type(self, t: UnboundType) -> Type:
def visit_type_list(self, t: TypeList) -> Type:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also a recursive call here!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... or try out what happens if TypeTranslator isn't SyntheticTypeVisitor in another diff.

@@ -11,12 +11,12 @@ Optional = 0
TypeVar = 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember why I did all this. Try removing it and see what happens


class Mapping(Generic[T, U]): pass

class MutableMapping(Generic[T, U]): pass

def NewType(name: str, tp: Type[T]) -> Callable[[T], T]:
def new_type(x):
def new_type(x: T) -> T:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just remove impl?

@sixolet sixolet changed the title Better callable: Callable[[Arg('x', int), StarArg(str)], int] now a thing you can do Better callable: Callable[[Arg('x', int), VarArg(str)], int] now a thing you can do Apr 20, 2017
@gvanrossum
Copy link
Member

We need another merge. (No need to rebase, in fact we prefer merges now.)

We're going to discuss soon who should review this.

@sixolet
Copy link
Collaborator Author

sixolet commented Apr 22, 2017

@JukkaL and I spent an hour or so on the phone yesterday to get a good amount of review in. I've just merged master and thrown fixes in for (I think) all the review comments we collectively came up with.

@sixolet
Copy link
Collaborator Author

sixolet commented Apr 22, 2017

(NB: The pile of review comments I added myself are from that phone/screenshare conversation)

@sixolet
Copy link
Collaborator Author

sixolet commented Apr 22, 2017

... Oh, another merge is needed since I just merged. On it. Also realized I missed a comment.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 22, 2017

Yes, this is mostly reviewed and looks good -- I just want to study the tests a bit more, and maybe one or two other places in the PR.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 25, 2017

When running against Dropbox internal code bases, I found an issue with named tuples:

from collections import namedtuple
class C(namedtuple('t', 'x')):  # Name 't' is not defined
    pass

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Just a few ideas for test case improvements. (EDIT: And one potential bug.)

fail("Var args may not appear after named or var args", node)
break
is_var_arg = True
elif kind == ARG_NAMED or kind == ARG_NAMED_OPT:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we shouldn't allow these after **kwargs, since Python doesn't seem to allow anything after **kwargs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

f(x="foo") # E: Argument 1 has incompatible type "str"; expected "int"
[builtins fixtures/dict.pyi]

[case testCallableWithKeywordOnlyOptionalArg]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add similar test cases for VarArg() and KwArg().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

from mypy_extensions import Arg, VarArg as VARG, KwArg
import mypy_extensions as ext

def WrongArg(x, y): return y
Copy link
Collaborator

Choose a reason for hiding this comment

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

Define function called Arg in the current module and verify that it can't be used within Callable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 25, 2017

Thanks for the updates! This is pretty close to ready.

We also need python/typeshed#793 before these work outside tests.

@sixolet
Copy link
Collaborator Author

sixolet commented May 1, 2017

Kay. I think that's the latest round of @JukkaL commentary and a fix for the bug found by running against the DBX codebase.

@JukkaL
Copy link
Collaborator

JukkaL commented May 2, 2017

Thanks for the final fixes! Looks good, just in time for the next release. Once more, thanks for working on this and being so patient!

@JukkaL JukkaL merged commit ddf03d1 into python:master May 2, 2017
@gvanrossum
Copy link
Member

Congratulations on getting this in!

Now I have a little bit of bad news: I found a crasher.

from typing import Callable
from mypy_extensions import Arg
C = Callable[[Arg(int), Arg(int, 'x')], int]
def deco(func: C) -> C: pass
@deco
def foo(__b: int, x: int) -> int: pass
reveal_type(foo(x=2))

The last line gives a traceback:

__tmp__.py:7: error: INTERNAL ERROR -- please report a bug at https://github.com/python/mypy/issues version: 0.510-dev-d432240c653bcc6f45906324a0d3560b4e697a83
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/local/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/Users/guido/src/mypy/mypy/__main__.py", line 5, in <module>
    main(None)
  File "/Users/guido/src/mypy/mypy/main.py", line 46, in main
    res = type_check_only(sources, bin_dir, options)
  File "/Users/guido/src/mypy/mypy/main.py", line 93, in type_check_only
    options=options)
  File "/Users/guido/src/mypy/mypy/build.py", line 188, in build
    graph = dispatch(sources, manager)
  File "/Users/guido/src/mypy/mypy/build.py", line 1595, in dispatch
    process_graph(graph, manager)
  File "/Users/guido/src/mypy/mypy/build.py", line 1838, in process_graph
    process_stale_scc(graph, scc, manager)
  File "/Users/guido/src/mypy/mypy/build.py", line 1937, in process_stale_scc
    graph[id].type_check_first_pass()
  File "/Users/guido/src/mypy/mypy/build.py", line 1510, in type_check_first_pass
    self.type_checker.check_first_pass()
  File "/Users/guido/src/mypy/mypy/checker.py", line 177, in check_first_pass
    self.accept(d)
  File "/Users/guido/src/mypy/mypy/checker.py", line 264, in accept
    stmt.accept(self)
  File "/Users/guido/src/mypy/mypy/nodes.py", line 824, in accept
    return visitor.visit_expression_stmt(self)
  File "/Users/guido/src/mypy/mypy/checker.py", line 1823, in visit_expression_stmt
    self.expr_checker.accept(s.expr, allow_none_return=True)
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 2054, in accept
    typ = self.visit_call_expr(node, allow_none_return=True)
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 176, in visit_call_expr
    return self.accept(e.analyzed, self.type_context[-1])
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 2058, in accept
    typ = node.accept(self)
  File "/Users/guido/src/mypy/mypy/nodes.py", line 1517, in accept
    return visitor.visit_reveal_type_expr(self)
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 1552, in visit_reveal_type_expr
    revealed_type = self.accept(expr.expr, type_context=self.type_context[-1])
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 2058, in accept
    typ = node.accept(self)
  File "/Users/guido/src/mypy/mypy/nodes.py", line 1302, in accept
    return visitor.visit_call_expr(self)
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 201, in visit_call_expr
    ret_type = self.check_call_expr_with_callee_type(callee_type, e)
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 341, in check_call_expr_with_callee_type
    e.arg_names, callable_node=e.callee)[0]
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 396, in check_call
    arg_names, formal_to_actual, context, self.msg)
  File "/Users/guido/src/mypy/mypy/checkexpr.py", line 778, in check_argument_count
    messages.too_few_arguments(callee, context, actual_names)
  File "/Users/guido/src/mypy/mypy/messages.py", line 579, in too_few_arguments
    msg += ' "{}" in call to {}'.format('", "'.join(diff), callee.name)
TypeError: sequence item 0: expected str instance, NoneType found
__tmp__.py:7: note: use --pdb to drop into pdb

It's probably simple, but I have other things to attend to before I can debug this.

@sixolet
Copy link
Collaborator Author

sixolet commented May 3, 2017 via email

@sixolet
Copy link
Collaborator Author

sixolet commented May 3, 2017

Heh. Hilariously, it's a bug that I probably introduced but not in this diff, a few weeks ago when I was making functions ok to have anonymous arguments. I'll have a fix in a sec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants