Skip to content

unidiomatic-typecheck not flagged for type(x) is type(y) #10365

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

Open
jacobtylerwalls opened this issue May 4, 2025 · 6 comments · May be fixed by #10372
Open

unidiomatic-typecheck not flagged for type(x) is type(y) #10365

jacobtylerwalls opened this issue May 4, 2025 · 6 comments · May be fixed by #10372
Labels
Enhancement ✨ Improvement to a component False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection
Milestone

Comments

@jacobtylerwalls
Copy link
Member

The spirit of unidiomatic-typecheck is to flag unidiomatic typechecks.

This is unidiomatic:

type(x) is type(y)

It should be one of:

isinstance(x, type(y))
issubclass(type(x), type(y))
@jacobtylerwalls jacobtylerwalls added Enhancement ✨ Improvement to a component False Negative 🦋 No message is emitted but something is wrong with the code labels May 4, 2025
@Pierre-Sassoulas Pierre-Sassoulas added this to the 4.0.0 milestone May 6, 2025
@DanielNoord
Copy link
Collaborator

None of those replacements deal with the case where you actually want to check the exact type, right? They would all pass if the type of x is a subclass of the type of y.

@jacobtylerwalls
Copy link
Member Author

Exactly. The spirit of the message as I understand it is to discourage that kind of exact check.

@DanielNoord
Copy link
Collaborator

I have some use cases in my codebases where I need this, but would personally be fine with having to disable the check there. This is likely a case where we need input from the primer to determine how good/bad it would be?

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented May 6, 2025

The linked MR with a fix already exists and does not have a very good primer imo, see #10372 (comment). I think this is an opinionated check and my intuition is that it's been toned down in the past. I'm currently git blaming the code to see the reasoning for the way the code is right now and have a more informed opinion. (made the comment to point out that the fix/primer already exists)/

Edit: ff4e14 from #299, the discussion from bitbucket is lost https://bitbucket.org/logilab/pylint/issues/299

@Pierre-Sassoulas
Copy link
Member

Following up, there's not a lot of argument from the git blame. But I personally think the initial intention was to warn when isinstance should clearly be preferred (i.e. for is type([])). We should clarify the reasoning in the documentation, and either add a new separate message or an option if we want to warn on type(x) is type(y) or do nothing.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs decision 🔒 Needs a decision before implemention or rejection label May 6, 2025
@jacobtylerwalls
Copy link
Member Author

We should add an exception for uses inside __eq__(self, other). A lot of the primer cases are coming from traffic like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component False Negative 🦋 No message is emitted but something is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants