bpo-46571: improve typing.no_type_check to skip foreign objects by sobolevn · Pull Request #31042 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation10 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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.

@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 good one! Very evil!

I've addressed this.

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

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

Not yet, I am still fighting classmethod 🙂

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.

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).

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.