-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix used-before-assignment for PEP 695 type aliases + parameters #10488
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
Open
cdce8p
wants to merge
3
commits into
pylint-dev:main
Choose a base branch
from
cdce8p:fix-used-before-assignment
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+58
−20
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Fix used-before-assignment for PEP 695 type aliases and parameters. | ||
|
||
Closes #9815 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,23 @@ | ||
"""used-before-assignment re: python 3.12 generic typing syntax (PEP 695)""" | ||
"""Tests for used-before-assignment with Python 3.12 generic typing syntax (PEP 695)""" | ||
# pylint: disable = invalid-name,missing-docstring,too-few-public-methods,unused-argument | ||
|
||
from typing import TYPE_CHECKING, Callable | ||
|
||
from typing import Callable | ||
type Point[T] = tuple[T, ...] | ||
type Alias[*Ts] = tuple[*Ts] | ||
type Alias[**P] = Callable[P] | ||
|
||
# pylint: disable = invalid-name, missing-class-docstring, too-few-public-methods | ||
type Alias2[**P] = Callable[P, None] | ||
|
||
# https://github.com/pylint-dev/pylint/issues/9815 | ||
type IntOrX = int | X # [used-before-assignment] FALSE POSITIVE | ||
type AliasType = int | X | Y | ||
|
||
class X: | ||
pass | ||
|
||
if TYPE_CHECKING: | ||
class Y: ... | ||
|
||
class Good[T: Y]: ... | ||
type OtherAlias[T: Y] = T | None | ||
|
||
# https://github.com/pylint-dev/pylint/issues/9884 | ||
def func[T: Y](x: T) -> None: # [redefined-outer-name] FALSE POSITIVE | ||
... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
used-before-assignment:11:20:11:21::Using variable 'X' before assignment:HIGH | ||
redefined-outer-name:22:9:22:13:func:Redefining name 'T' from outer scope (line 6):UNDEFINED |
16 changes: 16 additions & 0 deletions
16
tests/functional/u/used_02/used_before_assignment_py313.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
"""Tests for used-before-assignment with Python 3.13 type var defaults (PEP 696)""" | ||
# pylint: disable=missing-docstring,unused-argument,too-few-public-methods | ||
|
||
from typing import TYPE_CHECKING | ||
|
||
if TYPE_CHECKING: | ||
class Y: ... | ||
|
||
class Good1[T = Y]: ... | ||
class Good2[*Ts = tuple[int, Y]]: ... | ||
class Good3[**P = [int, Y]]: ... | ||
type Alias[T = Y] = T | None | ||
|
||
# https://github.com/pylint-dev/pylint/issues/9884 | ||
def func[T = Y](x: T) -> None: # [redefined-outer-name] FALSE POSITIVE | ||
... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
[testoptions] | ||
min_pyver=3.13 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
redefined-outer-name:15:9:15:14:func:Redefining name 'T' from outer scope (line 12):UNDEFINED |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes I wonder if it still makes sense to keep the artificial file limit around. IMO it's just annoying most of the time. Should we remove it?
pylint/pylint/testutils/functional/find_functional_tests.py
Lines 13 to 17 in 32304fe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the file limit it was really annoying to find a functional tests though (scrolling for eternity). It's not going to be annoying for a while if we remove the file limit contraint though. Might be a release time check maybe ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly use
Go to File ...
in VS Code so I can't speak to that.My issue here was a more practical one. I wanted to add one more test which should be placed alongside the other
used_before_assignment
test cases but when I did it immediately triggered limit and I wasn't even able to continue working on the test case. Sure I could move a lot of tests around here but that would just inflate the diff. Furthermore, moving tests makes git blame a lot more difficult. Sure there is.git-blame-ignore-revs
but it only contains one commit.That would probably just delay the issue and make releases more time consuming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you never just browse the functional tests to see if something match what you want to test ? I agree with the git blame not working well if we move a lot of file. (It's already the case almost everywhere, I did refactor a lot in the past, and I'm not alone in 20 years, some git blame in pylint already make me feel like a cyber-crime forensic. I'm not saying we should make it worse voluntarily though).
I'm a little reluctant to remove all rules. One possible solution would be to find another clear organization for the functional tests (like in the extension functional tests directory, probably). One directory per message, avoid testing multiple message in the same file. Or remove constraint on regression directory only : the issue always arise in this directory, right ? Or remove constraint on regression directory but force regression tests to be inside
tests/functional/regression/
with a file name being an int as intests/functional/regression/8736.py
. What do you think ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh not really. I usually just search for the error message and see which existing tests pop up or which tests fail if I change something 😅
Yeah, it's broken already basically. Thankfully it still mostly works for the checkers themselves which is usually enough to find the right PR.
We mostly do that already but sometimes it's just more practical to test multiple different things in one go. Otherwise we end up duplicating a lot of test code which will make maintaining it just more difficult.
The
unused_variable
andused_before_assignment
folders are also frequent issues, so it's not just the regression dirs. If it were just one test file, that would be fine. However, with #10382 there are now tests with 3 or even 4 supporting files. Maybe a first small step could be to ignore these and only count.py
files?--
We don't need to find a particular solution for the issue here. It was just something that bothered me, so I wanted to highlight it and start the discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but isn't the discussion's goal to find a solution to the annoyance ;) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#10490