Fix #5399: Fix false negatives for further variable messages for inva…

…lid type annotations or default arguments (#5727)
pylint-dev · Jan 29, 2022 · 6ad1ff3 · 6ad1ff3
1 parent e0dbce1
commit 6ad1ff3
Show file tree

Hide file tree

Showing 9 changed files with 61 additions and 44 deletions.
diff --git a/ChangeLog b/ChangeLog
@@ -107,6 +107,12 @@ Release date: TBA
 
   Closes #5568
 
+* Fix false negative for ``undefined-variable`` and related variable messages
+  when the same undefined variable is used as a type annotation and is
+  accessed multiple times, or is used as a default argument to a function.
+
+  Closes #5399
+
 * Pyreverse - add output in mermaidjs format
 
 * Emit ``used-before-assignment`` instead of ``undefined-variable`` when attempting

diff --git a/doc/whatsnew/2.13.rst b/doc/whatsnew/2.13.rst
@@ -134,6 +134,12 @@ Other Changes
   Closes #4798
   Closes #5081
 
+* Fix false negative for ``undefined-variable`` and related variable messages
+  when the same undefined variable is used as a type annotation and is
+  accessed multiple times, or is used as a default argument to a function.
+
+  Closes #5399
+
 * Emit ``used-before-assignment`` instead of ``undefined-variable`` when attempting
   to access unused type annotations.
 

diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
@@ -170,7 +170,8 @@
 
 
 class VariableVisitConsumerAction(Enum):
-    """Used after _visit_consumer to determine the action to be taken
+    """Reported by _check_consumer() and its sub-methods to determine the
+    subsequent action to take in _undefined_and_used_before_checker().
 
     Continue -> continue loop to next consumer
     Return -> return and thereby break the loop
@@ -182,6 +183,18 @@ class VariableVisitConsumerAction(Enum):
     CONSUME = 2
 
 
+VariableVisitConsumerActionAndOptionalNodesType = Union[
+    Tuple[
+        Union[
+            Literal[VariableVisitConsumerAction.CONTINUE],
+            Literal[VariableVisitConsumerAction.RETURN],
+        ],
+        None,
+    ],
+    Tuple[Literal[VariableVisitConsumerAction.CONSUME], List[nodes.NodeNG]],
+]
+
+
 def _is_from_future_import(stmt, name):
     """Check if the name is a future import from another module."""
     try:
@@ -1469,16 +1482,7 @@ def _check_consumer(
         current_consumer: NamesConsumer,
         consumer_level: int,
         base_scope_type: Any,
-    ) -> Union[
-        Tuple[
-            Union[
-                Literal[VariableVisitConsumerAction.CONTINUE],
-                Literal[VariableVisitConsumerAction.RETURN],
-            ],
-            None,
-        ],
-        Tuple[Literal[VariableVisitConsumerAction.CONSUME], List[nodes.NodeNG]],
-    ]:
+    ) -> VariableVisitConsumerActionAndOptionalNodesType:
         """Checks a consumer for conditions that should trigger messages"""
         # If the name has already been consumed, only check it's not a loop
         # variable used outside the loop.
@@ -1626,10 +1630,9 @@ def _check_consumer(
                         )
                         and node.name in node.root().locals
                     ):
-                        self.add_message(
-                            "undefined-variable", args=node.name, node=node
-                        )
-                        return (VariableVisitConsumerAction.CONSUME, found_nodes)
+                        if defined_by_stmt:
+                            current_consumer.mark_as_consumed(node.name, [node])
+                        return (VariableVisitConsumerAction.CONTINUE, None)
 
             elif base_scope_type != "lambda":
                 # E0601 may *not* occurs in lambda scope.
