[clang-tidy] bugprone-use-after-move: Ctor arguments should be sequenced if ctor call is written as list-initialization.

[mboehme]

Summary

See

https://timsong-cpp.github.io/cppwp/n4868/dcl.init#list-4

This eliminates a false positive in bugprone-use-after-move; this newly added
test used to be falsely classified as a use-after-move:

A a;
S3 s3{a.getInt(), std::move(a)};

[njames93]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1163

Whats with this namespace addition? looks unnecessary and should be removed

1204

Whats with the todo comment, surery a comment explaining that this shouldn't trigger a warning should suffice

[mboehme]

Changes in response to review comments.

[mboehme]

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

1163

I'd like to avoid the definitions of the structs with short names spilling out into the global namespace and possibly conflicting with other definitions.

The structs used to be defined within the function initializerListSequences(), which is better, but I now need a class template S3, and those can't be defined with a function, so I decided to add a namespace. There are other places in this file that do the same, and I'm following that example. It's probably a better idea to actually name the namespace though, so I've done that now. WDYT?

1204

Sorry, this was a note-to-self that I neglected to address before uploading the patch. I've replaced this with a more meaningful comment.

[carlosgalvezp]

The commit message doesn't really tell me "what" this commit is fixing, it only points to a section of the Standard. It talks about "a false positive" but it doesn't tell what this FP is about. Could you write a little bit more about what the problem is? Preferably if you can link a Github issue describing the problem. The subject of the commit message should indicate which particular check it relates to.

Document change in release notes.

[PiotrZSL]

Tests in lines 1195, 1201 actually tests InitListExpr.
Test in line 1207 test negative scenario of CXXConstructExpr (list), but I didn't found any positive test for CXXConstructExpr with list expr, please add such test to show that warnings are still produced for such case.

[mboehme]

Changes in response to review comments, and rebased to head.

[mboehme]

In D148110#4290400, @carlosgalvezp wrote:

The commit message doesn't really tell me "what" this commit is fixing, it only points to a section of the Standard. It talks about "a false positive" but it doesn't tell what this FP is about. Could you write a little bit more about what the problem is? Preferably if you can link a Github issue describing the problem.

I've expanded the patch description to reference the specific test added here that used to be falsely diagnosed as a use-after-free.

I hope this is acceptable in lieu of a github issue?

The subject of the commit message should indicate which particular check it relates to.

Done.

Document change in release notes.

Done.

In D148110#4290905, @PiotrZSL wrote:

Tests in lines 1195, 1201 actually tests InitListExpr.

That's understood. (These tests existed before this patch.)

Test in line 1207 test negative scenario of CXXConstructExpr (list), but I didn't found any positive test for CXXConstructExpr with list expr, please add such test to show that warnings are still produced for such case.

Good point, done.

[PiotrZSL]

clang-tools-extra/docs/ReleaseNotes.rst

216–220

NOTE: Consider: "Improved XYZ check by including coverage for constructor initializers, and by correcting the handling of constructor arguments when they are sequenced during a constructor call that is written as list-initialization." or "The XYZ check now covers constructor initializers and handles constructor arguments correctly during list-initialization."

I just pointing this because I don't like this "check: Also"

[mboehme]

Reworded release notes.

[mboehme]

Reworded release notes.

clang-tools-extra/docs/ReleaseNotes.rst

216–220

Makes sense. How about this? (I have more fixes in the pipeline, so I've gone for two separate sentences for the two fixes instead of a single sentence.)

[PiotrZSL]

+- LGTM

clang-tools-extra/docs/ReleaseNotes.rst

217

':' -> '.'

[mboehme]

Fix typo and rebase to head

[mboehme]

Sorry, missed your LGTM. Will wait for pre-merge checks to complete and will then land.

clang-tools-extra/clang-tidy/utils/ExprSequence.cpp

if (const auto *BO = dyn_cast<BinaryOperator>(Parent)) {
       // clauses that precede it.
       for (const InitListExpr *Form : getAllInitListForms(InitList)) {
         for (unsigned I = 1; I < Form->getNumInits(); ++I) {
           if (Form->getInit(I - 1) == S) {
             return Form->getInit(I);
           }
         }
       }
+    } else if (const auto *ConstructExpr = dyn_cast<CXXConstructExpr>(Parent)) {
+      // Constructor arguments are sequenced if the constructor call is written
+      // as list-initialization.
+      if (ConstructExpr->isListInitialization()) {
+        for (unsigned I = 1; I < ConstructExpr->getNumArgs(); ++I) {
+          if (ConstructExpr->getArg(I - 1) == S) {
+            return ConstructExpr->getArg(I);
+          }
+        }
+      }
     } else if (const auto *Compound = dyn_cast<CompoundStmt>(Parent)) {
       // Compound statement: Each sub-statement is sequenced after the
       // statements that precede it.
       const Stmt *Previous = nullptr;
       for (const auto *Child : Compound->body()) {
         if (Previous == S)
           return Child;
         Previous = Child;

clang-tools-extra/docs/ReleaseNotes.rst

- Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
   in :doc:`bugprone-suspicious-include
   <clang-tidy/checks/bugprone/suspicious-include>` check.
   Global options of the same name should be used instead.
 
 - Improved :doc:`bugprone-unchecked-optional-access
   <clang-tidy/checks/bugprone/unchecked-optional-access>` check to properly handle calls
   to ``std::forward``.
 
 - Improved :doc:`bugprone-use-after-move
-  <clang-tidy/checks/bugprone/use-after-move>` check to also cover constructor
-  initializers.
+  <clang-tidy/checks/bugprone/use-after-move>` check: Detect uses and moves in
+  constructor initializers. Correctly handle constructor arguments as being
+  sequenced when constructor call is written as list-initialization.
 
 - Deprecated :doc:`cert-dcl21-cpp
   <clang-tidy/checks/cert/dcl21-cpp>` check.
 
 - Deprecated C.48 enforcement from :doc:`cppcoreguidelines-prefer-member-initializer
   <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>`. Please use
   :doc:`cppcoreguidelines-use-default-member-init
   <clang-tidy/checks/cppcoreguidelines/use-default-member-init>` instead.
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp

void commaOperatorSequences() {
     A a;
     (a = A()), A(std::move(a));
     a.foo();
     // CHECK-NOTES: [[@LINE-1]]:5: warning: 'a' used after it was moved
     // CHECK-NOTES: [[@LINE-3]]:16: note: move occurred here
   }
 }
 
-// An initializer list sequences its initialization clauses.
-void initializerListSequences() {
-  {
-    struct S1 {
-      int i;
-      A a;
-    };
-    {
-      A a;
-      S1 s1{a.getInt(), std::move(a)};
-    }
-    {
-      A a;
-      S1 s1{.i = a.getInt(), .a = std::move(a)};
-    }
-  }
-  {
-    struct S2 {
-      A a;
-      int i;
-    };
-    {
-      A a;
-      S2 s2{std::move(a), a.getInt()};
-      // CHECK-NOTES: [[@LINE-1]]:27: warning: 'a' used after it was moved
-      // CHECK-NOTES: [[@LINE-2]]:13: note: move occurred here
-    }
-    {
-      A a;
-      S2 s2{.a = std::move(a), .i = a.getInt()};
-      // CHECK-NOTES: [[@LINE-1]]:37: warning: 'a' used after it was moved
-      // CHECK-NOTES: [[@LINE-2]]:13: note: move occurred here
-    }
+namespace InitializerListSequences {
+
+
+struct S2 {
+  A a;
+  int i;
+};
+
+struct S3 {
+  S3() {}
+  S3(int, A) {}
+  S3(A, int) {}
+};
+
+// An initializer list sequences its initialization clauses.
+void initializerListSequences() {
+    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+    // CHECK-NOTES: [[@LINE-1]]:35: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
+  {
+    // Check the case where the constructed type has a constructor and the
+    // initializer list therefore manifests as a `CXXConstructExpr` instead of
+    // an `InitListExpr`.
+    A a;
+    S3 s3{a.getInt(), std::move(a)};
+  }
+  {
+    A a;
+    S3 s3{std::move(a), a.getInt()};
+    // CHECK-NOTES: [[@LINE-1]]:25: warning: 'a' used after it was moved
+    // CHECK-NOTES: [[@LINE-2]]:11: note: move occurred here
   }
 }
 
+} // namespace InitializerListSequences
+
 // A declaration statement containing multiple declarations sequences the
 // initializer expressions.
 void declarationSequences() {
   {
     A a;
     A a1 = a, a2 = std::move(a);
   }
   {