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

incorrect scenario of unidiomatic-typecheck #10161

Open
dgutson opened this issue Dec 30, 2024 · 5 comments · May be fixed by #10170
Open

incorrect scenario of unidiomatic-typecheck #10161

dgutson opened this issue Dec 30, 2024 · 5 comments · May be fixed by #10170
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation

Comments

@dgutson
Copy link

dgutson commented Dec 30, 2024

Bug description

I think that pylint should detect the case when type() is used to compare two types, in which case IMHO isisntance() should not be used:

if type(a) == type(b):
    ...

Configuration

Command used

pylint

Pylint output

C0123: Use isinstance() rather than type() for a typecheck. (unidiomatic-typecheck)

Expected behavior

No error triggered.

Pylint version

pylint 3.0.3
astroid 3.0.2
Python 3.12.3 (main, Nov  6 2024, 18:32:19) [GCC 13.2.0]

OS / Environment

No response

Additional dependencies

@dgutson dgutson added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Dec 30, 2024
@zenlyj
Copy link
Contributor

zenlyj commented Dec 31, 2024

thank you for highlighting this case. currently, no message is raised when the is operator is used to compare two types:

type(a) is type(b)

and imo this seems like an inconsistency as both cases (type() comparisons with == and is) should be semantically equivalent in most practical scenarios.

@zenlyj zenlyj added False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Dec 31, 2024
@jacobtylerwalls
Copy link
Member

I like the idea of encouraging is, so I'm actually surprised we don't have some other message for discouraging == here.

@nedelcu-ioan
Copy link
Contributor

Hi,

I'd like to work on fixing this issue. Could you confirm whether the behavior described in the table below reflects the expected behavior? (Perhaps add a new message suggesting the use of 'is' instead of '==').

Code Example Current Behavior Proposed Behavior
type(a) == type(b): C0123 raised Detect and suggest using is instead of ==.
type(a) is type(b): C0123 raised No message.
type(a) == int: C0123 raised Detect and suggest using is instead of ==.
type(a) is int: C0123 raised No message.
isinstance(a, type(b)): No message No message (no change).

@zenlyj
Copy link
Contributor

zenlyj commented Jan 6, 2025

Thanks for expressing interest, @nedelcu-ioan. A bit of context about this issue, OP is referring to a "strict" type comparison here. In most cases, we want to use the idiomatic way isinstance to perform type checks, which accounts for inheritance. i.e. Given that class C extends class B, isinstance(C(), B) returns True.

But there may be specific cases where we want to perform a "strict" comparison to check if both types are strictly equal, that's when == and is should be used over isinstance.

That's why we should maintain current behavior for all cases except for type(a) == type(b) - which should not raise C0123. @jacobtylerwalls suggests that we should raise a different message (in another check) for type(a) == type(b), because is is safer than == in this scenario, where == behavior can be overridden.

type(a) == type(b)  # do not raise C0123, and perhaps suggest using "is" instead of "==" in another check
type(a) is type(b)  # do not raise C0123 (no change in behavior)
type(a) == int  # raise C0123 (no change in behavior)
type(a) is int  # raise C0123 (no change in behavior)
isinstance(a, type(b))  # do not raise C0123 (no change in behavior)

@jacobtylerwalls
Copy link
Member

Agree with @zenlyj's analysis. Sorry if my tangent was distracting. I'd be interested to explore expanding comparison-with-callable to include classes (constructors), not just functions, but we'd need to look at how disruptive that would be and should probably be discussed in a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants