pylint icon indicating copy to clipboard operation
pylint copied to clipboard

false negative: used-before-assignment w/ TYPE_CHECKING guard

Open Redoubts opened this issue 2 years ago • 7 comments

Bug description

Consider the following code:

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    import datetime

class Z:
    def something(self) -> None:
        z = datetime.datetime.now() # not imported at runtime

Pylint can and does flag the z = ... line as used-before-assignment. However, if we use datetime somewhere as a type annotation, say in

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    import datetime


def x() -> datetime.datetime: # this is good
    return datetime.datetime.now() # this should be bad


class Z:
    def something(self) -> None:
        z = datetime.datetime.now() # this should still be bad

pylint returns "10/10, would lint again". pylint should flag the used-before-assignment errors here, since this code will blow up at runtime when called.

Command used

pylint --disable all --enable E <file>

Pylint output

* without type annotation
% pylint --disable all --enable E xyz.py
************* Module xyz
xyz.py:15:12: E0601: Using variable 'datetime' before assignment (used-before-assignment)

------------------------------------------------------------------
Your code has been rated at 2.86/10 (previous run: 0.00/10, +2.86)

* with type annotation
% pylint --disable all --enable E xyz.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 0.00/10, +10.00)

Expected behavior

consistent used-before-assignment lints.

Pylint version

% pylint --version                      
pylint 2.17.3
astroid 2.15.4
Python 3.10.12 (main, Jun 12 2023, 08:08:55) [GCC 11.2.0]

OS / Environment

Debian

Redoubts avatar Jul 27 '23 20:07 Redoubts

Kinda feels like the obverse of #8111

Redoubts avatar Jul 27 '23 20:07 Redoubts

This was partially fixed on the main brach by https://github.com/pylint-dev/pylint/issues/8198 (I think).

if TYPE_CHECKING:
    import datetime

def f():
    return datetime.datetime.now()  # flagged -- good

def g() -> datetime.datetime:
    return datetime.datetime.now()  # not flagged -- bad

Perhaps it's a strange case, but there is also a false negative for classes defined in the TYPE_CHECKING block:

if TYPE_CHECKING:
    class X:
        pass

def f():
    return X()  # not flagged -- bad

def g() -> X:
    return X()  # not flagged -- bad

nickdrozd avatar Jul 27 '23 23:07 nickdrozd

Yeah I think that’s interesting that only the first issue is flagged. In my worst case, only the first datetime usage is flagged as well.

Redoubts avatar Jul 28 '23 00:07 Redoubts

I observe a quite similar issue which was introduced in v3.0.0a7, v3.0.0a6 works fine.

Steps to reproduce:

@dataclass
class Foo:
    my_list: list[Bar]

which fails with E0601: Using variable 'list' before assignment (used-before-assignment). It can be fixed by using the typing.List over list for the type hint, which it not what's recommended by pyupgrade.

hofbi avatar Sep 05 '23 07:09 hofbi

@hofbi Does that file include from __future__ import annotations?

DanielNoord avatar Sep 05 '23 07:09 DanielNoord

@DanielNoord No, no __future__ imports

hofbi avatar Sep 05 '23 07:09 hofbi

#9587 was a duplicate for g() in https://github.com/pylint-dev/pylint/issues/8893#issuecomment-1654728749.

jacobtylerwalls avatar May 04 '24 15:05 jacobtylerwalls

if TYPE_CHECKING behaves the same way as if False so my impression is just that this was never implemented because there was never a use case for it until the TYPE_CHECKING constant.

Shooting from the hip, I bet in get_next_to_consume() we could filter out nodes where in_type_checking_block(other_node) is true and the current node meets the conditions for where runtime imports are needed.

jacobtylerwalls avatar Sep 30 '24 14:09 jacobtylerwalls

However, #8431 added a different approach for this, so it's worth examining whether we should tweak it or drop it. It probably shouldn't be assuming that we can have one approach to type-checking imports per scope.

jacobtylerwalls avatar Sep 30 '24 14:09 jacobtylerwalls