possibly-used-before-assignment doesn't understand negation of conditionals

[chrisburr]

Bug description

# pylint: disable=missing-module-docstring,invalid-name
a = bool(input("Enter something or nothing: "))

if a:
    b = "1234"

# pylint doesn't understand this one
if not a:
    pass
else:
    print(b)

# pylint does understand this one
if a:
    print(b)
else:
    pass

Configuration

No response

Command used

pylint test.py

Pylint output

************* Module test
test.py:11:10: E0606: Possibly using variable 'b' before assignment (possibly-used-before-assignment)

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

Expected behavior

Pylint should understand both if a: ... and if not a: pass; else: ...

Pylint version

pylint 3.2.0
astroid 3.2.0
Python 3.11.7 | packaged by conda-forge | (main, Dec 23 2023, 14:43:09) [GCC 12.3.0]

OS / Environment

No response

Additional dependencies

No response

[jacobtylerwalls]

Thanks @chrisburr. This is actually working as designed. The last example where no message is emitted is because of the limited exception we added in #8581 for identical guards. We should update the docs for the message, because the discussion there of an example guarded() guarding the variable when used doesn't account for this limited exception.

The rationale being that if a is an instance variable, module constant, etc., any other part of your program could have changed its value before it's accessed in the second part of your minimal example. (And thus, for a more meaningful example, the message provides more meaningful value.)

You may want to disable this message if you rely on this pattern. Pylint itself disabled this message for the time being.

[chrisburr]

Thanks for the prompt response (and more generally for the work on this really nice new lint rule)!

I can definitely see why relying on a globals/callables/properties/inner functions with nonlocal would be a problem for static analysis and deemed out-of-scope for this rule.

The reason I had for having the inverted condition was that my code is actually of the form:

if xxx:
    yyy = ...

if not xxx:
    ...
elif zzz:
    print(yyy)

In my case the code is contained inside of a function so there is no way xxx could be modified without doing unreasonable things. So in general I still think the rule shouldn't trigger for:

def my_func(a: bool):
    if a:
        b = "1234"

    if not a:
        pass
    else:
        print(b)

[jacobtylerwalls]

Right, but what about the following. pylint is completely ignorant of what may or may not have happened between the top and bottom parts of your function. We just don't have "line-by-line" control flow knowledge of the values of variables.

def my_func(a: bool):
    if a:
        b = "1234"

    a = not a

    if not a:
        pass
    else:
        print(b)

[chrisburr]

Ah! I didn’t realise pylint didn’t have control flow knowledge. That probably kills this suggestion, or at least turns it into a feature suggestion which sounds like it would be non-trivial.

I think it would be reasonable to say that if you mutate the variable in then conditional between invocations that it’s an expected that pylint looses track and gives a false result. Though as mentioned before, if that’s not possible then I totally agree with the current behaviour.

[nickdrozd]

If variable b is only defined in certain branches, why not only use it in those branches? For example:

a = bool(input("Enter something or nothing: "))

if a:
    b = "1234"
    print(b)
else:
    pass

[jacobtylerwalls]

Though as mentioned before, if that’s not possible then I totally agree with the current behaviour.

Yeah, that's kind of the rub. It may or may not be feasible if a complete redesign of the variables checker is ever attempted, but in the meantime, we were thinking it was an okay compromise to ship with the current behavior so long as we weasel worded it with "possibly".

That does make this error kind of a cross between an error and a code-style warning. I edited your example which was safe to make it unsafe, and all I did was insert one line in the middle of a function. That seems likely to happen in real-life, and authors can defend against that possibility by doing what @nickdrozd suggests instead, which is more of a future-proofing code-style convention than an error if we choose to look at it that way.

[chrisburr]

Yeah I can see that. For the safety of this pattern I think it depends on what the variable actually is. In the cases I've seen it's a flag that is passed in externally so there is no reasonable change of someone mutating it though I'm sure I could find examples to the contrary if I looked (like if it was a flag that is set in a loop).

why not only use it in those branches?

As you've asked, the real case that prompted this issue was the use of with_pfns in this function:

https://gitlab.cern.ch/lhcb-dirac/LHCbDIRAC/-/blob/4ddc1b8996b4e41e5aef9e03d794d54e3da11552/src/LHCbDIRAC/ProductionManagementSystem/Service/TornadoAnalysisProductionsHandler.py#L187-216

[nickdrozd]

As you've asked, the real case that prompted this issue was the use of with_pfns in this function:

https://gitlab.cern.ch/lhcb-dirac/LHCbDIRAC/-/blob/4ddc1b8996b4e41e5aef9e03d794d54e3da11552/src/LHCbDIRAC/ProductionManagementSystem/Service/TornadoAnalysisProductionsHandler.py#L187-216

That's a nice one! There is a similar function in the Pylint codebase that gets flagged with the same warning: https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/similar.py#L579

I think it's safe to say that both functions are pretty nasty. A bunch of different flags floating around, multi-way branches, nested loops -- these are generally things to be avoided. Of course they have to be done from time to time. But probably it isn't a coincidence that these "false positives" tend to crop up as comorbidities.

If it happens to work out okay at runtime, great. And if that's the case, silencing the warning locally seems like a fine approach. Another alternative would be to define the variable unconditionally with some kind of null value:

replicas = _getReplicas(list(chain(*lfns.values()))) if with_pfns else {}  # or [], None, whatever

In any case, I don't think Pylint should attempt to accommodate this. The variable really is possibly undefined, in the sense that there are runtime circumstances that could lead to an undefined variable error.

How would this pattern be handled in a language like Rust?

[nmlorg]

Given:

# pylint: disable=missing-module-docstring,missing-function-docstring

def make_bot(token):
    assert token
    return token.upper()

def function(token):
⋮

function('pass')
function('')

$ cat a.py
def function(token):
    bot = make_bot(token)

    print(bot)

$ python a.py
PASS
Traceback (most recent call last):
  File "a.py", line 13, in <module>
    function('')
  File "a.py", line 8, in function
    bot = make_bot(token)
          ^^^^^^^^^^^^^^^
  File "a.py", line 4, in make_bot
    assert token
AssertionError

$ pylint a.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10

👍
 

$ cat b.py
def function(token):
    if token:
        bot = make_bot(token)

    print(bot)

$ python b.py
PASS
Traceback (most recent call last):
  File "b.py", line 14, in <module>
    function('')
  File "b.py", line 11, in function
    print(bot)
          ^^^
UnboundLocalError: cannot access local variable 'bot' where it is not associated with a value

$ pylint b.py
************* Module b
b.py:11:10: E0606: Possibly using variable 'bot' before assignment (possibly-used-before-assignment)

-------------------------------------------------------------------
Your code has been rated at 4.44/10

👍
 

$ cat c.py
def function(token):
    if token:
        bot = make_bot(token)
    else:
        return

    print(bot)

$ python c.py
PASS

$ pylint c.py

------------------------------------
Your code has been rated at 10.00/10

👍 — however:

$ cat d.py
def function(token):
    if token:
        bot = make_bot(token)

    if not token:
        return

    print(bot)

$ python d.py
PASS

$ pylint d.py
************* Module d
d.py:14:10: E0606: Possibly using variable 'bot' before assignment (possibly-used-before-assignment)

-----------------------------------
Your code has been rated at 5.45/10

👎 (as above)

And:

$ cat e.py
def function(token):
    if not token:
        return

    bot = make_bot(token)

    print(bot)

$ python e.py
PASS

$ pylint e.py

-------------------------------------------------------------------
Your code has been rated at 10.00/10

👍 — however:

$ cat f.py
def function(token):
    if not token:
        return

    if token:
        bot = make_bot(token)

    print(bot)

$ python f.py
PASS

$ pylint f.py
************* Module f
f.py:14:10: E0606: Possibly using variable 'bot' before assignment (possibly-used-before-assignment)

-------------------------------------------------------------------
Your code has been rated at 5.45/10

👎

I'm doing something like:

if token:
    try:
        bot = make_bot(token)
    except AssertionError:
        token = None

if not token:
    print('Instructions for generating a [valid] token.')
    return

bot.dereference()

https://github.com/nmlorg/metabot/blob/1233df439b81eabb38daa489e31f5bf16a414c84/metabot/modules/newbot.py#L26-L47

and it is possible for me to mess the logic up at some point so that bot wouldn't be defined, so ideally I wouldn't just disable the check altogether.

[jacobtylerwalls]

Thanks for the use cases here. Going to close as a duplicate of #9662, which has more discussion about the direction/effort involved here.