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

4

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.

178

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

4

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;
+  // 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,

 }
 
 /// \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())
+    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()]) {
+        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/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

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
+
+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:      [[@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;
+}

test/clang-reorder-fields/FieldDependencyWarningDerived.cpp

+// RUN: clang-reorder-fields -record-name bar::Derived -fields-order z,y %s -- 2>&1 | FileCheck %s
+
+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: [[@LINE]]: 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-NOT: [[@LINE]]: Warning: reordering field x after z makes x uninitialized when used in init expression
+
+{}
+
+} // namespace bar