Emit possibly-used-before-assignment after if/else switches
#8952
Conversation
jacobtylerwalls
added
Enhancement ✨
jacobtylerwalls
force-pushed
the
1727-uba-false-negative
branch
from
1b88187
to
f0be36d
Compare
This comment has been minimized.
This comment has been minimized.
|
Many of the new messages in the primer look like reasonable reliances on repeating a variation of the same of the if condition (which we already test) but just not exactly. Probably worth a new |
Pierre-Sassoulas
added
the
Needs decision 🔒
|
@jacobtylerwalls I see this PR is still a draft - is it completed and ready for review? |
|
I'll add some tests and then open it for review 👍 EDIT: there's one test revealing an issue. I may not have time to debug it in the near term. Help appreciated! |
jacobtylerwalls
force-pushed
the
1727-uba-false-negative
branch
from
3557f76
to
f17a83c
Compare
This comment has been minimized.
This comment has been minimized.
|
I looked at the three astroid new messages and they look like they could very well be actual error (or if that's not the case it would be for non obvious reasons related to how the ast is constructed or how the code is called). Generally we favor false negatives over false positives but the ratio seems good in this case. Although, there's more than a hundred such examples to review to have the real ratio, which I don't see myself doing anytime soon... |
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8952 +/- ##
==========================================
- Coverage 95.82% 95.81% -0.01%
==========================================
Files 173 173
Lines 18797 18825 +28
==========================================
+ Hits 18013 18038 +25
- Misses 784 787 +3
|
This comment has been minimized.
This comment has been minimized.
|
I like a new message because it's easier to configure pylint for it for a user. It get raised you disable it (thus we can make it opt-out without too much pain), or you need to learn about it and then enable it. Options, you need to at least learn what the possible choices are and that it exists. |
|
I think a new message is ideal. There will be situations where it's not possible for the linter to know if it's truly used before assignment. The point of the linter is to help the user as much as possible, in the way that they would like to be helped. For users who don't disable the new message type, pylint can inform of potential-but-maybe-not-actual issues without bothering other users who prefer to take on more risk or just want less noise. |
Pierre-Sassoulas
removed
the
Needs decision 🔒
This comment has been minimized.
This comment has been minimized.
| if x == a: # [used-before-assignment] | ||
| if x == a: # [possibly-used-before-assignment] | ||
| if x > 3: | ||
| x = 2 # [redefined-outer-name] |
There was a problem hiding this comment.
This is kind of a shame, but I can live with it.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jacobtylerwalls
changed the title
used-before-assignment after if/else switchespossibly-used-before-assignment after if/else switches
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Only some nitpicks from me :) I think the primer looks great. Testament to the code quality in pygame is the fact that we can immediately see that it's a false positive that we raise (but not unjustified of course), when some variable is only defined on windows because the function is named initsysfonts_win32 (https://github.com/pygame/pygame/blob/9cb30af95d9143dbbf52c13817753c54d5d2f5fe/src_py/sysfont.py#L67).
Great addition that will clarify both used-before-assignment and possibly-used-before-assignment !
doc/data/messages/p/possibly-used-before-assignment/details.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Pierre Sassoulas <[email protected]>
|
Should this be enabled by default? |
|
Perhaps first as an extension and then after 1 or 2 minor version by default? |
|
I think making this an extension would involve moving the code around (and we have a lot of optimization if the code is mixed with used-before-assignement, right ?), so I would say:
|
|
Let's go with option 2 then! |
Type of Changes
Description
Closes #1727