Improve some behavior around global handling (#1154)

src/check_ast.rs

@@ -236,10 +236,28 @@ where
             StmtKind::Global { names } => {
                 let scope_index = *self.scope_stack.last().expect("No current scope found");
                 if scope_index != GLOBAL_SCOPE_INDEX {
+                    // If the binding doesn't already exist in the global scope, add it.
+                    for name in names {
+                        if !self.scopes[GLOBAL_SCOPE_INDEX]
+                            .values
+                            .contains_key(&name.as_str())
+                        {
+                            let index = self.bindings.len();
+                            self.bindings.push(Binding {
+                                kind: BindingKind::Assignment,
+                                used: None,
+                                range: Range::from_located(stmt),
+                                source: None,
+                                redefined: vec![],
+                            });
+                            self.scopes[GLOBAL_SCOPE_INDEX].values.insert(name, index);
+                        }
+                    }
+
+                    // Add the binding to the current scope.
                     let scope = &mut self.scopes[scope_index];
                     let usage = Some((scope.id, Range::from_located(stmt)));
                     for name in names {
-                        // Add a binding to the current scope.
                         let index = self.bindings.len();
                         self.bindings.push(Binding {
                             kind: BindingKind::Global,
@@ -250,15 +268,6 @@ where
                         });
                         scope.values.insert(name, index);
                     }
-
-                    // Mark the binding in the global scope as used.
-                    for name in names {
-                        if let Some(index) =
-                            self.scopes[GLOBAL_SCOPE_INDEX].values.get(&name.as_str())
-                        {
-                            self.bindings[*index].used = usage;
-                        }
-                    }
                 }
 
                 if self.settings.enabled.contains(&CheckCode::E741) {
@@ -2948,6 +2957,7 @@ impl<'a> Checker<'a> {
     }
 
     fn check_deferred_type_definitions(&mut self) {
+        self.deferred_type_definitions.reverse();
         while let Some((expr, in_annotation, (scopes, parents))) =
             self.deferred_type_definitions.pop()
         {
@@ -2967,6 +2977,7 @@ impl<'a> Checker<'a> {
         'b: 'a,
     {
         let mut stacks = vec![];
+        self.deferred_string_type_definitions.reverse();
         while let Some((range, expression, in_annotation, context)) =
             self.deferred_string_type_definitions.pop()
         {
@@ -2996,6 +3007,7 @@ impl<'a> Checker<'a> {
     }
 
     fn check_deferred_functions(&mut self) {
+        self.deferred_functions.reverse();
         while let Some((stmt, (scopes, parents), visibility)) = self.deferred_functions.pop() {
             self.scope_stack = scopes.clone();
             self.parents = parents.clone();
@@ -3041,6 +3053,7 @@ impl<'a> Checker<'a> {
     }
 
     fn check_deferred_lambdas(&mut self) {
+        self.deferred_lambdas.reverse();
         while let Some((expr, (scopes, parents))) = self.deferred_lambdas.pop() {
             self.scope_stack = scopes.clone();
             self.parents = parents.clone();
@@ -3063,6 +3076,7 @@ impl<'a> Checker<'a> {
     }
 
     fn check_deferred_assignments(&mut self) {
+        self.deferred_assignments.reverse();
         while let Some((index, (scopes, _parents))) = self.deferred_assignments.pop() {
             if self.settings.enabled.contains(&CheckCode::F841) {
                 self.add_checks(
@@ -3105,7 +3119,12 @@ impl<'a> Checker<'a> {
         }
 
         let mut checks: Vec<Check> = vec![];
-        for scope in self.dead_scopes.iter().map(|index| &self.scopes[*index]) {
+        for scope in self
+            .dead_scopes
+            .iter()
+            .rev()
+            .map(|index| &self.scopes[*index])
+        {
             let all_binding: Option<&Binding> = scope
                 .values
                 .get("__all__")

src/pyflakes/mod.rs

@@ -392,11 +392,9 @@ mod tests {
         Ok(())
     }
 
-    // TODO(charlie): Bubble global assignments up to the module scope.
-    #[ignore]
     #[test]
     fn defined_by_global() -> Result<()> {
-        // global" can make an otherwise undefined name in another function
+        // "global" can make an otherwise undefined name in another function
         // defined.
         flakes(
             r#"
@@ -405,21 +403,20 @@ mod tests {
         "#,
             &[],
         )?;
-        flakes(
+        // Pyflakes allows this, but it causes other issues.
-            r#"
+        // flakes(
-        def c(): bar
+        //     r#"
-        def b(): global bar; bar = 1
+        // def c(): bar
-        "#,
+        // def b(): global bar; bar = 1
-            &[],
+        // "#,
-        )?;
+        //     &[],
+        // )?;
         Ok(())
     }
 
-    // TODO(charlie): Bubble global assignments up to the module scope.
-    #[ignore]
     #[test]
     fn defined_by_global_multiple_names() -> Result<()> {
-        // global" can accept multiple names.
+        // "global" can accept multiple names.
         flakes(
             r#"
         def a(): global fu, bar; fu = 1; bar = 2
@@ -1390,9 +1387,9 @@ mod tests {
                     pass
         "#,
             &[
-                CheckCode::F401,
                 CheckCode::F401,
                 CheckCode::F811,
+                CheckCode::F401,
                 CheckCode::F811,
             ],
         )?;
@@ -1981,7 +1978,6 @@ mod tests {
         Ok(())
     }
 
-    #[ignore]
     #[test]
     fn used_in_global() -> Result<()> {
         // A 'global' statement shadowing an unused import should not prevent it
@@ -2013,7 +2009,6 @@ mod tests {
         Ok(())
     }
 
-    #[ignore]
     #[test]
     fn assigned_to_global() -> Result<()> {
         // Binding an import to a declared global should not cause it to be