ruff icon indicating copy to clipboard operation
ruff copied to clipboard

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

Open DetachHead opened this issue 2 years ago • 8 comments

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

DetachHead avatar Aug 10 '23 01:08 DetachHead

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 avatar Aug 10 '23 02:08 charliermarsh

(I added the missing builtin types at least.)

charliermarsh avatar Aug 10 '23 02:08 charliermarsh

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

bdowning avatar Jan 08 '24 22:01 bdowning

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

charliermarsh avatar Jan 08 '24 22:01 charliermarsh

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

bdowning avatar Jan 09 '24 00:01 bdowning

@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'
$

bdowning avatar Feb 02 '24 01:02 bdowning

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 avatar Feb 02 '24 01:02 zanieb

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

bdowning avatar Apr 06 '24 00:04 bdowning

Hi. I'm hitting this warning.

Now and then I want to do type(a) is B or type(a) is type(c) to do strict type checking, not subclassing, namely in unit tests.

The type(...) is ... check with the is operator should be allowed and not trigger a warning, or in an alternative case, produce a separate warning from E721 which we can choose to ignore. Cheers.

joaoe avatar Jun 07 '24 12:06 joaoe

@joaoe Can you try your example with --preview mode? This is resolved in preview mode, we just need to move it into stable in the next minor release.

dhruvmanila avatar Jun 12 '24 16:06 dhruvmanila

Ok, there's a PR up which will resolve this: https://github.com/astral-sh/ruff/pull/11220

dhruvmanila avatar Jun 12 '24 16:06 dhruvmanila

The preview logic has been moved to stable and will be released as part of v0.5.

Resolved by #11220

dhruvmanila avatar Jun 25 '24 16:06 dhruvmanila