Skip to content
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
57 changes: 57 additions & 0 deletions crates/common/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1083,9 +1083,19 @@ impl FormatSpec {
self.validate_format(FormatType::String)?;
match self.format_type {
Some(FormatType::String) | None => {
// CPython rejects these four in this order: sign, z, #, then '='.
if let Some(sign) = self.sign {
return Err(FormatSpecError::StringSpecNotAllowed(match sign {
FormatSign::MinusOrSpace => "Space",
FormatSign::Plus | FormatSign::Minus => "Sign",
}));
}
if self.no_neg_0 {
return Err(FormatSpecError::NegativeZeroCoercionNotAllowed("string"));
}
if self.alternate_form {
return Err(FormatSpecError::StringSpecNotAllowed("Alternate form (#)"));
}
if self.align == Some(FormatAlign::AfterSign) && self.align_specified {
return Err(FormatSpecError::StringAlignmentFlag);
}
Expand Down Expand Up @@ -1334,6 +1344,7 @@ pub enum FormatSpecError {
AlignmentFlag,
NegativeZeroCoercionNotAllowed(&'static str),
StringAlignmentFlag,
StringSpecNotAllowed(&'static str),
NotImplemented(char, &'static str),
}

Expand Down Expand Up @@ -1788,6 +1799,52 @@ mod tests {
);
}

#[test]
fn format_string_rejects_sign_space_and_alternate_form() {
let value = "result".to_owned();
let cases = [
("+", "Sign"),
("-", "Sign"),
("+8s", "Sign"),
(" ", "Space"),
(" 8s", "Space"),
("#", "Alternate form (#)"),
("#8s", "Alternate form (#)"),
];

for (text, flag) in cases {
let spec = FormatSpec::parse(text).unwrap();
assert_eq!(
spec.format_string(&value),
Err(FormatSpecError::StringSpecNotAllowed(flag)),
"{text}"
);
}
}

#[test]
fn format_string_reports_the_flag_cpython_reports_first() {
let value = "result".to_owned();
// Sign beats z, z beats the alternate form, and the alternate form
// beats an explicit '=' alignment.
let cases = [
("+z#5", FormatSpecError::StringSpecNotAllowed("Sign")),
(
"x=z#5",
FormatSpecError::NegativeZeroCoercionNotAllowed("string"),
),
(
"x=#5",
FormatSpecError::StringSpecNotAllowed("Alternate form (#)"),
),
];

for (text, expected) in cases {
let spec = FormatSpec::parse(text).unwrap();
assert_eq!(spec.format_string(&value), Err(expected), "{text}");
}
}

#[test]
fn format_complex_rejects_zero_padding_before_after_sign_alignment() {
for text in [
Expand Down
4 changes: 4 additions & 0 deletions crates/vm/src/format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ impl IntoPyException for FormatSpecError {
Self::StringAlignmentFlag => {
vm.new_value_error("'=' alignment not allowed in string format specifier")
}
Self::StringSpecNotAllowed(s) => {
let msg = format!("{s} not allowed in string format specifier");
vm.new_value_error(msg)
}
Self::NotImplemented(c, s) => {
let msg = format!("Format code '{c}' for object of type '{s}' not implemented yet");
vm.new_value_error(msg)
Expand Down
24 changes: 24 additions & 0 deletions extra_tests/snippets/builtin_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,30 @@ def test_zero_padding():
else:
raise AssertionError("expected ValueError for '=8s' string format specifier")

# regression: a sign, a space or an alternate form used to be accepted and dropped.
for spec, flag in [
("+", "Sign"),
("-", "Sign"),
("+5", "Sign"),
(" ", "Space"),
(" 5", "Space"),
("#", "Alternate form (#)"),
("#5", "Alternate form (#)"),
("+.2", "Sign"),
]:
try:
format("result", spec)
except ValueError as error:
expected = f"{flag} not allowed in string format specifier"
if str(error) != expected:
raise AssertionError(
f"{spec!r}: unexpected error message: {error}"
) from error
else:
raise AssertionError(
f"expected ValueError for {spec!r} string format specifier"
)

# regression: unknown conversion specifiers used to be silently ignored instead of raising.
# The ValueError case itself is covered by test_str, but here we're testing the error message.
try:
Expand Down
Loading