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

[RiverDave]

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]

@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<int, 3> 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]

@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<int, 3> 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]

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.

[carlosgalvezp]

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]

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

[carlosgalvezp]

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

[RiverDave]

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]

Ping

[carlosgalvezp]

Please document this exception also in IgnoreSingleElementAggregates

[RiverDave]

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

[carlosgalvezp]

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

[RiverDave]

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.