Fix #5399: Fix false negatives for further variable messages for invalid type annotations or default arguments #5727

jacobtylerwalls · 2022-01-26T15:57:07Z

Type of Changes

	Type
✓	🐛 Bug fix

Description

There were two short-circuit-and-consume-names paths in the variables checker preventing reporting further errors:

undefined variable as default function argument
undefined variable as type annotation

Removing some of these "special early consume" messages has the benefit that going through the regular path raises the correct flavor of used-before-assignment versus undefined-variable.

Closes #5399

Will need a rebase after #5709. (edit: done!)

coveralls · 2022-01-26T16:08:37Z

Pull Request Test Coverage Report for Build 1764446244

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
55 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.02%) to 93.864%

Files with Coverage Reduction	New Missed Lines	%
pylint/lint/parallel.py	2	95.71%
pylint/checkers/similar.py	13	96.11%
pylint/lint/pylinter.py	14	93.83%
pylint/checkers/variables.py	26	96.33%

Totals
Change from base Build 1751757499:	0.02%
Covered Lines:	14763
Relevant Lines:	15728

💛 - Coveralls

… a type annotation is accessed multiple times

Pierre-Sassoulas

LGTM. I like the simplification when simply removing the add_message makes the code simpler and removes an inconsistency.

tests/functional/u/undefined/undefined_variable.py

tests/functional/u/use/used_before_assignment_py37.py

pylint/checkers/variables.py

Pierre-Sassoulas

Thank you @jacobtylerwalls !

DanielNoord

Just some questions. Change itself is very nice!
Stuff like this make me happy about all the work we've been putting in typing and make the code more readable over the last couple of months. The change to _is_first_level_self_reference shows how because of earlier simplification we can now actually improve the messages by building on that previous groundwork. 🎉

DanielNoord · 2022-01-26T19:09:47Z

tests/functional/u/undefined/undefined_variable.py

-    augvar += 1  # [undefined-variable]
-    del vardel  # [undefined-variable]
+    augvar += 1  # [undefined-variable, unused-variable]
+    del vardel  # [undefined-variable, unused-variable]


Isn't this a false positive? Why are we reporting unused-variable on a del operation with a undefined-variable.

Thanks, thinking more deeply about this I now see why the existing code did what it did. I might be able to handle both cases by using the consumed_uncertain functionality to delay consuming this until it's raised all the undefined errors it needs to raise. I'll revisit over the next week or so and see if that works.

If I understand correctly we're going to merge as it is and then open another MR to handle this comment ? I'm ok with this plan.

I don't think so, and if @jacobtylerwalls wanted to do so I would advise against.
Nothing personal, but we never know what might come up in any of our personal lives and I don't think it's good to add code we know raises a false-positive instead of correctly warning previously. I have 100% confidence @jacobtylerwalls will get to this, but I don't think we should merge the "faulty" code.

Addressed this in 53c5200, happy to hear your thoughts. These are statements used and defined in the same line, and we have a variable already to track that, so the approach is to just check that condition and mark as consumed.

pylint/checkers/variables.py

Pierre-Sassoulas

👍

Pierre-Sassoulas · 2022-01-27T20:38:51Z

tests/functional/u/undefined/undefined_variable.py

-    augvar += 1  # [undefined-variable]
-    del vardel  # [undefined-variable]
+    augvar += 1  # [undefined-variable, unused-variable]
+    del vardel  # [undefined-variable, unused-variable]


If I understand correctly we're going to merge as it is and then open another MR to handle this comment ? I'm ok with this plan.

#5727 (comment) need to be taken care off before merging.

DanielNoord · 2022-01-29T16:15:29Z

pylint/checkers/variables.py

-                        return (VariableVisitConsumerAction.CONSUME, found_nodes)
+                        if defined_by_stmt:
+                            current_consumer.mark_as_consumed(node.name, [node])
+                        return (VariableVisitConsumerAction.CONTINUE, None)


I'm probably missing something here, but why can't we return Consume with [node] instead of found_nodes?

CONTINUE is the only action that will allow further messages to be emitted in the same scope (the original issue being fixed), but when leaving the scope, if it's a defined_by_stmt line we want to make sure the name is consumed and does not emit unused-variable.

Otherwise, we would need a fourth VariableVisitConsumerAction action along the lines of CONSUME_AND_CONTINUE.

Hm, what do you thinks makes most sense? CONSUME_AND_CONTINUE sounds the most future-proof to me? I feel like having a mark_as_consumed somewhere in this method makes the consumption harder to comprehend again.

An alternative might be to just reduce the actions to CONTINUE and RETURN and then permit the second element of the tuple to have elements to mark as consumed no matter the first value?

