Repository: serge-sans-paille/beniget

Issue:
Scoping rule not correctly honored for type annotation
There is no ambiguity in the following code:

```
class Attr:...
class Cls:
    def Attr(self) -> Attr:...
```

but beniget issues a warning about `-> Attr`  being potentially unbound at runtime.

Discussion:

Patch:
diff --git a/beniget/beniget.py b/beniget/beniget.py
index 6b2cc20..dd77efc 100644
--- a/beniget/beniget.py
+++ b/beniget/beniget.py
@@ -173,7 +173,6 @@ class CollectLocals(ast.NodeVisitor):
 
     def visit_FunctionDef(self, node):
         self.Locals.add(node.name)
-        # no recursion
 
     visit_AsyncFunctionDef = visit_FunctionDef
 
@@ -208,6 +207,11 @@ class CollectLocals(ast.NodeVisitor):
 
 
 def collect_locals(node):
+    '''
+    Compute the set of identifiers local to a given node.
+
+    This is meant to emulate a call to locals()
+    '''
     visitor = CollectLocals()
     visitor.generic_visit(node)
     return visitor.Locals
@@ -271,11 +275,12 @@ class DefUseChains(ast.NodeVisitor):
         self._breaks = []
         self._continues = []
 
-        # dead code levels
-        self.deadcode = 0
-
-    # helpers
+        # dead code levels, it's non null for code that cannot be executed
+        self._deadcode = 0
 
+    #
+    ## helpers
+    #
     def dump_definitions(self, node, ignore_builtins=True):
         if isinstance(node, ast.Module) and not ignore_builtins:
             builtins = {d for d in self._builtins.values()}
@@ -305,12 +310,35 @@ class DefUseChains(ast.NodeVisitor):
         location = self.location(node)
         print("W: unbound identifier '{}'{}".format(name, location))
 
-    def maybe_unbound_identifier(self, name, node):
-        location = self.location(node)
-        print("W: identifier '{}' may be unbound at runtime{}".format(name, location))
+    def invalid_name_lookup(self, name, scope, precomputed_locals, local_defs):
+        # We may hit the situation where we refer to a local variable which is
+        # not bound yet. This is a runtime error in Python, so we try to detec
+        # it statically.
 
+        # not a local variable => fine
+        if name not in precomputed_locals:
+            return
+
+        # It's meant to be a local, but can we resolve it by a local lookup?
+        islocal = any((name in defs or '*' in defs) for defs in local_defs)
+
+        # At class scope, it's ok to refer to a global even if we also have a
+        # local definition for that variable. Stated other wise
+        #
+        # >>> a = 1
+        # >>> def foo(): a = a
+        # >>> foo() # fails, a is a local referenced before being assigned
+        # >>> class bar: a = a
+        # >>> bar() # ok, and `bar.a is a`
+        if isinstance(scope, ast.ClassDef):
+            top_level_definitions = self._definitions[0:-self._scope_depths[0]]
+            isglobal = any((name in top_lvl_def or '*' in top_lvl_def)
+                           for top_lvl_def in top_level_definitions)
+            return not islocal and not isglobal
+        else:
+            return not islocal
 
