[PATCH] D146288: clang-tidy: Detect use-after-move in CXXCtorInitializer

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL requested changes to this revision.
PiotrZSL added a comment.
This revision now requires changes to proceed.

Requesting changes due to lack of support for base class initializes & got some concerns related lambdas used in initializers.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:400
 void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
   auto CallMoveMatcher =
+      callExpr(
----------------
This is correct but consider this:

```
auto TryEmplaceMatcher = cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));

auto CallMoveMatcher =
callExpr(argumentCountIs(1), // because matching this will be faster than checking name.
             callee(functionDecl(hasName("::std::move"))),
             unless(inDecltypeOrTemplateArg()),
             expr().bind("call-move"),
             unless(hasParent(TryEmplaceMatcher)),
             anyOf(hasAncestor(compoundStmt(hasParent(lambdaExpr().bind("containing-lambda")))),
                        hasAncestor(functionDecl(anyOf(cxxConstructorDecl(hasAnyConstructorInitializer(
                                                                                                                   withInitializer(expr(
                                                                                                                          anyOf(equalsBounNode("call-move"), 
                                                                                                                                     hasDescendant(equalsBounNode("call-move")))
                                                                                                                     ).bind("containing-ctor-init-stmt")
                                                                                                              ))
                                                                                                             ).bind("containing-ctor"),
                                                                             functionDecl().bind("containing-func")))))
            );
```
              


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:474
+      for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+        if (Init->getInit() == ContainingCtorInitStmt) {
+          ContainingCtorField = Init->getMember();
----------------
consider using here `->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp:479-488
+      if (ContainingCtorField) {
+        // Collect the constructor initializer expressions.
+        for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
+          if (Init->getMember() && Init->getMember()->getFieldIndex() >=
+                                       ContainingCtorField->getFieldIndex()) {
+            if (Init->getInit())
+              CodeBlocks.push_back(Init->getInit());
----------------
This won't work for std::move used to initialize base class. Assuming that Inits are in specific order, then you just should include all init's after one found, or use things like `SourceManager::isBeforeInTranslationUnit`, but still probably using something like:

```
bool PastMove = false;
for (CXXCtorInitializer *Init : ContainingCtor->inits()) {
if (!PastMove  && Init->getInit()->IgnoreImplicit() == ContainingCtorInitStmt->IgnoreImplicit()) {
    PastMove  = true;
}

if (PastMove && Init->getInit())
{
    CodeBlocks.push_back(Init->getInit());
}
```

would be better, as it should work for both field inits and base constructor calls



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp:1431
   std::string val_;
 };
----------------
Missing tests:
- Test with A derive from B, C, D, and argument is moved into C, but D initialization also uses it.
- Test with lambda call inside init, and in lambda std::move of captured by reference constructor parameter, and lambda could be mutable, and lambda could be saved into member (or passed to base class), or called instantly (better both tests).
- Test with lambda that takes argument into capture by move

In short:
```
struct Obj {};
struct D
{
    D(Obj b);
};

struct C
{
     C(Obj b);
};

struct B
{
     B(Obj& b);
}

struct A : B, C, D
{
    A(Obj b)
      : B(b), C(std::move(b)), D(b)
    {
    }
};

and second:

struct Base
{
    Base(Obj b) : bb(std::move(b)) {}
     template<typename T>
     Base(T&& b) : bb(b()) {};
   
     Obj bb;
};

struct Derived : Base, C
{
     Derived(Obj b) 
           : Base([&] mutable { return std::move(b); }()), C(b)
   {
   }
};

struct Derived2 : Base, C
{
     Derived2(Obj b) 
           : Base([&] mutable { return std::move(b); }), C(b)
   {
   }
};

struct Derived3 : Base, C
{
     Derived3(Obj b) 
           : Base([c = std::move(b)] mutable { return std::move(c); }), C(b)
   {
   }
};
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146288/new/

https://reviews.llvm.org/D146288