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

False positive: possibly-used-before-assignment and pytest.skip (and other NoReturn functions) #9674

Closed
The-Compiler opened this issue May 24, 2024 · 10 comments · Fixed by #9714
Assignees
Labels
C: used-before-assignment Issues related to 'used-before-assignment' check False Positive 🦟 A message is emitted but nothing is wrong with the code Lib specific 💅 This affect the code from a particular library Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@The-Compiler
Copy link
Contributor

Bug description

import pytest
import sys

def test_something():
    if sys.platform == "linux":
        cmd = "ls"
    elif sys.platform == "win32":
        cmd = "dir"
    else:
        pytest.skip("only runs on Linux/Windows")

    print(cmd)

Configuration

No response

Command used

pylint -E test_x.py

Pylint output

************* Module test_x
test_x.py:12:10: E0606: Possibly using variable 'cmd' before assignment (possibly-used-before-assignment)

Expected behavior

No error: The pytest module has a couple of functions that never return:

  • pytest.exit(...)
  • pytest.skip(...)
  • pytest.fail(...)
  • pytest.xfail(...)

Related:


Instead of maintaining a list of functions like this, could pylint maybe instead check whether they have a NoReturn return type, if type information is available? That would cover assert_never(), all the pytest functions, possibly sys.exit() and friends (if stdlib typing information is available via typeshed), and lots and lots of other third-party cases like this.

Pylint version

pylint 3.2.1
astroid 3.2.2
Python 3.12.3 (main, Apr 23 2024, 09:16:07) [GCC 13.2.1 20240417]

OS / Environment

Archlinux

Additional dependencies

No response

@The-Compiler The-Compiler added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label May 24, 2024
@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code Lib specific 💅 This affect the code from a particular library Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels May 24, 2024
@jacobtylerwalls
Copy link
Member

Agree we should do this, and also update the docs for --never-returning-functions at the same time.

@jacobtylerwalls jacobtylerwalls added C: used-before-assignment Issues related to 'used-before-assignment' check Good first issue Friendly and approachable by new contributors labels May 24, 2024
The-Compiler added a commit to qutebrowser/qutebrowser that referenced this issue May 24, 2024
- Add a couple new "raise utils.Unreachable" to avoid
  possibly-used-before-assignment issues.
- Simplify an "if" for the same reason
- Remove an unneeded "return"
- Use "NoReturn" to prepare for pylint knowing about it in the future:
  pylint-dev/pylint#9674
- Add some ignores for used-before-assignment false-positives
- Ignore new undefined-variable messages for Qt wrapers
- Ignore a new no-member warning for KeySequence:
  pylint-dev/astroid#2448 (comment)
@Pierre-Sassoulas
Copy link
Member

I was wondering if it should be in pylint-pytest only but it's easy enough to do to be in pylint directly. (@stdedos fyi)

@stdedos
Copy link
Contributor

stdedos commented May 24, 2024

Sure, why not? In pylint context, it does feel like a https://github.com/pylint-dev/pylint-pytest thing

@The-Compiler
Copy link
Contributor Author

I still think this should be more generic than hardcoding pytest methods. I had another scenario in my codebase where I have something like:

def _raise_invalid_node(...) -> NoReturn:
    # do some additional work
    raise Exception(...)

def func():
    if ...:
        var = 1
    else:
        _raise_invalid_node(...)

    print(var)

and I'd expect pylint to know that var is always defined here, because _raise_invalid_node is declared to never return.

@stdedos
Copy link
Contributor

stdedos commented May 25, 2024

I still think this should be more generic than hardcoding pytest methods.

I don't object the correct-ness of your statement. What concerns me is how much should pylint enter the fields of mypy 😅

I don't know how strong pylint is / invested in data flow analysis

@Pierre-Sassoulas
Copy link
Member

What concerns me is how much should pylint enter the fields of mypy

There's a lot of work to start taking typing into account in pylint (generically at least). Hypothetically, pylint would infer first, and use typing if inference fail (#4813)

Making inference handle a calls that always exit well is something to consider for sure. I don't know if we have an open issue for that. But the reasonable short term solution is to pre-populate --never-returning-functions with the most common/used one as Jacob said.

@jacobtylerwalls jacobtylerwalls removed the Good first issue Friendly and approachable by new contributors label May 25, 2024
@stdedos
Copy link
Contributor

stdedos commented May 25, 2024

But the reasonable short term solution is to pre-populate --never-returning-functions with the most common/used one as Jacob said.

What if pylint does the short-term solution, and then pylint-pytest builds on top of that, and auto-inject more --never-returning-functions scoped at the test/ directory?

@Pierre-Sassoulas
Copy link
Member

There are great test cases for this in #9692

@jacobtylerwalls
Copy link
Member

What concerns me is how much should pylint enter the fields of mypy 😅

Yes, there is not enough volunteer labor nor would it be a wise use of it to emulate mypy in pylint. Part of pylint's value prop is "not trusting your typing", making it especially useful for legacy/untyped codebases. (see "realist pylint user" quoted in the repo readme)

However... it strikes me that that's still consistent with observing declarations like NoReturn in order to avoid asking users to go through the more friction-ful path of scouring their code bases and dependencies for NoReturn and syncing it up with their config file. Observing a well-known return annotation is different than doing full-fledged type analysis.


This diff makes the false positive go away (someone should kick the tires and make sure it's right):

diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py
index a3e649651..a7938528a 100644
--- a/pylint/checkers/utils.py
+++ b/pylint/checkers/utils.py
@@ -2165,6 +2165,13 @@ def is_terminating_func(node: nodes.Call) -> bool:
                 and inferred.qname() in TERMINATING_FUNCS_QNAMES
             ):
                 return True
+            if (
+                isinstance(inferred, nodes.FunctionDef)
+                and isinstance(inferred.returns, nodes.Name)
+                and (inferred_func := inferred.returns.inferred()[0])
+                and (inferred_func.qname() == 'typing.NoReturn')
+            ):
+                return True
     except (StopIteration, astroid.InferenceError):
         pass

@stdedos would you potentially be interested in being the person to kick the tires? Make sure that's right, and get a test and changelog added?

@stdedos
Copy link
Contributor

stdedos commented Jun 7, 2024

@stdedos would you potentially be interested in being the person to kick the tires? Make sure that's right, and get a test and changelog added?

I would love to be that person. ... However, I have "lack-of-knowledge" of my own that I need to kick sometime (pylint-dev/pylint-pytest#68), and also "lack-of-time" these days 😕

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: used-before-assignment Issues related to 'used-before-assignment' check False Positive 🦟 A message is emitted but nothing is wrong with the code Lib specific 💅 This affect the code from a particular library Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants