Add missing function attributes to builtins.function

[jpy-git]

Follow up PR to #6803. In that PR we tried getting builtins.function to share attributes/methods with types.FunctionType but it didn't work. Instead I'm just manually adding the missing attributes to builtins.function to resolve this issue: python/mypy#11896 (comment)

Changes:

• Add @final decorator to builtins.function.
• Add missing attributes from types.FunctionType to builtins.function.
• Add __get__ method from types.FunctionType to builtins.function.
• Update return type of builtins.function.__get__ to Any to account for property and getset_descriptor returns.

[AlexWaygood]

Could you try copying over the __get__ method and decorating it with @final, as well? It might not work, but would be good to try.

[jpy-git]

I also notice that types.FunctionType is missing the attribute __module__ which is in builtins.function. Is it worth adding that in (can be a separate PR if needed)?

[AlexWaygood]

I also notice that types.FunctionType is missing the attribute __module__ which is in builtins.function. Is it worth adding that in (can be a separate PR if needed)?

Neither builtins.function nor types.FunctionType should really need to define __module__, since __module__ is defined in the stub for object, and all classes implicitly inherit from object. So, you could try deleting it from builtins.function.

But obviously this is a really weird class that doesn't actually exist, and is only here because of a weird mypy implementation detail that says it has to exist. So maybe deleting __module__ from builtins.function breaks mypy. Who knows. The only way to find out is to try!

[AlexWaygood]

I'd argue the proper response to the failing stubtest checks would be to add entries to https://github.com/python/typeshed/blob/master/tests/stubtest_allowlists/py3_common.txt for types.FunctionType.__get__ and types.LambdaType.__get__. I'm pretty sure the new signature is better than the old one; for whatever reason, stubtest is wrong here.

[jpy-git]

I'd argue the proper response to the failing stubtest checks would be to add entries to https://github.com/python/typeshed/blob/master/tests/stubtest_allowlists/py3_common.txt for types.FunctionType.__get__ and types.LambdaType.__get__. I'm pretty sure the new signature is better than the old one; for whatever reason, stubtest is wrong here.

@AlexWaygood agree with you. A single argument in __get__ works fine for me:

>>> class Foo: ...
... 
>>> def bar(self): print("hi")
... 
>>> bar.__get__(Foo)
<bound method bar of <class '__main__.Foo'>>
>>> bar.__get__(Foo)()
hi

[jpy-git]

The Unused type imports in mypy_primer are a good sign that this fixes some mypy issues.

For this one I think it's a mistake in urllib3 rather than a mypy issue:

src/urllib3/response.py:773: error: Incompatible return value type (got "MethodType", expected "bool")  [return-value]

https://github.com/urllib3/urllib3/blob/fd2759aa16b12b33298900c77d29b3813c6582de/src/urllib3/response.py#L773
Think it should be return io.IOBase.closed.__get__(self)() since they get the bound method and should then call it

[jpy-git]

So maybe deleting __module__ from builtins.function breaks mypy. Who knows. The only way to find out is to try!

will try this now 👍

[AlexWaygood]

On the other hand:

>>> object().__module__
Traceback (most recent call last):
  File "<string>", line 1, in <module>
AttributeError: 'object' object has no attribute '__module__'

So, even though it does now, it's questionable whether object should have __module__ defined in the stub.

Maybe it would be best to leave the __module__ changes for another PR (if at all) since they're basically irrelevant to what this PR is trying to achieve. Sorry for leading you astray!

[jpy-git]

hold off on merging for a sec, just want to check something quickly on the get method

[AlexWaygood]

hold of on merging for a sec, just want to check something quickly on the get method

I don't have the power to merge, don't worry 😄

[jpy-git]

So the urllib3 issue raised by mypy_primer is valid.

Currently we have __get__ in both builtins.function and types.FunctionType returning MethodType which is defined here (basically just a dummy method).

However this doesn't account for the fact that this method could be a property. Consider this script:

class Foo:

    def __init__(self) -> None: ...

    def bar(self) -> str:
        return "bar"
    
    @property
    def baz(self) -> str:
        return "baz"

class OtherClass:

    def __init__(self) -> None: ...

if __name__ == "__main__":

    foo = Foo()
    print(type(foo.bar))
    print(type(foo.baz))

    other_class = OtherClass()
    print(type(Foo.bar.__get__(other_class)))
    print(type(Foo.baz.__get__(other_class)))

This prints:

<class 'method'>
<class 'str'>
<class 'method'>
<class 'str'>

So __get__ should actually return Any rather than just MethodType.

Think I should drop the __get__ change from this PR and put it through separately?

[AlexWaygood]

Think I should drop the __get__ change from this PR and put it through separately?

Personally, I see nothing wrong with including it in this PR (but, I'm not a maintainer, just a contributor). The PR is about false-positive mypy errors for function attributes, and __get__ seems to be the single most common false-positive error reported by mypy. (I've come across it myself.)

I agree that it would be good to change the return type to Any, though (along with a comment linking to your good explanation above, to say why the return type is Any rather than MethodType).

[Akuli]

For this one I think it's a mistake in urllib3 rather than a mypy issue:

urllib3 is correct. Because closed is a property (technically a getset_descriptor, but behaves the same), its __get__ returns a bool, not a method object:

>>> import sys
>>> io.IOBase.closed.__get__(sys.stdout)
False

Here's an example with a preoperty:

>>> import sys
>>> class Foo:
...     @property
...     def blah(self): return True
... 
>>> Foo.blah.__get__(Foo())
True

IMO the best solution would be to return Any from builtins.function.__get__.

[Akuli]

...and I just realized that you already realized this, nevermind :)

[jpy-git]

primer output looks good now 😄 I've added a summary of what's been added to the PR description

[github-actions]

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

urllib3 (https://github.com/urllib3/urllib3)
+ src/urllib3/response.py:773: error: Unused "type: ignore[attr-defined]" comment

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/local.py:283: error: Unused "type: ignore" comment
+ src/werkzeug/routing.py:880: error: Unused "type: ignore" comment
+ src/werkzeug/routing.py:882: error: Unused "type: ignore" comment

[Akuli]

Thanks!