Fix false negative `no-member` when the code was a `manual` augmented assignments #7509

clavedeluna · 2022-09-21T15:54:00Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

Add code to detect no-member when an augmented assign is done "manually", that is, when a user does this

obj.value = 1 + obj.value
instead of
obj.value += 1

If no-member would be raised in the second case, it should also be raised in the first case, which is why this PR is here!

Closes #4562 (altho I can remove this auto-close if we want to keep it open for the pydantic false positive case, though I Personally think it should be a separate issue)

clavedeluna · 2022-09-21T15:55:34Z

pylint/checkers/utils.py

+    if not attr:
+        return False
+
+    return binop.op == "+" and attr.attrname == node.attrname


this entire util func is pretty ugly ... all ears for improvements

coveralls · 2022-09-21T16:00:55Z

Pull Request Test Coverage Report for Build 3112831510

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.315%

Totals
Change from base Build 3108882423:	0.0%
Covered Lines:	17109
Relevant Lines:	17950

💛 - Coveralls

Pierre-Sassoulas

Looks pretty good already, thank you. I made a small review, there's an issue with the pipeline to fix too.

Pierre-Sassoulas · 2022-09-21T18:03:05Z

pylint/checkers/utils.py

+        return False
+
+    children = [x for x in parent.get_children()]
+    binops = [x for x in children if isinstance(x, nodes.BinOp)]


As children is only used in the second list comprehension, imbricating both list comprehension would be more efficient.

I'm on mobile so I can't check get_children api but I wouldn't be surprised if you can get all the children of a certain type directly.

I don't see a way to get children of a certain type https://pylint.pycqa.org/projects/astroid/en/latest/api/astroid.nodes.html#astroid.nodes.AugAssign.get_children
but will def combine the list comps

Maybe node_of_classes but I'm not sure it's only the children that are returned. Sorry for suggesting things that I can't test myself right now, I'm on mobile.

Pierre-Sassoulas · 2022-09-21T18:06:31Z

pylint/checkers/utils.py

+
+    children = [x for x in parent.get_children()]
+    binops = [x for x in children if isinstance(x, nodes.BinOp)]
+    if not binops:


We can avoid doing the following if in some cases like this:

Suggested change

if not binops:

if not binops or binops[0] ! = "+":

good thinking!

Pierre-Sassoulas · 2022-09-21T18:07:36Z

pylint/checkers/utils.py

+    if not attr:
+        return False
+
+    return binop.op == "+" and attr.attrname == node.attrname


Suggested change

return binop.op == "+" and attr.attrname == node.attrname

return attr.attrname == node.attrname

Pierre-Sassoulas · 2022-09-21T18:08:05Z

pylint/checkers/utils.py

+
+    binop = binops[0]
+
+    attr = None


Suggested change

attr = None

I'm much more in favor of

var = None if ... var = 1 elif ... var = 2 if not var

but hey I"ll take your suggestion

I did not consider the case where var is none from the if, I'm not opinionated about this you asked about possible style fix so I tried :)

Pierre-Sassoulas · 2022-09-21T18:08:37Z

pylint/checkers/utils.py

+
+    if not attr:


Suggested change

if not attr:

else:

DanielNoord · 2022-09-21T18:14:32Z

@Pierre-Sassoulas I was thinking: does it make sense to make this a code style check? consider-using-augmented-assign. We would get a nice primer result to check for false positives on this util function.

Pierre-Sassoulas · 2022-09-21T18:16:52Z

Sounds good !

DanielNoord · 2022-09-21T18:18:24Z

@clavedeluna I'll have a look at converting this into a message tomorrow. You can do so as well if you want to? I'd be happy to guide you in the right direction?

clavedeluna · 2022-09-21T18:53:11Z

@clavedeluna I'll have a look at converting this into a message tomorrow. You can do so as well if you want to? I'd be happy to guide you in the right direction?

go right ahead with adding it, I'll look at Pierre's review. I think doing consider-using-augmented-assign should be a separate PR

clavedeluna · 2022-09-21T19:14:07Z

whoops... broke the tests

clavedeluna · 2022-09-21T19:23:47Z

Alright so maybe I'm missing something obvious but how is
pylint/checkers/utils.py:1996: error: Returning Any from function declared to return "bool
an issue for return attr.attrname == node.attrname

is that not always a bool? do I wrap it in bool??

DanielNoord · 2022-09-21T19:25:26Z

Alright so maybe I'm missing something obvious but how is pylint/checkers/utils.py:1996: error: Returning Any from function declared to return "bool an issue for return attr.attrname == node.attrname

is that not always a bool? do I wrap it in bool??

This is because astroid is not typed so you're essentially comparing Any with Any. If you add a disable for this specific error with type: ignore we will remove it after astroid does have typing.

clavedeluna · 2022-09-22T11:12:00Z

we will remove it after astroid does have typing.

ah makes sense! I'm getting more and more familiar with the code base so thanks for the tip.

Pierre-Sassoulas

LGTM, thank you @clavedeluna !

DanielNoord · 2022-09-22T11:33:15Z

I'm working a PR that incorporates these changes to get a primer result. Should be done in 30 minutes or something like that.

Pierre-Sassoulas · 2022-09-22T11:34:21Z

I'll let you merge this one @DanielNoord

DanielNoord · 2022-09-22T12:27:00Z

