Repository: serge-sans-paille/beniget

Issue:
Add support for the del statement
Since fixing #111 is basically considering that the exception name is deleted at the end of block, we might as well to try to fully support the del keyword. Just like the islive flag, a Def might have a isdeleted flag that could be used to track the deleted state of a definition.

Discussion:

Solution patch:
diff --git a/beniget/beniget.py b/beniget/beniget.py
index 9c9ee25..e7714d3 100644
--- a/beniget/beniget.py
+++ b/beniget/beniget.py
@@ -161,6 +161,12 @@ def users(self):
         """
         return self._users
 
+    def isdel(self):
+        ast = pkg(self.node)
+        if not isinstance(self.node, ast.Name):
+            return False
+        return isinstance(self.node.ctx, ast.Del)
+
     def __repr__(self):
         return self._repr({})
 
@@ -342,6 +348,9 @@ def __init__(self, filename=None):
         # stack of variable defined with the global keywords
         self._globals = []
 
+        # stack of variable defined with the nonlocal keywords
+        self._nonlocals = []
+
         # stack of local identifiers, used to detect 'read before assign'
         self._precomputed_locals = []
 
@@ -388,8 +397,8 @@ def _dump_locals(self, node, only_live=False):
     def dump_definitions(self, node, ignore_builtins=True):
         if isinstance(node, pkg(node).Module) and not ignore_builtins:
             builtins = {d for d in self._builtins.values()}
-            return sorted(d.name()
-                          for d in self.locals[node] if d not in builtins)
+            return sorted(d.name() for d in self.locals[node]
+                          if d not in builtins)
         else:
             return sorted(d.name() for d in self.locals[node])
 
@@ -465,46 +474,62 @@ def compute_defs(self, node, quiet=False):
         name = node.id
         stars = []
 
+        # List of definitions to check. This includes all non-class
+        # definitions *and* the last definition. Class definitions are not
+        # included because they require fully qualified access.
+        looked_up_definitions = []
+
+        scopes_iter = iter(reversed(self._scopes))
+        depths_iter = iter(reversed(self._scope_depths))
+        precomputed_locals_iter = iter(reversed(self._precomputed_locals))
+
+        # Keep the last scope because we could be in class scope, in which
+        # case we don't need fully qualified access.
+        lvl = depth = next(depths_iter)
+        precomputed_locals = next(precomputed_locals_iter)
+        base_scope = next(scopes_iter)
+        defs = self._definitions[depth:]
+        if not self.invalid_name_lookup(name, base_scope, precomputed_locals, defs):
+
+            def add_defs(looked_up_definitions, defs):
+                if self.is_global(name):
+                    # only add deleted node, if any
+                    for defs_ in reversed(defs):
+                        for dnode in defs_.get(name, ()):
+                            if dnode.isdel():
+                                # This create a del that shadows the global
+                                looked_up_definitions.append({name: [dnode]})
+                else:
+                    looked_up_definitions.extend(reversed(defs))
+
+            add_defs(looked_up_definitions, defs)
+
+            ast = pkg(node)
+            # Iterate over scopes, filtering out class scopes.
+            for scope, depth, precomputed_locals in zip(scopes_iter,
+                                                        depths_iter,
+                                                        precomputed_locals_iter):
+                if not isinstance(scope, ast.ClassDef):
+                    defs = self._definitions[lvl + depth: lvl]
+                    if self.invalid_name_lookup(name, base_scope, precomputed_locals, defs):
+                        looked_up_definitions.append(StopIteration)
+                        break
+                    add_defs(looked_up_definitions, defs)
+                lvl += depth
+
         # If the `global` keyword has been used, honor it
-        if any(name in _globals for _globals in self._globals):
-            looked_up_definitions = self._definitions[0:-self._scope_depths[0]]
-        else:
-            # List of definitions to check. This includes all non-class
-            # definitions *and* the last definition. Class definitions are not
-            # included because they require fully qualified access.
-            looked_up_definitions = []
-
-            scopes_iter = iter(reversed(self._scopes))
-            depths_iter = iter(reversed(self._scope_depths))
-            precomputed_locals_iter = iter(reversed(self._precomputed_locals))
-
-            # Keep the last scope because we could be in class scope, in which
-            # case we don't need fully qualified access.
-            lvl = depth = next(depths_iter)
-            precomputed_locals = next(precomputed_locals_iter)
-            base_scope = next(scopes_iter)
-            defs = self._definitions[depth:]
-            if not self.invalid_name_lookup(name, base_scope, precomputed_locals, defs):
-                looked_up_definitions.extend(reversed(defs))
-
-                ast = pkg(node)
-                # Iterate over scopes, filtering out class scopes.
-                for scope, depth, precomputed_locals in zip(scopes_iter,
-                                                            depths_iter,
-                                                            precomputed_locals_iter):
-                    if not isinstance(scope, ast.ClassDef):
-                        defs = self._definitions[lvl + depth: lvl]
-                        if self.invalid_name_lookup(name, base_scope, precomputed_locals, defs):
-                            looked_up_definitions.append(StopIteration)
-                            break
-                        looked_up_definitions.extend(reversed(defs))
-                    lvl += depth
+        if self.is_global(name):
+            looked_up_definitions.extend(self._definitions[0:-self._scope_depths[0]])
 
         for defs in looked_up_definitions:
             if defs is StopIteration:
                 break
             elif name in defs:
-                return defs[name] if not stars else stars + list(defs[name])
+                name_defs = {dnode for dnode in defs[name] if not
+                             dnode.isdel()}
+                if not name_defs:
+                    break
+                return name_defs if not stars else stars + list(name_defs)
             elif "*" in defs:
                 stars.extend(defs["*"])
 
@@ -556,9 +581,11 @@ def ScopeContext(self, node):
         self._scope_depths.append(-1)
         self._definitions.append(defaultdict(ordered_set))
         self._globals.append(set())
+        self._nonlocals.append(set())
         self._precomputed_locals.append(collect_locals(node))
         yield
         self._precomputed_locals.pop()
+        self._nonlocals.pop()
         self._globals.pop()
         self._definitions.pop()
         self._scope_depths.pop()
@@ -705,6 +732,13 @@ def visit_Module(self, node):
                 nb_heads = len({d.name() for d in self.locals[node]})
                 assert nb_defs == nb_heads + nb_bltns - nb_overloaded_bltns
 
+        # Deleted nodes are not actual definitions, but are useful for
+        # intermediate analysis. Prune them once the processing is done.
+        for dnodes in self.locals.values():
+            del_indices = [i for i, n in enumerate(dnodes) if n.isdel()]
+            for del_index in reversed(del_indices):
+                dnodes.pop(del_index)
+
         assert not self._definitions
         assert not self._defered_annotations
         assert not self._scopes
@@ -888,7 +922,7 @@ def visit_AugAssign(self, node):
             dtarget = self.visit(node.target)
             dvalue.add_user(dtarget)
             node.target.ctx = ctx
-            if any(node.target.id in _globals for _globals in self._globals):
+            if self.is_global(node.target.id):
                 self.extend_global(node.target.id, dtarget)
             else:
                 loaded_from = [d.name() for d in self.defs(node.target,
@@ -1048,7 +1082,7 @@ def visit_Assert(self, node):
             self.visit(node.msg)
 
     def add_to_locals(self, name, dnode):
-        if any(name in _globals for _globals in self._globals):
+        if self.is_global(name):
             self.set_or_extend_global(name, dnode)
         else:
             self.locals[self._scopes[-1]].append(dnode)
@@ -1075,6 +1109,7 @@ def visit_Global(self, node):
 
     def visit_Nonlocal(self, node):
         for name in node.names:
+            self._nonlocals[-1].add(name)
             # Exclude global scope
             global_scope_depth = -self._scope_depths[0]
             for d in reversed(self._definitions[global_scope_depth: -1]):
@@ -1257,9 +1292,11 @@ def visit_NamedExpr(self, node):
             self.visit_Name(node.target, named_expr=True)
         return dnode
 
-    def is_in_current_scope(self, name):
-        return any(name in defs
-                   for defs in self._definitions[self._scope_depths[-1]:])
+    def is_global(self, name):
+        return any(name in _globals for _globals in self._globals)
+
+    def is_nonlocal(self, name):
+        return any(name in _nonlocals for _nonlocals in self._nonlocals)
 
     def _first_non_comprehension_scope(self):
         index = -1
@@ -1273,10 +1310,38 @@ def _first_non_comprehension_scope(self):
 
     def visit_Name(self, node, skip_annotation=False, named_expr=False):
         ast = pkg(node)
-        if isinstance(node.ctx, (ast.Param, ast.Store)):
+
+        if isinstance(node.ctx, (ast.Load, ast.Del)):
+            node_in_chains = node in self.chains
+            if node_in_chains:
+                dnode = self.chains[node]
+            else:
+                dnode = Def(node)
+
+            # Extra linting for Del context: it is invalid to delete a
+            # global name from local scope unless it's marked global
+            if isinstance(node.ctx, ast.Del) and len(self._scopes) > 1:
+                non_local_definitions = self._definitions[0:-sum(self._scope_depths[:-1])]
+                for d in self.defs(node):
+                    if isinstance(getattr(d.node, 'ctx', None), ast.Store):
+                        if any(d in defs.get(node.id, ()) for defs in non_local_definitions):
+                            if not self.is_global(node.id) and not self.is_nonlocal(node.id):
+                                self.warn("deleting unreachable variable", node)
+                                continue
+                    d.add_user(dnode)
+            else:
+                for d in self.defs(node):
+                    d.add_user(dnode)
+            if not node_in_chains:
+                self.chains[node] = dnode
+
+        # Note that nodes with ast.Del context are considered as a Load (they
+        # actually read the identifier <> value binding) and as a Store (they
+        # somehow create a new binding from the identifier to an unbound value).
+        if isinstance(node.ctx, (ast.Param, ast.Store, ast.Del)):
             dnode = self.chains.setdefault(node, Def(node))
             # FIXME: find a smart way to merge the code below with add_to_locals
-            if any(node.id in _globals for _globals in self._globals):
+            if self.is_global(node.id) and not dnode.isdel():
                 self.set_or_extend_global(node.id, dnode)
             else:
                 # special code for warlus target: should be
@@ -1298,20 +1363,6 @@ def visit_Name(self, node, skip_annotation=False, named_expr=False):
             if getattr(node, 'annotation', None) is not None and not skip_annotation and not self.future_annotations:
                 self.visit(node.annotation)
 
-
-        elif isinstance(node.ctx, (ast.Load, ast.Del)):
-            node_in_chains = node in self.chains
-            if node_in_chains:
-                dnode = self.chains[node]
-            else:
-                dnode = Def(node)
-            for d in self.defs(node):
-                d.add_user(dnode)
-            if not node_in_chains:
-                self.chains[node] = dnode
-            # currently ignore the effect of a del
-        else:
-            raise NotImplementedError()
         return dnode
 
     def visit_Destructured(self, node):


Test patch:
diff --git a/tests/test_chains.py b/tests/test_chains.py
index daaa395..396f66d 100644
--- a/tests/test_chains.py
+++ b/tests/test_chains.py
@@ -547,6 +547,177 @@ def test_unbound_local_identifier_nonlocal_points_to_scoped_global(self):
         self.check_message(code,
                            ["W: unbound identifier 'x' at <unknown>:3:2"])
 
+    def test_deleted_identifier(self):
+        code = "x = 1; del x"
+        self.checkChains(code, ['x -> (x -> ())'])
+
+    def test_deleted_unknown_identifier(self):
+        code = "del x"
+        self.check_message(code,
+                           ["W: unbound identifier 'x' at <unknown>:1:4"])
+        self.checkChains(code, [], strict=False)
+
+    def test_deleted_identifier_redefined(self):
+        code = "x = 1; del x; x = 2"
+        self.checkChains(code, ['x -> (x -> ())', 'x -> ()'])
+
+    def test_unbound_deleted_identifier(self):
+        code = "x = 1; del x; x"
+        self.check_message(code,
+                           ["W: unbound identifier 'x' at <unknown>:1:14"])
+
+    def test_unbound_deleted_identifier_in_class(self):
+        code = "class X:\n x = 1\n del x\n x"
+        self.check_message(code,
+                           ["W: unbound identifier 'x' at <unknown>:4:1"])
+
+    def test_bound_deleted_identifier(self):
+        code = "x = 1; del x; x = 1; x"
+        self.check_message(code,
+                           [])
+
+    def test_del_in_for(self):
+        code = "for x in [1]:\n del x\nx"
+        self.check_message(code,
+                           ["W: unbound identifier 'x' at <unknown>:3:0"])
+
+    def test_del_predef_in_for(self):
+        code = "x = 1\nfor x in [1]:\n del x\nx"
+        self.check_message(code,
+                           [])
+
+    def test_bound_deleted_identifier_in_if(self):
+        code = "x = 1\ndel x\nif 1:\n x = 1\nx"
+        self.check_message(code,
+                           [])
+
+    def test_deleted_annotation(self):
+        # Passes pyright and mypy. But fails at runtime, because of the annotation X.
+        code = """\
+class X: ...
+del X
+def f() -> X: ..."""
+        self.check_message(code, ["W: unbound identifier 'X' at <unknown>:3:11"])
+
+    def test_deleted_pep649_deferred_annotation(self):
+        # This will not pass at runtime anymore starting at Python 3.14
+        code = """\
+class X: ...
+def f() -> X: ...
+del X
+f.__annotations__"""
+        self.check_message(code, [])
+
+    def test_deleted_pep563_deferred_annotation(self):
+        # Passes pyright and mypy and runtime
+        code = """\
+from __future__ import annotations
+class X: ...
+del X
+def f() -> X: ...
+f()"""
+        self.check_message(code, [])
+
+    def test_unsafe_use_in_function_before_deleted(self):
+        # Passes at runtime, but fairly unsafe.
+        code = """\
+class X: ...
+def f(): return X()
+f(); del X"""
+        self.check_message(code, ["W: unbound identifier 'X' at <unknown>:2:16"])
+
+    def test_use_in_function_after_deleted(self):
+        # Passes pyright and mypy but fails at runtime.
+        code = """\
+class X: ...
+def f(): return X()
+del X; f()"""
+        self.check_message(code, ["W: unbound identifier 'X' at <unknown>:2:16"])
+
+    def test_deleted_non_local_var(self):
+        code = """\
+def f():
+    v = 1
+    def q():
+        nonlocal v
+        del v
+        v # unbound
+    q()
+    v # not great but we must stay over approximated
+f()"""
+        self.check_message(code, ["W: unbound identifier 'v' at <unknown>:6:8"])
+
+    def test_cant_delete_nonlocal_not_declared_with_nonlocal_keyword(self):
+        code = """\
+def f():
+    v = 1
+    def q():
+        del v # unbound
+    q()
+    v # not great but we must stay over approximated
+f()"""
+
+        self.check_message(code, ["W: deleting unreachable variable at <unknown>:4:12"])
+        self.checkChains(code, ['f -> (f -> (<Call> -> ()))'], strict=False)
+
+    def test_deleted_global(self):
+        code = """\
+v = 1
+def q():
+    global v
+    del v
+    v # unbound
+q()
+v # not great but we must stay over approximated"""
+
+        self.check_message(code, ["W: unbound identifier 'v' at <unknown>:5:4"])
+        self.checkChains(code, ['v -> (v -> (), v -> ())',
+                                'q -> (q -> (<Call> -> ()))'], strict=False)
+
+    def test_cant_delete_global_not_declared_with_global_keyword(self):
+        code = """\
+v = 1
+def q():
+    del v # unbound
+q()
+v # not great but we must stay over approximated"""
+
+        self.check_message(code, ["W: deleting unreachable variable at <unknown>:3:8"])
+        self.checkChains(code, ['v -> (v -> ())',
+                                'q -> (q -> (<Call> -> ()))'], strict=False)
+
+    def test_maybe_unbound_in_if(self):
+        code = "def foo(x):\n  if x: del x\n  print(x)"
+        self.check_message(code,
+                           [])
+
+    def test_always_unbound_in_if(self):
+        code = "def foo(x):\n  if x: del x\n  else: del x\n  x"
+        self.check_message(code,
+                           ["W: unbound identifier 'x' at <unknown>:4:2"])
+
+    def test_delete_list_syntax(self):
+        code = "x,y = {}, 1; del x, y[0]; x, y"
+        self.check_message(code,
+                           ["W: unbound identifier 'x' at <unknown>:1:26"])
+
+    def test_redefined_global_deleted_lower_scope_var(self):
+        # test inspired by https://github.com/python/mypy/issues/9600
+        code = '''\
+x = 1
+class y:
+    @property(x)
+    def a(self):...
+    x = 2
+    @property(x)
+    def b(self):...
+    del x
+    @property(x)
+    def c(self):...
+'''
+        self.check_message(code, ["W: unbound identifier 'x' at <unknown>:9:14"])
+        self.checkChains(code, ['x -> (x -> (<Call> -> ()))', 'y -> ()'], strict=False)
+
     def test_assign_uses_class_level_name(self):
         code = '''
 visit_Name = object
diff --git a/tests/test_definitions.py b/tests/test_definitions.py
index 46171d6..982135d 100644
--- a/tests/test_definitions.py
+++ b/tests/test_definitions.py
@@ -346,10 +346,10 @@ def test_LocalAssign(self):
     def test_LocalAssignRedef(self):
         code = "def foo(a): a = 1"
         self.checkLocals(code, ["a", "a"])
-    
+
     def test_LocalAssignRedefIfElseOverride(self):
         code = """
-            def foo(): 
+            def foo():
                 if NotImplemented:
                     x = 2
                 else:
@@ -409,9 +409,9 @@ class TestDefIsLive(TestCase):
     ast = _gast
 
     def checkLocals(self, c, node, ref, only_live=False):
-        self.assertEqual(sorted(c._dump_locals(node, only_live=only_live)), 
+        self.assertEqual(sorted(c._dump_locals(node, only_live=only_live)),
                          sorted(ref))
-    
+
     def checkLiveLocals(self, code, livelocals, locals):
         node = self.ast.parse(dedent(code))
         c = StrictDefUseChains()
@@ -419,7 +419,7 @@ def checkLiveLocals(self, code, livelocals, locals):
         self.checkLocals(c, node, locals)
         self.checkLocals(c, node, livelocals, only_live=True)
         return node, c
-    
+
     def test_LocalAssignRedefIfElseOverride(self):
         code = """
             if NotImplemented:
@@ -429,7 +429,18 @@ def test_LocalAssignRedefIfElseOverride(self):
             x = 0
         """
         self.checkLiveLocals(code, ["x:6"], ["x:3,5,6"])
-    
+
+    def test_DeletedLocalAssignRedefIfElseOverride(self):
+        code = """
+            if NotImplemented:
+                x = 2
+            else:
+                x = 3
+            x = 0
+            del x
+        """
+        self.checkLiveLocals(code, [], ["x:3,5,6"])
+
     def test_LocalAssignmentRedefInEachBranch(self):
         code = """
         x = 10
@@ -440,6 +451,18 @@ def test_LocalAssignmentRedefInEachBranch(self):
         """
         self.checkLiveLocals(code, ["x:4,6"], ["x:2,4,6"])
 
+    def test_DeletedLocalAssignmentRedefInEachBranch(self):
+        code = """
+        x = 10
+        if NotImplemented:
+            x = 100
+            del x
+        else:
+            x = 1000
+            del x
+        """
+        self.checkLiveLocals(code, [], ["x:2,4,7"])
+
     def test_AssignmentInsideBothBranchesOfTryExcept(self):
         code = """
             try:
@@ -451,6 +474,31 @@ def test_AssignmentInsideBothBranchesOfTryExcept(self):
         """
         self.checkLiveLocals(code, ["x:5,7"], ["x:5,7"])
 
+    def test_BothDeletedAssignmentInsideBothBranchesOfTryExcept(self):
+        code = """
+            try:
+                1 / 0
+            except ZeroDivisionError:
+                x = 10
+                del x
+            except RuntimeError:
+                x = -1
+                del x
+        """
+        self.checkLiveLocals(code, [], ["x:5,8"])
+
+    def test_DeletedAssignmentInsideBothBranchesOfTryExcept(self):
+        code = """
+            try:
+                1 / 0
+            except ZeroDivisionError:
+                x = 10
+                del x
+            except RuntimeError:
+                x = -1
+        """
+        self.checkLiveLocals(code, ["x:8"], ["x:5,8"])
+
     def test_AssignmentOverrideFinallyBlock(self):
         code = """
             try:
@@ -463,7 +511,7 @@ def test_AssignmentOverrideFinallyBlock(self):
                 x = None
         """
         self.checkLiveLocals(code, ["x:9"], ["x:5,7,9"])
-    
+
     def test_AssignmentSimple(self):
         code = """
             a = 1
@@ -484,7 +532,7 @@ def test_BothLive(self):
             self.checkLiveLocals(code, ["sys:2", "_PY37PLUS:4,6"], ["sys:2", "_PY37PLUS:4,6"])
         else:
             self.checkLiveLocals(code, ["sys:None", "_PY37PLUS:4,6"], ["sys:None", "_PY37PLUS:4,6"])
-    
+
     def test_BuiltinNameRedefConditional(self):
         code = '''
         import sys
@@ -497,20 +545,29 @@ def exceptions(self):
                     pass
         '''
         if sys.version_info>=(3,10):
-            self.checkLiveLocals(code, ['sys:2', 'property:3', 'ExceptionGroup:6'], 
+            self.checkLiveLocals(code, ['sys:2', 'property:3', 'ExceptionGroup:6'],
                                 ['sys:2', 'property:3', 'ExceptionGroup:6'])
         else:
-            self.checkLiveLocals(code, ['sys:None', 'property:3', 'ExceptionGroup:6'], 
+            self.checkLiveLocals(code, ['sys:None', 'property:3', 'ExceptionGroup:6'],
                                 ['sys:None', 'property:3', 'ExceptionGroup:6'])
-    
+
     def test_loop_body_might_not_run(self):
         code = """
         i = 2
-        while int: 
+        while int:
             i = 3
         """
         self.checkLiveLocals(code, ['i:2,4'], ['i:2,4'])
-    
+
+    def test_loop_body_might_not_run_deletion(self):
+        code = """
+        i = 2
+        while int:
+            i = 3
+            del i
+        """
+        self.checkLiveLocals(code, ['i:2'], ['i:2,4'])
+
     def test_var_in_comp_doesnt_kill_upper_scope_var(self):
         code = '''
         x = True
