Implement Py38 named expression (PEP 572) (RustPython#1934)

* Initial implementation of named expression and  import  according CPython tests

* added new instruction with inversed evaluation order for dict comprehension, in other cases use regular evaluation order

* added further aspects to implementation, cleaned up, imported test from CPython

* implemented first parts of scoping enhancement and extended checks

* completion of name resolution ongoing, now more test passing, still warinings and cleanup required

* further optimization of name resolution in nested scopes

* Initialize the vm with imports from _io instead of io

* Add the OpenBSD support that I am smart enough to

* adapted grammer to full support, most test are passing now, esp. all invalids are passed. Cleaned up in symboltable

* more conditional compiling, this time for errors

* rustfmt was not pleased

* premature push, whoops

* Add expected_failure result type to jsontests

* Initial implementation of named expression and  import  according CPython tests

* added new instruction with inversed evaluation order for dict comprehension, in other cases use regular evaluation order

* added further aspects to implementation, cleaned up, imported test from CPython

* implemented first parts of scoping enhancement and extended checks

* completion of name resolution ongoing, now more test passing, still warinings and cleanup required

* further optimization of name resolution in nested scopes

* adapted grammer to full support, most test are passing now, esp. all invalids are passed. Cleaned up in symboltable

* Fixed nameing convention violation and removed unnecessary information from symbol resolution. Added some more comments.

* Fixed nameing convention violation and removed unnecessary information from symbol resolution. Added some more comments.

Co-authored-by: Noah <[email protected]>
Co-authored-by: Reuben Staley <[email protected]>

Author: 3 people

Lib/test/test_named_expression.py

@@ -273,6 +273,7 @@ pub enum Instruction {
     MapAdd {
         i: usize,
     },
+
     PrintExpr,
     LoadBuildClass,
     UnpackSequence {
@@ -296,6 +297,13 @@ pub enum Instruction {
     },
     GetAIter,
     GetANext,
+
+    /// Reverse order evaluation in MapAdd
+    /// required to support named expressions of Python 3.8 in dict comprehension
+    /// today (including Py3.9) only required in dict comprehension.
+    MapAddRev {
+        i: usize,
+    },
 }
 
 use self::Instruction::*;
@@ -586,7 +594,7 @@ impl Instruction {
             BuildSlice { size } => w!(BuildSlice, size),
             ListAppend { i } => w!(ListAppend, i),
             SetAdd { i } => w!(SetAdd, i),
-            MapAdd { i } => w!(MapAdd, i),
+            MapAddRev { i } => w!(MapAddRev, i),
             PrintExpr => w!(PrintExpr),
             LoadBuildClass => w!(LoadBuildClass),
             UnpackSequence { size } => w!(UnpackSequence, size),
@@ -597,6 +605,7 @@ impl Instruction {
             GetAwaitable => w!(GetAwaitable),
             GetAIter => w!(GetAIter),
             GetANext => w!(GetANext),
+            MapAdd { i } => w!(MapAdd, i),
         }
     }
 }

bytecode/src/bytecode.rs

@@ -1804,6 +1804,12 @@ impl<O: OutputStream> Compiler<O> {
                 // End
                 self.set_label(end_label);
             }
+
+            NamedExpression { left, right } => {
+                self.compile_expression(right)?;
+                self.emit(Instruction::Duplicate);
+                self.compile_store(left)?;
+            }
         }
         Ok(())
     }