Hmm that's not bad actually. Performance wise it would be similar, as if action == CONSUME or if consume_nodes should be quite similar.
Perhaps that should be done in another PR though?

Look at me thinking this PR showed how good and future-proof our previous code was 😅

Follow up PR sounds right.

Just making sure I follow: did you want to avoid adding a fourth consumer action for now?

Yes, if we know we're going to reduce down to two actions in the foreseeable future we might as well keep it like this.

DanielNoord

Approving, on the condition that we don't forget about that follow up PR 😄

jacobtylerwalls · 2022-01-29T19:17:28Z

Approving, on the condition that we don't forget about that follow up PR 😄

Easy! Will rebase #5745 on this when merged.

Pierre-Sassoulas

Thank you @jacobtylerwalls !

jacobtylerwalls changed the title ~~Fix #5399: Fix false negative for undefined-variable when a type annotation is accessed multiple times~~ Fix #5399: Fix false negatives for further variable messages for invalid type annotations or default arguments Jan 26, 2022

Fix pylint-dev#5399: Fix false negative for undefined-variable when…

ba0a8ac

… a type annotation is accessed multiple times

jacobtylerwalls force-pushed the repeat-undefined-variable branch from 1c1ed91 to ba0a8ac Compare January 26, 2022 16:09

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

Pierre-Sassoulas added this to the 2.13.0 milestone Jan 26, 2022

Pierre-Sassoulas reviewed Jan 26, 2022

View reviewed changes

tests/functional/u/undefined/undefined_variable.py Show resolved Hide resolved

tests/functional/u/use/used_before_assignment_py37.py Show resolved Hide resolved

pylint/checkers/variables.py Outdated Show resolved Hide resolved

Directly return enum values

c3ca565

Pierre-Sassoulas approved these changes Jan 26, 2022

View reviewed changes

DanielNoord reviewed Jan 26, 2022

View reviewed changes

Update docstring

1b5d942

DanielNoord self-requested a review January 27, 2022 07:31

Pierre-Sassoulas previously approved these changes Jan 27, 2022

View reviewed changes

Pierre-Sassoulas self-requested a review January 27, 2022 20:43

Avoid regression for nodes defined and used in same line

53c5200

DanielNoord reviewed Jan 29, 2022

View reviewed changes

DanielNoord approved these changes Jan 29, 2022

View reviewed changes

jacobtylerwalls mentioned this pull request Jan 29, 2022

Remove VariableVisitConsumerAction.CONSUME #5745

Merged

Pierre-Sassoulas approved these changes Jan 29, 2022

View reviewed changes

Pierre-Sassoulas merged commit 6ad1ff3 into pylint-dev:main Jan 29, 2022

jacobtylerwalls deleted the repeat-undefined-variable branch January 29, 2022 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #5399: Fix false negatives for further variable messages for invalid type annotations or default arguments #5727

Fix #5399: Fix false negatives for further variable messages for invalid type annotations or default arguments #5727

jacobtylerwalls commented Jan 26, 2022 •

edited

Loading

coveralls commented Jan 26, 2022 •

edited

Loading

Pierre-Sassoulas left a comment

Pierre-Sassoulas left a comment

DanielNoord left a comment

DanielNoord Jan 26, 2022

jacobtylerwalls Jan 27, 2022

Pierre-Sassoulas Jan 27, 2022

DanielNoord Jan 27, 2022

jacobtylerwalls Jan 29, 2022

Pierre-Sassoulas left a comment

Pierre-Sassoulas Jan 27, 2022

DanielNoord Jan 29, 2022

jacobtylerwalls Jan 29, 2022 •

edited

Loading

DanielNoord Jan 29, 2022

jacobtylerwalls Jan 29, 2022

DanielNoord Jan 29, 2022

jacobtylerwalls Jan 29, 2022

DanielNoord Jan 29, 2022

DanielNoord left a comment

jacobtylerwalls commented Jan 29, 2022

Pierre-Sassoulas left a comment

Fix #5399: Fix false negatives for further variable messages for invalid type annotations or default arguments #5727

Fix #5399: Fix false negatives for further variable messages for invalid type annotations or default arguments #5727

Conversation

jacobtylerwalls commented Jan 26, 2022 • edited Loading

Type of Changes

Description

coveralls commented Jan 26, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1764446244

💛 - Coveralls

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord 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

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

jacobtylerwalls Jan 29, 2022 • edited Loading

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 left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Jan 29, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Jan 26, 2022 •

edited

Loading

coveralls commented Jan 26, 2022 •

edited

Loading

jacobtylerwalls Jan 29, 2022 •

edited

Loading