Report error on duplicate named arg names #4367

dinomight · 2025-02-26T15:10:35Z

Compile error will happen if static named args are used and enabled. Runtime exception will be thrown in other cases.
#4282

fmtlib#4282

dinomight · 2025-02-26T15:41:24Z

Wanted to mention that this does not handle the case of using FMT_COMPILE strings. That infrastructure is a bit different and doesn't utilize either of the functions I've updated here. It should be doable for FMT_COMPILE string, but might require additional memory to do the duplicate name check. I'm not sure what the preferred technique is there. Should I file a separate ticket to track?

vitaut

Thanks for the PR!

vitaut · 2025-02-28T21:16:04Z

include/fmt/base.h

+  for (auto i = 0; i < named_arg_index; ++i) {
+    if (basic_string_view<Char>(named_args[i].name) ==
+        basic_string_view<Char>(arg.name)) {
+      report_error("duplicate named args found");
+    }
+  }


Please move it to a separate function in detail and reuse here and below.

vitaut · 2025-02-28T21:16:37Z

include/fmt/base.h

@@ -1071,6 +1071,12 @@ void init_named_arg(named_arg_info<Char>*, int& arg_index, int&, const T&) {
 template <typename Char, typename T, FMT_ENABLE_IF(is_named_arg<T>::value)>
 void init_named_arg(named_arg_info<Char>* named_args, int& arg_index,
                    int& named_arg_index, const T& arg) {
+  for (auto i = 0; i < named_arg_index; ++i) {
+    if (basic_string_view<Char>(named_args[i].name) ==
+        basic_string_view<Char>(arg.name)) {


It's better to move this out of the loop to avoid recomputing length.

vitaut · 2025-02-28T21:19:08Z

test/compile-error-test/CMakeLists.txt

+  expect_compile(format-string-duplicate-name-error "
+    #if defined(FMT_HAS_CONSTEVAL) && FMT_USE_NONTYPE_TEMPLATE_ARGS
+      using namespace fmt::literals;
+      fmt::print(\"{bar}\", \"bar\"_a=42, \"bar\"_a=43);
+    #else
+      #error
+    #endif
+  " ERROR)


These tests are expensive and we don't need to use them here, please remove.

What are the situations where you'd want to add these? I figured it was important to verify that compile fails in this situation.

It is basically an integration test that checks that the internal checking machinery translates into actual compiler errors. It shouldn't be used for unit testing.

I guess I'm a little confused because that's what this is doing. I wrote a unit test for the runtime exception version. I figured since there's also a compile time failure, a test would be required to make sure it functions, especially since the two routes are different in the code.

You could unit test that exception is thrown but I think it's fine as is.

I did add a test to verify dynamic named args cause exceptions at runtime. In order to compile the above lines and verify it throws would require I only enable the test when we're not using non-type template parameters. In that situation, it just devolves to the dynamic named args and we don't test the static, compile-time check machinery, right?

Compile-time check machinery just translates exceptions to compile-time errors and it's (roughly) the same for all errors so it doesn't need to be tested multiple times. It's sufficient to test the exception path.

vitaut · 2025-02-28T21:22:04Z

Should I file a separate ticket to track?

Just add a comment in #4124 since it's part of general named argument support in format string compilation.

dinomight · 2025-03-01T15:58:59Z

I'm not sure why the refactoring made GCC13 unhappy, but it's complaining about possibly uninitialized values being referenced. I can initialize those values in the field and make GCC13 happy, but then GCC10 with C++11 breaks because of some initializer list-style initializations. I'm going to try and add a constructor to make everybody happy and you can let me know if there is a better way.

For reference, this was the warning that was treated as an error:

In function ‘void fmt::v11::detail::init_named_arg(named_arg_info<Char>*, int&, int&, const T&) [with Char = char; T = static_named_arg<const char (&)[5], char, 6, fixed_string<char, 6>{"first"}>; typename std::enable_if<is_named_arg<T>::value, int>::type <anonymous> = 0]’,
    inlined from ‘constexpr fmt::v11::detail::named_arg_store<Context, NUM_ARGS, NUM_NAMED_ARGS, DESC>::named_arg_store(T& ...) [with T = {fmt::v11::detail::static_named_arg<const char (&)[5], char, 6, fmt::v11::detail::fixed_string<char, 6>{"first"}>, fmt::v11::detail::static_named_arg<const char (&)[4], char, 7, fmt::v11::detail::fixed_string<char, 7>{"second"}>, fmt::v11::detail::static_named_arg<int, char, 6, fmt::v11::detail::fixed_string<char, 6>{"third"}>}; Context = fmt::v11::context; int NUM_ARGS = 3; int NUM_NAMED_ARGS = 3; long long unsigned int DESC = 460]’ at /home/runner/work/fmt/fmt/include/fmt/base.h:2336:5,
    inlined from ‘std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {detail::static_named_arg<const char (&)[5], char, 6, detail::fixed_string<char, 6>{"first"}>, detail::static_named_arg<const char (&)[4], char, 7, detail::fixed_string<char, 7>{"second"}>, detail::static_named_arg<int, char, 6, detail::fixed_string<char, 6>{"third"}>}]’ at /home/runner/work/fmt/fmt/include/fmt/format.h:4200:49,
    inlined from ‘virtual void format_test_named_arg_udl_Test::TestBody()’ at /home/runner/work/fmt/fmt/test/format-test.cc:2035:62:
/home/runner/work/fmt/fmt/include/fmt/base.h:1087:22: error: ‘<anonymous>’ may be used uninitialized [-Werror=maybe-uninitialized]
 1087 |   check_for_duplicate(named_args, named_arg_index, arg.name);
      |   ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/fmt/fmt/include/fmt/base.h: In member function ‘virtual void format_test_named_arg_udl_Test::TestBody()’:
/home/runner/work/fmt/fmt/include/fmt/base.h:1068:20: note: by argument 1 of type ‘const fmt::v11::detail::named_arg_info<char>*’ to ‘constexpr void fmt::v11::detail::check_for_duplicate(const named_arg_info<Char>*, int, const Char*) [with Char = char]’ declared here
 1068 | FMT_CONSTEXPR void check_for_duplicate(
      |                    ^~~~~~~~~~~~~~~~~~~
/home/runner/work/fmt/fmt/include/fmt/format.h:4200:17: note: ‘<anonymous>’ declared here
 4200 |   return vformat(fmt.str, vargs<T...>{{args...}});
      |          ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vitaut · 2025-03-01T19:41:59Z

include/fmt/base.h

+  FMT_CONSTEXPR named_arg_info() : name(nullptr), id(0) {}
+  FMT_CONSTEXPR named_arg_info(const Char* a_name, const int an_id)
+      : name(a_name), id(an_id) {}


Initialization is done in init*named_arg, we shouldn't duplicate it here.

I'm going to try your idea of only calling if the index is greater than zero, but if that doesn't work, I need the default construct and if I have the default construct, GCC10 will need the other one.

Both functions have been removed. The if check appeased GCC13, it seems.

vitaut · 2025-03-01T19:43:00Z

include/fmt/base.h

@@ -1084,6 +1101,7 @@ template <typename T, typename Char,
          FMT_ENABLE_IF(is_static_named_arg<T>::value)>
 FMT_CONSTEXPR void init_static_named_arg(named_arg_info<Char>* named_args,
                                         int& arg_index, int& named_arg_index) {
+  check_for_duplicate(named_args, named_arg_index, T::name);


I wonder if calling check_for_duplicate only if named_arg_index is nonzero fixes the uninitialized warning.

vitaut · 2025-03-01T19:43:53Z

include/fmt/base.h

+    const Char* const arg_name) {
+  const basic_string_view<Char> arg_name_view(arg_name);


You can simplify this by passing arg_name as a string_view.

vitaut · 2025-03-01T19:44:59Z

include/fmt/base.h

  const Char* name;
  int id;
 };

+template <typename Char>
+FMT_CONSTEXPR void check_for_duplicate(
+    const named_arg_info<Char>* const named_args, const int named_arg_index,


Let's not overuse const - it just clutters the code without bringing much benefit when the scope is small.

I just want my immutable by default like rust 😄 . This is also an area I disagree with the CPP core guidelines. I'll begrudgingly update this, though.

…eturn early based on named_arg_index

vitaut · 2025-03-02T15:47:46Z

Merged, thanks. One drawback of this is quadratic complexity but this can be addressed later.

vitaut · 2025-03-02T17:07:10Z

include/fmt/base.h

+FMT_CONSTEXPR void check_for_duplicate(named_arg_info<Char>* named_args,
+                                       int named_arg_index,
+                                       basic_string_view<Char> arg_name) {
+  if (named_arg_index <= 0) return;


As it turned out the actual change that worked around the gcc bug was removing const in named_args, not adding the check =)

OK, that's kind of funny. Which const was it, cause I had a lot 😄 Makes me curious why the const additions made it seem like it might not be initialized to GCC. Maybe it just generated some weird assumptions in the compiler checks as part of the inlining process?

const named_arg_info<Char>*, looks like https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100417

dinomight added 3 commits February 24, 2025 18:44

Report error on duplicate named arg names

167ec54

fmtlib#4282

Fix unused return warning

233d8d9

Fix formatting

3d9a3e6

vitaut requested changes Feb 28, 2025

View reviewed changes

dinomight added 2 commits March 1, 2025 07:57

Refactor check function, optimize check function, remove expensive test

93b1425

Fix missing CTAD in C++11

36a6908

dinomight added 3 commits March 1, 2025 09:08

Fix GCC13 build and enable GCC10 C++11 build

b6be6fe

Fix formatting

1c156a3

Fix formatting, again

9800147

vitaut requested changes Mar 1, 2025

View reviewed changes

dinomight added 2 commits March 1, 2025 20:35

Remove init methods, simplify const usage, simplify check function, r…

7cf7957

…eturn early based on named_arg_index

Run clang-format

e3bfecd

vitaut merged commit 864bdf9 into fmtlib:master Mar 2, 2025
45 checks passed

dinomight deleted the duplicate-named-args branch March 2, 2025 16:04

vitaut reviewed Mar 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report error on duplicate named arg names #4367

Report error on duplicate named arg names #4367

dinomight commented Feb 26, 2025

dinomight commented Feb 26, 2025

vitaut left a comment

vitaut Feb 28, 2025

vitaut Feb 28, 2025

vitaut Feb 28, 2025

dinomight Mar 1, 2025

vitaut Mar 1, 2025

dinomight Mar 2, 2025

vitaut Mar 2, 2025

dinomight Mar 2, 2025

vitaut Mar 2, 2025

vitaut commented Feb 28, 2025

dinomight commented Mar 1, 2025 •

edited

Loading

vitaut Mar 1, 2025

dinomight Mar 2, 2025

dinomight Mar 2, 2025

vitaut Mar 1, 2025

vitaut Mar 1, 2025

vitaut Mar 1, 2025

dinomight Mar 2, 2025 •

edited

Loading

vitaut commented Mar 2, 2025

vitaut Mar 2, 2025

dinomight Mar 2, 2025

vitaut Mar 2, 2025

		const Char* const arg_name) {
		const basic_string_view<Char> arg_name_view(arg_name);

Report error on duplicate named arg names #4367

Report error on duplicate named arg names #4367

Conversation

dinomight commented Feb 26, 2025

dinomight commented Feb 26, 2025

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut commented Feb 28, 2025

dinomight commented Mar 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinomight Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

vitaut commented Mar 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dinomight commented Mar 1, 2025 •

edited

Loading

dinomight Mar 2, 2025 •

edited

Loading