@@ -532,19 +589,19 @@ def __init__(self):
         node, c = self.checkLiveLocals(code, ['v:1', 'C:2'], ['v:1', 'C:2'])
         self.checkLocals(c, node.body[-1], ['v:4', '__init__:5'], only_live=True)
         self.checkLocals(c, node.body[-1].body[-1], ['self:5', 'v:7'], only_live=True)
-    
+
     def test_if_body_might_not_run(self):
         code = """
         i = 2
-        if int: 
+        if int:
             i = 3
         """
         self.checkLiveLocals(code, ['i:2,4'], ['i:2,4'])
 
     def test_more_loops(self):
         # All variables here are live for beniget. Constant
-        # folding with control-flow understanding will reveal 
-        # that the else branch of the while loop is unreachable 
+        # folding with control-flow understanding will reveal
+        # that the else branch of the while loop is unreachable
         # (so the k assignment is never executed).
         # But beniget over-approximate this.
         code = '''
@@ -561,8 +618,8 @@ def test_more_loops(self):
             else:
                 k = 2
         '''
-        self.checkLiveLocals(code, ['b:2,6', 'v:9,10,4', 'k:10,13'],  
+        self.checkLiveLocals(code, ['b:2,6', 'v:9,10,4', 'k:10,13'],
                                 ['b:2,6', 'v:9,10,4', 'k:10,13'])
 
 class TestDefIsLiveStdlib(TestDefIsLive):
-    ast = _ast
\ No newline at end of file
+    ast = _ast