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

[possibly-used-before-assignment] Avoid FP for typing.NoReturn #9714

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jacobtylerwalls
Copy link
Member

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

Closes #9674 for the general case, but not necessarily for pytest.skip due to the following:

from astroid import extract_node
n = extract_node("""
... import pytest
... pytest.skip('foo')""")
>>> n.func.inferred()
[Uninferable]

This comment has been minimized.

This comment has been minimized.

in (
*TYPING_NORETURN,
# In Python 3.7 - 3.8, NoReturn is alias of '_SpecialForm'
"typing._SpecialForm",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll be dropping support for 3.8 in the next minor release, so this inelegance doesn't bother me ATM.

Copy link

codecov bot commented Jun 8, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.85%. Comparing base (3f1f7b8) to head (6481da5).
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #9714   +/-   ##
=======================================
  Coverage   95.84%   95.85%           
=======================================
  Files         174      174           
  Lines       18865    18870    +5     
=======================================
+ Hits        18082    18087    +5     
  Misses        783      783           
Files Coverage Ξ”
pylint/checkers/utils.py 96.16% <100.00%> (+0.01%) ⬆️

Copy link
Contributor

github-actions bot commented Jun 8, 2024

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on home-assistant:
The following messages are no longer emitted:

  1. possibly-used-before-assignment:
    Possibly using variable 'max_bind_vars' before assignment
    https://github.com/home-assistant/core/blob/522a1e9d56ce8571b730c9ddf2108d58969b5044/homeassistant/components/recorder/util.py#L601

Effect on black:
The following messages are now emitted:

  1. unreachable:
    Unreachable code
    https://github.com/psf/black/blob/b677a643c5322b6af2dbca1f8f7a4f8c98e732aa/src/blib2to3/pgen2/pgen.py#L346

Effect on pytest:
The following messages are no longer emitted:

  1. possibly-used-before-assignment:
    Possibly using variable 'reprentry' before assignment
    https://github.com/pytest-dev/pytest/blob/f85289ba872a26eb41836cdaefe97a93b311bf29/src/_pytest/reports.py#L573

This comment was generated for commit 6481da5

@Pierre-Sassoulas
Copy link
Member

I'm on the fence about this one. On the one hand, it's going to prevent false positives. And we have false positives for code that is too dynamic for astroid to understand. But on the other hand, one of the value proposition of pylint is not trusting the typing. If we start trusting the typing then we're going to raise the same class of error that type checker already raise, and they do that better than us and faster than us. Also, if we don't do it generically we're going to have some message that trust the typing and some that don't and how are we drawing the line when someone ask for typing use making the point that "possibly-user-before-assignment" does it for "NoReturn" already ? Then again I don't see massive improvements happening in astroid's inference any time soon.

@jacobtylerwalls
Copy link
Member Author

I hear that. I think NoReturn is very special -- it's basically assert_never, which is basically a more expressive pylint disable.

Allowing someone to use a stdlib feature that works well with type checkers instead of a pylint disable is a pretty isolated case compared to always trusting typing.

@Pierre-Sassoulas
Copy link
Member

I like the idea if seeing some type hint as implicit disables. A passing thought : Taking other tool's noqas into account is somewhat similar and would also make pylint more useful.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the fact that the genericity if the code permits to also makes 'unreachable' better. It's done properly because it's easy to see that kind of helper reuse as a dirty source of unintented side effects.

@Pierre-Sassoulas
Copy link
Member

@DanielNoord do you want to chime in :) ?

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it! However, I would like to document that this is a special case and not something we do for other checks. Then we have something we can point to when more requests start coming in.

@jacobtylerwalls
Copy link
Member Author

Other checks or other type annotation concepts? I've made change in the general util, I think we probably should simplify the code for other checks by using the common utils more, so I don't know there's much more to document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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