From b50001e7b65843f3365b8cad84989a280b121766 Mon Sep 17 00:00:00 2001 From: Andrew Simmons Date: Mon, 22 Jun 2020 21:26:00 +1000 Subject: [PATCH] Fix scoping for function annotations, decorators and base classes (#1082, #3434, #3461) --- ChangeLog | 4 ++ pylint/checkers/utils.py | 12 +++-- pylint/checkers/variables.py | 64 +++++++++++++++-------- tests/checkers/unittest_variables.py | 20 +++++++ tests/functional/c/class_scope.py | 20 +++++++ tests/functional/c/class_scope.txt | 1 + tests/functional/u/undefined_variable.py | 7 +++ tests/functional/u/undefined_variable.txt | 2 + 8 files changed, 105 insertions(+), 25 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5f9939822b..75b0f99c52 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 ed2c1478c9..912cac06c0 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 a6208f146c..5a2889165d 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 @@ def visit_name(self, node): 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,15 +973,40 @@ def visit_name(self, node): # 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 - # 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): + + if current_consumer.scope_type == "class": + # The list of base classes in the class definition is not part + # of the class body + if utils.is_ancestor_name(current_consumer.node, node): + continue + + # 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 i != start_index: + # The only exceptions are: when the variable forms an iter + # within a comprehension scope; is an ancestor name; and/or + # when used as a default, decorator, or annotation within a function. + if 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": + in_annotation_or_default_or_decorator = self._defined_in_function_definition( + node, current_consumer.node + ) + if in_annotation_or_default_or_decorator: + # ignore function scope if is an annotation/default/decorator, as not in the body + continue + + if current_consumer.scope_type == "lambda": + in_lambda_default = utils.is_default_argument( + node, current_consumer.node + ) + if in_lambda_default: continue # the name has already been consumed, only check it's not a loop @@ -1463,13 +1479,19 @@ def _ignore_class_scope(self, node): # 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 9b817d8d21..995138cb28 100644 --- a/tests/checkers/unittest_variables.py +++ b/tests/checkers/unittest_variables.py @@ -152,6 +152,26 @@ def fun(self): 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 527e5efa28..4e1561c9a3 100644 --- a/tests/functional/c/class_scope.py +++ b/tests/functional/c/class_scope.py @@ -21,3 +21,23 @@ class Sub(Data): 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 348c2b5103..decd708c2c 100644 --- a/tests/functional/c/class_scope.txt +++ b/tests/functional/c/class_scope.txt @@ -4,3 +4,4 @@ undefined-variable:13:Well.: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 82059997dd..7fe205cc9f 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 d9aa6c86db..0ac8a530f2 100644 --- a/tests/functional/u/undefined_variable.txt +++ b/tests/functional/u/undefined_variable.txt @@ -26,3 +26,5 @@ undefined-variable:225:LambdaClass4.:Undefined variable 'LambdaClass4' undefined-variable:233:LambdaClass5.: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