Skip to content

bpo-46571: improve typing.no_type_check to skip foreign objects#31042

Merged
JelleZijlstra merged 3 commits into
python:mainfrom
sobolevn:issue-46571
Feb 19, 2022
Merged

bpo-46571: improve typing.no_type_check to skip foreign objects#31042
JelleZijlstra merged 3 commits into
python:mainfrom
sobolevn:issue-46571

Conversation

@sobolevn

@sobolevn sobolevn commented Feb 1, 2022

Copy link
Copy Markdown
Member

There are several changes:

  1. We now don't explicitly check for any base / sub types, because new name check covers it
  2. I've also checked that no_type_check do not modify foreign functions. It was the same as with types
  3. I've also covered except TypeError in no_type_check with a simple test case, it was not covered at all
  4. I also felt like adding lambda test is a good idea: because lambda is a bit of both in class bodies: a function and an assignment

Are there any other cases we want to cover?

https://bugs.python.org/issue46571

@AlexWaygood

AlexWaygood commented Feb 1, 2022

Copy link
Copy Markdown
Member

Here's a slightly evil corner case that this patch doesn't account for:

If I have a module typing_test.py, like so:

# typing_test.py
class A:
    class AA:
        foo: int

Then, observe the following behaviour (with your patch applied):

>>> from typing import get_type_hints, no_type_check
>>> import typing_test
>>> get_type_hints(typing_test.A.AA)
{'foo': <class 'int'>}
>>> @no_type_check
... class A:
...     AA = typing_test.A.AA
... 
...     
>>> get_type_hints(typing_test.A.AA)
{}

We might be able to get around this by looking at the __module__ attribute.

Comment thread Lib/test/test_typing.py
Comment thread Lib/test/test_typing.py
Comment thread Lib/typing.py Outdated
@sobolevn

sobolevn commented Feb 2, 2022

Copy link
Copy Markdown
Member Author

@AlexWaygood good one! Very evil!

I've addressed this.

@JelleZijlstra I agree that we should also fix classmethod / staticmethod problem. Because we declare to support all methods, not just instance methods.

The problem is that types.MethodType does not support attribute assignment:

>>> class Some:
...     @staticmethod
...     def st(x: int) -> int: ...
...     @classmethod
...     def cl(cls, y: int) -> int: ...

>>> Some.st.__no_type_check__ = True

>>> Some.cl.__no_type_check__ = True
AttributeError: 'method' object has no attribute '__no_type_check__'

Ideas?

Moreover, do we need some special @property support?

@AlexWaygood

Copy link
Copy Markdown
Member

@AlexWaygood good one! Very evil!

I've addressed this.

Doesn't look like you've pushed anything -- did you mean to? :)

@sobolevn

sobolevn commented Feb 2, 2022

Copy link
Copy Markdown
Member Author

Doesn't look like you've pushed anything -- did you mean to? :)

Not yet, I am still fighting classmethod 🙂

@sobolevn

sobolevn commented Feb 2, 2022

Copy link
Copy Markdown
Member Author

Probably

if isinstance(obj, types.MethodType):
                obj.__func__.__no_type_check__ = True

is the way to go 🤔

Will write some more tests to be sure.

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good.

Do you think this should be backported? I'm leaning no as it's a behavior change and nobody directly complained about the old behavior (looks like you found it while reviewing typing test coverage).

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed, a backport doesn't seem asked for.

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.

6 participants