-    def defs(self, node):
+    def defs(self, node, quiet=False):
         '''
         Performs an actual lookup of node's id in current context, returning
         the list of def linked to that use.
@@ -320,7 +348,7 @@ class DefUseChains(ast.NodeVisitor):
 
         # 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:1]
+            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
@@ -329,25 +357,34 @@ class DefUseChains(ast.NodeVisitor):
 
             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)
-            looked_up_definitions.extend(reversed(self._definitions[depth:]))
-            _ = next(scopes_iter)
-
-            # Iterate over scopes, filtering out class scopes.
-            for scope, depth in zip(scopes_iter, depths_iter):
-                if not isinstance(scope, ast.ClassDef):
-                    looked_up_definitions.extend(
-                            reversed(self._definitions[lvl + depth: lvl]))
-                lvl += depth
-
-        for d in looked_up_definitions:
-            if name in d:
-                return d[name] if not stars else stars + list(d[name])
-            if "*" in d:
-                stars.extend(d["*"])
+            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))
+
+                # 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.clear()
+                            break
+                        looked_up_definitions.extend(reversed(defs))
+                    lvl += depth
+
+        for defs in looked_up_definitions:
+            if name in defs:
+                return defs[name] if not stars else stars + list(defs[name])
+            if "*" in defs:
+                stars.extend(defs["*"])
 
         d = self.chains.setdefault(node, Def(node))
 
@@ -357,7 +394,7 @@ class DefUseChains(ast.NodeVisitor):
         if stars:
             return stars + [d]
         else:
-            if not self._undefs:
+            if not self._undefs and not quiet:
                 self.unbound_identifier(name, node)
             return [d]
 
@@ -367,10 +404,10 @@ class DefUseChains(ast.NodeVisitor):
             if isinstance(stmt, (ast.Break, ast.Continue, ast.Raise)):
                 if not deadcode:
                     deadcode = True
-                    self.deadcode += 1
+                    self._deadcode += 1
             self.visit(stmt)
         if deadcode:
-            self.deadcode -= 1
+            self._deadcode -= 1
 
     def process_undefs(self):
         for undef_name, _undefs in self._undefs[-1].items():
@@ -385,6 +422,7 @@ class DefUseChains(ast.NodeVisitor):
                         self.unbound_identifier(undef_name, undef.node)
         self._undefs.pop()
 
+
     @contextmanager
     def ScopeContext(self, node):
         self._scopes.append(node)
@@ -415,14 +453,16 @@ class DefUseChains(ast.NodeVisitor):
         self._definitions.pop()
 
     @contextmanager
-    def SwitchScopeContext(self, defs, scopes, scope_depths):
+    def SwitchScopeContext(self, defs, scopes, scope_depths, precomputed_locals):
         scope_depths, self._scope_depths = self._scope_depths, scope_depths
         scopes, self._scopes = self._scopes, scopes
         defs, self._definitions = self._definitions, defs
+        precomputed_locals, self._precomputed_locals = self._precomputed_locals, precomputed_locals
         yield
         self._definitions = defs
         self._scopes = scopes
         self._scope_depths = scope_depths
+        self._precomputed_locals = precomputed_locals
 
 
     # stmt
@@ -437,10 +477,11 @@ class DefUseChains(ast.NodeVisitor):
             self.process_body(node.body)
 
             # handle function bodies
-            for fnode, defs, scopes, scope_depths in self._defered:
+            for fnode, defs, scopes, scope_depths, precomputed_locals in self._defered:
                 visitor = getattr(self,
                                   "visit_{}".format(type(fnode).__name__))
-                with self.SwitchScopeContext(defs, scopes, scope_depths):
+                with self.SwitchScopeContext(defs, scopes, scope_depths,
+                                             precomputed_locals):
                     visitor(fnode, step=DefinitionStep)
 
             # various sanity checks
@@ -461,9 +502,10 @@ class DefUseChains(ast.NodeVisitor):
         assert not self._definitions
         assert not self._scopes
         assert not self._scope_depths
+        assert not self._precomputed_locals
 
     def set_definition(self, name, dnode_or_dnodes):
-        if self.deadcode:
+        if self._deadcode:
             return
         if isinstance(dnode_or_dnodes, Def):
             self._definitions[-1][name] = ordered_set((dnode_or_dnodes,))
@@ -478,19 +520,19 @@ class DefUseChains(ast.NodeVisitor):
             definition[name].update(dnode_or_dnodes)
 
     def extend_definition(self, name, dnode_or_dnodes):
-        if self.deadcode:
+        if self._deadcode:
             return
         DefUseChains.add_to_definition(self._definitions[-1], name,
                                        dnode_or_dnodes)
 
     def extend_global(self, name, dnode_or_dnodes):
-        if self.deadcode:
+        if self._deadcode:
             return
         DefUseChains.add_to_definition(self._definitions[0], name,
                                        dnode_or_dnodes)
 
     def set_or_extend_global(self, name, dnode):
-        if self.deadcode:
+        if self._deadcode:
             return
         if name not in self._definitions[0]:
             self.locals[self.module].append(dnode)
@@ -537,7 +579,8 @@ class DefUseChains(ast.NodeVisitor):
             self._defered.append((node,
                                   list(self._definitions),
                                   list(self._scopes),
-                                  list(self._scope_depths)))
+                                  list(self._scope_depths),
+                                  list(self._precomputed_locals)))
         elif step is DefinitionStep:
             with self.ScopeContext(node):
                 for arg in node.args.args:
@@ -617,7 +660,8 @@ class DefUseChains(ast.NodeVisitor):
             if any(node.target.id in _globals for _globals in self._globals):
                 self.extend_global(node.target.id, dtarget)
             else:
-                loaded_from = [d.name() for d in self.defs(node.target)]
+                loaded_from = [d.name() for d in self.defs(node.target,
+                                                           quiet=True)]
                 self.set_definition(node.target.id, dtarget)
                 # If we augassign from a value that comes from '*', let's use
                 # this node as the definition point.
@@ -996,25 +1040,15 @@ class DefUseChains(ast.NodeVisitor):
                 self.visit(node.annotation)
 
         elif isinstance(node.ctx, (ast.Load, ast.Del)):
-            # We hit the situation where we refer to a local variable that's not
-            # bound yet. This is a runtime error in Python, so we issue a warning.
-            # Note that because we may be in a condition, it's just a *may* and it
-            # *may* be fine to ignore this warning.
-            current_locals = self._precomputed_locals[-1]
-            if node.id in current_locals and not self.is_in_current_scope(node.id):
-                self.maybe_unbound_identifier(node.id, node)
+            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
-            else:
-                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()


Test patch:
diff --git a/tests/chains.py b/tests/chains.py
index c477328..f82abc1 100644
--- a/tests/chains.py
+++ b/tests/chains.py
@@ -94,6 +94,12 @@ class TestDefUseChains(TestCase):
         code = "a = 1; a += 2; a"
         self.checkChains(code, ['a -> (a -> (a -> ()))'])
 
+    def test_read_global_from_branch(self):
+        code = "if 1: a = 1\ndef foo():\n def bar(): global a; return a"
+        self.checkChains(code, ['a -> (a -> ())',
+                                'foo -> ()'])
+
+
     def test_expanded_augassign(self):
         code = "a = 1; a = a + 2"
         self.checkChains(code, ["a -> (a -> (BinOp -> ()))", "a -> ()"])
@@ -344,10 +350,17 @@ while done:
         code = "from some import decorator\n@decorator\nclass C:pass"
         self.checkChains(code, ["decorator -> (decorator -> (C -> ()))", "C -> ()"])
 
+    def test_class_base(self):
+        code = "class A:...\nclass B(A):..."
+        self.checkChains(code, ["A -> (A -> (B -> ()))", "B -> ()"])
+
     @skipIf(sys.version_info.major < 3, "Python 3 syntax")
     def test_def_used_in_self_default(self):
         code = "def foo(x:foo): return foo"
-        self.checkChains(code, ["foo -> (foo -> ())"])
+        c = beniget.DefUseChains()
+        node = ast.parse(code)
+        c.visit(node)
+        self.assertEqual(c.dump_chains(node), ["foo -> (foo -> ())"])
 
     def test_unbound_class_variable(self):
         code = '''
@@ -361,7 +374,7 @@ def middle():
         node = ast.parse(code)
         c.visit(node)
         self.assertEqual(c.dump_chains(node.body[0]), ['x -> (x -> ())', 'mytype -> ()'])
-    
+
     def test_unbound_class_variable2(self):
         code = '''class A:\n  a = 10\n  def f(self):\n    return a # a is not defined'''
         c = beniget.DefUseChains()
@@ -375,7 +388,7 @@ def middle():
         node = ast.parse(code)
         c.visit(node)
         self.assertEqual(c.dump_chains(node.body[0]), ['a -> ()', 'I -> ()'])
-    
+
     def test_unbound_class_variable4(self):
         code = '''class A:\n  a = 10\n  f = lambda: a # a is not defined'''
         c = beniget.DefUseChains()
@@ -400,7 +413,7 @@ def middle():
         self.checkChains(code, ['x -> (x -> ())', 'foo -> ()'])
 
     @skipIf(sys.version_info.major < 3, "Python 3 syntax")
-    def test_class_annotation(self):
+    def test_arg_annotation(self):
         code = "type_ = int\ndef foo(bar: type_): pass"
         self.checkChains(code, ["type_ -> (type_ -> ())", "foo -> ()"])
 
@@ -454,22 +467,97 @@ def outer():
     def test_maybe_unbound_identifier_message_format(self):
         code = "x = 1\ndef foo(): y = x; x = 2"
         self.check_message(code,
-                           ["'x' may be unbound at runtime at <unknown>:2"])
+                           ["unbound identifier 'x' at <unknown>:2"])
 
     def test_unbound_local_identifier_in_func(self):
         code = "def A():\n x = 1\n class B: x = x"
         self.check_message(code,
-                           ["'x' may be unbound at runtime at <unknown>:3"])
+                           ["unbound identifier 'x' at <unknown>:3"])
+
+    @skipIf(sys.version_info < (3, 0), 'Python 3 syntax')
+    def test_unbound_local_identifier_in_method(self):
+        code = "class A:pass\nclass B:\n def A(self) -> A:pass"
+        self.check_message(code, [])
 
     @skipIf(sys.version_info < (3, 0), 'Python 3 syntax')
     def test_unbound_local_identifier_nonlocal(self):
         code = "def A():\n x = 1\n class B: nonlocal x; x = x"
         self.check_message(code, [])
 
