Skip to content

[libc++][format] Improves diagnostics. #127234

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[libc++][format] Improves diagnostics. #127234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 7 additions & 12 deletions libcxx/include/__format/format_arg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ class __basic_format_arg_value {
__format_([](basic_format_parse_context<_CharT>& __parse_ctx, _Context& __ctx, const void* __ptr) {
using _Dp = remove_const_t<_Tp>;
using _Qp = conditional_t<__formattable_with<const _Dp, _Context>, const _Dp, _Dp>;
static_assert(__formattable_with<_Qp, _Context>, "Mandated by [format.arg]/10");
static_assert(__formattable_with<_Qp, _Context>, "Mandated by [format.arg]/12");

typename _Context::template formatter_type<_Dp> __f;
__parse_ctx.advance_to(__f.parse(__parse_ctx));
Expand Down Expand Up @@ -334,21 +334,16 @@ class _LIBCPP_TEMPLATE_VIS _LIBCPP_NO_SPECIALIZATIONS basic_format_arg {
private:
using char_type = typename _Context::char_type;

// TODO FMT Implement constrain [format.arg]/4
// Constraints: The template specialization
// typename Context::template formatter_type<T>
// meets the Formatter requirements ([formatter.requirements]). The extent
// to which an implementation determines that the specialization meets the
// Formatter requirements is unspecified, except that as a minimum the
// expression
// typename Context::template formatter_type<T>()
// .format(declval<const T&>(), declval<Context&>())
// shall be well-formed when treated as an unevaluated operand.

public:
__basic_format_arg_value<_Context> __value_;
__format::__arg_t __type_;

// This constructor is used for the exposition only constructor.
// Per [format.arg]/4
// template<class T> explicit basic_format_arg(T& v) noexcept;
// Constraints: T satisfies formattable-with<Context>.
//
// This constraint is implemented in __create_format_arg
_LIBCPP_HIDE_FROM_ABI explicit basic_format_arg(__format::__arg_t __type,
__basic_format_arg_value<_Context> __value) noexcept
: __value_(__value), __type_(__type) {}
Expand Down
150 changes: 148 additions & 2 deletions libcxx/include/__format/format_arg_store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@

#include <__concepts/arithmetic.h>
#include <__concepts/same_as.h>
#include <__concepts/semiregular.h>
#include <__config>
#include <__format/concepts.h>
#include <__format/format_arg.h>
#include <__format/format_parse_context.h>
#include <__type_traits/conditional.h>
#include <__type_traits/extent.h>
#include <__type_traits/is_assignable.h>
#include <__type_traits/is_constructible.h>
#include <__type_traits/remove_const.h>
#include <cstdint>
#include <string>
Expand Down Expand Up @@ -161,12 +165,11 @@ consteval __arg_t __determine_arg_t() {
//
// Modeled after template<class T> explicit basic_format_arg(T& v) noexcept;
// [format.arg]/4-6
template <class _Context, class _Tp>
template <class _Context, __formattable_with<_Context> _Tp>
Copy link
Member

@ldionne ldionne Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only add this constraint like you've done here but we don't use __diagnose_invalid_formatter, what do we get? I would assume that the compiler machinery for not satisfying a concept should kick in, and it should give us an hopefully fairly reasonable diagnostic? I am a bit concerned to try to replace how the compiler presents concept errors to the user -- it seems that there is no end to the complexity that this can have.

_LIBCPP_HIDE_FROM_ABI basic_format_arg<_Context> __create_format_arg(_Tp& __value) noexcept {
using _Dp = remove_const_t<_Tp>;
constexpr __arg_t __arg = __format::__determine_arg_t<_Context, _Dp>();
static_assert(__arg != __arg_t::__none, "the supplied type is not formattable");
static_assert(__formattable_with<_Tp, _Context>);

// Not all types can be used to directly initialize the
// __basic_format_arg_value. First handle all types needing adjustment, the
Expand Down Expand Up @@ -205,6 +208,149 @@ _LIBCPP_HIDE_FROM_ABI basic_format_arg<_Context> __create_format_arg(_Tp& __valu
return basic_format_arg<_Context>{__arg, __value};
}

// Helper function that issues a diagnostic when __formattable_with<_Tp, _Context> is false.
//
// Since it's quite easy to make a mistake writing a formatter specialization
// this function tries to give a better explanation. This should improve the
// diagnostics when trying to format type that has no properly specialized
// formatter.
template <class _Context, class _Tp>
[[noreturn]] _LIBCPP_HIDE_FROM_ABI constexpr void __diagnose_invalid_formatter() {
using _Formatter = typename _Context::template formatter_type<remove_const_t<_Tp>>;
constexpr bool __is_disabled =
!is_default_constructible_v<_Formatter> && !is_copy_constructible_v<_Formatter> &&
!is_move_constructible_v<_Formatter> && !is_copy_assignable_v<_Formatter> && !is_move_assignable_v<_Formatter>;
constexpr bool __is_semiregular = semiregular<_Formatter>;

constexpr bool __has_parse_function =
requires(_Formatter& __f, basic_format_parse_context<typename _Context::char_type> __pc) {
{ __f.parse(__pc) };
};
constexpr bool __correct_parse_function_return_type =
requires(_Formatter& __f, basic_format_parse_context<typename _Context::char_type> __pc) {
{ __f.parse(__pc) } -> same_as<typename decltype(__pc)::iterator>;
};

// The reason these static_asserts are placed in an if-constexpr-chain is to
// only show one error. For example, when the formatter is not specialized it
// would show all static_assert messages. With this chain the compiler only
// evaluates one static_assert.

if constexpr (__is_disabled)

static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization has not been provided.");
else if constexpr (!__is_semiregular)
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization is not semiregular.");

else if constexpr (!__has_parse_function)
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization does not have a parse function taking the proper arguments.");
else if constexpr (!__correct_parse_function_return_type)
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
Comment on lines +270 to +274
Copy link
Member

@ldionne ldionne Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
sizeof(_Tp) == 0

IMO the added complexity of using sizeof(_Tp) == 0 isn't worth carrying a compiler-dependent diff, even if it goes away soon.

,
"The required formatter specialization's parse function does not return the required type.");

else {
// During constant evaluation this function is called, but the format
// member function has not been evaluated. This means these functions
// can't be evaluated at that time.
//
// Note this else branch should never been taken during constant
// eveluation, the static_asserts in one of the branches above should
// trigger.
constexpr bool __has_format_function = requires(_Formatter& __f, _Tp&& __t, _Context __fc) {
{ __f.format(__t, __fc) };
};
constexpr bool __is_format_function_const_qualified = requires(const _Formatter& __cf, _Tp&& __t, _Context __fc) {
{ __cf.format(__t, __fc) };
};
constexpr bool __correct_format_function_return_type = requires(_Formatter& __f, _Tp&& __t, _Context __fc) {
{ __f.format(__t, __fc)->template same_as<typename _Context::iterator> };
};

if constexpr (!__has_format_function)
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization does not have a format function taking the proper arguments.");
else if constexpr (!__is_format_function_const_qualified)
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization's format function is not const qualified.");
else if constexpr (!__correct_format_function_return_type)
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization's format function does not return the required type.");

else
// This should not happen; it makes sure the code is ill-formed.
static_assert(
# if defined(_LIBCPP_APPLE_CLANG_VER) && _LIBCPP_APPLE_CLANG_VER < 1600
sizeof(_Tp) == 0
# else
false
# endif
,
"The required formatter specialization is not formattable with its context.");
}
}

// The Psuedo constructor is constrained per [format.arg]/4.
Copy link
Member

@ldionne ldionne Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The Psuedo constructor is constrained per [format.arg]/4.
// The pseudo constructor is constrained per [format.arg]/4.

// The only way for a user to call __create_format_arg is from std::make_format_args.
// Per [format.arg.store]/3
// template<class Context = format_context, class... Args>
// format-arg-store<Context, Args...> make_format_args(Args&... fmt_args);
//
// Preconditions: The type typename Context::template formatter_type<remove_const_t<Ti>>
// meets the BasicFormatter requirements ([formatter.requirements]) for each Ti in Args.
//
// This function is only called when the precondition is violated.
// Without this function the compilation would fail since the overload is
// SFINAE'ed away. This function is used to provide better diagnostics.
template <class _Context, class _Tp>
_LIBCPP_HIDE_FROM_ABI basic_format_arg<_Context> __create_format_arg(_Tp&) noexcept {
__format::__diagnose_invalid_formatter<_Context, _Tp>();
}

template <class _Context, class... _Args>
_LIBCPP_HIDE_FROM_ABI void
__create_packed_storage(uint64_t& __types, __basic_format_arg_value<_Context>* __values, _Args&... __args) noexcept {
Expand Down
8 changes: 6 additions & 2 deletions libcxx/include/__format/format_context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,13 @@ __format_context_create(_OutIt __out_it, basic_format_args<basic_format_context<
}
# endif

using format_context = basic_format_context<back_insert_iterator<__format::__output_buffer<char>>, char>;
template <class _CharT>
using __format_context _LIBCPP_NODEBUG =
basic_format_context<back_insert_iterator<__format::__output_buffer<_CharT>>, _CharT>;

using format_context = __format_context<char>;
# if _LIBCPP_HAS_WIDE_CHARACTERS
using wformat_context = basic_format_context< back_insert_iterator<__format::__output_buffer<wchar_t>>, wchar_t>;
using wformat_context = __format_context<wchar_t>;
# endif

template <class _OutIt, class _CharT>
Expand Down
31 changes: 31 additions & 0 deletions libcxx/include/__format/format_functions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <__algorithm/clamp.h>
#include <__concepts/convertible_to.h>
#include <__concepts/same_as.h>
#include <__concepts/semiregular.h>
#include <__config>
#include <__format/buffer.h>
#include <__format/format_arg.h>
Expand Down Expand Up @@ -78,6 +79,27 @@ template <class... _Args>

namespace __format {

/// A "watered-down" __formattable_with for usage during constant evaluation.
///
/// The member function "format" can have a decuded return type. When
/// constraining the __compile_time_handle this leads to a nested concept
/// eveluation that is always false. Instead ignore the "format" member function.
///
/// In theory the same could happen to the "parse" member function. However
/// thisfunction does not is less likely to have odd cased, and this function
/// is "required" to be called during constant evaluation. So if this
/// function's return type needs to be deduced things will probably not work.
///
/// See https://github.com/llvm/llvm-project/issues/81590 and
/// test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp for
/// an example of a problematic "format" member function.
Comment on lines +82 to +95
Copy link
Member

@ldionne ldionne Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A "watered-down" __formattable_with for usage during constant evaluation.
///
/// The member function "format" can have a decuded return type. When
/// constraining the __compile_time_handle this leads to a nested concept
/// eveluation that is always false. Instead ignore the "format" member function.
///
/// In theory the same could happen to the "parse" member function. However
/// thisfunction does not is less likely to have odd cased, and this function
/// is "required" to be called during constant evaluation. So if this
/// function's return type needs to be deduced things will probably not work.
///
/// See https://github.com/llvm/llvm-project/issues/81590 and
/// test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp for
/// an example of a problematic "format" member function.
// A "watered-down" __formattable_with for usage during constant evaluation.
//
// The member function `format` can have a deduced return type. When that is
// the case and when that `format` function also requires calling `std::format`
// or related APIs, this can lead to a recursive evaluation of the `formattable`
// concept when trying to instantiate `__compile_time_handle::__enable()`.
// This leads to a horrible error message and can in fact reject valid code.
//
// In theory the same could happen to the `parse` member function. However
// this function is less likely to have a deduced return type, and it is required
// to be constexpr-friendly if one wants to call a `<format>` API that performs
// compile-time validation. That differs from the `format` member function,
// which is never required to be constexpr-friendly.
//
// For that reason, when we're doing constant evaluation, we check for a
// reduced `__formattable_with_constexpr` concept that omits checking
// the `format` function (but not the `parse` function).
//
// See https://github.com/llvm/llvm-project/issues/81590 and
// test/std/utilities/format/format.functions/bug_81590.compile.pass.cpp for
// an example of a problematic "format" member function.

template <class _Tp, class _Context, class _Formatter = typename _Context::template formatter_type<remove_const_t<_Tp>>>
concept __formattable_with_constexpr =
semiregular<_Formatter> &&
requires(_Formatter& __f, _Tp&& __t, basic_format_parse_context<typename _Context::char_type> __pc) {
{ __f.parse(__pc) } -> same_as<typename decltype(__pc)::iterator>;
};

/// Helper class parse and handle argument.
///
/// When parsing a handle which is not enabled the code is ill-formed.
Expand All @@ -91,13 +113,22 @@ class _LIBCPP_TEMPLATE_VIS __compile_time_handle {
}

template <class _Tp>
requires __formattable_with_constexpr<_Tp, __format_context<_CharT>>
_LIBCPP_HIDE_FROM_ABI constexpr void __enable() {
__parse_ = [](basic_format_parse_context<_CharT>& __ctx) {
formatter<_Tp, _CharT> __f;
__ctx.advance_to(__f.parse(__ctx));
Comment on lines 115 to 120
Copy link
Member

@ldionne ldionne Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
template <class _Tp>
requires __formattable_with_constexpr<_Tp, __format_context<_CharT>>
_LIBCPP_HIDE_FROM_ABI constexpr void __enable() {
__parse_ = [](basic_format_parse_context<_CharT>& __ctx) {
formatter<_Tp, _CharT> __f;
__ctx.advance_to(__f.parse(__ctx));
template <class _Tp>
_LIBCPP_HIDE_FROM_ABI constexpr void __enable() {
static_assert(__formattable_with_constexpr<_Tp, __format_context<_CharT>>, "message");
if constexpr (__formattable_with_constexpr<_Tp, __format_context<_CharT>>
__parse_ = [](basic_format_parse_context<_CharT>& __ctx) {
formatter<_Tp, _CharT> __f;
__ctx.advance_to(__f.parse(__ctx));
}

Perhaps that is also worth exploring? It's less heavyweight than the current approach. I still think the if constexpr goes against the goal of not reimplementing how the compiler issues diagnostics, but that could potentially be something that we get consensus on.

};
}

template <class _Tp>
_LIBCPP_HIDE_FROM_ABI constexpr void __enable() {
// Avoids the throw set in the constructor.
// Otherwise two diagnostics will be issued.
__parse_ = [](basic_format_parse_context<_CharT>&) {};
__format::__diagnose_invalid_formatter<__format_context<_CharT>, _Tp>();
}

// Before calling __parse the proper handler needs to be set with __enable.
// The default handler isn't a core constant expression.
_LIBCPP_HIDE_FROM_ABI constexpr __compile_time_handle()
Expand Down
Loading