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

Handling de-facto positional-only arguments with overloads #5235

Closed
Michael0x2a opened this issue Jun 19, 2018 · 5 comments
Closed

Handling de-facto positional-only arguments with overloads #5235

Michael0x2a opened this issue Jun 19, 2018 · 5 comments

Comments

@Michael0x2a
Copy link
Collaborator

In my PR for updating the mypy docs, @gvanrossum pointed out that one of my examples didn't typecheck -- the simplified repro was this:

class A: pass
class B: pass

@overload
def foo(a: int) -> A: pass
@overload
def foo(a: int, b: int) -> B: pass
def foo(*args: int) -> Union[A, B]:
    raise NotImplementedError()

After debugging, I realized that the checks are actually technically correct: the overloads suggest that running foo(a=1, b=2) is type-safe; that call will crash at runtime.

This feels a bit annoying though, so I'm thinking it'd be nice if we could relax these checks -- basically, allow the existence of "de-facto positional-only arguments".

I'm thinking one way we can do this is to:

  1. Relax the overload definition checks so they ignore the argument names
  2. Perform an extra check every time we call an overload: if the overload has an implementation, we check and see names match and report a (custom) error if they don't.

So under this proposal, foo would continue to type-check, but we'd report an error if somebody tried doing foo(a=1, a=2).

The main disadvantage is that this would cause feature disparity with overloads in stubs since stubs don't allow you to include an implementation. We could either live with this or maybe allow users to add an implementations with empty bodies in stubs if we really want to maintain parity.

Thoughts? (I wanted to file an issue to get consensus/see if anybody is able to come up with a better solution before I start implementation.)

@JelleZijlstra
Copy link
Member

In stubs, you can just use the __a syntax to indicate that the arguments are positional-only, right?

Perhaps that's also what we should have users do in implementation files.

@Michael0x2a
Copy link
Collaborator Author

Michael0x2a commented Jun 19, 2018

Huh, I didn't know that was even a thing.

It looks like the __a syntax actually already works in regular .py files, so I guess problem solved?

I guess the only downside is that it leaves the signatures looking sort of ugly when you have a lot of those parameters -- maybe I just need to find a different example for the docs altogether...

I'll close this in a little bit if nobody else chimes in.

@ilevkivskyi
Copy link
Member

I think there is still an action item here. Using __name is quite non-obvious, so I would propose to detect the situation where the error is because of missing **kwargs in the overload implementation and add a note that proposes to either:

  • Use positional only __name if this is the intention
  • Add **kwargs to the implementation otherwise

@nyanpasu64
Copy link

nyanpasu64 commented Mar 5, 2019

Related failure case: Overloads have meaningful parameter names, but implementation uses arg1 and arg2=None.

@overload
def add_row(right: Type[Right]) -> Any:
    ...

@overload
def add_row(left: Type[Left], right: Type[Right]) -> Any:
    ...

@overload
def add_row(label: str, right: Type[Right]) -> Any:
    ...

def add_row(arg1: Any, arg2: Any = None) -> Any:
    impl

@hauntsaninja
Copy link
Collaborator

mypy is behaving as expected, and with Python 3.7 dead it is much easier to mark positional-only args

@hauntsaninja hauntsaninja closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants