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

Flexible callable #2426

Closed
wants to merge 6 commits into from
Closed

Flexible callable #2426

wants to merge 6 commits into from

Conversation

sixolet
Copy link
Collaborator

@sixolet sixolet commented Nov 9, 2016

This is a draft implementation of the work discussed in python/typing#264

Most of the subtlety was in getting the function subtyping rules (what I believe to be) right.

Unfortunately basically all the typeshed stub files violate these rules all over the place -- specifically, arguments in a function overriding a superclass function must be named the same thing, otherwise calling that function by keyword is going to fail. So I still need to resolve that stuff.

Also I need to write a pile more tests.

Also also apparently I haven't implemented StarArg and KwArg yet.

Actually there's a decent amount left to do.

arg_type,
verbosity = max(verbosity-1, 0))))
else:
constructor = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: Move this constant dict thing out to a constant

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.

Prelim comments on my own diff.

ARG_STAR: "StarArg",
ARG_STAR2: "KwArg",
}[arg_kind]
if arg_kind in (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Move tuple constant out to a constant.

def __init__(name=None, typ=Any, keyword_only=False):
self.name = name
self.typ = typ
self.named_only = named_only
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add the rest of the kinds of arguments.

@@ -821,7 +839,12 @@ def construct_function_type(self, args: List[Argument], ret_type: Type,
if ret_type is None:
ret_type = AnyType(implicit=True)
arg_kinds = [arg.kind for arg in args]
arg_names = [arg.variable.name() for arg in args]
if include_names:
arg_names = [arg.variable.name()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This implements "names starting with __ are unnamed arguments". It's probably necessary to actually get the typeshed files passing.

if right_name is not None:
left_by_name = left.argument_by_name(right_name)
else:
left_by_name = None

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Blank lines totally match each other! For sure!

@@ -22,220 +22,220 @@ class NodeVisitor(Generic[T]):

# Module structure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are all overloaded later with different arg names. See also needing the positional-only syntax.

@@ -1230,9 +1230,9 @@ class D:
def __getattr__(self, x: str) -> None: pass
[out]
main: note: In member "__getattr__" of class "B":
main:4: error: Invalid signature "def (self: __main__.B, x: __main__.A) -> __main__.B"
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 removed argument names from special functions so these error messages changed.

int_named_str_fun = isf # E: Incompatible types in assignment (expression has type Callable[[Arg('ii', int, False), Arg('ss', str, False)], str], variable has type Callable[[int, Arg('s', str, False)], str])
int_named_str_fun = iosf

[builtins fixtures/dict.pyi]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So many more to come.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 10, 2016

Mypy doesn't check argument name compatibility in method overrides by design -- it would generate a huge number of errors for a lot of real-world code. Generally code like this implicitly expects callers not to use keyword arguments. Clearly this is not sound, which is unfortunate. On the other hand, users have generally not been complaining about this.

Also, many methods implemented in C only accept positional arguments. For them, we'd need a way of specifying in stub files that the arguments don't actually have names, and tracking this down for every single C method would be a pretty big task. When we previously discussed this, one idea was to use __argname to mean a positional-only argument.

I think that argument name compatibility checking for overrides should be left out of this PR and discussed separately, as it's a much bigger change than making Callable more flexible, and it would require major typeshed rework. What about having two kinds of compatibility checking for functions, one of which doesn't check argument name compatibility (except for keyword-only arguments)? We'd use the more lenient variant for checking method overrides. This way we could remain compatible with existing practice, including typeshed.

@sixolet
Copy link
Collaborator Author

sixolet commented Nov 10, 2016

@JukkaL How about I split this into a few different diffs:

  • Allow __argname to produce a positional-only argument (easy)
  • Implement the difference between required and optional keyword-only arguments
  • Function compatibility checks argument names, but not for positional arguments in situations where you're checking override compatibility. It checks all of them for assignments.
  • The part where we actually parse the new Callable syntax and use it.

And submit them each as separate pull requests?

They might have a dependency tree among them.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 11, 2016

Sounds like a good plan!

@sixolet
Copy link
Collaborator Author

sixolet commented Nov 12, 2016

I have started the series of smaller pull requests that this turned into.

@sixolet sixolet closed this Nov 12, 2016
@gvanrossum gvanrossum mentioned this pull request May 3, 2017
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.

2 participants