Skip to content

Improve abc module and builtin function decorators (with ParamSpec) #5682

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

Closed
wants to merge 12 commits into from

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Jun 23, 2021

  • Make classmethod and staticmethod generic over the function.
  • abstractclassmethod and abstractstaticmethod are classes, not
    functions.
  • Remove mypy-specific comments.

* Make classmethod and staticmethod generic over the function.
* abstractclassmethod and abstractstaticmethod are classes, not
  functions.
* Remove mypy-specific comments.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment on lines 122 to 125
def __init__(self, f: Callable[..., Any]) -> None: ...
def __init__(self, f: _FuncT) -> None: ...
def __new__(cls: Type[_T], *args: Any, **kwargs: Any) -> _T: ...
def __get__(self, obj: _T, type: Optional[Type[_T]] = ...) -> Callable[..., Any]: ...
def __get__(self, obj: _T, type: Type[_T] | None = ...) -> _FuncT: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right, because the callable returned from __get__ doesn't take the class as a first argument.

>>> class Foo:
...     @classmethod
...     def bar(cls):
...             print("Bar runs:", cls)
... 
>>> Foo.bar()
Bar runs: <class '__main__.Foo'>
>>> Foo.__dict__['bar'].__func__(Foo)
Bar runs: <class '__main__.Foo'>

Here Foo.__dict__['bar'] is the classmethod object, and for Foo.bar, its __get__ method gets called. The result is a bound method, which basically means that it includes the class similarly to partial:

>>> Foo.bar == Foo.__dict__['bar'].__get__(Foo(), None)
True
>>> Foo.bar
<bound method Foo.bar of <class '__main__.Foo'>>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another problem: I think it calls __get__(None, Foo) when looking up Foo.bar, so None should be allowed for the first argument to __get__.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. I tried to express this using ParamSpec, but I'm not sure this is correct. Also, do you know in what circumstances the owner argument can be None or not set?

Copy link
Collaborator

@Akuli Akuli Jun 28, 2021

Choose a reason for hiding this comment

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

It appears to be a technicality that doesn't really occur in practice: "Python’s own __getattribute__() implementation always passes in both arguments whether they are required or not." Docs

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator Author

srittau commented Jun 28, 2021

It seems that mypy doesn't support Concatenate yet, at least not in callables. Cf. #4827.

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Jun 28, 2021
@github-actions

This comment has been minimized.

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

The Concatenates look good to me, but I have never actually used Concatenate.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

freqtrade (https://github.com/freqtrade/freqtrade.git)
+ freqtrade/data/history/jsondatahandler.py:26: error: Signature of "ohlcv_get_available_data" incompatible with supertype "IDataHandler"
+ freqtrade/data/history/jsondatahandler.py:38: error: Signature of "ohlcv_get_pairs" incompatible with supertype "IDataHandler"
+ freqtrade/data/history/jsondatahandler.py:126: error: Signature of "trades_get_pairs" incompatible with supertype "IDataHandler"
+ freqtrade/data/history/hdf5datahandler.py:25: error: Signature of "ohlcv_get_available_data" incompatible with supertype "IDataHandler"
+ freqtrade/data/history/hdf5datahandler.py:37: error: Signature of "ohlcv_get_pairs" incompatible with supertype "IDataHandler"
+ freqtrade/data/history/hdf5datahandler.py:125: error: Signature of "trades_get_pairs" incompatible with supertype "IDataHandler"

graphql-core (https://github.com/graphql-python/graphql-core.git)
+ src/graphql/execution/execute.py:212: error: Incompatible types in assignment (expression has type "Callable[[Any], bool]", variable has type "Callable[[Arg(Any, 'value')], bool]")

pydantic (https://github.com/samuelcolvin/pydantic.git)
+ pydantic/class_validators.py:57: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:84: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:100: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:107: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:113: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:125: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:135: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/class_validators.py:328: error: Missing type parameters for generic type "classmethod"  [type-arg]
+ pydantic/main.py:953: error: Missing type parameters for generic type "classmethod"  [type-arg]

@@ -109,19 +113,19 @@ class object:
def __dir__(self) -> Iterable[str]: ...
def __init_subclass__(cls) -> None: ...

class staticmethod(object): # Special, only valid as a decorator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: These comments should be deleted. They seem misleading and mypy-specific to me.

@Akuli
Copy link
Collaborator

Akuli commented Jun 28, 2021

It's starting to LGTM, but I'm concerned about the new errors in freqtrade. They seem to be coming from usual @classmethod usage.

@srittau srittau changed the title Improve abc module and builtin function decorators Improve abc module and builtin function decorators (with ParamSpec) Jun 28, 2021
@JelleZijlstra
Copy link
Member

staticmethod and classmethod are now generic on master. Do we still need the abc.pyi changes?

@JelleZijlstra
Copy link
Member

Going to close this as stale for now to keep the list of open PRs manageable. If you're still interested in seeing this through, feel free to reopen or open a new PR.

@srittau srittau deleted the abcmeta branch January 9, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: deferred Issue or PR deferred until some precondition is fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants