From c3703f08d7423e9b96e44591170324c73ea4d4d6 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 13 Dec 2019 11:33:19 +0200 Subject: [PATCH 1/5] Support expression in f-strings spec --- bytecode/src/bytecode.rs | 3 +- compiler/src/compile.rs | 9 +++- parser/src/ast.rs | 2 +- parser/src/error.rs | 4 ++ parser/src/fstring.rs | 88 ++++++++++++++++++++++++++++++++++---- tests/snippets/fstrings.py | 3 ++ vm/src/frame.rs | 4 +- 7 files changed, 99 insertions(+), 14 deletions(-) diff --git a/bytecode/src/bytecode.rs b/bytecode/src/bytecode.rs index 7f84163b75..24e59a3f93 100644 --- a/bytecode/src/bytecode.rs +++ b/bytecode/src/bytecode.rs @@ -274,7 +274,6 @@ pub enum Instruction { }, FormatValue { conversion: Option, - spec: String, }, PopException, Reverse { @@ -564,7 +563,7 @@ impl Instruction { LoadBuildClass => w!(LoadBuildClass), UnpackSequence { size } => w!(UnpackSequence, size), UnpackEx { before, after } => w!(UnpackEx, before, after), - FormatValue { spec, .. } => w!(FormatValue, spec), // TODO: write conversion + FormatValue { .. } => w!(FormatValue), // TODO: write conversion PopException => w!(PopException), Reverse { amount } => w!(Reverse, amount), GetAwaitable => w!(GetAwaitable), diff --git a/compiler/src/compile.rs b/compiler/src/compile.rs index 24df923b7e..d02d3d3878 100644 --- a/compiler/src/compile.rs +++ b/compiler/src/compile.rs @@ -2108,10 +2108,17 @@ impl Compiler { conversion, spec, } => { + match spec { + Some(spec) => self.compile_string(spec)?, + None => self.emit(Instruction::LoadConst { + value: bytecode::Constant::String { + value: String::new(), + }, + }), + }; self.compile_expression(value)?; self.emit(Instruction::FormatValue { conversion: conversion.map(compile_conversion_flag), - spec: spec.clone(), }); } } diff --git a/parser/src/ast.rs b/parser/src/ast.rs index c211d296ca..f8e104757c 100644 --- a/parser/src/ast.rs +++ b/parser/src/ast.rs @@ -505,7 +505,7 @@ pub enum StringGroup { FormattedValue { value: Box, conversion: Option, - spec: String, + spec: Option>, }, Joined { values: Vec, diff --git a/parser/src/error.rs b/parser/src/error.rs index 7b6e1369a6..86fa763874 100644 --- a/parser/src/error.rs +++ b/parser/src/error.rs @@ -81,6 +81,7 @@ pub enum FStringErrorType { InvalidConversionFlag, EmptyExpression, MismatchedDelimiter, + ExpressionNestedTooDeeply, } impl fmt::Display for FStringErrorType { @@ -94,6 +95,9 @@ impl fmt::Display for FStringErrorType { FStringErrorType::InvalidConversionFlag => write!(f, "Invalid conversion flag"), FStringErrorType::EmptyExpression => write!(f, "Empty expression"), FStringErrorType::MismatchedDelimiter => write!(f, "Mismatched delimiter"), + FStringErrorType::ExpressionNestedTooDeeply => { + write!(f, "expressions nested too deeply") + } } } } diff --git a/parser/src/fstring.rs b/parser/src/fstring.rs index e836c8c4bf..6215e70134 100644 --- a/parser/src/fstring.rs +++ b/parser/src/fstring.rs @@ -23,7 +23,7 @@ impl<'a> FStringParser<'a> { fn parse_formatted_value(&mut self) -> Result { let mut expression = String::new(); - let mut spec = String::new(); + let mut spec = None; let mut delims = Vec::new(); let mut conversion = None; @@ -43,13 +43,49 @@ impl<'a> FStringParser<'a> { }) } ':' if delims.is_empty() => { + let mut nested = false; + let mut in_nested = false; + let mut spec_expression = String::new(); while let Some(&next) = self.chars.peek() { - if next != '}' { - spec.push(next); - self.chars.next(); - } else { - break; + match next { + '{' => { + if in_nested { + return Err(ExpressionNestedTooDeeply); + } else { + in_nested = true; + nested = true; + self.chars.next(); + continue; + } + } + '}' => { + if in_nested { + in_nested = false; + self.chars.next(); + } + break; + } + _ => (), } + spec_expression.push(next); + self.chars.next(); + } + if in_nested { + return Err(UnclosedLbrace); + } + if nested { + spec = Some(Box::new(FormattedValue { + value: Box::new( + parse_expression(spec_expression.trim()) + .map_err(|e| InvalidExpression(Box::new(e.error)))?, + ), + conversion: None, + spec: None, + })) + } else { + spec = Some(Box::new(Constant { + value: spec_expression.trim().to_string(), + })) } } '(' | '{' | '[' => { @@ -194,12 +230,12 @@ mod tests { FormattedValue { value: Box::new(mk_ident("a", 1, 1)), conversion: None, - spec: String::new(), + spec: None, }, FormattedValue { value: Box::new(mk_ident("b", 1, 1)), conversion: None, - spec: String::new(), + spec: None, }, Constant { value: "{foo}".to_owned() @@ -209,6 +245,42 @@ mod tests { ); } + #[test] + fn test_parse_fstring_nested_spec() { + let source = String::from("{foo:{spec}}"); + let parse_ast = parse_fstring(&source).unwrap(); + + assert_eq!( + parse_ast, + FormattedValue { + value: Box::new(mk_ident("foo", 1, 1)), + conversion: None, + spec: Some(Box::new(FormattedValue { + value: Box::new(mk_ident("spec", 1, 1)), + conversion: None, + spec: None, + })), + } + ); + } + + #[test] + fn test_parse_fstring_not_nested_spec() { + let source = String::from("{foo:spec}"); + let parse_ast = parse_fstring(&source).unwrap(); + + assert_eq!( + parse_ast, + FormattedValue { + value: Box::new(mk_ident("foo", 1, 1)), + conversion: None, + spec: Some(Box::new(Constant { + value: "spec".to_string(), + })), + } + ); + } + #[test] fn test_parse_empty_fstring() { assert_eq!( diff --git a/tests/snippets/fstrings.py b/tests/snippets/fstrings.py index 67da5e1a7d..bbda1302d7 100644 --- a/tests/snippets/fstrings.py +++ b/tests/snippets/fstrings.py @@ -16,6 +16,9 @@ assert f'{16:0>+#10x}' == '00000+0x10' assert f"{{{(lambda x: f'hello, {x}')('world}')}" == '{hello, world}' +spec = "0>+#10x" +assert f"{16:{spec}}{foo}" == '00000+0x10bar' + # Normally `!` cannot appear outside of delimiters in the expression but # cpython makes an exception for `!=`, so we should too. diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 307f93cc58..0aaa079e00 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -616,7 +616,7 @@ impl Frame { bytecode::Instruction::UnpackEx { before, after } => { self.execute_unpack_ex(vm, *before, *after) } - bytecode::Instruction::FormatValue { conversion, spec } => { + bytecode::Instruction::FormatValue { conversion } => { use bytecode::ConversionFlag::*; let value = match conversion { Some(Str) => vm.to_str(&self.pop_value())?.into_object(), @@ -625,7 +625,7 @@ impl Frame { None => self.pop_value(), }; - let spec = vm.new_str(spec.clone()); + let spec = vm.to_str(&self.pop_value())?.into_object(); let formatted = vm.call_method(&value, "__format__", vec![spec])?; self.push_value(formatted); Ok(None) From 8e84a85b0ceeb244f0b8c29b63353c55a2f966cd Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Sat, 14 Dec 2019 09:46:53 +0200 Subject: [PATCH 2/5] Add more tests for nested f-string spec --- parser/src/fstring.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/parser/src/fstring.rs b/parser/src/fstring.rs index 6215e70134..f5a2ad24b6 100644 --- a/parser/src/fstring.rs +++ b/parser/src/fstring.rs @@ -51,12 +51,11 @@ impl<'a> FStringParser<'a> { '{' => { if in_nested { return Err(ExpressionNestedTooDeeply); - } else { - in_nested = true; - nested = true; - self.chars.next(); - continue; } + in_nested = true; + nested = true; + self.chars.next(); + continue; } '}' => { if in_nested { @@ -295,6 +294,9 @@ mod tests { fn test_parse_invalid_fstring() { assert_eq!(parse_fstring("{"), Err(UnclosedLbrace)); assert_eq!(parse_fstring("}"), Err(UnopenedRbrace)); + assert_eq!(parse_fstring("{a:{a:{b}}"), Err(ExpressionNestedTooDeeply)); + assert_eq!(parse_fstring("{a:b}}"), Err(UnopenedRbrace)); + assert_eq!(parse_fstring("{a:{b}"), Err(UnclosedLbrace)); // TODO: check for InvalidExpression enum? assert!(parse_fstring("{class}").is_err()); From b394ad4e05a0c80d4c0207f54357e41d5ff93b93 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Sat, 21 Dec 2019 11:27:52 +0200 Subject: [PATCH 3/5] Parse FormattedValue spec in symboltable --- compiler/src/symboltable.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/compiler/src/symboltable.rs b/compiler/src/symboltable.rs index 2813854661..955ee48c20 100644 --- a/compiler/src/symboltable.rs +++ b/compiler/src/symboltable.rs @@ -726,8 +726,15 @@ impl SymbolTableBuilder { fn scan_string_group(&mut self, group: &ast::StringGroup) -> SymbolTableResult { match group { ast::StringGroup::Constant { .. } => {} - ast::StringGroup::FormattedValue { value, .. } => { + ast::StringGroup::FormattedValue { + value, + conversion: _, + spec, + } => { self.scan_expression(value, &ExpressionContext::Load)?; + if let Some(spec) = spec { + self.scan_string_group(spec)?; + } } ast::StringGroup::Joined { values } => { for subgroup in values { From 6698fdee12a4f0df418d7bc3e832ea249ba391e5 Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Sat, 21 Dec 2019 11:28:23 +0200 Subject: [PATCH 4/5] Add TODO for spec validation --- tests/snippets/fstrings.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/snippets/fstrings.py b/tests/snippets/fstrings.py index bbda1302d7..2d25abf6dc 100644 --- a/tests/snippets/fstrings.py +++ b/tests/snippets/fstrings.py @@ -1,3 +1,4 @@ +from testutils import assert_raises foo = 'bar' assert f"{''}" == '' @@ -19,6 +20,10 @@ spec = "0>+#10x" assert f"{16:{spec}}{foo}" == '00000+0x10bar' +# TODO: +# spec = "bla" +# assert_raises(ValueError, lambda: f"{16:{spec}}") + # Normally `!` cannot appear outside of delimiters in the expression but # cpython makes an exception for `!=`, so we should too. From 6a4c3da822a8b268cc56774ab442a4f5e62af15c Mon Sep 17 00:00:00 2001 From: Aviv Palivoda Date: Fri, 27 Dec 2019 10:27:51 +0200 Subject: [PATCH 5/5] Fix clippy warning --- compiler/src/symboltable.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler/src/symboltable.rs b/compiler/src/symboltable.rs index 955ee48c20..c3e2099bcf 100644 --- a/compiler/src/symboltable.rs +++ b/compiler/src/symboltable.rs @@ -726,11 +726,7 @@ impl SymbolTableBuilder { fn scan_string_group(&mut self, group: &ast::StringGroup) -> SymbolTableResult { match group { ast::StringGroup::Constant { .. } => {} - ast::StringGroup::FormattedValue { - value, - conversion: _, - spec, - } => { + ast::StringGroup::FormattedValue { value, spec, .. } => { self.scan_expression(value, &ExpressionContext::Load)?; if let Some(spec) = spec { self.scan_string_group(spec)?;