+
+    def test_assign_uses_class_level_name(self):
+        code = '''
+visit_Name = object
+class Visitor:
+    def visit_Name(self, node):...
+    visit_Attribute = visit_Name
+'''
+        node = ast.parse(code)
+        c = beniget.DefUseChains()
+        c.visit(node)
+        self.assertEqual(c.dump_chains(node), ['visit_Name -> ()',
+                                               'Visitor -> ()'])
+        self.assertEqual(c.dump_chains(node.body[-1]),
+                         ['visit_Name -> (visit_Name -> ())',
+                          'visit_Attribute -> ()'])
+
+
+    def test_base_class_uses_class_level_same_name(self):
+            code = '''
+class Attr(object):...
+class Visitor:
+    class Attr(Attr):...
+    '''
+            node = ast.parse(code)
+            c = beniget.DefUseChains()
+            c.visit(node)
+            self.assertEqual(c.dump_chains(node),
+                             ['Attr -> (Attr -> (Attr -> ()))',
+                              'Visitor -> ()'])
+            self.assertEqual(c.dump_chains(node.body[-1]), ['Attr -> ()'])
+
+
+    @skipIf(sys.version_info < (3, 0), 'Python 3 syntax')
+    def test_annotation_uses_class_level_name(self):
+        code = '''
+Thing = object
+def f():...
+class Visitor:
+    Thing = bytes
+    def f(): return f()
+    def visit_Name(self, node:Thing, fn:f):...
+'''
+        node = ast.parse(code)
+        c = beniget.DefUseChains()
+        c.visit(node)
+        self.assertEqual(c.dump_chains(node),
+                         ['Thing -> ()',
+                          'f -> (f -> (Call -> ()))',
+                          'Visitor -> ()'])
+        self.assertEqual(c.dump_chains(node.body[-1]),
+                         ['Thing -> (Thing -> ())',
+                          'f -> (f -> ())',
+                          'visit_Name -> ()'])
+
+    def test_assign_uses_class_level_same_name(self):
+        code = '''
+def visit_Attribute(self, node):...
+class Visitor:
+    visit_Attribute = visit_Attribute
+'''
+        node = ast.parse(code)
+        c = beniget.DefUseChains()
+        c.visit(node)
+        self.assertEqual(c.dump_chains(node),
+            ['visit_Attribute -> (visit_Attribute -> ())',
+             'Visitor -> ()'])
+        self.assertEqual(c.dump_chains(node.body[-1]),
+                         ['visit_Attribute -> ()'])
+
     def test_unbound_local_identifier_in_augassign(self):
         code = "def A():\n x = 1\n class B: x += 1"
         self.check_message(code,
-                           ["'x' may be unbound at runtime at <unknown>:3"])
+                           ["unbound identifier 'x' at <unknown>:3"])
 
     def test_star_import_with_conditional_redef(self):
         code = '''