Skip to content
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

fmtlib relies on arg_mapper implicit ctor declaration before class specialization is made complete. #2635

Closed
seanbaxter opened this issue Dec 7, 2021 · 6 comments

Comments

@seanbaxter
Copy link

seanbaxter commented Dec 7, 2021

fmtlib has thrown me for a huge loop. I have an issue related to caching of alias template specializations. After a lot of hunting, I determined it's caused by this trailing return type during class template instantiation:

fmt/include/fmt/core.h

Lines 1360 to 1361 in 9d5b9de

FMT_CONSTEXPR FMT_INLINE auto map(const signed char* val)
-> decltype(this->map("")) {

The return type is calling this->map(""), but this (arg_mapper) is still an incomplete type - I've only instantiated the functions above the map at line 1360 at this point.

When overload resolution considers the constructor at line 1345, the std::is_constructible<> constraint on 1342 ends up (many levels of indirection down) attempting to construct an instance of arg_mapper in an unevaluated context. However, this attempt fails, because no default ctor is declared because I haven't finished injecting all the member functions, and therefore no implicit declaration of the default ctor.

The problem is that this lack of ctor causes an alias template to fail substitution, and I've noted that failure in my alias template cache. After the arg_mapper class is complete, there's another is_formattable evaluation on the same arguments. This time it should succeed, but I've already cached it is a SFINAE failure, so am returning that, which causes the range test to fail.

/usr/include/c++/10/type_traits:906:5 - is_constructible
/usr/include/c++/10/type_traits:900:5 - __is_constructible_impl
Here the __is_constructible compiler builtin is invoked with T = formatter<char[1], char, void>

The partial specializations of formatter are examined, since one must be selected in order to consider construction. The ranges partial makes things go haywire:

struct formatter<

This guy is in the list of constraints:

is_formattable<detail::value_type<T>, Char>::value

That alias template is defined like so:

fmt/include/fmt/core.h

Lines 1776 to 1781 in 9d5b9de

template <typename T, typename Char = char>
using is_formattable = bool_constant<
!std::is_base_of<detail::unformattable,
decltype(detail::arg_mapper<buffer_context<Char>>().map(
std::declval<T>()))>::value &&
!detail::has_fallback_formatter<T, Char>::value>;

The compiler attempts to construct arg_mapper in this unevaluated context:

detail::arg_mapper<buffer_context<Char>>()

However, arg_mapper is still incomplete, because we're still trying to evaluate the trailing-return-type for the map functions. Therefore, the implicit default ctor hasn't yet been declared. __is_constructible returns false, since there is no yet-declared ctor, and the alias template specialization is_formattable gets its SFINAE failure written to the alias template cache.

This substitution failure doesn't effect arg_mapper's definition at all. All it did was knock this ranges formatter partial out of the candidate set, but that wasn't going to be the partial chosen, since there are character formatters that are chosen.

struct formatter<

The problem is that later on we really do need this partial template definition of formatter:

(void)fmt::format(FMT_STRING("{}"), hello);

The vector formatter is supposed to match it. But we've already attempted evaluated is_formattable on those same arguments, and it failed, so we just use that cached value, and this time the program is ill-formed, since there is no partial candidate.

You have a note about an MSVC workaround at this place:

// Workaround a bug in MSVC 2019 and earlier.

Was there a discussion about this? I don't know if this is a compiler bug or a library bug. Should two specializations of the same alias template with the same arguments be considered equivalent?

Either way, I think it's very dangerous to be writing this->map("") in your trailing-return-type, since the arg_mapper class is incomplete, you're not getting an accurate picture of the class.

FWIW, I've never experienced a break like this with Hana, Boost.PFR, mdspan or the other really advanced template libraries. I think it's cursed code.

If we removed the trailing-return-type from those map overloads and rely on return type deduction, everything works:

  FMT_CONSTEXPR FMT_INLINE auto map(const signed char* val) {
    return map(reinterpret_cast<const char*>(val));
  }

Since it's in the body of the function, it's not instantiated until after the arg_mapper specialization is made complete, so the arg_mapper implicit ctor is defined.

;tldr:
Can we not do decltype(this->map("")) in the return type and instead just name the type?

@vitaut
Copy link
Contributor

vitaut commented Dec 10, 2021

That's an interesting one, thanks for reporting.

Was there a discussion about this?

I don't think so.

Can we not do decltype(this->map("")) in the return type and instead just name the type?

Yes, done in ac1b5f3. Does it solve the issue?

@seanbaxter
Copy link
Author

Yes, that does fix it. It's a bummer there is no diagnostic to indicate when an incomplete type is used in this way.

@vitaut
Copy link
Contributor

vitaut commented Dec 10, 2021

Pointed all links to the correct commit instead of master for future reference.

@vitaut
Copy link
Contributor

vitaut commented Dec 10, 2021

@seanbaxter, although the issue has been fixed, I'm still trying to make sure that I fully understand the problem and have a few questions.

When overload resolution considers the constructor at line 1345

Line 1345 contains the map function:

FMT_CONSTEXPR FMT_INLINE auto map(const T& val)
Are you referring to it and not the constructor in this sentence?

the std::is_constructible<> constraint on 1342 ends up (many levels of indirection down) attempting to construct an instance of arg_mapper in an unevaluated context.

Why does it try to construct arg_mapper? The is_constructible constraint in question only checks basic_string_view constructibility:

std::is_constructible<basic_string_view<char_type>, T>::value &&
Are you referring to some other is_constructible?

You have a note about an MSVC workaround at this place

Are you suggesting that this might be the same issue? I tried disabling the workaround after the current fix but it didn't help: https://github.com/fmtlib/fmt/runs/4479740148?check_suite_focus=true.

@seanbaxter
Copy link
Author

seanbaxter commented Dec 10, 2021

Are you referring to it and not the constructor in this sentence?

My bad. Yes, the member function map.

The is_constructible that fails is part of the has_formatter alias template, not the one on the basic_string_view.

  template <typename T,
            FMT_ENABLE_IF(
                std::is_constructible<basic_string_view<char_type>, T>::value &&
                !is_string<T>::value && !has_formatter<T, Context>::value &&
                !has_fallback_formatter<T, char_type>::value)>
  FMT_CONSTEXPR FMT_INLINE auto map(const T& val)
      -> basic_string_view<char_type> {
    return basic_string_view<char_type>(val);
  }

has_formatter<T, Context>::value. :(

I redid my investigation, and think this is a cleaner presentation:

Starting from the top:

1

instantiate function format:

FMT_NODISCARD FMT_INLINE auto format(format_string<T...> fmt, T&&... args)

2

instantiate fmt::format_arg_store<fmt::basic_format_context<fmt::appender, char>, int>:

class format_arg_store

3

instantiate the initializer for
fmt::format_arg_store<fmt::basic_format_context<fmt::appender, char>, int>::desc:

static constexpr unsigned long long desc =

4

instantiate function unsigned long long fmt::detail::encode_types():

constexpr auto encode_types() -> unsigned long long {

5

instantiate alias template mapped_type_constant<int, fmt::basic_format_context<fmt::appender, char> >:

return static_cast<unsigned>(mapped_type_constant<Arg, Context>::value) |

6

instantiate class template arg_mapper<fmt::basic_format_context<fmt::appender, char> >:

type_constant<decltype(arg_mapper<Context>().map(std::declval<const T&>())),

The compiler forms the declarations for all member functions. That involves
substituting the trailing-return-type for map:

-> decltype(this->map("")) {

7

Overload resolution looks at all the currently declared map member functions.
Problems begin on this one:

FMT_CONSTEXPR FMT_INLINE auto map(const T& val)

  template <typename T,
            FMT_ENABLE_IF(
                std::is_constructible<basic_string_view<char_type>, T>::value &&
                !is_string<T>::value && !has_formatter<T, Context>::value &&
                !has_fallback_formatter<T, char_type>::value)>
  FMT_CONSTEXPR FMT_INLINE auto map(const T& val)
      -> basic_string_view<char_type> {
    return basic_string_view<char_type>(val);
  }

The constraint that causes all heck to break loose is

!has_formatter<T, Context>::value

8

using has_formatter =

// Specifies if T has an enabled formatter specialization. A type can be
// formattable even if it doesn't have a formatter e.g. via a conversion.
template <typename T, typename Context>
using has_formatter =
    std::is_constructible<typename Context::template formatter_type<T>>;

The formatter type is fmt::formatter<char[1], char, void>.

9

fmtlib has 23 partial specializations for formatter. Partial 22/23 is defined here:

struct formatter<

template <typename T, typename Char>
struct formatter<
    T, Char,
    enable_if_t<
        fmt::is_range<T, Char>::value
// Workaround a bug in MSVC 2019 and earlier.
#if !FMT_MSC_VER
        && (is_formattable<detail::value_type<T>, Char>::value ||
            detail::has_fallback_formatter<detail::value_type<T>, Char>::value)
#endif
        >> {

The is_formattable<char, char> specialization
T = char and Char = char.

using is_formattable = bool_constant<

template <typename T, typename Char = char>
using is_formattable = bool_constant<
    !std::is_base_of<detail::unformattable,
                     decltype(detail::arg_mapper<buffer_context<Char>>().map(
                         std::declval<T>()))>::value &&
    !detail::has_fallback_formatter<T, Char>::value>;

The is_formattable alias template calls arg_mapper::map, but we're already in a call to arg_mapper::map! The class's implicit default ctor hasn't been injected yet, because the class is still incomplete. Therefore, detail::arg_mapper<buffer_context<Char>>() is a substitution failure, making is_formattible on these arguments a substitution failure. In this context, the substitution failure is okay, because it merely disqualifies formatter partial specialization 22/23. But later on, when we actually want this partial specialization to work, we've already evaluated the is_formattable<char, char> type as a substitution failure, so partial 22/23 will be skipped over.

10

This is the problem with incomplete and the equivalence of alias template specializations. The standard says that alias template specializations are equivalent for the purpose of redeclarations:

template<typename T>      // #1
f(alias<T>);

// Other stuff!

// This must be
template<typename T>      // #2
f(alias<T>);

#2 must match #1, even though "other stuff" could change what this alias specialization actually resolves to. So where is the equivalence of alias template specializations expected? Nobody really knows. Certainly for redeclarations. I was hoping it would hold generally, because that really speeds up build times. I've never had a problem until this this->map("") trailing return type fiasco.

@vitaut
Copy link
Contributor

vitaut commented Dec 11, 2021

Thanks a lot for the detailed explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants