used-before-assignment false negative after conditionals

[eddieViscosity]

Running pylint on this simple example does not detect that x can be used before assignment. This is a very common programming mistake and should be caught.

def a_function(d):
    if d:
        x=1
    return x

Current behavior - Fails to detect x is used before assignment

Expected behavior: detect variable use inside logic branches.

pylint --version output: pylint-2 1.7.4

[kylebebak]

Just here to second this.

[nickdrozd]

Ran into this at work today!

[DanielNoord]

Do we want to create a new checker called possibly-used-before-assignment checker for this? Similar to how there is possibly-unused-variable. If we can't infer d we will never be able to emit used-before-assignment without any doubt.

[Pierre-Sassoulas]

possibly-unused-before-assignment

Did you mean "possibly-used-before-assignment" ?

[DanielNoord]

Yeah 😅

[Pierre-Sassoulas]

I think it makes sense. Adding a good control flow to astroid would be nice but is also difficult.

[nickdrozd]

This kind of problem would be avoided entirely if the variable were set with a ternary expression!

[jacobtylerwalls]

Duplicate of #267 but some good discussion here, so maybe we can choose one issue to keep open.

We can possibly explore handling this after the precedent in #5402 of storing consumed_uncertain (name to be finalized before merge).

[Pierre-Sassoulas]

There's some more example in #267 or #1807 for functional tests.

[jacobtylerwalls]

The reason this is not a parent duplicate for #4913 is that we could issue possibly-used-before-assignment here for the ambiguity. Whereas in #4913 we could be bolder, detect "always False", and issue the garden variety used-before-assignment (with a CONTROL_FLOW confidence level).

[Pierre-Sassoulas]

@jacobtylerwalls I think you wanted to post this message in another issue because this one is #1727 :D

[jacobtylerwalls]

Thanks, I fixed the numbers in my comment. :-)

[alonme]

@jacobtylerwalls @Pierre-Sassoulas - i would like to help implement this,
Any pointers?

[jacobtylerwalls]

Very cool!

I'd familiarize yourself with this method and how it is used, focusing on the three return statements at the end:

pylint/pylint/checkers/variables.py

Lines 687 to 690 in 7eaac57

	def _inferred_to_define_name_raise_or_return(name: str, node: nodes.NodeNG) -> bool:
	"""Return True if there is a path under this `if_node`
	that is inferred to define `name`, raise, or return.
	"""

Essentially, thanks to hard work by @zenlyj, we have a pretty clean interface for emitting used-before-assignment or not based on whether or not we care about a certain if/else pattern. We've been ratcheting up the messages slowly. First we just started emitting used-before-assignment only when we can tell that all possible values of the test are False (and therefore the if is always skipped).

So, this issue is about being even bolder, and emitting something (either used-before-assignment or a new message possibly-used-before-assignment) regardless of the inferred possible values of the test (see the all_inferred variable).

It might be nice to get a draft PR going without defining a separate message just to see how noisy it would be. Then we can evaluate whether to create a second message for it. Does that make sense?

[jacobtylerwalls]

Do we want to create a new checker called possibly-used-before-assignment checker for this? Similar to how there is possibly-unused-variable. If we can't infer d we will never be able to emit used-before-assignment without any doubt.

Ah -- I'm thinking we shouldn't have a second message type, as it would blur the boundary between message type and confidence. We should just be really specific about the confidence, including using INFERENCE_FAILURE for this case. And then mention in the release notes that if you find this noisy, you need to experiment with adjusting confidence levels.

[Pierre-Sassoulas]

Thank you for your interest in this issue :) Jacob worked a lot on it and is definitely the code owner on this message, I agree with what was said do not hesitate to open a draft PR and use primers in our CI as a dev tool.