[libc++][format] Don't treat a closing '}' as part of format-spec

[poyaoc97]

This allows:

std::println("{}>42", std::thread::id{});
std::println("{}>42", std::span<int>{});
std::println("{}>42", std::pair{42, "Hello"sv});
std::println("{:}>42", std::thread::id{});
std::println("{:}>42", std::span<int>{});
std::println("{:}>42", std::pair{42, "Hello"sv});

to compile and run.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Po-yao Chang (poyaoc97)

Changes

This allows:

std::println("{}>42", std::thread::id{});
std::println("{}>42", std::span<int>{});
std::println("{}>42", std::pair{42, "Hello"sv});

to compile and run.

---

Full diff: https://github.com/llvm/llvm-project/pull/81305.diff

4 Files Affected:

• (modified) libcxx/include/__format/format_functions.h (+2-2)
• (modified) libcxx/include/__format/format_parse_context.h (+5-1)
• (modified) libcxx/include/__format/parser_std_format_spec.h (+1-1)
• (modified) libcxx/test/std/utilities/format/format.tuple/format.functions.tests.h (+1)

diff --git a/libcxx/include/__format/format_functions.h b/libcxx/include/__format/format_functions.h
index 3ee53539f4ee6c..ad0ec354686807 100644
--- a/libcxx/include/__format/format_functions.h
+++ b/libcxx/include/__format/format_functions.h
@@ -251,7 +251,7 @@ __handle_replacement_field(_Iterator __begin, _Iterator __end, _ParseCtx& __pars
   if (__r.__last == __end)
     std::__throw_format_error("The argument index should end with a ':' or a '}'");
 
-  bool __parse = *__r.__last == _CharT(':');
+  bool __parse = __parse_ctx.__should_parse() = *__r.__last == _CharT(':');
   switch (*__r.__last) {
   case _CharT(':'):
     // The arg-id has a format-specifier, advance the input to the format-spec.
@@ -269,7 +269,7 @@ __handle_replacement_field(_Iterator __begin, _Iterator __end, _ParseCtx& __pars
     __arg_t __type = __ctx.arg(__r.__value);
     if (__type == __arg_t::__none)
       std::__throw_format_error("The argument index value is too large for the number of arguments supplied");
-    else if (__type == __arg_t::__handle)
+    else if (__parse && __type == __arg_t::__handle)
       __ctx.__handle(__r.__value).__parse(__parse_ctx);
     else if (__parse)
       __format::__compile_time_visit_format_arg(__parse_ctx, __ctx, __type);
diff --git a/libcxx/include/__format/format_parse_context.h b/libcxx/include/__format/format_parse_context.h
index aefcd5497f3b9b..2a975d7db70e54 100644
--- a/libcxx/include/__format/format_parse_context.h
+++ b/libcxx/include/__format/format_parse_context.h
@@ -36,7 +36,8 @@ class _LIBCPP_TEMPLATE_VIS basic_format_parse_context {
         __end_(__fmt.end()),
         __indexing_(__unknown),
         __next_arg_id_(0),
-        __num_args_(__num_args) {}
+        __num_args_(__num_args),
+        __parse(true) {}
 
   basic_format_parse_context(const basic_format_parse_context&)            = delete;
   basic_format_parse_context& operator=(const basic_format_parse_context&) = delete;
@@ -83,6 +84,8 @@ class _LIBCPP_TEMPLATE_VIS basic_format_parse_context {
       std::__throw_format_error("Argument index outside the valid range");
   }
 
+  _LIBCPP_HIDE_FROM_ABI constexpr bool& __should_parse() { return __parse; }
+
 private:
   iterator __begin_;
   iterator __end_;
@@ -90,6 +93,7 @@ class _LIBCPP_TEMPLATE_VIS basic_format_parse_context {
   _Indexing __indexing_;
   size_t __next_arg_id_;
   size_t __num_args_;
+  bool __parse;
 };
 _LIBCPP_CTAD_SUPPORTED_FOR_TYPE(basic_format_parse_context);
 
diff --git a/libcxx/include/__format/parser_std_format_spec.h b/libcxx/include/__format/parser_std_format_spec.h
index cf8af87b212849..d42ce2dbb456aa 100644
--- a/libcxx/include/__format/parser_std_format_spec.h
+++ b/libcxx/include/__format/parser_std_format_spec.h
@@ -355,7 +355,7 @@ class _LIBCPP_TEMPLATE_VIS __parser {
   _LIBCPP_HIDE_FROM_ABI constexpr typename _ParseContext::iterator __parse(_ParseContext& __ctx, __fields __fields) {
     auto __begin = __ctx.begin();
     auto __end   = __ctx.end();
-    if (__begin == __end)
+    if (__begin == __end || !__ctx.__should_parse())
       return __begin;
 
     if (__parse_fill_align(__begin, __end, __fields.__use_range_fill_) && __begin == __end)
diff --git a/libcxx/test/std/utilities/format/format.tuple/format.functions.tests.h b/libcxx/test/std/utilities/format/format.tuple/format.functions.tests.h
index dd99d2070dbb8a..bcb2bc7ebe03b8 100644
--- a/libcxx/test/std/utilities/format/format.tuple/format.functions.tests.h
+++ b/libcxx/test/std/utilities/format/format.tuple/format.functions.tests.h
@@ -77,6 +77,7 @@ void test_tuple_or_pair_int_int(TestFunction check, ExceptionTest check_exceptio
 template <class CharT, class TestFunction, class ExceptionTest, class TupleOrPair>
 void test_tuple_or_pair_int_string(TestFunction check, ExceptionTest check_exception, TupleOrPair&& input) {
   check(SV("(42, \"hello\")"), SV("{}"), input);
+  check(SV("(42, \"hello\")^42"), SV("{}^42"), input);
 
   // *** align-fill & width ***
   check(SV("(42, \"hello\")     "), SV("{:18}"), input);

[mordante]

Thanks for the patch! I had a short look at it, before looking in more detail I want to verify some things in the Standard. I know we changed some rules in this area and I want to make sure what the correct behaviour should be.

[mordante]

You found an interesting bug, it seems the scope of the issue is larger than one corner case. I've left comments for a better approach to fix the root cause of the bug.

[mordante]

This looks great! Thanks!

Do you have permission to land patches? If not I'll land it on your behalf.

':' for range formatters in more detail. "{:::<<}" is probably rejected. I expect it should be an underlying formatter with left-alignment and < as fill character. (I have not tested that.) Maybe it would be good to investigate that for a follow-up patch.

Did you investigate this? If not do you want to do that for a followup patch, if it's a bug.

[poyaoc97]

Do you have permission to land patches? If not I'll land it on your behalf.

Yes

':' for range formatters in more detail. "{:::<<}" is probably rejected. I expect it should be an underlying formatter with left-alignment and < as fill character. (I have not tested that.) Maybe it would be good to investigate that for a follow-up patch.

Did you investigate this? If not do you want to do that for a followup patch, if it's a bug.

Sure, I'm happy to help! It is indeed rejected now.