Add warning to clang-reorder-fields when reordering breaks member init list dependencies

[sameconrad]

Summary

This adds a warning emitted by clang-reorder-fields when the reordering fields breaks dependencies in the initializer list (such that -Wuninitialized would warn when compiling). For example, given:

Foo::Foo(int x)
  : a(x)
  , b(a) {}

Reordering fields to [b,a] gives:

Foo::Foo(int x)
  : b(a)
  , a(x) {}

Emits the warning:
2: Warning: reordering field a after b makes a uninitialized when used in init expression.

[compnerd]

clang-reorder-fields/ReorderFieldsAction.cpp

111

What if the FieldDecl belongs to a base class? Can you add a test case for that scenario?

[alexander-shaposhnikov]

test/clang-reorder-fields/FieldDependencyWarning.cpp

3

tests should not depend on STL (for good examples see how things are done in clang-tidy tests), so simply remove this include and define a small class with a constructor right here

[sameconrad]

Add additional tests for cases with base and derived classes.
Add clarifying comments about emitting warnings for base class members during reordering checks.
Add fix for bug where reordering in init lists that contain non-member initializers causes segfault.

[sameconrad]

clang-reorder-fields/ReorderFieldsAction.cpp

111

I've added tests for this scenario and an explanatory comment. It doesn't seem this is actually a problem due to the way Clang constructs the AST. Sema::PerformObjectMemberConversion always inserts an implicit UncheckedDerivedToBase cast when accessing a base class member which means that hasObjectExpression(cxxThisExpr()) won't actually match against such an expression.

174

While adding tests for emitting warnings about base class members I found another bug here- non-member initializers don't have a field decl and so would result in undefind behavior further down when accessing field indexes (I added the test case ClassDerived which would cause a segfault before this change).

test/clang-reorder-fields/FieldDependencyWarning.cpp

3

This has been updated to use a simple struct.

[alexander-shaposhnikov]

LGTM, thanks!
do you have commit access ? (if not i can commit this patch for you)

[sameconrad]

In D35972#825348, @alexshap wrote:

LGTM, thanks!
do you have commit access ? (if not i can commit this patch for you)

I don't have commit access, so you will have to commit it.
Thank you

[alexander-shaposhnikov]

https://reviews.llvm.org/rL309505
plus I changed the code to emit this warning via DiagnosticEngine and updated the tests accordingly.

clang-reorder-fields/ReorderFieldsAction.cpp

 #include "clang/AST/AST.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Refactoring.h"
