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

type-comparison (E721) only works on some builtin classes and nothing else #6465

Open
DetachHead opened this issue Aug 10, 2023 · 8 comments
Assignees
Labels
needs-decision Awaiting a decision from a maintainer

Comments

@DetachHead
Copy link

def asdf(value: object):
    type(value) is str  # error
    type(value) is int  # error
    type(value) is list  # no error
    type(value) is dict  # no error

    class Foo:
        ...

    type(value) is Foo  # no error

pylint's unidiomatic-typecheck detects all of these cases

@charliermarsh
Copy link
Member

I'm honestly tempted to revert the changes to this rule and just go back to pycodestyle parity, because I don't really understand the contexts in which pycodestyle is meant to be enforcing it, and it doesn't seem worth it to me to keep spending time refining it. (This rule in pycodestyle is not the same as unidiomatic-typecheck, and we're now deviating from both subtly.)

For example, pycodestyle allows type(a1) is type(b1), type(a1) is int, etc., but disallows type(a) is type(1). I don't really understand it.

Separately, comparing type(value) is int is actually reasonable to me in some cases if you want to do a direct comparison, not a subclass comparison. I assume that's why pycodestyle is more limited in its rule.

@charliermarsh charliermarsh added the needs-decision Awaiting a decision from a maintainer label Aug 10, 2023
@charliermarsh
Copy link
Member

(I added the missing builtin types at least.)

@bdowning
Copy link

bdowning commented Jan 8, 2024

IMHO ruff needs to allow stuff like type(foo) is int. flake8 allowed this, it just disallowed type(foo) == int and recommended replacing == with is and not replacing with isinstance, which obviously has completely different behavior. I just had code break from "fixing" lint violations with isinstance due to a behavior change (because bool is a subclass of int for some insane reason).

@charliermarsh
Copy link
Member

@bdowning -- I believe we already allowed that if you run with --preview: https://play.ruff.rs/90c3ddba-0c1f-450d-96bc-df09ee8e6adb. (This will become stable behavior when we ship v0.2.0.)

@bdowning
Copy link

bdowning commented Jan 9, 2024

Thanks. BTW, the playground seems more than a little busted on Firefox right now:
image

@bdowning
Copy link

bdowning commented Feb 2, 2024

@charliermarsh So I updated to 0.2.0 and I'm still only seeing stuff like type(foo) is int being allowed with --preview. Did this not make the cut?

$ poetry run ruff --version
ruff 0.2.0
$ poetry run ruff xxx/
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'isort' -> 'lint.isort'
xxx/yyy.py:193:8: E721 Do not compare types, use `isinstance()`
xxx/yyy.py:195:10: E721 Do not compare types, use `isinstance()`
xxx/yyy.py:199:10: E721 Do not compare types, use `isinstance()`
Found 3 errors.
$ poetry run ruff --preview xxx/
warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'isort' -> 'lint.isort'
$

@zanieb
Copy link
Member

zanieb commented Feb 2, 2024

It's possible we missed this in the 0.2.0 stabilizations. For what it's worth, I think this is a bug and we should fix it immediately.

@zanieb zanieb self-assigned this Feb 2, 2024
@bdowning
Copy link

bdowning commented Apr 6, 2024

@zanieb Any word on this? Just checked with 0.3.5 and it looks like this is still behind the preview barrier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer
Projects
None yet
Development

No branches or pull requests

4 participants