From b701e5cbe91ce66e685f1692683fc7e6c50abff6 Mon Sep 17 00:00:00 2001 From: Roshan Ramani <154859727+rawsun007@users.noreply.github.com> Date: Tue, 8 Sep 2026 14:22:57 +0530 Subject: [PATCH] Report which format string mistake was made (#8659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(vm): report which format string mistake was made `ToPyException for FormatParseError` mapped two variants and sent the rest to "Unexpected error parsing format string", so most format-string mistakes gave no hint about the mistake. Against CPython 3.14.0: | expression | before | CPython | | --- | --- | --- | | `"a}b".format()` | Unexpected error… | Single `}` encountered in format string | | `"{0.}".format(1)` | Unexpected error… | Empty attribute in format string | | `"{0[]}".format([1])` | Unexpected error… | Empty attribute in format string | | `"{0[1}".format([1,2])` | Unexpected error… | expected `}` before end of string | | `"{0[0]x}".format([1])` | Unexpected error… | Only `.` or `[` may follow `]` in format field specifier | | `"{:{{}}".format(1)` | Unexpected error… | unmatched `{` in format spec | The match is now exhaustive, so a new variant cannot silently inherit the generic message. Two cases are deliberately left generic, with the reason in a comment beside each. `UnknownConversion` is raised both for a multi-character conversion (`{0!xy}`, where CPython says "expected ':' after conversion specifier") and for an empty one (`{0!}`, "unmatched '{' in format spec"), so one message cannot serve both without the parser saying which it hit. And a trailing lone `{` reaches `UnmatchedBracket` rather than `UnescapedStartBracketInLiteral`, so `"{".format()` still differs from CPython; that needs a parser change, not a message. Expectations in the new snippet are CPython 3.14.0's output and pass under both interpreters. Reverting only format.rs fails the snippet's first assertion. Assisted-by: Claude Code:claude-opus-5 * fix(common): separate a stray `{` from an unclosed field `parse_literal` already reports `UnescapedStartBracketInLiteral` for a brace that was not doubled, but `from_str` discards it with `or_else` and retries as a field, so `"{".format()` surfaced `UnmatchedBracket` and read as "expected '}' before end of string". CPython separates the two by whether anything follows the brace: "{" "a{" Single '{' encountered in format string "{s" "a{b" expected '}' before end of string "{0" expected '}' before end of string `parse_spec` now returns the literal error when the text is the brace alone, which is also what makes `UnescapedStartBracketInLiteral` reachable — the previous commit could map it but not trigger it. `format_parse_lone_start_bracket` pins both directions at the parser level, and the snippet covers the four messages under CPython 3.14.0 as well. The existing `format_parse_fail` assertion for `"{s"` is unchanged. Assisted-by: Claude Code:claude-opus-5 --- crates/common/src/format.rs | 26 ++++++++++++++++ crates/vm/src/format.rs | 28 +++++++++++++++-- extra_tests/snippets/builtin_format.py | 42 ++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/crates/common/src/format.rs b/crates/common/src/format.rs index 4357a0c66..28afac735 100644 --- a/crates/common/src/format.rs +++ b/crates/common/src/format.rs @@ -1618,6 +1618,12 @@ impl FormatString { let right = &text[pos..]; let format_part = Self::parse_part_in_brackets(&left)?; Ok((format_part, &right[1..])) + } else if text.len() == 1 { + // Nothing follows the brace, so it is a stray one rather than a field + // that was left open: CPython separates "{" and "a{" from "{0" and + // "a{b" the same way. `{` is one byte, so a length of one is the brace + // on its own. + Err(FormatParseError::UnescapedStartBracketInLiteral) } else { Err(FormatParseError::UnmatchedBracket) } @@ -2338,6 +2344,26 @@ mod tests { ); } + #[test] + fn format_parse_lone_start_bracket() { + // A brace with nothing after it is a stray brace; one holding a field + // that was never closed is not. CPython words the two differently. + for lone in ["{", "a{"] { + assert_eq!( + FormatString::from_str(lone.as_ref()), + Err(FormatParseError::UnescapedStartBracketInLiteral), + "{lone:?}" + ); + } + for unclosed in ["{s", "a{b", "{0"] { + assert_eq!( + FormatString::from_str(unclosed.as_ref()), + Err(FormatParseError::UnmatchedBracket), + "{unclosed:?}" + ); + } + } + #[test] fn square_brackets_inside_format() { assert_eq!( diff --git a/crates/vm/src/format.rs b/crates/vm/src/format.rs index 9a54df3b4..d95def768 100644 --- a/crates/vm/src/format.rs +++ b/crates/vm/src/format.rs @@ -103,12 +103,36 @@ impl IntoPyException for FormatSpecError { impl ToPyException for FormatParseError { fn to_pyexception(&self, vm: &VirtualMachine) -> PyBaseExceptionRef { + // Matched exhaustively on purpose: a catch-all sent most of these to + // "Unexpected error parsing format string", which tells the reader nothing + // about the format string they mistyped. match self { - Self::UnmatchedBracket => vm.new_value_error("expected '}' before end of string"), + Self::UnmatchedBracket | Self::MissingRightBracket => { + vm.new_value_error("expected '}' before end of string") + } + Self::MissingStartBracket => { + vm.new_value_error("Single '}' encountered in format string") + } + // A brace in literal text that was not doubled, i.e. a stray `{` with + // nothing after it. `parse_spec` reports this rather than + // UnmatchedBracket so that "{" and "a{" are separated from "{0" and + // "a{b", which are fields left open. + Self::UnescapedStartBracketInLiteral => { + vm.new_value_error("Single '{' encountered in format string") + } + Self::InvalidFormatSpecifier => vm.new_value_error("unmatched '{' in format spec"), + // Reached both for a conversion that is not one character (`{0!xy}`, + // where CPython says "expected ':' after conversion specifier") and for + // an empty one (`{0!}`, "unmatched '{' in format spec"). Telling those + // apart needs the parser to say which, so the generic message stays. + Self::UnknownConversion => vm.new_value_error("Unexpected error parsing format string"), + Self::EmptyAttribute => vm.new_value_error("Empty attribute in format string"), + Self::InvalidCharacterAfterRightBracket => { + vm.new_value_error("Only '.' or '[' may follow ']' in format field specifier") + } Self::TooManyDecimalDigits => { vm.new_value_error("Too many decimal digits in format string") } - _ => vm.new_value_error("Unexpected error parsing format string"), } } } diff --git a/extra_tests/snippets/builtin_format.py b/extra_tests/snippets/builtin_format.py index 844d9e451..920256c0d 100644 --- a/extra_tests/snippets/builtin_format.py +++ b/extra_tests/snippets/builtin_format.py @@ -336,3 +336,45 @@ assert_raises(ValueError, format, True, ".2") assert_raises(ValueError, format, True, "5.2") assert_raises(ValueError, format, True, "z") assert_raises(ValueError, format, True, "s") + + +def test_format_parse_error_messages(): + # Most FormatParseError variants used to fall into a catch-all that said + # "Unexpected error parsing format string", which does not say what is wrong + # with the format string. Messages are CPython 3.14.0's. + def message(fmt, *args): + try: + fmt.format(*args) + except ValueError as err: + return str(err) + raise AssertionError("ValueError was not raised for " + repr(fmt)) + + assert message("a}b") == "Single '}' encountered in format string" + assert message("{{a}") == "Single '}' encountered in format string" + + assert message("{0.}", 1) == "Empty attribute in format string" + assert message("{0[]}", [1]) == "Empty attribute in format string" + + assert message("{0[1}", [1, 2]) == "expected '}' before end of string" + assert message("{0", 1) == "expected '}' before end of string" + + assert ( + message("{0[0]x}", [1]) + == "Only '.' or '[' may follow ']' in format field specifier" + ) + assert ( + message("{0[0]]}", [1]) + == "Only '.' or '[' may follow ']' in format field specifier" + ) + + assert message("{:{{}}", 1) == "unmatched '{' in format spec" + + # A brace with nothing after it is a stray brace; one holding an unclosed + # field is not. + assert message("{") == "Single '{' encountered in format string" + assert message("a{") == "Single '{' encountered in format string" + assert message("{s") == "expected '}' before end of string" + assert message("a{b") == "expected '}' before end of string" + + +test_format_parse_error_messages()