Fix scoping for function annotations, decorators and base classes (#3713)

Fix scoping for function annotations, decorators and base classes

Closes #1082, #3434, #3461

Reduce number of branches in variables checker

Co-authored-by: Andrew Simmons <a.simmons@deakin.edu.au>

8 files changed, 95 insertions, 23 deletions

diff --git a/ChangeLog b/ChangeLog
index f27df2c4c..197255275 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -7,6 +7,10 @@ What's New in Pylint 2.6.0?
 
 Release date: TBA
 
+* Fix various scope-related bugs in ``undefined-variable`` checker
+
+  Close #1082, #3434, #3461
+
 * bad-continuation and bad-whitespace have been removed, black or another formatter can help you with this better than Pylint
 
   Close #246, #289, #638, #747, #1148, #1179, #1943, #2041, #2301, #2304, #2944, #3565
diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py
index ed2c1478c..912cac06c 100644
--- a/pylint/checkers/utils.py
+++ b/pylint/checkers/utils.py
@@ -374,13 +374,17 @@ def is_defined_before(var_node: astroid.Name) -> bool:
     return False
 
 
-def is_default_argument(node: astroid.node_classes.NodeNG) -> bool:
+def is_default_argument(
+    node: astroid.node_classes.NodeNG,
+    scope: Optional[astroid.node_classes.NodeNG] = None,
+) -> bool:
     """return true if the given Name node is used in function or lambda
     default argument's value
     """
-    parent = node.scope()
-    if isinstance(parent, (astroid.FunctionDef, astroid.Lambda)):
-        for default_node in parent.args.defaults:
+    if not scope:
+        scope = node.scope()
+    if isinstance(scope, (astroid.FunctionDef, astroid.Lambda)):
+        for default_node in scope.args.defaults:
             for default_name_node in default_node.nodes_of_class(astroid.Name):
                 if default_name_node is node:
                     return True
diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index a6208f146..951cc62a9 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -512,6 +512,7 @@ class NamesConsumer:
 
     def __init__(self, node, scope_type):
         self._atomic = ScopeConsumer(copy.copy(node.locals), {}, scope_type)
+        self.node = node
 
     def __repr__(self):
         msg = "\nto_consume : {:s}\n".format(
@@ -958,17 +959,7 @@ class VariablesChecker(BaseChecker):
 
         name = node.name
         frame = stmt.scope()
-        # if the name node is used as a function default argument's value or as
-        # a decorator, then start from the parent frame of the function instead
-        # of the function frame - and thus open an inner class scope
-        if (
-            utils.is_default_argument(node)
-            or utils.is_func_decorator(node)
-            or utils.is_ancestor_name(frame, node)
-        ):
-            start_index = len(self._to_consume) - 2
-        else:
-            start_index = len(self._to_consume) - 1
+        start_index = len(self._to_consume) - 1
 
         undefined_variable_is_enabled = self.linter.is_message_enabled(
             "undefined-variable"
@@ -982,16 +973,33 @@ class VariablesChecker(BaseChecker):
         # pylint: disable=too-many-nested-blocks; refactoring this block is a pain.
         for i in range(start_index, -1, -1):
             current_consumer = self._to_consume[i]
-            # if the current scope is a class scope but it's not the inner
+
+            # The list of base classes in the class definition is not part
+            # of the class body.
+            # If the current scope is a class scope but it's not the inner
             # scope, ignore it. This prevents to access this scope instead of
             # the globals one in function members when there are some common
             # names.
-            if current_consumer.scope_type == "class" and i != start_index:
-                # The only exceptions are: when the variable forms an iter within a
-                # comprehension scope; and/or when used as a default, decorator,
-                # or annotation within a function.
-                if self._ignore_class_scope(node):
-                    continue
+            if current_consumer.scope_type == "class" and (
+                utils.is_ancestor_name(current_consumer.node, node)
+                or (i != start_index and self._ignore_class_scope(node))
+            ):
+                continue
+
+            # if the name node is used as a function default argument's value or as
+            # a decorator, then start from the parent frame of the function instead
+            # of the function frame - and thus open an inner class scope
+            if (
+                current_consumer.scope_type == "function"
+                and self._defined_in_function_definition(node, current_consumer.node)
+            ):
+                # ignore function scope if is an annotation/default/decorator, as not in the body
+                continue
+
+            if current_consumer.scope_type == "lambda" and utils.is_default_argument(
+                node, current_consumer.node
+            ):
+                continue
 
             # the name has already been consumed, only check it's not a loop
             # variable used outside the loop
@@ -1463,13 +1471,19 @@ class VariablesChecker(BaseChecker):
         #    tp = 2
         #    def func(self, arg=tp):
         #        ...
+        # class C:
+        #    class Tp:
+        #        pass
+        #    class D(Tp):
+        #        ...
 
         name = node.name
         frame = node.statement().scope()
         in_annotation_or_default_or_decorator = self._defined_in_function_definition(
             node, frame
         )
-        if in_annotation_or_default_or_decorator:
+        in_ancestor_list = utils.is_ancestor_name(frame, node)
+        if in_annotation_or_default_or_decorator or in_ancestor_list:
             frame_locals = frame.parent.scope().locals
         else:
             frame_locals = frame.locals
diff --git a/tests/checkers/unittest_variables.py b/tests/checkers/unittest_variables.py
index 9b817d8d2..995138cb2 100644
--- a/tests/checkers/unittest_variables.py
+++ b/tests/checkers/unittest_variables.py
@@ -152,6 +152,26 @@ class TestVariablesChecker(CheckerTestCase):
         with self.assertNoMessages():
             self.walk(module)
 
+    def test_listcomp_in_ancestors(self):
+        """ Ensure list comprehensions in base classes are scoped correctly
+
+        https://github.com/PyCQA/pylint/issues/3434
+        """
+        module = astroid.parse(
+            """
+        import collections
+
+
+        l = ["a","b","c"]
+
+
+        class Foo(collections.namedtuple("Foo",[x+"_foo" for x in l])):
+            pass
+        """
+        )
+        with self.assertNoMessages():
+            self.walk(module)
+
     def test_return_type_annotation(self):
         """ Make sure class attributes in scope for return type annotations.
 
diff --git a/tests/functional/c/class_scope.py b/tests/functional/c/class_scope.py
index 527e5efa2..4e1561c9a 100644
--- a/tests/functional/c/class_scope.py
+++ b/tests/functional/c/class_scope.py
@@ -21,3 +21,23 @@ class Well(object):
     def func(self):
         """check Sub is not defined here"""
         return Sub(), self # [undefined-variable]
+
+
+class Right:
+    """right"""
+    class Result1:
+        """result one"""
+        OK = 0
+    def work(self) -> Result1:
+        """good type hint"""
+        return self.Result1.OK
+
+
+class Wrong:
+    """wrong"""
+    class Result2:
+        """result two"""
+        OK = 0
+    def work(self) -> self.Result2: # [undefined-variable]
+        """bad type hint"""
+        return self.Result2.OK
diff --git a/tests/functional/c/class_scope.txt b/tests/functional/c/class_scope.txt
index 348c2b510..decd708c2 100644
--- a/tests/functional/c/class_scope.txt
+++ b/tests/functional/c/class_scope.txt
@@ -4,3 +4,4 @@ undefined-variable:13:Well.<lambda>:Undefined variable 'get_attr_bad'
 undefined-variable:14:Well:Undefined variable 'attr'
 undefined-variable:20:Well.Sub:Undefined variable 'Data'
 undefined-variable:23:Well.func:Undefined variable 'Sub'
+undefined-variable:41:Wrong.work:Undefined variable 'self'
diff --git a/tests/functional/u/undefined_variable.py b/tests/functional/u/undefined_variable.py
index 82059997d..7fe205cc9 100644
--- a/tests/functional/u/undefined_variable.py
+++ b/tests/functional/u/undefined_variable.py
@@ -287,3 +287,10 @@ class DunderClass:
     def method(self):
         # This name is not defined in the AST but it's present at runtime
         return __class__
+
+
+def undefined_annotation(a:x): # [undefined-variable]
+    if x == 2: # [used-before-assignment]
+        for x in [1, 2]:
+            pass
+    return a
diff --git a/tests/functional/u/undefined_variable.txt b/tests/functional/u/undefined_variable.txt
index d9aa6c86d..0ac8a530f 100644
--- a/tests/functional/u/undefined_variable.txt
+++ b/tests/functional/u/undefined_variable.txt
@@ -26,3 +26,5 @@ undefined-variable:225:LambdaClass4.<lambda>:Undefined variable 'LambdaClass4'
 undefined-variable:233:LambdaClass5.<lambda>:Undefined variable 'LambdaClass5'
 used-before-assignment:254:func_should_fail:Using variable 'datetime' before assignment
 undefined-variable:281:not_using_loop_variable_accordingly:Undefined variable 'iteree'
+undefined-variable:292:undefined_annotation:Undefined variable 'x'
+used-before-assignment:293:undefined_annotation:Using variable 'x' before assignment