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 ↗

(On Diff #108570)

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 ↗

(On Diff #108570)

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 ↗

(On Diff #108570)

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.

178 ↗

(On Diff #108812)

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 ↗

(On Diff #108570)

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-tools-extra/trunk/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"),
+  auto Results =
+      match(recordDecl(hasName(RecordName), isDefinition()).bind("recordDecl"),
             Context);
   if (Results.empty()) {
     llvm::errs() << "Definition of " << RecordName << "  not found\n";
     return nullptr;
   }
   if (Results.size() > 1) {
     llvm::errs() << "The name " << RecordName
                  << " is ambiguous, several definitions found\n";
     return nullptr;
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;
+  // Note that this does not pick up member fields of base classes since
+  // for those accesses Sema::PerformObjectMemberConversion always inserts an
+  // UncheckedDerivedToBase ImplicitCastExpr between the this expr and the
+  // object expression
+  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,
addReplacement(Field->getSourceRange(),
                    Fields[NewFieldsOrder[FieldIndex]]->getSourceRange(),
                    Context, Replacements);
   }
   return true;
 }
 
 /// \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.
+/// 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())
+    if (!Initializer->isMemberInitializer() || !Initializer->isWritten())
       continue;
+
+    // Warn if this reordering violates initialization expr dependencies.
+    const FieldDecl *ThisM = Initializer->getMember();
+    const auto UsedMembers = findMembersUsedInInitExpr(Initializer, Context);
+    for (const FieldDecl *UM : UsedMembers) {
+      if (NewFieldsPositions[UM->getFieldIndex()] >
+          NewFieldsPositions[ThisM->getFieldIndex()]) {
+        DiagnosticsEngine &DiagEngine = Context.getDiagnostics();
+        auto Description = ("reordering field " + UM->getName() + " after " +
+                            ThisM->getName() + " makes " + UM->getName() +
+                            " uninitialized when used in init expression")
+                               .str();
+        unsigned ID = DiagEngine.getDiagnosticIDs()->getCustomDiagID(
+            DiagnosticIDs::Warning, Description);
+        DiagEngine.Report(Initializer->getSourceLocation(), ID);
+      }
+    }
+
     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()];

 ///
 /// At the moment partial initialization is not supported.
 /// \returns true on success
 static bool reorderFieldsInInitListExpr(
     const InitListExpr *InitListEx, ArrayRef<unsigned> NewFieldsOrder,
     const ASTContext &Context,
     std::map<std::string, tooling::Replacements> &Replacements) {
   assert(InitListEx && "Init list expression is null");
-  // We care only about InitListExprs which originate from source code. 
+  // We care only about InitListExprs which originate from source code.
   // Implicit InitListExprs are created by the semantic analyzer.
   if (!InitListEx->isExplicit())
     return true;
-  // The method InitListExpr::getSyntacticForm may return nullptr indicating that
-  // the current initializer list also serves as its syntactic form.
+  // The method InitListExpr::getSyntacticForm may return nullptr indicating
+  // that the current initializer list also serves as its syntactic form.
   if (const auto *SyntacticForm = InitListEx->getSyntacticForm())
     InitListEx = SyntacticForm;
   // If there are no initializers we do not need to change anything.
   if (!InitListEx->getNumInits())
     return true;
   if (InitListEx->getNumInits() != NewFieldsOrder.size()) {
     llvm::errs() << "Currently only full initialization is supported\n";
     return false;
   }
   for (unsigned i = 0, e = InitListEx->getNumInits(); i < e; ++i)
     if (i != NewFieldsOrder[i])
-      addReplacement(
-          InitListEx->getInit(i)->getSourceRange(),
-          InitListEx->getInit(NewFieldsOrder[i])->getSourceRange(), Context,
-          Replacements);
+      addReplacement(InitListEx->getInit(i)->getSourceRange(),
+                     InitListEx->getInit(NewFieldsOrder[i])->getSourceRange(),
+                     Context, Replacements);
   return true;
 }
 
 namespace {
 class ReorderingConsumer : public ASTConsumer {
   StringRef RecordName;
   ArrayRef<std::string> DesiredFieldsOrder;
   std::map<std::string, tooling::Replacements> &Replacements;
if (!reorderFieldsInDefinition(RD, NewFieldsOrder, Context, Replacements))
       return;
 
     // CXXRD will be nullptr if C code (not C++) is being processed.
     const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD);
     if (CXXRD)
       for (const auto *C : CXXRD->ctors())
         if (const auto *D = dyn_cast<CXXConstructorDecl>(C->getDefinition()))
           reorderFieldsInConstructor(cast<const CXXConstructorDecl>(D),
-                                      NewFieldsOrder, Context, Replacements);
 
-    // We only need to reorder init list expressions for 
+    // We only need to reorder init list expressions for
     // plain C structs or C++ aggregate types.
     // For other types the order of constructor parameters is used,
     // which we don't change at the moment.
     // Now (v0) partial initialization is not supported.
     if (!CXXRD || CXXRD->isAggregate())
       for (auto Result :
            match(initListExpr(hasType(equalsNode(RD))).bind("initListExpr"),
                  Context))

clang-tools-extra/trunk/test/clang-reorder-fields/ClassDerived.cpp

+// RUN: clang-reorder-fields -record-name bar::Derived -fields-order z,y %s -- | FileCheck %s
+
+namespace bar {
+class Base {
+public:
+  Base(int nx, int np) : x(nx), p(np) {}
+  int x;
+  int p;
+};
+
+
+class Derived : public Base {
+public:
+  Derived(long ny);
+  Derived(char nz);
+private:
+  long y;
+  char z;
+};
+
+Derived::Derived(long ny) : 
+    Base(ny, 0),
+    y(ny),                   // CHECK:       {{^  z\(static_cast<char>\(ny\)\),}}
+    z(static_cast<char>(ny)) // CHECK-NEXT:  {{^  y\(ny\)}}
+{}
+
+Derived::Derived(char nz) : 
+    Base(1, 2),
+    y(nz),  // CHECK:       {{^  z\(x\),}}
+    z(x)    // CHECK-NEXT:  {{^  y\(nz\)}}
+{}
+
+} // namespace bar

clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarning.cpp

+// RUN: clang-reorder-fields -record-name bar::Foo -fields-order y,z,c,x %s -- 2>&1 | FileCheck --check-prefix=CHECK-MESSAGES %s
+// FIXME: clang-reorder-fields should provide -verify mode to make writing these checks
+// easier and more accurate, for now we follow clang-tidy's approach.
+
+namespace bar {
+
+struct Dummy {
+  Dummy(int x, char c) : x(x), c(c) {}
+  int x;
+  char c;
+};
+
+class Foo {
+public:
+  Foo(int x, double y, char cin);
+  Foo(int nx);
+  Foo();
+  int x;
+  double y;
+  char c;
+  Dummy z;
+};
+
+static char bar(char c) {
+  return c + 1;
+}
+
+Foo::Foo() : x(), y(), c(), z(0, 'a') {}
+
+Foo::Foo(int x, double y, char cin) :  
+  x(x),                 
+  y(y),                 
+  c(cin),               
+  z(this->x, bar(c))    
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: reordering field x after z makes x uninitialized when used in init expression
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: reordering field c after z makes c uninitialized when used in init expression
+{}
+
+Foo::Foo(int nx) :
+  x(nx),              
+  y(x),
+  c(0),            
+  z(bar(bar(x)), c)     
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: reordering field x after y makes x uninitialized when used in init expression
+  // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: reordering field x after z makes x uninitialized when used in init expression
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: 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;
+}

clang-tools-extra/trunk/test/clang-reorder-fields/FieldDependencyWarningDerived.cpp

+// RUN: clang-reorder-fields -record-name bar::Derived -fields-order z,y %s -- 2>&1 | FileCheck --check-prefix=CHECK-MESSAGES %s
+// FIXME: clang-reorder-fields should provide -verify mode to make writing these checks
+// easier and more accurate, for now we follow clang-tidy's approach.
+
+namespace bar {
+struct Base {
+  int x;
+  int p;
+};
+
+class Derived : public Base {
+public:
+  Derived(long ny);
+  Derived(char nz);
+private:
+  long y;
+  char z;
+};
+
+Derived::Derived(long ny) : 
+  Base(),
+  y(ny), 
+  z(static_cast<char>(y)) 
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: reordering field y after z makes y uninitialized when used in init expression
+{}
+
+Derived::Derived(char nz) : 
+  Base(),
+  y(nz),
+  // Check that base class fields are correctly ignored in reordering checks
+  // x has field index 1 and so would improperly warn if this wasn't the case since the command for this file swaps field indexes 1 and 2
+  z(x) 
+  // CHECK-MESSAGES-NOT: :[[@LINE-1]]:3: warning: reordering field x after z makes x uninitialized when used in init expression
+{}
+
+} // namespace bar