@@ -1681,13 +1684,7 @@ def _check_consumer(
             return (VariableVisitConsumerAction.CONSUME, found_nodes)
 
         elif isinstance(defstmt, nodes.ClassDef):
-            is_first_level_ref = self._is_first_level_self_reference(node, defstmt)
-            if is_first_level_ref == 2:
-                self.add_message(
-                    "used-before-assignment", node=node, args=node.name, confidence=HIGH
-                )
-            if is_first_level_ref:
-                return (VariableVisitConsumerAction.RETURN, None)
+            return self._is_first_level_self_reference(node, defstmt, found_nodes)
 
         elif isinstance(defnode, nodes.NamedExpr):
             if isinstance(defnode.parent, nodes.IfExp):
@@ -2080,31 +2077,26 @@ def _is_only_type_assignment(node: nodes.Name, defstmt: nodes.Statement) -> bool
 
     @staticmethod
     def _is_first_level_self_reference(
-        node: nodes.Name, defstmt: nodes.ClassDef
-    ) -> Literal[0, 1, 2]:
+        node: nodes.Name, defstmt: nodes.ClassDef, found_nodes: List[nodes.NodeNG]
+    ) -> VariableVisitConsumerActionAndOptionalNodesType:
         """Check if a first level method's annotation or default values
-        refers to its own class.
-
-        Return values correspond to:
-            0 = Continue
-            1 = Break
-            2 = Break + emit message
+        refers to its own class, and return a consumer action
         """
         if node.frame(future=True).parent == defstmt and node.statement(
             future=True
         ) == node.frame(future=True):
             # Check if used as type annotation
-            # Break but don't emit message if postponed evaluation is enabled
+            # Break if postponed evaluation is enabled
             if utils.is_node_in_type_annotation_context(node):
                 if not utils.is_postponed_evaluation_enabled(node):
-                    return 2
-                return 1
+                    return (VariableVisitConsumerAction.CONTINUE, None)
+                return (VariableVisitConsumerAction.RETURN, None)
             # Check if used as default value by calling the class
             if isinstance(node.parent, nodes.Call) and isinstance(
                 node.parent.parent, nodes.Arguments
             ):
