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

Fix Callable type to not be types.FunctionType #614

Closed
KotlinIsland opened this issue Jan 11, 2024 · 7 comments · Fixed by #619
Closed

Fix Callable type to not be types.FunctionType #614

KotlinIsland opened this issue Jan 11, 2024 · 7 comments · Fixed by #619
Assignees

Comments

@KotlinIsland
Copy link
Owner

KotlinIsland commented Jan 11, 2024

from __future__ import annotations
from typing import *
from types import FunctionType

fn1: Callable[[], None]
reveal_type(fn.__name__)  # expect error

fn2: Callable[[], None] & FunctionType
reveal_type(fn.__name__)  # expect 'str'

This on it's own would be fantastic and fix a huge hole in the type system. But maybe we can go further and create aliases and specializations for the callable types in types:

  • FunctionType
  • LambdaType - same as FunctionType
  • MethodType
  • BuiltinFunctionType
  • BuiltinMethodType - same as BuiltinFunctionType
  • WrapperDescriptorType
  • MethodWrapperType
  • MethodDescriptorType
  • ClassMethodDescriptorType

The common attributes are: ['__name__', '__qualname__'], maybe there would be value in predefining a type like:

class Named(Protocol):
    __name__: str
    __qualname__: str
# OR
class FunctionLike(Protocol):
    def __call__(*x, **y) -> z: ...
    __name__: str
    __qualname__: str

And the most useful one would be FunctionType with it's ['__annotations__', '__closure__', '__code__', '__defaults__', '__get__', '__globals__', '__kwdefaults__', '__name__', '__qualname__'].

So we could introduce a GenericAlias for FunctionType in basedtyping, it could also work with __future__.annotations:

from __future__ import annotations
from types import FunctionType

f: FunctionType[[int], str] = lambda x: str(x)

Depending on how much work it is, we could just do all of them.

method binding / __get__

function defines __get__ which will bind self with instances, currently mypy doesn't handle this correctly:

def f():
    print("f")

class C:
    f: Callable[[], None] = f  # no error

C().f()  # runtime error: too many arguments

I think that there is a larger problem of descriptors on subtypes breaking LSP(here), so even if we made this change, I don't think this aspect would be resolved. But if we used FunctionType here the error could be appeared correctly:

def f():
    print("f")

class C:
    f: FunctionType[[C], None] = f  # error: Incompatible types in assignment (expression has type "FunctionType[[], None]", variable has type "Callable[[C], None]")  [assignment]
    f2: FunctionType[[], None] = f
C().f2()  # error: too many arguments

(The semantics are up for discussion, but you would at least get some kind of error in this scenario)

The team has discussed this issue, and we are inclined to agree that FunctionType is not a subtype of Callable, because it's __get__ can return MethodType, which breaks LSP.

This is only a problem on assignment to the class, so it is fine like:

class A:
    def __init__(self):
       self.fn = lambda x: x
a = A()
a.fn(1)

We propose a new type alias CallableType be added to basedtyping with a value of Callable | FunctionType.

@AnonymouX47
Copy link

AnonymouX47 commented Jan 11, 2024

Great idea!... but I'll suggest the scope of this issue be broadened to takle the actual problem once and for all.

There's more than just __name__ to deal with. So, I'd suggest making Callable define only __call__ and having separate subtypes of Callable for every single built-in callable type (though classes should already be covered by type), not just user-defined functions.

Of course, I suppose users can manually define these but having to define the for every project can be a pain in the ass. Hence, the point of exporting such subtypes from basedtyping.

If you deem this fit and eventually implement it, I'd suggest adding a note in the docs about the incompatibility of Callable and the use of the more specific subtypes.

See also: python#14392 (comment)

@KotlinIsland

This comment was marked as outdated.

@KotlinIsland KotlinIsland changed the title Fix Callable type to not have __name__ Fix Callable type to not be types.FunctionType Jan 11, 2024
@KotlinIsland
Copy link
Owner Author

@daniil-berg @AnonymouX47 @helpmefindaname How does this proposal look?

@DetachHead
Copy link
Collaborator

DetachHead commented Jan 12, 2024

another issue caused by the same thing:

# mypy: enable-error-code=redundant-expr
from types import MethodType, FunctionType
from typing import Callable

a: Callable[[], None]
if isinstance(a, FunctionType) or bool(): ...  # error: Left operand of "or" is always false  [redundant-expr]
if isinstance(a, MethodType) or bool(): ...  # error: Left operand of "or" is always false  [redundant-expr]
if isinstance(a, type) or bool(): ...  # error: Left operand of "or" is always false  [redundant-expr]

This was referenced Jan 13, 2024
@AnonymouX47
Copy link

@AnonymouX47 What do you think would be best? providing GenericAliass and specialization for all of:

  • FunctionType
  • LambdaType - same as FunctionType
  • MethodType
  • BuiltinFunctionType
  • BuiltinMethodType
  • WrapperDescriptorType
  • MethodWrapperType
  • MethodDescriptorType
  • ClassMethodDescriptorType
  • AsyncGeneratorType
  • CoroutineType
  • GeneratorType

Or a subset of those perhaps?

Everythings looks okay to me except the last three types which I believe are not callable... instead, they're the types returned by their corresponding kinds of functions i.e Generator Function (of FunctionType) returns GeneratorType, etc.

I could imagine that it could get hairy specifying all of the different types that can come out of different things.

Covering all those in the types module should be sufficient, which I believe you've listed here.


Sorry for the late reply.

@KotlinIsland
Copy link
Owner Author

KotlinIsland commented Jan 15, 2024

So we've got an open pr:

With most of the work done already, not exactly what was discussed here.

@KotlinIsland KotlinIsland self-assigned this Jan 16, 2024
@dhruvinsh
Copy link

dhruvinsh commented Oct 2, 2024

I have open a discuss to understand this feature with more clarity, #770

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

Successfully merging a pull request may close this issue.

4 participants