[clang-tidy] Avoid diagnosing std::array initializations for modernize-use-designated-initializers #134774

RiverDave · 2025-04-08T03:30:32Z

Edit:
I suggest we avoid diagnosing initializers for std::array type. The fixit provided is incorrect as observed in #133715. The only workaround would require C99-style array designators which don’t really align with the purpose of this check. This would also generate extra compiler warnings.

Fixes #133715

llvmbot · 2025-04-08T03:31:11Z

@llvm/pr-subscribers-clang-tidy

Author: David Rivera (RiverDave)

Changes

I suggest we avoid diagnosing designated initializers for std::array initializations. The fixit provided is incorrect as observed in #133715. The only workaround would require C99-style array designators which don’t really align with the purpose of this check. This would also generate extra compiler warnings.

Fun fact:
Something I found interesting was the fact that you could access the implementation-defined internal array field. see:

    std::array&lt;int, 3&gt; arr2 = {._M_elems[0] = {3}};

That compiles, however it wouldn’t make sense to provide this as suggestion considering it wouldn’t be portable at all.

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

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp (+6-1)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
index 3132067f3d5ec..9e2ac149d0868 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
@@ -119,13 +119,18 @@ UseDesignatedInitializersCheck::UseDesignatedInitializersCheck(
 void UseDesignatedInitializersCheck::registerMatchers(MatchFinder *Finder) {
   const auto HasBaseWithFields =
       hasAnyBase(hasType(cxxRecordDecl(has(fieldDecl()))));
+
+  // see #133715
+  const auto IsSTLArray =
+      hasType(qualType(hasDeclaration(recordDecl(hasName("::std::array")))));
+
   Finder->addMatcher(
       initListExpr(
           hasType(cxxRecordDecl(RestrictToPODTypes ? isPOD() : isAggregate(),
                                 unless(HasBaseWithFields))
                       .bind("type")),
           IgnoreSingleElementAggregates ? hasMoreThanOneElement() : anything(),
-          unless(isFullyDesignated()))
+          unless(anyOf(isFullyDesignated(), IsSTLArray)))
           .bind("init"),
       this);
 }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6c1f05009df98..44c348f453543 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -171,6 +171,10 @@ Changes in existing checks
   ``constexpr`` and ``static``` values on member initialization and by detecting
   explicit casting of built-in types within member list initialization.
 
+- Improved :doc:`modernize-use-designated-initializers
+  <clang-tidy/checks/modernize/use-designated-initializers>` check by avoiding
+  diagnosing designated initializers for ``std::array`` initializations.
+
 - Improved :doc:`modernize-use-ranges
   <clang-tidy/checks/modernize/use-ranges>` check by updating suppress 
   warnings logic for ``nullptr`` in ``std::find``.

llvmbot · 2025-04-08T03:31:12Z

@llvm/pr-subscribers-clang-tools-extra

Author: David Rivera (RiverDave)

Changes

I suggest we avoid diagnosing designated initializers for std::array initializations. The fixit provided is incorrect as observed in #133715. The only workaround would require C99-style array designators which don’t really align with the purpose of this check. This would also generate extra compiler warnings.

Fun fact:
Something I found interesting was the fact that you could access the implementation-defined internal array field. see:

    std::array&lt;int, 3&gt; arr2 = {._M_elems[0] = {3}};

That compiles, however it wouldn’t make sense to provide this as suggestion considering it wouldn’t be portable at all.

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

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp (+6-1)
(modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)

diff --git a/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
index 3132067f3d5ec..9e2ac149d0868 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp
@@ -119,13 +119,18 @@ UseDesignatedInitializersCheck::UseDesignatedInitializersCheck(
 void UseDesignatedInitializersCheck::registerMatchers(MatchFinder *Finder) {
   const auto HasBaseWithFields =
       hasAnyBase(hasType(cxxRecordDecl(has(fieldDecl()))));
+
+  // see #133715
+  const auto IsSTLArray =
+      hasType(qualType(hasDeclaration(recordDecl(hasName("::std::array")))));
+
   Finder->addMatcher(
       initListExpr(
           hasType(cxxRecordDecl(RestrictToPODTypes ? isPOD() : isAggregate(),
                                 unless(HasBaseWithFields))
                       .bind("type")),
           IgnoreSingleElementAggregates ? hasMoreThanOneElement() : anything(),
-          unless(isFullyDesignated()))
+          unless(anyOf(isFullyDesignated(), IsSTLArray)))
           .bind("init"),
       this);
 }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6c1f05009df98..44c348f453543 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -171,6 +171,10 @@ Changes in existing checks
   ``constexpr`` and ``static``` values on member initialization and by detecting
   explicit casting of built-in types within member list initialization.
 
+- Improved :doc:`modernize-use-designated-initializers
+  <clang-tidy/checks/modernize/use-designated-initializers>` check by avoiding
+  diagnosing designated initializers for ``std::array`` initializations.
+
 - Improved :doc:`modernize-use-ranges
   <clang-tidy/checks/modernize/use-ranges>` check by updating suppress 
   warnings logic for ``nullptr`` in ``std::find``.

carlosgalvezp · 2025-04-08T06:46:30Z

I found interesting was the fact that you could access the implementation-defined internal array field

Yes, this is required in order for std::array to be an aggregate type.

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

carlosgalvezp · 2025-04-08T06:48:51Z

Actually, it seems to me that IgnoreSingleElementAggregates should already be handling this situation. If it doesn't, there's a bug there that should be fixed, instead of patching it for std::array?

carlosgalvezp

We should fix IgnoreSingleElementAggregates instead.

RiverDave · 2025-04-09T03:11:48Z

We should fix IgnoreSingleElementAggregates instead.

Just to be clear, I suppose we're looking to fix std::array + IgnoreSingleElementAggregates = false as It's the only problematic combination I could analyze. The fixit this check provides on aggregate types with a single field of array type, seem to be correct to me: https://godbolt.org/z/zx4aofaso

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

carlosgalvezp · 2025-04-09T18:59:45Z

Ok, I see, let's disable for ::std::array then, generalize in the future if we come across a similar use case.

RiverDave · 2025-04-13T00:08:21Z

Ok, I see, let's disable for ::std::array then, generalize in the future if we come across a similar use case.

Thanks, your feedback has been addressed.

RiverDave · 2025-04-18T15:14:55Z

Ping

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp

carlosgalvezp · 2025-04-20T18:46:07Z

clang-tools-extra/docs/ReleaseNotes.rst

