fmt::join doesn't handle move-only iterators #3802

tcbrindle · 2024-01-10T19:46:32Z

C++20 allows iterators for input ranges to be move-only, for example std::ranges::basic_istream_view::iterator. Unfortunately fmt::join_view copies iterators in various places, meaning it can't be used with such ranges:

int main()
{
    std::istringstream iss("1 2 3 4 5");

    auto view = std::views::istream<int>(iss);

    fmt::println("{}", fmt::join(view, ", ")); // Error, use of deleted iterator copy constructor
}

https://godbolt.org/z/1ndGahqb7

The text was updated successfully, but these errors were encountered:

vitaut · 2024-01-10T20:23:37Z

~~Fixed by #3800 (thanks).~~

vitaut · 2024-01-10T20:25:24Z

Or is it unrelated?

tcbrindle · 2024-01-10T20:26:35Z

Or is it unrelated?

This is a different to #3800

tcbrindle · 2024-01-12T12:33:49Z

There are a couple of places in the join_view constructor where iterators are copied, but these are easily solved with std::move. The difficulty is with formatter<join_view>::format():

fmt/include/fmt/ranges.h

Lines 587 to 603 in 810d175

    
           template <typename FormatContext> 
        
           auto format(const join_view<It, Sentinel, Char>& value, 
        
                       FormatContext& ctx) const -> decltype(ctx.out()) { 
        
             auto it = value.begin; 
        
             auto out = ctx.out(); 
        
             if (it != value.end) { 
        
               out = value_formatter_.format(*it, ctx); 
        
               ++it; 
        
               while (it != value.end) { 
        
                 out = detail::copy_str<Char>(value.sep.begin(), value.sep.end(), out); 
        
                 ctx.advance_to(out); 
        
                 out = value_formatter_.format(*it, ctx); 
        
                 ++it; 
        
               } 
        
             } 
        
             return out; 
        
           }

Line 590 takes a copy of the iterator, which it then modifies. Because the join_view is passed to this function as a const reference, there's no way we can move the iterator out of it -- and we also can't directly operate on the stored iterator, as it's const.

It seems like we need to take the join_view argument to format() by mutable & rather than const&, but making this change results in fmt considering join_view to be non-formattable. Unfortunately I don't know enough about the library to understand why that happens. I also tried using a forwarding reference, but this always deduces to const& and so doesn't help.

The generic range support without join (i.e. fmt::format("{}", my_range)) handles move-only iterators just fine, so this definitely seems like it should be solvable, but I don't understand enough about the fmt internals to follow what's going on.

vitaut · 2024-01-12T15:08:30Z

It seems like we need to take the join_view argument to format() by mutable & rather than const&, but making this change results in fmt considering join_view to be non-formattable.

This should work. What error do you get?

tcbrindle · 2024-01-12T16:49:29Z

Changing the signature of formatter<join_view>::format to

 template <typename FormatContext> 
 auto format(join_view<It, Sentinel, Char>& value, 
             FormatContext& ctx) const -> decltype(ctx.out())

(that is, removing the const from the join_view argument) generates the following from Clang:

/Users/tristan/Coding/fmt/include/fmt/base.h:1522:63: error: implicit instantiation of undefined template 'fmt::detail::type_is_unformattable_for<const fmt::join_view<unsigned char *, unsigned char *>, char>'
 1522 |     type_is_unformattable_for<T, typename Context::char_type> _;
      |                                                               ^