-                return 2
-        return 0
+                return (VariableVisitConsumerAction.CONTINUE, None)
+        return (VariableVisitConsumerAction.CONSUME, found_nodes)
 
     @staticmethod
     def _is_never_evaluated(

diff --git a/tests/functional/u/undefined/undefined_variable.py b/tests/functional/u/undefined/undefined_variable.py
@@ -133,7 +133,7 @@ class Ancestor1(object):
     """ No op """
 
 NANA = BAT # [undefined-variable]
-del BAT
+del BAT  # [undefined-variable]
 
 
 class KeywordArgument(object):
@@ -356,3 +356,12 @@ def global_var_mixed_assignment():
 
 GLOBAL_VAR: int
 GLOBAL_VAR_TWO: int
+
+
+class RepeatedReturnAnnotations:
+    def x(self, o: RepeatedReturnAnnotations) -> bool:  # [undefined-variable]
+        pass
+    def y(self) -> RepeatedReturnAnnotations:  # [undefined-variable]
+        pass
+    def z(self) -> RepeatedReturnAnnotations:  # [undefined-variable]
+        pass
diff --git a/tests/functional/u/undefined/undefined_variable.txt b/tests/functional/u/undefined/undefined_variable.txt
@@ -19,6 +19,7 @@ used-before-assignment:98:26:98:35:TestClass.MissingAncestor:Using variable 'Anc
 used-before-assignment:105:36:105:41:TestClass.test1.UsingBeforeDefinition:Using variable 'Empty' before assignment:HIGH
 undefined-variable:119:10:119:14:Self:Undefined variable 'Self':UNDEFINED
 undefined-variable:135:7:135:10::Undefined variable 'BAT':UNDEFINED
+undefined-variable:136:4:136:7::Undefined variable 'BAT':UNDEFINED
 used-before-assignment:146:31:146:38:KeywordArgument.test1:Using variable 'enabled' before assignment:HIGH
 undefined-variable:149:32:149:40:KeywordArgument.test2:Undefined variable 'disabled':UNDEFINED
 undefined-variable:154:22:154:25:KeywordArgument.<lambda>:Undefined variable 'arg':UNDEFINED
@@ -33,3 +34,6 @@ used-before-assignment:294:7:294:8:undefined_annotation:Using variable 'x' befor
 undefined-variable:324:11:324:12:decorated3:Undefined variable 'x':UNDEFINED
 undefined-variable:329:19:329:20:decorated4:Undefined variable 'y':UNDEFINED
 undefined-variable:350:10:350:20:global_var_mixed_assignment:Undefined variable 'GLOBAL_VAR':HIGH
+undefined-variable:362:19:362:44:RepeatedReturnAnnotations.x:Undefined variable 'RepeatedReturnAnnotations':UNDEFINED
+undefined-variable:364:19:364:44:RepeatedReturnAnnotations.y:Undefined variable 'RepeatedReturnAnnotations':UNDEFINED
+undefined-variable:366:19:366:44:RepeatedReturnAnnotations.z:Undefined variable 'RepeatedReturnAnnotations':UNDEFINED
diff --git a/tests/functional/u/use/used_before_assignment_py37.py b/tests/functional/u/use/used_before_assignment_py37.py
@@ -14,7 +14,7 @@ def second_correct_typing_method(self, other: List[MyClass]) -> bool:
         return self == other[0]
 
     def incorrect_default_method(
-        self, other=MyClass() # [used-before-assignment]
+        self, other=MyClass() # [undefined-variable]
     ) -> bool:
         return self == other
 

diff --git a/tests/functional/u/use/used_before_assignment_py37.txt b/tests/functional/u/use/used_before_assignment_py37.txt
@@ -1 +1 @@
-used-before-assignment:17:20:17:27:MyClass.incorrect_default_method:Using variable 'MyClass' before assignment:HIGH
+undefined-variable:17:20:17:27:MyClass.incorrect_default_method:Undefined variable 'MyClass':UNDEFINED
diff --git a/tests/functional/u/use/used_before_assignment_typing.py b/tests/functional/u/use/used_before_assignment_typing.py
@@ -9,17 +9,17 @@ class MyClass:
     """Type annotation or default values for first level methods can't refer to their own class"""
 
     def incorrect_typing_method(
-        self, other: MyClass  # [used-before-assignment]
+        self, other: MyClass  # [undefined-variable]
     ) -> bool:
         return self == other
 
     def incorrect_nested_typing_method(
-        self, other: List[MyClass]  # [used-before-assignment]
+        self, other: List[MyClass]  # [undefined-variable]
     ) -> bool:
         return self == other[0]
 
     def incorrect_default_method(
-        self, other=MyClass()  # [used-before-assignment]
+        self, other=MyClass()  # [undefined-variable]
     ) -> bool:
         return self == other
 

diff --git a/tests/functional/u/use/used_before_assignment_typing.txt b/tests/functional/u/use/used_before_assignment_typing.txt
@@ -1,3 +1,3 @@
-used-before-assignment:12:21:12:28:MyClass.incorrect_typing_method:Using variable 'MyClass' before assignment:HIGH
-used-before-assignment:17:26:17:33:MyClass.incorrect_nested_typing_method:Using variable 'MyClass' before assignment:HIGH
-used-before-assignment:22:20:22:27:MyClass.incorrect_default_method:Using variable 'MyClass' before assignment:HIGH
+undefined-variable:12:21:12:28:MyClass.incorrect_typing_method:Undefined variable 'MyClass':UNDEFINED
+undefined-variable:17:26:17:33:MyClass.incorrect_nested_typing_method:Undefined variable 'MyClass':UNDEFINED
+undefined-variable:22:20:22:27:MyClass.incorrect_default_method:Undefined variable 'MyClass':UNDEFINED
Original file line number	Diff line number	Diff line change
		@@ -1 +1 @@
		used-before-assignment:17:20:17:27:MyClass.incorrect_default_method:Using variable 'MyClass' before assignment:HIGH
		undefined-variable:17:20:17:27:MyClass.incorrect_default_method:Undefined variable 'MyClass':UNDEFINED