Merge pull request RustPython#855 from RustPython/nonlocal-errors

Add check for nonlocal on top level.

Author: cthulahoops

tests/snippets/global_nonlocal.py

@@ -1,3 +1,4 @@
+from testutils import assertRaises
 
 # Test global and nonlocal funkyness
 
@@ -11,6 +12,7 @@ def b():
 b()
 assert a == 4
 
+
 def x():
     def y():
         nonlocal b
@@ -21,3 +23,38 @@ def y():
 
 res = x()
 assert res == 3, str(res)
+
+# Invalid syntax:
+src = """
+b = 2
+global b
+"""
+
+with assertRaises(SyntaxError):
+    exec(src)
+
+# Invalid syntax:
+src = """
+nonlocal c
+"""
+
+with assertRaises(SyntaxError):
+    exec(src)
+
+
+# Invalid syntax:
+src = """
+def f():
+    def x():
+        nonlocal c
+c = 2
+"""
+
+with assertRaises(SyntaxError):
+    exec(src)
+
+
+# class X:
+#     nonlocal c
+#     c = 2
+

vm/src/compile.rs

@@ -41,17 +41,17 @@ pub fn compile(
     match mode {
         Mode::Exec => {
             let ast = parser::parse_program(source)?;
-            let symbol_table = make_symbol_table(&ast);
+            let symbol_table = make_symbol_table(&ast)?;
             compiler.compile_program(&ast, symbol_table)
         }
         Mode::Eval => {
             let statement = parser::parse_statement(source)?;
-            let symbol_table = statements_to_symbol_table(&statement);
+            let symbol_table = statements_to_symbol_table(&statement)?;
             compiler.compile_statement_eval(&statement, symbol_table)
         }
         Mode::Single => {
             let ast = parser::parse_program(source)?;
-            let symbol_table = make_symbol_table(&ast);
+            let symbol_table = make_symbol_table(&ast)?;
             compiler.compile_program_single(&ast, symbol_table)
         }
     }?;
@@ -1776,7 +1776,7 @@ mod tests {
         compiler.source_path = Some("source_path".to_string());
         compiler.push_new_code_object("<module>".to_string());
         let ast = parser::parse_program(&source.to_string()).unwrap();
-        let symbol_scope = make_symbol_table(&ast);
+        let symbol_scope = make_symbol_table(&ast).unwrap();
         compiler.compile_program(&ast, symbol_scope).unwrap();
         compiler.pop_code_object()
     }

vm/src/error.rs

@@ -29,6 +29,7 @@ pub enum CompileErrorType {
     ExpectExpr,
     /// Parser error
     Parse(ParseError),
+    SyntaxError(String),
     /// Multiple `*` detected
     StarArgs,
     /// Break statement outside of loop.
@@ -46,6 +47,7 @@ impl fmt::Display for CompileError {
             CompileErrorType::Delete(target) => write!(f, "can't delete {}", target),
             CompileErrorType::ExpectExpr => write!(f, "Expecting expression, got statement"),
             CompileErrorType::Parse(err) => write!(f, "{}", err),
+            CompileErrorType::SyntaxError(err) => write!(f, "{}", err),
             CompileErrorType::StarArgs => write!(f, "Two starred expressions in assignment"),
             CompileErrorType::InvalidBreak => write!(f, "'break' outside loop"),
             CompileErrorType::InvalidContinue => write!(f, "'continue' outside loop"),

vm/src/symboltable.rs

@@ -3,25 +3,35 @@
 This ensures that global and nonlocal keywords are picked up.
 Then the compiler can use the symbol table to generate proper
 load and store instructions for names.
+
+Inspirational file: https://github.com/python/cpython/blob/master/Python/symtable.c
 */
 
+use crate::error::{CompileError, CompileErrorType};
 use rustpython_parser::ast;
+use rustpython_parser::lexer::Location;
 use std::collections::HashMap;
 
-pub fn make_symbol_table(program: &ast::Program) -> SymbolScope {
+pub fn make_symbol_table(program: &ast::Program) -> Result<SymbolScope, SymbolTableError> {
     let mut builder = SymbolTableBuilder::new();
     builder.enter_scope();
-    builder.scan_program(program).unwrap();
+    builder.scan_program(program)?;
     assert!(builder.scopes.len() == 1);
-    builder.scopes.pop().unwrap()
+    let symbol_table = builder.scopes.pop().unwrap();
+    analyze_symbol_table(&symbol_table, None)?;
+    Ok(symbol_table)
 }
 
-pub fn statements_to_symbol_table(statements: &[ast::LocatedStatement]) -> SymbolScope {
+pub fn statements_to_symbol_table(
+    statements: &[ast::LocatedStatement],
+) -> Result<SymbolScope, SymbolTableError> {
     let mut builder = SymbolTableBuilder::new();
     builder.enter_scope();
-    builder.scan_statements(statements).unwrap();
+    builder.scan_statements(statements)?;
     assert!(builder.scopes.len() == 1);
-    builder.scopes.pop().unwrap()
+    let symbol_table = builder.scopes.pop().unwrap();
+    analyze_symbol_table(&symbol_table, None)?;
+    Ok(symbol_table)
 }
 
 #[derive(Debug)]
@@ -43,7 +53,22 @@ pub struct SymbolScope {
     pub sub_scopes: Vec<SymbolScope>,
 }
 
-type SymbolTableResult = Result<(), String>;
+#[derive(Debug)]
+pub struct SymbolTableError {
+    error: String,
+    location: Location,
+}
+
+impl From<SymbolTableError> for CompileError {
+    fn from(error: SymbolTableError) -> Self {
+        CompileError {
+            error: CompileErrorType::SyntaxError(error.error),
+            location: error.location,
+        }
+    }
+}
+
+type SymbolTableResult = Result<(), SymbolTableError>;
 
 impl SymbolScope {
     pub fn new() -> Self {
@@ -69,6 +94,58 @@ impl std::fmt::Debug for SymbolScope {
     }
 }
 
+/* Perform some sort of analysis on nonlocals, globals etc..
+  See also: https://github.com/python/cpython/blob/master/Python/symtable.c#L410
+*/
+fn analyze_symbol_table(
+    symbol_scope: &SymbolScope,
+    parent_symbol_scope: Option<&SymbolScope>,
+) -> SymbolTableResult {
+    // println!("Analyzing {:?}, parent={:?} symbols={:?}", symbol_scope,  parent_symbol_scope, symbol_scope.symbols);
+    // Analyze sub scopes:
+    for sub_scope in &symbol_scope.sub_scopes {
+        analyze_symbol_table(&sub_scope, Some(symbol_scope))?;
+    }
+
+    // Analyze symbols:
+    for (symbol_name, symbol_role) in &symbol_scope.symbols {
+        analyze_symbol(symbol_name, symbol_role, parent_symbol_scope)?;
+    }
+
+    Ok(())
+}
+
+fn analyze_symbol(
+    symbol_name: &str,
+    symbol_role: &SymbolRole,
+    parent_symbol_scope: Option<&SymbolScope>,
+) -> SymbolTableResult {
+    match symbol_role {
+        SymbolRole::Nonlocal => {
+            // check if name is defined in parent scope!
+            if let Some(parent_symbol_scope) = parent_symbol_scope {
+                if !parent_symbol_scope.symbols.contains_key(symbol_name) {
+                    return Err(SymbolTableError {
+                        error: format!("no binding for nonlocal '{}' found", symbol_name),
+                        location: Default::default(),
+                    });
+                }
+            } else {
+                return Err(SymbolTableError {
+                    error: format!(
+                        "nonlocal {} defined at place without an enclosing scope",
+                        symbol_name
+                    ),
+                    location: Default::default(),
+                });
+            }
+        }
+        // TODO: add more checks for globals
+        _ => {}
+    }
+    Ok(())
+}
+
 pub struct SymbolTableBuilder {
     // Scope stack.
     pub scopes: Vec<SymbolScope>,
@@ -85,7 +162,7 @@ impl SymbolTableBuilder {
         self.scopes.push(scope);
     }
 
-    pub fn leave_scope(&mut self) {
+    fn leave_scope(&mut self) {
         // Pop scope and add to subscopes of parent scope.
         let scope = self.scopes.pop().unwrap();
         self.scopes.last_mut().unwrap().sub_scopes.push(scope);
@@ -445,17 +522,43 @@ impl SymbolTableBuilder {
     }
 
     fn register_name(&mut self, name: &str, role: SymbolRole) -> SymbolTableResult {
+        let scope_depth = self.scopes.len();
         let current_scope = self.scopes.last_mut().unwrap();
+        let location = Default::default();
         if let Some(_old_role) = current_scope.symbols.get(name) {
             // Role already set..
             // debug!("TODO: {:?}", old_role);
             match role {
-                SymbolRole::Global => return Err("global must appear first".to_string()),
+                SymbolRole::Global => {
+                    return Err(SymbolTableError {
+                        error: format!("name '{}' is used prior to global declaration", name),
+                        location,
+                    })
+                }
+                SymbolRole::Nonlocal => {
+                    return Err(SymbolTableError {
+                        error: format!("name '{}' is used prior to nonlocal declaration", name),
+                        location,
+                    })
+                }
                 _ => {
                     // Ok?
                 }
             }
         } else {
+            match role {
+                SymbolRole::Nonlocal => {
+                    if scope_depth < 2 {
+                        return Err(SymbolTableError {
+                            error: format!("cannot define nonlocal '{}' at top level.", name),
+                            location,
+                        });
+                    }
+                }
+                _ => {
+                    // Ok!
+                }
+            }
             current_scope.symbols.insert(name.to_string(), role);
         }
         Ok(())