/Users/tristan/Coding/fmt/include/fmt/base.h:1908:20: note: in instantiation of function template specialization 'fmt::detail::make_arg<true, fmt::generic_context<std::back_insert_iterator<std::string>, char>, const fmt::join_view<unsigned char *, unsigned char *>, 0>' requested here
 1908 |   return {{detail::make_arg<NUM_ARGS <= detail::max_packed_args, Context>(
      |                    ^
/Users/tristan/Coding/fmt/include/fmt/compile.h:201:14: note: in instantiation of function template specialization 'fmt::make_format_args<fmt::generic_context<std::back_insert_iterator<std::string>, char>, const fmt::join_view<unsigned char *, unsigned char *>, 1UL, 0UL, 15ULL, 0>' requested here
  201 |         fmt::make_format_args<basic_format_context<OutputIt, Char>>(args...);
      |              ^
/Users/tristan/Coding/fmt/include/fmt/compile.h:438:6: note: in instantiation of function template specialization 'fmt::detail::spec_field<char, fmt::join_view<unsigned char *, unsigned char *>, 0>::format<std::back_insert_iterator<std::string>, fmt::join_view<unsigned char *, unsigned char *>>' requested here
  438 |   cf.format(std::back_inserter(s), args...);
      |      ^
/Users/tristan/Coding/fmt/include/fmt/compile.h:472:17: note: in instantiation of function template specialization 'fmt::format<fmt::detail::spec_field<char, fmt::join_view<unsigned char *, unsigned char *>, 0>, fmt::join_view<unsigned char *, unsigned char *>, char, 0>' requested here
  472 |     return fmt::format(compiled, std::forward<Args>(args)...);
      |                 ^
/Users/tristan/Coding/fmt/test/compile-test.cc:185:28: note: in instantiation of function template specialization 'fmt::format<FMT_COMPILE_STRING, fmt::join_view<unsigned char *, unsigned char *>, 0>' requested here
  185 |   EXPECT_EQ("0102af", fmt::format(FMT_COMPILE("{:02x}"), fmt::join(data, "")));
      |                            ^
/Users/tristan/Coding/fmt/include/fmt/base.h:1497:8: note: template is declared here
 1497 | struct type_is_unformattable_for;
      |        ^
/Users/tristan/Coding/fmt/include/fmt/base.h:1525:7: error: static assertion failed due to requirement 'formattable': Cannot format an argument. To make type T formattable provide a formatter<T> specialization: https://fmt.dev/latest/api.html#udt
 1525 |       formattable,
      |       ^~~~~~~~~~~
In file included from /Users/tristan/Coding/fmt/test/compile-test.cc:8:
/Users/tristan/Coding/fmt/include/fmt/compile.h:203:16: error: no matching member function for call to 'format'
  203 |     return fmt.format(get_arg_checked<T, N>(args...), ctx);
      |            ~~~~^~~~~~
/Users/tristan/Coding/fmt/include/fmt/compile.h:438:6: note: in instantiation of function template specialization 'fmt::detail::spec_field<char, fmt::join_view<unsigned char *, unsigned char *>, 0>::format<std::back_insert_iterator<std::string>, fmt::join_view<unsigned char *, unsigned char *>>' requested here
  438 |   cf.format(std::back_inserter(s), args...);
      |      ^
/Users/tristan/Coding/fmt/include/fmt/compile.h:472:17: note: in instantiation of function template specialization 'fmt::format<fmt::detail::spec_field<char, fmt::join_view<unsigned char *, unsigned char *>, 0>, fmt::join_view<unsigned char *, unsigned char *>, char, 0>' requested here
  472 |     return fmt::format(compiled, std::forward<Args>(args)...);
      |                 ^
/Users/tristan/Coding/fmt/test/compile-test.cc:185:28: note: in instantiation of function template specialization 'fmt::format<FMT_COMPILE_STRING, fmt::join_view<unsigned char *, unsigned char *>, 0>' requested here
  185 |   EXPECT_EQ("0102af", fmt::format(FMT_COMPILE("{:02x}"), fmt::join(data, "")));
      |                            ^
/Users/tristan/Coding/fmt/include/fmt/ranges.h:588:8: note: candidate function template not viable: 1st argument ('const fmt::join_view<unsigned char *, unsigned char *>') would lose const qualifier
  588 |   auto format(join_view<It, Sentinel, Char>& value,
      |        ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 errors generated.

From the last line, it seems that something is trying to pass a const join_view to the format function, but I'm not sure where that call is coming from

tcbrindle · 2024-01-12T17:33:05Z

GCC gives a little more context: an error occurs in the return value of spec_field::format:

fmt/include/fmt/compile.h

Lines 197 to 204 in 3c96084

    
           template <typename OutputIt, typename... Args> 
        
           constexpr FMT_INLINE OutputIt format(OutputIt out, 
        
                                                const Args&... args) const { 
        
             const auto& vargs = 
        
                 fmt::make_format_args<basic_format_context<OutputIt, Char>>(args...); 
        
             basic_format_context<OutputIt, Char> ctx(out, vargs); 
        
             return fmt.format(get_arg_checked<T, N>(args...), ctx); 
        
           }

Here, get_arg_checked<T, N>(args...) returns const join_view&, which causes the error as it cannot be passed to the join_view's modified format function. get_checked_arg says

fmt/include/fmt/compile.h

Lines 128 to 130 in 3c96084

    
           // This ensures that the argument type is convertible to `const T&`. 
        
           template <typename T, int N, typename... Args> 
        
           constexpr const T& get_arg_checked(const Args&... args) {

so I guess adding the const& is intentional, but I don't get why this triggers an error when using join and not with plain fmt::format("{}", my_range)?

vitaut · 2024-01-12T17:45:45Z

Looks like we need to propagate (lack of) constness during format string compilation. Currently it passes everything by const&.

dvirtz · 2024-04-09T16:42:45Z

I get a similar error without join: https://godbolt.org/z/nqGx9Yb3T

/opt/compiler-explorer/libs/fmt/trunk/include/fmt/ranges.h:473:44: error: use of deleted function 'std::counted_iterator<std::generator<int>::_Iterator>::counted_iterator(const std::counted_iterator<std::generator<int>::_Iterator>&)'
  473 |     if (is_debug) return write_debug_string(out, it, end);

Fixes issue fmtlib#3802

Addresses issue (fmtlib#3802) For input_ranges (ie. a range with an input iterator backing it) we should not copy the iterator and should mutate the iterator on the view. For forward_ranges (or better), we can keep using them as const and make a copy of the iterator etc. This is an issue for code like: std::istringstream iss("1 2 3 4 5"); auto view = std::views::istream<int>(iss) fmt::join(std::move(view), ", ") Since the std::ranges::basic_istream_view::__iterator is not copyable And the struct formatter<join_view<It, Sentinel, Char>, Char> only has a template <typename FormatContext> auto format(const join_view<It, Sentinel, Char>& value, FormatContext& ctx) const -> decltype(ctx.out()) { auto it = value.begin; Which takes the value param by const ref and copies the iterator Fix is disabling the const ref format function overload when we have an input iterator passed to our join_view, and instead then just mutate the iterator in place and use a mutable join_view&

Addresses issue (fmtlib#3802) For input_ranges (ie. a range with an input iterator backing it) we should not copy the iterator and should mutate the iterator on the view. For forward_ranges (or better), we can keep using them as const and make a copy of the iterator etc. This is an issue for code like: std::istringstream iss("1 2 3 4 5"); auto view = std::views::istream<int>(iss) fmt::join(std::move(view), ", ") Since the std::ranges::basic_istream_view::__iterator is not copyable And the struct formatter<join_view<It, Sentinel, Char>, Char> only has a template <typename FormatContext> auto format(const join_view<It, Sentinel, Char>& value, FormatContext& ctx) const -> decltype(ctx.out()) { auto it = value.begin; Which takes the value param by const ref and copies the iterator Fix is disabling the const ref format function overload when we have an input iterator passed to our join_view, and instead then just mutate the iterator in place and use a mutable join_view& overload for this case

If iterator not copyable mutate the underlying iterator Notably std::ranges::basic_istream_view::__iterator Addresses issue (fmtlib#3802)

If iterator not copyable mutate the underlying iterator Notably std::ranges::basic_istream_view::__iterator Addresses issue (#3802)

vitaut · 2024-05-06T14:52:20Z

Fixed by #3946.

vitaut closed this as completed Jan 10, 2024

vitaut reopened this Jan 10, 2024

vitaut added the help wanted label Jan 10, 2024

hmbj added a commit to hmbj/fmt that referenced this issue Apr 14, 2024

Fix format_to + FMT_STRING for wide character type

53c5bd7

Fixes issue fmtlib#3802

hmbj mentioned this issue Apr 14, 2024

Fix format_to + FMT_STRING for wide character type #3931

Merged

Arghnews mentioned this issue May 1, 2024

Fix fmt::join for views with input iterators #3946

Merged

Arghnews added a commit to Arghnews/fmt that referenced this issue May 5, 2024

Enable fmt::join for uncopyable iterators

7013466

If iterator not copyable mutate the underlying iterator Notably std::ranges::basic_istream_view::__iterator Addresses issue (fmtlib#3802)

Arghnews added a commit to Arghnews/fmt that referenced this issue May 5, 2024

Enable fmt::join for uncopyable iterators

9e0ba96

If iterator not copyable mutate the underlying iterator Notably std::ranges::basic_istream_view::__iterator Addresses issue (fmtlib#3802)

vitaut pushed a commit that referenced this issue May 5, 2024

Enable fmt::join for uncopyable iterators (#3946)

10508a3

If iterator not copyable mutate the underlying iterator Notably std::ranges::basic_istream_view::__iterator Addresses issue (#3802)

vitaut closed this as completed May 6, 2024

BrewTestBot mentioned this issue Jul 1, 2024

fmt 11.0.0 Homebrew/homebrew-core#176079

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fmt::join doesn't handle move-only iterators #3802

fmt::join doesn't handle move-only iterators #3802

tcbrindle commented Jan 10, 2024

vitaut commented Jan 10, 2024 •

edited

Loading

vitaut commented Jan 10, 2024

tcbrindle commented Jan 10, 2024

tcbrindle commented Jan 12, 2024

vitaut commented Jan 12, 2024

tcbrindle commented Jan 12, 2024

tcbrindle commented Jan 12, 2024

vitaut commented Jan 12, 2024

dvirtz commented Apr 9, 2024 •

edited

Loading

vitaut commented May 6, 2024 •

edited

Loading

fmt::join doesn't handle move-only iterators #3802

fmt::join doesn't handle move-only iterators #3802

Comments

tcbrindle commented Jan 10, 2024

vitaut commented Jan 10, 2024 • edited Loading

vitaut commented Jan 10, 2024

tcbrindle commented Jan 10, 2024

tcbrindle commented Jan 12, 2024

vitaut commented Jan 12, 2024

tcbrindle commented Jan 12, 2024

tcbrindle commented Jan 12, 2024

vitaut commented Jan 12, 2024

dvirtz commented Apr 9, 2024 • edited Loading

vitaut commented May 6, 2024 • edited Loading

vitaut commented Jan 10, 2024 •

edited

Loading

dvirtz commented Apr 9, 2024 •

edited

Loading

vitaut commented May 6, 2024 •

edited

Loading