See #7514.

I had to change quite a bit. Preferably we merge that PR first and then use the utility introduced there in this PR. The robustness we get from checking the new checker on the primer is a huge benefit compared to the functional tests we can think of ourselves.

clavedeluna · 2022-09-22T13:38:09Z

See #7514.

I had to change quite a bit. Preferably we merge that PR first and then use the utility introduced there in this PR. The robustness we get from checking the new checker on the primer is a huge benefit compared to the functional tests we can think of ourselves.

I will rebase and re-do this PR once that one is merged

Pierre-Sassoulas

Thank you for inspiring the work on the augmented assignment checker, and thank you for fixing this !

clavedeluna force-pushed the no-member branch from d454dbe to d56ed59 Compare September 21, 2022 15:54

clavedeluna commented Sep 21, 2022

View reviewed changes

Pierre-Sassoulas requested changes Sep 21, 2022

View reviewed changes

clavedeluna requested a review from Pierre-Sassoulas September 21, 2022 19:10

clavedeluna force-pushed the no-member branch from 2146e03 to 8b3de4d Compare September 21, 2022 19:15

Pierre-Sassoulas added the False Negative 🦋 No message is emitted but something is wrong with the code label Sep 22, 2022

Pierre-Sassoulas added this to the 2.16.0 milestone Sep 22, 2022

Pierre-Sassoulas previously approved these changes Sep 22, 2022

View reviewed changes

Pierre-Sassoulas changed the title ~~add case to detect manual augomented assign~~ Add necessary handling to detect manual augmented assignments Sep 22, 2022

DanielNoord mentioned this pull request Sep 22, 2022

Add consider-using-augmented-assign checker #7514

Merged

3 tasks

clavedeluna added 3 commits September 23, 2022 09:22

add case to detect manual augomented assign

0935147

add ignore no-any-return

bd0f507

use new utility check for augmented assign

0216731

clavedeluna dismissed Pierre-Sassoulas’s stale review via 0216731 September 23, 2022 12:44

clavedeluna force-pushed the no-member branch from de151b6 to 0216731 Compare September 23, 2022 12:44

clavedeluna requested a review from Pierre-Sassoulas September 23, 2022 12:49

Pierre-Sassoulas approved these changes Sep 23, 2022

View reviewed changes

Pierre-Sassoulas changed the title ~~Add necessary handling to detect manual augmented assignments~~ Fix false negative no-member when the code was a manual augmented assignments Sep 23, 2022

Pierre-Sassoulas merged commit ca6648a into pylint-dev:main Sep 23, 2022

cdce8p mentioned this pull request Oct 16, 2022

Regression no-member with inherited AnnAssign nodes #7631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix false negative `no-member` when the code was a `manual` augmented assignments #7509

Fix false negative `no-member` when the code was a `manual` augmented assignments #7509

clavedeluna commented Sep 21, 2022

clavedeluna Sep 21, 2022

coveralls commented Sep 21, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

Pierre-Sassoulas Sep 21, 2022

clavedeluna Sep 21, 2022

Pierre-Sassoulas Sep 21, 2022

Pierre-Sassoulas Sep 21, 2022

clavedeluna Sep 21, 2022

Pierre-Sassoulas Sep 21, 2022

Pierre-Sassoulas Sep 21, 2022

clavedeluna Sep 21, 2022

Pierre-Sassoulas Sep 21, 2022

Pierre-Sassoulas Sep 21, 2022

DanielNoord commented Sep 21, 2022

Pierre-Sassoulas commented Sep 21, 2022

DanielNoord commented Sep 21, 2022

clavedeluna commented Sep 21, 2022 •

edited

Loading

clavedeluna commented Sep 21, 2022

clavedeluna commented Sep 21, 2022

DanielNoord commented Sep 21, 2022

clavedeluna commented Sep 22, 2022

Pierre-Sassoulas left a comment

DanielNoord commented Sep 22, 2022

Pierre-Sassoulas commented Sep 22, 2022

DanielNoord commented Sep 22, 2022

clavedeluna commented Sep 22, 2022

Pierre-Sassoulas left a comment

	return binop.op == "+" and attr.attrname == node.attrname
	return attr.attrname == node.attrname

Fix false negative no-member when the code was a manual augmented assignments #7509

Fix false negative no-member when the code was a manual augmented assignments #7509

Conversation

clavedeluna commented Sep 21, 2022

Type of Changes

Description

Choose a reason for hiding this comment

coveralls commented Sep 21, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3112831510

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Sep 21, 2022

Pierre-Sassoulas commented Sep 21, 2022

DanielNoord commented Sep 21, 2022

clavedeluna commented Sep 21, 2022 • edited Loading

clavedeluna commented Sep 21, 2022

clavedeluna commented Sep 21, 2022

DanielNoord commented Sep 21, 2022

clavedeluna commented Sep 22, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Sep 22, 2022

Pierre-Sassoulas commented Sep 22, 2022

DanielNoord commented Sep 22, 2022

clavedeluna commented Sep 22, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Fix false negative `no-member` when the code was a `manual` augmented assignments #7509

Fix false negative `no-member` when the code was a `manual` augmented assignments #7509

coveralls commented Sep 21, 2022 •

edited

Loading

clavedeluna commented Sep 21, 2022 •

edited

Loading