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

False positive for unidiomatic-typecheck #3337

Closed
GoldsteinE opened this issue Jan 10, 2020 · 2 comments · Fixed by #3522
Closed

False positive for unidiomatic-typecheck #3337

GoldsteinE opened this issue Jan 10, 2020 · 2 comments · Fixed by #3522
Labels

Comments

@GoldsteinE
Copy link

Steps to reproduce

Consider following code:

is_optional = type(None) in typing.get_args(Optional[Union[int, Literal['a']]])

Current behavior

pylint tells that type(None) in ... is "unidiomatic typecheck" and should be replaced with isinstance()

Expected behavior

pylint shouldn't show a warning, because while it's not a type check and can't be replaced with isinstance().

pylint --version output

pylint 2.4.4
astroid 2.3.3
Python 3.8.1 (default, Dec 21 2019, 20:57:38)
[GCC 9.2.0]
@doublethefish
Copy link
Contributor

+1

Steps to reproduce

""" Demonstrate false-positive unidiomatic-typecheck """


class AClass:  # pylint: disable=too-few-public-methods
    """ Base class, all instantiations return True for isinstance(<obj>, AClass) """


class BClass(AClass):  # pylint: disable=too-few-public-methods
    """ Distinct type to AClass, but an inherited type of it """


class CClass(BClass):  # pylint: disable=too-few-public-methods
    """ Second, further nested type, still an inherited type of AClass """


EXACT_MATCH_TYPES = (BClass, CClass)


# All of the following asserts PASS at run-time
assert type(AClass()) not in EXACT_MATCH_TYPES  # NOTE the NOT statement, triggers check
assert type(BClass()) in EXACT_MATCH_TYPES  # triggers check
assert type(CClass()) in EXACT_MATCH_TYPES  # triggers check
assert isinstance(AClass(), AClass)
assert isinstance(BClass(), AClass)
assert isinstance(CClass(), AClass)

Current behavior

pylint seemingly assumes the two statements are equivalent in all cases:

  • isinstance(obj, (iterable, of, types) )
  • type(obj) in (iterable, of types)
    However isinstance() examines the inheritance tree of all types whereas the other form does exact matching.

The upshot is that following the suggestion and changing the code to the isinstance() variant can change the intended behavior of the code, is which case it is not desired. We do not want our inexperienced developers blinding implementing this change.

The docs do capture this and state:
Using type() instead of isinstance() for a typecheck. The idiomatic way to perform an explicit typecheck in Python is to use isinstance(x, Y) rather than type(x) == Y, type(x) is Y. Though there are unusual situations where these give different results.

However type(<obj>) in <cont> is not an "unusual situation", it is an explicit check for a known state, see item2 in import this.

Expected behavior

Because it is very much worth considering isinstance() instead (esp. for our inexperienced developers) but the issue is not "unidomatic" nor "unusual" in all cases, the message should report consider-idiomatic-typecheck instead, which is both more correct and still reports that there is a possible better way of doing the check.

I see no issue is disabling "consider-" type checks, as and when needed.

pylint --version output

pylint 2.4.4
astroid 2.3.3
Python 3.7.6 (default, Dec 30 2019, 19:38:26)
[Clang 11.0.0 (clang-1100.0.33.16)]

PCManticore added a commit that referenced this issue Apr 27, 2020
…n`` operators

The original use case for this check was to catch old style type checking idioms
such as `type(x) is ...`, but it should not have been extended to handle `in` operators
as well.

Close #3337
@PCManticore
Copy link
Contributor

Thanks for the report! I agree with @doublethefish as this check should not have handled in and not in respectively. We can definitely deprecate the old naming and put a new one in place to indicate it's more of a suggestion rather than a hard rule.

PCManticore added a commit that referenced this issue Apr 27, 2020
…n`` operators

The original use case for this check was to catch old style type checking idioms
such as `type(x) is ...`, but it should not have been extended to handle `in` operators
as well.

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

Successfully merging a pull request may close this issue.

3 participants