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

Add decorator for delegating arguments #5559

Closed
wants to merge 3 commits into from

Conversation

alexmojaki
Copy link

This is an attempt to implement python/typing#270. (I would suggest for now only reading the 2016 comments, after that it turns in a different direction which is for now irrelevant)

I've never contributed to this project before so I'm hesitant to try and do it all at once, hence I'm presenting this first draft to see if I'm on the right track (am I?) and ask some questions. If someone thinks I should just go ahead and fill in the remaining gaps as I see fit, I can do that too.

I've put this in mypy_extensions because it seems like the appropriate place to start (is there an alternative?) but eventually I would love to see this in the standard library. The applications go well beyond type checking, for example IDEs could use it for parameter hints and autocompletion. However they're not likely to implement that unless this is in the standard library.

Two major open questions are:

  1. What if a function is decorated with delegate and also other decorators? In most cases mypy doesn't support decent type checking with other decorators, so if we took them into account the result of delegate would either be lost or trivial. Even if there is a decorator that changes the signature in a way that mypy can understand, reading the function definition with the decorators and trying to figure out the correct signature may be very confusing for a reader. Therefore I'm considering ignoring all other decorators when determining the signature of the function - delegate has the final say. That also doesn't sound great. Thoughts?
  2. How should *args be delegated? Currently only **kwargs are handled, and they are replaced by any keyword arguments in the base function (whether optional, required, or variadic). I'm thinking *args should similarly be replaced by any positional arguments in the base function. Thoughts?

@ilevkivskyi
Copy link
Member

Thanks for the PR!

TBH I am not sure I like this idea. I understand the problem, but current approach has several problems:

  • It looks too ad-hoc: it solves one particular problem, but is non-trivial to extend to other situations
  • Syntax is not obvious, and may cause confusions for people who is not familiar with the problem in the first place.
  • There are potential corner cases (like interactions with fine-grained daemon)
  • I may be non-trivial to check that the function body actually performs the delegation

I would prefer just allow using TypedDict to annotate **kwargs see e.g. #4441. This may be a bit more verbose, but on the other hand one can use the same TypedDict for the original function too. Plus this feature has a broader scope.

@alexmojaki
Copy link
Author

Thanks for your feedback. Here are my counterarguments:

It looks too ad-hoc: it solves one particular problem, but is non-trivial to extend to other situations

It is meant for a particular problem, but I think it's a problem that appears often (as was pointed out repeatedly in the issue) and is worth solving on its own.

Syntax is not obvious, and may cause confusions for people who is not familiar with the problem in the first place.

Can you elaborate how it might be confusing? I don't see it. Is it more confusing than, say, functools.wraps? I think it's fairly simple to explain the concept. Start with the simplest, most common case:

@delegate(foo)
def bar(*args, **kwargs):

means that bar has the exact same signature as foo. No background needed. Then explain in more detail for cases where the original signature of bar is more complicated. In any case most decorators need some explaining. Personally when I want to understand a function the decorators are the last thing I think about because they generally have little to do with behaviour.

There are potential corner cases (like interactions with fine-grained daemon)

Is your concern now about implementation or still about the concept? In any case I will have to defer to you and other people who understand how mypy works, but if TypeChecker.visit_decorator works with the daemon at the moment, I'd be surprised if this change doesn't work.

I may be non-trivial to check that the function body actually performs the delegation

Is it important to check that it does? I don't see much potential for introducing errors if there's no delegation.

I would prefer just allow using TypedDict to annotate **kwargs

This doesn't work in general. If I have:

def bar(*args, **kwargs):
    foo(*args, **kwargs)

def foo(a: int, b: int = 0, c: int = 1): ...

Then bar(3, '5') can be type checked by delegate but not by annotating kwargs. In other words this is about positional arguments as well.

This may be a bit more verbose, but on the other hand one can use the same TypedDict for the original function too.

How would I do that with foo above?

Moreover, this is about more than type checking. I use PyCharm, as do many other people. If I encounter a call to bar in some code, or I'm writing a call to it, and I want to know about its parameters, I can place my cursor inside the parentheses and press Ctrl+P. This shows the parameters in a little bubble. Without any tooling, it just shows *args, **kwargs, which is annoying. With a TypedDict annotation it may somehow show the parameter names and types, but not the default values which it can currently show for foo. The same goes for the help builtin.

Suppose I want to see what the parameters mean. Typically my next step would be to Ctrl+click on bar to navigate to its definition and hopefully find out more there. The docstring is unlikely to repeat the meanings of the arguments (and we shouldn't make authors do so) so at the moment I probably have to eyeball for a phrase like 'the parameters are the same as in foo'. Then I manually find foo (Ctrl+click won't work in a docstring) or look for it in the function body so I can Ctrl+click that. If delegate(foo) is there it will be immediately noticeable and an obvious thing to Ctrl+click.

It was pointed out in the issue that the decorator aids readability. I think it's useful to know that bar uses foo for the heavy lifting without having to check the source code.

Personally I often can't be bothered to add type annotations to my own code. I certainly don't want to annotate kwargs with a TypedDict. @delegate(foo) is much less effort and immediately provides a lot of information for linters even in a codebase that has zero type annotations, e.g. which argument names are valid. Updating an existing large codebase to use delegate wherever the pattern appears is something some people might not mind doing, whereas adding a TypedDict annotation in all such places is much more unlikely.

