Make global-variable-not-assigned check local scope (#4990)

* Make ``global-variable-not-assigned`` check local scope
This checker now checks whether the names after the global keyword
are reassigned in the local scope.
This closes #1375

* Make ``global-variable-not-assigned`` check functions
This checker now also checks function defintions in the module and local scope
This closes #330

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

ChangeLog

@@ -77,6 +77,12 @@ Release date: TBA
 
   Closes #4900
 
+* The ``global-variable-not-assigned`` checker now catches global variables that are never reassigned in a
+  local scope and catches (reassigned) functions
+
+  Closes #1375
+  Closes #330
+
 * Fix ``no-self-use`` and ``docparams extension`` for async functions and methods.
 
 

doc/whatsnew/2.11.rst

@@ -79,3 +79,9 @@ Other Changes
 * Fix a bug where pylint complained if the cache's parent directory does not exist
 
   Closes #4900
+
+* The ``global-variable-not-assigned`` checker now catches global variables that are never reassigned in a
+  local scope and catches (reassigned) functions
+
+  Closes #1375
+  Closes #330

pylint/checkers/utils.py

@@ -1558,3 +1558,11 @@ def is_node_in_guarded_import_block(node: nodes.NodeNG) -> bool:
     return isinstance(node.parent, nodes.If) and (
         node.parent.is_sys_guard() or node.parent.is_typing_guard()
     )
+
+
+def is_reassigned_after_current(node: nodes.NodeNG, varname: str) -> bool:
+    """Check if the given variable name is reassigned in the same scope after the current node"""
+    return any(
+        a.name == varname and a.lineno > node.lineno
+        for a in node.scope().nodes_of_class((nodes.AssignName, nodes.FunctionDef))
+    )

pylint/checkers/variables.py

@@ -931,7 +931,10 @@ def visit_global(self, node: nodes.Global) -> None:
             not_defined_locally_by_import = not any(
                 isinstance(local, nodes.Import) for local in locals_.get(name, ())
             )
-            if not assign_nodes and not_defined_locally_by_import:
+            if (
+                not utils.is_reassigned_after_current(node, name)
+                and not_defined_locally_by_import
+            ):
                 self.add_message("global-variable-not-assigned", args=name, node=node)
                 default_message = False
                 continue
@@ -946,6 +949,9 @@ def visit_global(self, node: nodes.Global) -> None:
                 if anode.frame() is module:
                     # module level assignment
                     break
+                if isinstance(anode, nodes.FunctionDef) and anode.parent is module:
+                    # module level function assignment
+                    break
             else:
                 if not_defined_locally_by_import:
                     # global undefined at the module scope

tests/functional/g/globals.py

@@ -1,10 +1,13 @@
 """Warnings about global statements and usage of global variables."""
+# pylint: disable=invalid-name, redefined-outer-name, missing-function-docstring
 from __future__ import print_function
 
 global CSTE  # [global-at-module-level]
 print(CSTE)  # [undefined-variable]
 
 CONSTANT = 1
+def FUNC():
+    pass
 
 def fix_contant(value):
     """all this is ok, but try not using global ;)"""
@@ -25,8 +28,40 @@ def define_constant():
     SOMEVAR = 2
 
 
-# pylint: disable=invalid-name
 def global_with_import():
     """should only warn for global-statement"""
     global sys  # [global-statement]
     import sys  # pylint: disable=import-outside-toplevel
+
+
+def global_no_assign():
+    """Not assigning anything to the global makes 'global' superfluous"""
+    global CONSTANT  # [global-variable-not-assigned]
+    print(CONSTANT)
+
+
+def global_operator_assign():
+    """Operator assigns should only throw a global statement error"""
+    global CONSTANT  # [global-statement]
+    print(CONSTANT)
+    CONSTANT += 1
+
+
+def global_function_assign():
+    """Function assigns should only throw a global statement error"""
+    global CONSTANT  # [global-statement]
+
+    def CONSTANT():
+        pass
+
+    CONSTANT()
+
+
+def override_func():
+    """Overriding a function should only throw a global statement error"""
+    global FUNC # [global-statement]
+
+    def FUNC():
+        pass
+
+    FUNC()

tests/functional/g/globals.txt

@@ -1,7 +1,11 @@
-global-at-module-level:4:0::Using the global statement at the module level
-undefined-variable:5:6::Undefined variable 'CSTE'
-global-statement:11:4:fix_contant:Using the global statement
-global-variable-not-assigned:18:4:other:Using global for 'HOP' but no assignment is done
-undefined-variable:19:10:other:Undefined variable 'HOP'
-global-variable-undefined:24:4:define_constant:Global variable 'SOMEVAR' undefined at the module level
-global-statement:31:4:global_with_import:Using the global statement
+global-at-module-level:5:0::Using the global statement at the module level:HIGH
+undefined-variable:6:6::Undefined variable 'CSTE':HIGH
+global-statement:14:4:fix_contant:Using the global statement:HIGH
+global-variable-not-assigned:21:4:other:Using global for 'HOP' but no assignment is done:HIGH
+undefined-variable:22:10:other:Undefined variable 'HOP':HIGH
+global-variable-undefined:27:4:define_constant:Global variable 'SOMEVAR' undefined at the module level:HIGH
+global-statement:33:4:global_with_import:Using the global statement:HIGH
+global-variable-not-assigned:39:4:global_no_assign:Using global for 'CONSTANT' but no assignment is done:HIGH
+global-statement:45:4:global_operator_assign:Using the global statement:HIGH
+global-statement:52:4:global_function_assign:Using the global statement:HIGH
+global-statement:62:4:override_func:Using the global statement:HIGH

tests/functional/u/unused/unused_variable.py

@@ -1,4 +1,4 @@
-# pylint: disable=missing-docstring, invalid-name, too-few-public-methods, no-self-use, useless-object-inheritance,import-outside-toplevel, fixme
+# pylint: disable=missing-docstring, invalid-name, too-few-public-methods, no-self-use, useless-object-inheritance,import-outside-toplevel, fixme, line-too-long
 
 def test_regression_737():
     import xml # [unused-import]
@@ -94,7 +94,7 @@ def test_global():
     """ Test various assignments of global
     variables through imports.
     """
-    global PATH, OS, collections, deque  # [global-statement]
+    global PATH, OS, collections, deque  # [global-variable-not-assigned, global-variable-not-assigned]
     from os import path as PATH
     import os as OS
     import collections

tests/functional/u/unused/unused_variable.txt

@@ -13,7 +13,8 @@ unused-import:55:4:unused_import_from:Unused namedtuple imported from collection
 unused-import:59:4:unused_import_in_function:Unused hexdigits imported from string
 unused-variable:64:4:hello:Unused variable 'my_var'
 unused-variable:76:4:function:Unused variable 'aaaa'
-global-statement:97:4:test_global:Using the global statement
+global-variable-not-assigned:97:4:test_global:Using global for 'PATH' but no assignment is done:HIGH
+global-variable-not-assigned:97:4:test_global:Using global for 'deque' but no assignment is done:HIGH
 unused-import:103:4:test_global:Unused platform imported from sys
 unused-import:104:4:test_global:Unused version imported from sys as VERSION
 unused-import:105:4:test_global:Unused import this