8000 Support expression in f-strings spec by palaviv · Pull Request #1625 · RustPython/RustPython · GitHub
[go: up one dir, main page]

Skip to content

Support expression in f-strings spec #1625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Dec 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions bytecode/src/bytecode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,6 @@ pub enum Instruction {
},
FormatValue {
conversion: Option<ConversionFlag>,
spec: String,
},
PopException,
Reverse {
Expand Down Expand Up @@ -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),
Expand Down
9 changes: 8 additions & 1 deletion compiler/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2108,10 +2108,17 @@ impl<O: OutputStream> Compiler<O> {
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(),
});
}
}
Expand Down
5 changes: 4 additions & 1 deletion compiler/src/symboltable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -726,8 +726,11 @@ 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, 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 {
Expand Down
2 changes: 1 addition & 1 deletion parser/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ pub enum StringGroup {
FormattedValue {
value: Box<Expression>,
conversion: Option<ConversionFlag>,
spec: String,
spec: Option<Box<StringGroup>>,
},
Joined {
values: Vec<StringGroup>,
Expand Down
4 changes: 4 additions & 0 deletions parser/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ pub enum FStringErrorType {
InvalidConversionFlag,
EmptyExpression,
MismatchedDelimiter,
ExpressionNestedTooDeeply,
}

impl fmt::Display for FStringErrorType {
Expand All @@ -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")
}
}
}
}
Expand Down
90 changes: 82 additions & 8 deletions parser/src/fstring.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ impl<'a> FStringParser<'a> {

fn parse_formatted_value(&mut self) -> Result<StringGroup, FStringErrorType> {
let mut expression = String::new();
let mut spec = String::new();
let mut spec = None;
let mut delims = Vec::new();
let mut conversion = None;

Expand All @@ -43,13 +43,48 @@ 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);
}
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(),
}))
}
}
'(' | '{' | '[' => {
Expand Down Expand Up @@ -194,12 +229,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()
Expand All @@ -209,6 +244,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(),
})),
Copy link
Member
@coolreader18 coolreader18 Dec 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some more tests for some weirder cases? e.g. f"{val: {'#'} }" interprets the spec as ' {\'#\'} '; it only interpolates the value if there are no characters between : and { and none between the two closing }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is not perfect yet but we can improve on that later. I added a few more fail cases tests.

}
);
}

#[test]
fn test_parse_empty_fstring() {
assert_eq!(
Expand All @@ -223,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());
Expand Down
8 changes: 8 additions & 0 deletions tests/snippets/fstrings.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from testutils import assert_raises
foo = 'bar'

assert f"{''}" == ''
Expand All @@ -16,6 +17,13 @@
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'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some TypeError tests here for formatting with ints as specs or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be more specific?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. assert_raises(TypeError, lambda: f"{123:{45}}")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. assert_raises(TypeError, lambda: f"{123:{45}}")

This is actually a valid format string.
I added a TODO in the tests for an invalid one. This currently does not fail but it is not related to this change.

# 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.

Expand Down
4 changes: 2 additions & 2 deletions vm/src/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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)
Expand Down
0