+#include "llvm/ADT/SetVector.h"
 #include <algorithm>
 #include <string>
 
 namespace clang {
 namespace reorder_fields {
 using namespace clang::ast_matchers;
+using llvm::SmallSetVector;
 
 /// \brief Finds the definition of a record by name.
 ///
 /// \returns nullptr if the name is ambiguous or not found.
 static const RecordDecl *findDefinition(StringRef RecordName,
                                         ASTContext &Context) {
   auto Results = match(
       recordDecl(hasName(RecordName), isDefinition()).bind("recordDecl"),
StringRef NewText =
       Lexer::getSourceText(CharSourceRange::getTokenRange(New),
                            Context.getSourceManager(), Context.getLangOpts());
   tooling::Replacement R(Context.getSourceManager(),
                          CharSourceRange::getTokenRange(Old), NewText,
                          Context.getLangOpts());
   consumeError(Replacements[R.getFilePath()].add(R));
 }
 
+/// \brief Find all member fields used in the given init-list initializer expr
+/// that belong to the same record
+///
+/// \returns a set of field declarations, empty if none were present
+static SmallSetVector<FieldDecl *, 1>
+findMembersUsedInInitExpr(const CXXCtorInitializer *Initializer,
+                          ASTContext &Context) {
+  SmallSetVector<FieldDecl *, 1> Results;
+  auto FoundExprs =
+      match(findAll(memberExpr(hasObjectExpression(cxxThisExpr())).bind("ME")),
+            *Initializer->getInit(), Context);
+  for (BoundNodes &BN : FoundExprs)
+    if (auto *MemExpr = BN.getNodeAs<MemberExpr>("ME"))
+      if (auto *FD = dyn_cast<FieldDecl>(MemExpr->getMemberDecl()))
+        Results.insert(FD);
+  return Results;
+}
+
 /// \brief Reorders fields in the definition of a struct/class.
 ///
 /// At the moment reodering of fields with
 /// different accesses (public/protected/private) is not supported.
 /// \returns true on success.
 static bool reorderFieldsInDefinition(
     const RecordDecl *Definition, ArrayRef<unsigned> NewFieldsOrder,
     const ASTContext &Context,

 }
 
 /// \brief Reorders initializers in a C++ struct/class constructor.
 ///
 /// A constructor can have initializers for an arbitrary subset of the class's fields.
 /// Thus, we need to ensure that we reorder just the initializers that are present.
 static void reorderFieldsInConstructor(
     const CXXConstructorDecl *CtorDecl, ArrayRef<unsigned> NewFieldsOrder,
-    const ASTContext &Context,
+    ASTContext &Context,
     std::map<std::string, tooling::Replacements> &Replacements) {
   assert(CtorDecl && "Constructor declaration is null");
   if (CtorDecl->isImplicit() || CtorDecl->getNumCtorInitializers() <= 1)
     return;
 
   // The method FunctionDecl::isThisDeclarationADefinition returns false
   // for a defaulted function unless that function has been implicitly defined.
   // Thus this assert needs to be after the previous checks.
   assert(CtorDecl->isThisDeclarationADefinition() && "Not a definition");
 
   SmallVector<unsigned, 10> NewFieldsPositions(NewFieldsOrder.size());
   for (unsigned i = 0, e = NewFieldsOrder.size(); i < e; ++i)
     NewFieldsPositions[NewFieldsOrder[i]] = i;
 
   SmallVector<const CXXCtorInitializer *, 10> OldWrittenInitializersOrder;
   SmallVector<const CXXCtorInitializer *, 10> NewWrittenInitializersOrder;
   for (const auto *Initializer : CtorDecl->inits()) {
     if (!Initializer->isWritten())
       continue;
+
+    // Warn if this reordering violates initialization expr dependencies
+    const auto UsedMembers = findMembersUsedInInitExpr(Initializer, Context);
+    const FieldDecl *ThisM = Initializer->getMember();
+    for (const FieldDecl *UM : UsedMembers) {
+      if (NewFieldsPositions[UM->getFieldIndex()] >
+          NewFieldsPositions[ThisM->getFieldIndex()]) {
+        llvm::errs() << Context.getSourceManager().getSpellingLineNumber(
+                            Initializer->getSourceLocation())
+                     << ": Warning: reordering field " << UM->getName()
+                     << " after " << ThisM->getName() << " makes "
+                     << UM->getName()
+                     << " uninitialized when used in init expression\n";
+      }
+    }
+
     OldWrittenInitializersOrder.push_back(Initializer);
     NewWrittenInitializersOrder.push_back(Initializer);
   }
   auto ByFieldNewPosition = [&](const CXXCtorInitializer *LHS,
                                 const CXXCtorInitializer *RHS) {
     assert(LHS && RHS);
     return NewFieldsPositions[LHS->getMember()->getFieldIndex()] <
            NewFieldsPositions[RHS->getMember()->getFieldIndex()];

test/clang-reorder-fields/FieldDependencyWarning.cpp

+// RUN: clang-reorder-fields -record-name bar::Foo -fields-order y,z,c,x %s -- 2>&1 | FileCheck %s
+
+#include <string>
+
+namespace bar {
+class Foo { 
+public:
+  Foo(int x, double y, char cin);
+  Foo(int nx);
+  Foo();
+  int x;
+  double y;
+  char c;
+  std::string z;
+};
+
+static char bar(char c) {
+  return c + 1;
+}
+
+Foo::Foo(int x, double y, char cin) :  
+  x(x),                 
+  y(y),                 
+  c(cin),               
+  z(this->x, bar(c))    // CHECK:      [[@LINE]]: Warning: reordering field x after z makes x uninitialized when used in init expression
+                        // CHECK-NEXT: [[@LINE-1]]: Warning: reordering field c after z makes c uninitialized when used in init expression
+{}
+
+Foo::Foo(int nx) :
+  x(nx),              
+  y(x),                 // CHECK-NEXT: [[@LINE]]: Warning: reordering field x after y makes x uninitialized when used in init expression
+  c(0),               
+  z(bar(bar(x)), c)     // CHECK-NEXT: [[@LINE]]: Warning: reordering field x after z makes x uninitialized when used in init expression
+                        // CHECK-NEXT: [[@LINE-1]]: Warning: reordering field c after z makes c uninitialized when used in init expression
+{}
+
+} // namespace bar
+
+int main() {
+  bar::Foo F(5, 12.8, 'c');
+  return 0;
+}