@@ -182,6 +182,11 @@ Changes in existing checks
  ``constexpr`` and ``static``` values on member initialization and by detecting
  explicit casting of built-in types within member list initialization.

+- Improved :doc:`modernize-use-designated-initializers
+  <clang-tidy/checks/modernize/use-designated-initializers>` check by avoiding
+  diagnosing designated initializers for ``std::array`` initializations when


Please document this exception also in IgnoreSingleElementAggregates

Done, let me know if my explanation is not clear enough

carlosgalvezp

Please add a unit test demonstrating that the related issue is fixed.

…e-use-designated-initializers

RiverDave · 2025-04-22T05:30:45Z

Please add a unit test demonstrating that the related issue is fixed.

Added 👍

HerrCai0907

exclude std::array don't resolve root problem here. It can also happen in user defined class.
e.g. https://godbolt.org/z/cn46MT5Mn

But maybe it could be handled in another PR? since std::array is a special case that we don't know the detail member name in std::array.

RiverDave · 2025-04-28T01:40:41Z

Ping @carlosgalvezp :)

carlosgalvezp

LGTM!

carlosgalvezp · 2025-04-28T07:04:17Z

It can also happen in user defined class.

Yes. But it's unclear what the behavior of the check should be. For std::array, it's clear, because one must not touch the internal element. For a user-defined type, "it depends" on how the user wants to design their struct.

If the need comes, I envision we will need some sort of option so people can add other types on top std::array to be excluded from the check. But let's cross that bridge when we get there :)

carlosgalvezp · 2025-04-28T07:04:51Z

@RiverDave Do you have permissions to land the PR, or shall I do it for you?

RiverDave · 2025-04-28T16:32:46Z

@RiverDave Do you have permissions to land the PR, or shall I do it for you?

Don't have permissions yet.... But Yes! You can merge it for me.

…e-use-designated-initializers (llvm#134774) **Edit:** I suggest we avoid diagnosing initializers for `std::array` type. The fixit provided is incorrect as observed in **llvm#133715.** The only workaround would require C99-style array designators which don’t really align with the purpose of this check. This would also generate extra compiler warnings. Fixes llvm#133715

llvmbot added clang-tools-extra clang-tidy labels Apr 8, 2025

carlosgalvezp reviewed Apr 8, 2025

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp Outdated Show resolved Hide resolved

carlosgalvezp requested changes Apr 8, 2025

View reviewed changes

EugeneZelenko requested review from PiotrZSL and HerrCai0907 April 8, 2025 15:25

carlosgalvezp reviewed Apr 9, 2025

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp Outdated Show resolved Hide resolved

RiverDave force-pushed the modernize/133715 branch from 9727ea1 to 65fc264 Compare April 13, 2025 00:06

RiverDave force-pushed the modernize/133715 branch from 65fc264 to 05b4a20 Compare April 13, 2025 00:55

RiverDave requested a review from carlosgalvezp April 13, 2025 06:26

carlosgalvezp reviewed Apr 20, 2025

View reviewed changes

clang-tools-extra/clang-tidy/modernize/UseDesignatedInitializersCheck.cpp Outdated Show resolved Hide resolved

carlosgalvezp reviewed Apr 20, 2025

View reviewed changes

carlosgalvezp requested changes Apr 20, 2025

View reviewed changes

[clang-tidy] Avoid diagnosing std::array initializations for moderniz…

5dc7aa5

…e-use-designated-initializers

RiverDave force-pushed the modernize/133715 branch from 05b4a20 to 5dc7aa5 Compare April 22, 2025 05:25

HerrCai0907 requested changes Apr 23, 2025

View reviewed changes

HerrCai0907 approved these changes Apr 23, 2025

View reviewed changes

carlosgalvezp approved these changes Apr 28, 2025

View reviewed changes

carlosgalvezp merged commit 45411ac into llvm:main Apr 28, 2025
12 checks passed

[clang-tidy] Avoid diagnosing std::array initializations for modernize-use-designated-initializers #134774

[clang-tidy] Avoid diagnosing std::array initializations for modernize-use-designated-initializers #134774

Uh oh!

Conversation

RiverDave commented Apr 8, 2025 • edited by carlosgalvezp Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 8, 2025

Uh oh!

llvmbot commented Apr 8, 2025

Uh oh!

carlosgalvezp commented Apr 8, 2025

Uh oh!

Uh oh!

carlosgalvezp commented Apr 8, 2025

Uh oh!

carlosgalvezp left a comment

Choose a reason for hiding this comment

Uh oh!

RiverDave commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

carlosgalvezp commented Apr 9, 2025

Uh oh!

RiverDave commented Apr 13, 2025

Uh oh!

RiverDave commented Apr 18, 2025

Uh oh!

Uh oh!

carlosgalvezp Apr 20, 2025

Choose a reason for hiding this comment

Uh oh!

RiverDave Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp left a comment

Choose a reason for hiding this comment

Uh oh!

RiverDave commented Apr 22, 2025

Uh oh!

HerrCai0907 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RiverDave commented Apr 28, 2025

Uh oh!

carlosgalvezp left a comment

Choose a reason for hiding this comment

Uh oh!

carlosgalvezp commented Apr 28, 2025

Uh oh!

carlosgalvezp commented Apr 28, 2025

Uh oh!

RiverDave commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RiverDave commented Apr 8, 2025 •

edited by carlosgalvezp

Loading

RiverDave commented Apr 9, 2025 •

edited

Loading

RiverDave Apr 22, 2025 •

edited

Loading

HerrCai0907 left a comment •

edited

Loading

RiverDave commented Apr 28, 2025 •

edited

Loading