@@ -2066,10 +2072,11 @@ impl<O: OutputStream> Compiler<O> {
                 });
             }
             ast::ComprehensionKind::Dict { key, value } => {
-                self.compile_expression(value)?;
+                // changed evaluation order for Py38 named expression PEP 572
                 self.compile_expression(key)?;
+                self.compile_expression(value)?;
 
-                self.emit(Instruction::MapAdd {
+                self.emit(Instruction::MapAddRev {
                     i: 1 + generators.len(),
                 });
             }

compiler/src/compile.rs

@@ -66,6 +66,7 @@ pub enum SymbolTableType {
     Module,
     Class,
     Function,
+    Comprehension,
 }
 
 impl fmt::Display for SymbolTableType {
@@ -74,6 +75,7 @@ impl fmt::Display for SymbolTableType {
             SymbolTableType::Module => write!(f, "module"),
             SymbolTableType::Class => write!(f, "class"),
             SymbolTableType::Function => write!(f, "function"),
+            SymbolTableType::Comprehension => write!(f, "comprehension"),
         }
     }
 }
@@ -99,6 +101,10 @@ pub struct Symbol {
     pub is_assigned: bool,
     pub is_parameter: bool,
     pub is_free: bool,
+
+    // indicates if the symbol gets a value assigned by a named expression in a comprehension
+    // this is required to correct the scope in the analysis.
+    pub is_assign_namedexpr_in_comprehension: bool,
 }
 
 impl Symbol {
@@ -111,6 +117,7 @@ impl Symbol {
             is_assigned: false,
             is_parameter: false,
             is_free: false,
+            is_assign_namedexpr_in_comprehension: false,
         }
     }
 
@@ -193,72 +200,138 @@ impl<'a> SymbolTableAnalyzer<'a> {
         for sub_table in sub_tables {
             self.analyze_symbol_table(sub_table)?;
         }
-        let (symbols, _) = self.tables.pop().unwrap();
+        let (symbols, st_typ) = self.tables.pop().unwrap();
 
         // Analyze symbols:
         for symbol in symbols.values_mut() {
-            self.analyze_symbol(symbol)?;
+            self.analyze_symbol(symbol, st_typ)?;
         }
-
         Ok(())
     }
 
-    fn analyze_symbol(&self, symbol: &mut Symbol) -> SymbolTableResult {
-        match symbol.scope {
-            SymbolScope::Nonlocal => {
-                // check if name is defined in parent table!
-                let parent_symbol_table = self.tables.last();
-                // symbol.table.borrow().parent.clone();
-
-                if let Some((symbols, _)) = parent_symbol_table {
-                    let scope_depth = self.tables.len();
-                    if !symbols.contains_key(&symbol.name) || scope_depth < 2 {
+    fn analyze_symbol(
+        &mut self,
+        symbol: &mut Symbol,
+        curr_st_typ: SymbolTableType,
+    ) -> SymbolTableResult {
+        if symbol.is_assign_namedexpr_in_comprehension
+            && curr_st_typ == SymbolTableType::Comprehension
+        {
+            self.analyze_symbol_comprehension(symbol, 0)?
+        } else {
+            match symbol.scope {
+                SymbolScope::Nonlocal => {
+                    // check if name is defined in parent table!
+                    let parent_symbol_table = self.tables.last();
+                    if let Some((symbols, _)) = parent_symbol_table {
+                        let scope_depth = self.tables.len();
+                        if !symbols.contains_key(&symbol.name) || scope_depth < 2 {
+                            return Err(SymbolTableError {
+                                error: format!("no binding for nonlocal '{}' found", symbol.name),
+                                location: Default::default(),
+                            });
+                        }
+                    } else {
                         return Err(SymbolTableError {
-                            error: format!("no binding for nonlocal '{}' found", symbol.name),
+                            error: format!(
+                                "nonlocal {} defined at place without an enclosing scope",
+                                symbol.name
+                            ),
                             location: Default::default(),
                         });
                     }
-                } else {
-                    return Err(SymbolTableError {
-                        error: format!(
-                            "nonlocal {} defined at place without an enclosing scope",
-                            symbol.name
-                        ),
-                        location: Default::default(),
-                    });
+                }
+                SymbolScope::Global => {
+                    // TODO: add more checks for globals?
+                }
+                SymbolScope::Local => {
+                    // all is well
+                }
+                SymbolScope::Unknown => {
+                    // Try hard to figure out what the scope of this symbol is.
+                    self.analyze_unknown_symbol(symbol);
                 }
             }
-            SymbolScope::Global => {
-                // TODO: add more checks for globals?
-            }
-            SymbolScope::Local => {
-                // all is well
+        }
+        Ok(())
+    }
+
+    fn analyze_unknown_symbol(&self, symbol: &mut Symbol) {
+        if symbol.is_assigned || symbol.is_parameter {
+            symbol.scope = SymbolScope::Local;
+        } else {
+            // Interesting stuff about the __class__ variable:
+            // https://docs.python.org/3/reference/datamodel.html?highlight=__class__#creating-the-class-object
+            let found_in_outer_scope = symbol.name == "__class__"
+                || self.tables.iter().skip(1).any(|(symbols, typ)| {
+                    *typ != SymbolTableType::Class && symbols.contains_key(&symbol.name)
+                });
+
+            if found_in_outer_scope {
+                // Symbol is in some outer scope.
+                symbol.is_free = true;
+            } else if self.tables.is_empty() {
+                // Don't make assumptions when we don't know.
+                symbol.scope = SymbolScope::Unknown;
+            } else {
+                // If there are scopes above we can assume global.
+                symbol.scope = SymbolScope::Global;
             }
-            SymbolScope::Unknown => {
-                // Try hard to figure out what the scope of this symbol is.
+        }
+    }
 
-                if symbol.is_assigned || symbol.is_parameter {
-                    symbol.scope = SymbolScope::Local;
-                } else {
-                    // Interesting stuff about the __class__ variable:
-                    // https://docs.python.org/3/reference/datamodel.html?highlight=__class__#creating-the-class-object
-                    let found_in_outer_scope = symbol.name == "__class__"
-                        || self.tables.iter().skip(1).any(|(symbols, typ)| {
-                            *typ != SymbolTableType::Class && symbols.contains_key(&symbol.name)
-                        });
+    // Implements the symbol analysis and scope extension for names
+    // assigned by a named expression in a comprehension. See:
+    // https://github.com/python/cpython/blob/7b78e7f9fd77bb3280ee39fb74b86772a7d46a70/Python/symtable.c#L1435
+    fn analyze_symbol_comprehension(
+        &mut self,
+        symbol: &mut Symbol,
+        parent_offset: usize,
+    ) -> SymbolTableResult {
+        // TODO: quite C-ish way to implement the iteration
+        // when this is called, we expect to be in the direct parent scope of the scope that contains 'symbol'
+        let offs = self.tables.len() - 1 - parent_offset;
+        let last = self.tables.get_mut(offs).unwrap();
+        let symbols = &mut last.0;
+        let table_type = last.1;
+
+        match table_type {
+            SymbolTableType::Module => {
+                symbol.scope = SymbolScope::Global;
+            }
+            SymbolTableType::Class => {}
+            SymbolTableType::Function => {
+                if let Some(parent_symbol) = symbols.get_mut(&symbol.name) {
+                    if let SymbolScope::Unknown = parent_symbol.scope {
+                        parent_symbol.is_assigned = true; // this information is new, as the asignment is done in inner scope
+                        self.analyze_unknown_symbol(symbol);
+                    }
 
-                    if found_in_outer_scope {
-                        // Symbol is in some outer scope.
-                        symbol.is_free = true;
-                    } else if self.tables.is_empty() {
-                        // Don't make assumptions when we don't know.
-                        symbol.scope = SymbolScope::Unknown;
-                    } else {
-                        // If there are scopes above we can assume global.
-                        symbol.scope = SymbolScope::Global;
+                    match symbol.scope {
+                        SymbolScope::Global => {
+                            symbol.scope = SymbolScope::Global;
+                        }
+                        _ => {
+                            symbol.scope = SymbolScope::Nonlocal;
+                        }
                     }
                 }
             }
+            SymbolTableType::Comprehension => {
+                // TODO check for conflicts - requires more context information about variables
+                match symbols.get_mut(&symbol.name) {
+                    Some(parent_symbol) => {
+                        parent_symbol.is_assigned = true; // more checks are required
+                    }
+                    None => {
+                        let cloned_sym = symbol.clone();
+
+                        last.0.insert(cloned_sym.name.to_owned(), cloned_sym);
+                    }
+                }
+
+                self.analyze_symbol_comprehension(symbol, parent_offset + 1)?;
+            }
         }
         Ok(())
     }
@@ -271,6 +344,7 @@ enum SymbolUsage {
     Used,
     Assigned,
     Parameter,
+    AssignedNamedExprInCompr,
 }
 
 #[derive(Default)]
@@ -602,7 +676,7 @@ impl SymbolTableBuilder {
 
                 self.enter_scope(
                     scope_name,
-                    SymbolTableType::Function,
+                    SymbolTableType::Comprehension,
                     expression.location.row(),
                 );
 
@@ -679,6 +753,28 @@ impl SymbolTableBuilder {
                 self.scan_expression(body, &ExpressionContext::Load)?;
                 self.scan_expression(orelse, &ExpressionContext::Load)?;
             }
+
+            NamedExpression { left, right } => {
+                self.scan_expression(right, &ExpressionContext::Load)?;
+
+                // special handling for assigned identifier in named expressions
+                // that are used in comprehensions. This required to correctly
+                // propagate the scope of the named assigned named and not to
+                // propagate inner names.
+                if let Identifier { name } = &left.node {
+                    let table = self.tables.last().unwrap();
+                    if table.typ == SymbolTableType::Comprehension {
+                        self.register_name(name, SymbolUsage::AssignedNamedExprInCompr)?;
+                    } else {
+                        // omit one recursion. When the handling of an store changes for
+                        // Identifiers this needs adapted - more forward safe would be
+                        // calling scan_expression directly.
+                        self.register_name(name, SymbolUsage::Assigned)?;
+                    }
+                } else {
+                    self.scan_expression(left, &ExpressionContext::Store)?;
+                }
+            }
         }
         Ok(())
     }
@@ -810,6 +906,10 @@ impl SymbolTableBuilder {
             SymbolUsage::Assigned => {
                 symbol.is_assigned = true;
             }
+            SymbolUsage::AssignedNamedExprInCompr => {
+                symbol.is_assigned = true;
+                symbol.is_assign_namedexpr_in_comprehension = true;
+            }
             SymbolUsage::Global => {
                 if let SymbolScope::Unknown = symbol.scope {
                     symbol.scope = SymbolScope::Global;

compiler/src/symboltable.rs

@@ -309,6 +309,12 @@ pub enum ExpressionType {
         orelse: Box<Expression>,
     },
 
+    // A named expression
+    NamedExpression {
+        left: Box<Expression>,
+        right: Box<Expression>,
+    },
+
     /// The literal 'True'.
     True,
 
@@ -364,6 +370,7 @@ impl Expression {
             IfExpression { .. } => "conditional expression",
             True | False | None => "keyword",
             Ellipsis => "ellipsis",
+            NamedExpression { .. } => "named expression",
         }
     }
 }

parser/src/ast.rs

@@ -1075,7 +1075,16 @@ where
                 self.nesting -= 1;
             }
             ':' => {
-                self.eat_single_char(Tok::Colon);
+                let tok_start = self.get_pos();
+                self.next_char();
+                if let Some('=') = self.chr0 {
+                    self.next_char();
+                    let tok_end = self.get_pos();
+                    self.emit((tok_start, Tok::ColonEqual, tok_end));
+                } else {
+                    let tok_end = self.get_pos();
+                    self.emit((tok_start, Tok::Colon, tok_end));
+                }
             }
             ';' => {
                 self.eat_single_char(Tok::Semi);