@ilevkivskyi
Copy link
Member

It is meant for a particular problem, but I think it's a problem that appears often (as was pointed out repeatedly in the issue) and is worth solving on its own.

Contradiction is not an argument :-) (I don't know if you are familiar, this is a reference to a classic sketch). Seriously however mypy (== Python static type system here) is already complex. I would avoid any ad-hoc solutions, unless there are no other way to solve it.

Can you elaborate how it might be confusing? I don't see it.

It looked confusing to me, it was necessary to read the original issue to understand semantics.

In other words this is about positional arguments as well.

Hm, maybe then a better option would be to allow using callback protocols to re-use function signatures? For example,

class DoStuff(Protocol):
    def __call__(a: int, b: int = ..., c: int = ...) -> None: ...

f: DoStuff
def f(*args, **kwargs):
    ...

g: DoStuff
def g(*args, **kwargs):
    f(*args, **kwargs)

I certainly don't want to annotate kwargs with a TypedDict

Some other people may say similar about @delegate(). Note that there are other type checkers, so a feature of this scale might need to go though a PEP. Other solutions I mentioned either have a PEP or will have one soon.


To summarize, it was way too soon to make a PR for this without agreeing at least on large-scale design. I think it is better to return back to the issue.

@alexmojaki
Copy link
Author

it was necessary to read the original issue to understand semantics.

Well I'd expect that because at the moment there is no other documentation. I didn't feel the need to write any at this stage because (1) the PR was still tentative, and (2) I expected people to read the issue since it contains other important information. I don't think it should be a requirement that this decorator is so self-evident that it can be understood without reading anything.

Hm, maybe then a better option would be to allow using callback protocols to re-use function signatures?

Your example means that the body of f has to manually extract the arguments from the args and kwargs, which I don't think anyone wants to do. The most concise and correct way to extract arguments is simply a normal function signature. This means that using protocols would require writing the signature twice. There is already a way to solve this whole problem if one is willing to repeat themselves, and that is simply to rewrite the signature in g. We see the current pattern so often because people want to avoid that repetition. Protocols would mean less duplication than that, but I think it's much better to keep code entirely DRY.

A decorator also allows attaching information which can be retrieved at runtime by functions like help, inspect.signature, or typing.get_type_hints. AFAIK a variable annotation does not allow any of that, save typing.get_type_hints(module)[function_name]. We may or may not want to do anything at runtime at this stage, but it would be nice to allow that extension in the future.

I certainly don't want to annotate kwargs with a TypedDict
Some other people may say similar about @delegate().

Well if they say the same it would probably be for different reasons. I'm saying I don't want the effort, clutter, and repetition of a verbose type annotation (and this applies even more to Protocol). @delegate() is pretty much as concise as expressing this concept could possible be.

a feature of this scale might need to go though a PEP

Presumably it doesn't need a PEP just to get into mypy_extensions? I want this in the standard library eventually. Having it here first where it can be be experimented with seems best.

it was way too soon to make a PR for this without agreeing at least on large-scale design. I think it is better to return back to the issue.

The discussion on the issue had stopped and I didn't think there was anything to add. People seemed to think it was a good idea and it didn't seem very complicated. I just thought no one was willing to implement it, so I decided to do it myself. Doing so made me aware of some of the nitty gritty issues which I thought were worth discussing. I also wanted to share the work I had done so far and give people an opportunity to checkout the branch and play with the decorator themselves rather than just imagine what it would do. Had I not done any of this then the issue would still be dead and we wouldn't be debating alternatives.

I'm not sure what damage you think a PR does, since a PR is not that different from a non-PR issue and we're having plenty of valuable discussion here. I'm guessing perhaps you're worried people will be distracted by less important details in the code? In any case I will take your advice and put a proper proposal in the issue. For now the only comments I will make here will be in response to other comments here.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 6, 2018

I think that this might be a useful feature to have in mypy_extensions in the near future, at least as an experimental feature that may change in the future. Thanks for working on this!

This at least partially addresses a common use case that is a real pain point. However, I agree with @ilevkivskyi that we should continue to discuss the design of the feature a bit more in the original issue, though this PR is useful in getting the ball rolling.

(I can't promise that the feature will be accepted soon, but I see some hope.)

@ilevkivskyi
Copy link
Member

The discussion on the issue had stopped and I didn't think there was anything to add. People seemed to think it was a good idea and it didn't seem very complicated. I just thought no one was willing to implement it, so I decided to do it myself. Doing so made me aware of some of the nitty gritty issues which I thought were worth discussing.

Thanks for investing your time in this, and sorry if I sounded harsh. I just think this needs more thought before moving forward, I will add some comments in the issue.

@stereobutter
Copy link

class DoStuff(Protocol):
    def __call__(a: int, b: int = ..., c: int = ...) -> None: ...

f: DoStuff
def f(*args, **kwargs):
    ...

g: DoStuff
def g(*args, **kwargs):
    f(*args, **kwargs)

together with allowing the same for the Callable e.g.

add_int: Callable[[int, int], int]
def add_int(a,b):
    return a + b

is also more in line with how functional languages like Haskell annotate functions. See also #1641

@alexmojaki
Copy link
Author

@SaschaSchlemmer as others suggested, keep discussion in the issue python/typing#270. I suggest copying this comment there.

@hauntsaninja
Copy link
Collaborator

Closing, since the underlying issue has been closed following the acceptance of PEP 612.

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