[clang] Add fix-it for -Wreorder-ctor.

[adamcz]

Summary

This version is very limited. It does not work when comments are present
anywhere in the initializer list, since we do not have a good way to
associate them to specific initalizer and move them around.

The fix-it is associated with the last ctor-order diagnostic and
reorders all initializers. This is better than providing fix for each
diagnostic generated, since user would have to apply them one-by-one.

This also fixes a bug that would trigger an assert() in dependent
classes, when initializer present in code might not be found in the
idealized list of initializers. That is related to PR7179.

[adamcz]

fixed comment typo

[adamcz]

Add a missing const

[kadircet]

thanks, comments around some implementations. the only high level question i have is about the choice of location for fix-it (see the detailed comment inline)

clang/lib/Sema/SemaDeclCXX.cpp

5198

nit: assert for fileids of begin and end at the beginning.

5208

since Tok and End are in the same file you can just use < operator.

5298

i don't really follow when this happens. comments mentions a dependent base, but test case only has a templated constructor. why do we think it is safe to continue in this case, while we bail out in case of a dependent class? i would suggest just bailing out at the beginning of the function if constructor is dependent too (in addition to it's class being dependent)

5311

why not move this into previous if block i.e.

if (isoutoforder) {
 if(!Initswithidealindex.empty()) { ... }
 if(IdealIndex== NumIdealInits) { ..}
 if(!InitsWithIdealIndex.empty() {
    // emit diag
  }
}

5337

I totally agree with having a single fix-it to re-order all of the initializers, but I am not sure if last initializer is the best location for that fixit, especially for interactive tools (it is definitely the right behaviour for command line tools like clang and clang-tidy tho, as they'll either print the fix multiple times or try to apply conflicting fixes)

e.g. if user sees 3 out-of-order initializer warnings, they'll likely hover over one of them and say, "aaah" and fix the code themselves, if fixit wasn't available with that one.
Moreover when there's only 1 out-of-order initialzier warning, they'll see the fixit attached no matter what.
Next time, when there are multiple warnings they won't see it and will likely file bugs saying that "clang(d) doesn't generate fixes all the time".

But I also don't have any better suggestions, as having it multiple times will definitely be helpful for interactive tools but will regress the others.. Maybe attaching it to the first initializer might be better overall?

5348

can we have some tests for this case?

5355

nit: ShouldEmitFix = ShouldEmitFix && !Range...

5358

what if there are comments after last initializer ? I think we should rather use LBraceLoc for the constructor in here.

clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp

139

i think it is enough to have int a; as a member do we really need a separate type? + why don't we just merge this with the previous case?

[Jeroen]

clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp

171

What would be the behavior for :c(x), b(3), a(2)?
Does that get fixed in 1 go, or should this be done in 2 steps?

clang/lib/Sema/SemaDeclCXX.cpp

 static const void *GetKeyForMember(ASTContext &Context,
                                    CXXCtorInitializer *Member) {
   if (!Member->isAnyMemberInitializer())
     return GetKeyForBase(Context, QualType(Member->getBaseClass(), 0));
 
   return Member->getAnyMember()->getCanonicalDecl();
 }
 
+// Returns true iff there are comments between Begin and End. Expects that both
+// Begin and End are in the same file.
+static bool RangeContainsComments(Sema &SemaRef, SourceLocation Begin,
+                                  SourceLocation End) {
+  auto &SrcMgr = SemaRef.getSourceManager();
+  auto BeginLocInfo = SrcMgr.getDecomposedLoc(Begin);
+  StringRef File = SrcMgr.getBufferData(BeginLocInfo.first);
+  Lexer Lex(SrcMgr.getLocForStartOfFile(BeginLocInfo.first),
+            SemaRef.getLangOpts(), File.begin(),
+            File.begin() + BeginLocInfo.second, File.end());
+  Lex.SetCommentRetentionState(true);
+  Token Tok;
+  while (!Lex.LexFromRawLexer(Tok) &&
+         SrcMgr.isBeforeInTranslationUnit(Tok.getLocation(), End)) {
+    if (Tok.is(tok::comment))
+      return true;
+  }
+  return false;
+}
+
 static void DiagnoseBaseOrMemInitializerOrder(
     Sema &SemaRef, const CXXConstructorDecl *Constructor,
     ArrayRef<CXXCtorInitializer *> Inits) {
   if (Constructor->getDeclContext()->isDependentContext())
     return;
 
   // Don't check initializers order unless the warning is enabled at the
   // location of at least one initializer.
if (Field->isUnnamedBitfield())
       continue;
 
     PopulateKeysForFields(Field, IdealInitKeys);
   }
 
   unsigned NumIdealInits = IdealInitKeys.size();
   unsigned IdealIndex = 0;
 
-  CXXCtorInitializer *PrevInit = nullptr;
+  // Instead of emitting diagnostic right away, we keep it here and only emit
+  // when we find a new one. The last one is emitted outside of this loop, with
+  // a FixIt attached.
+  PartialDiagnosticAt PDiag = {SourceLocation(),
+                               PartialDiagnostic::NullDiagnostic()};
+  // Pairs {IdealIndex, pointer-to-initializer}, valid (i.e. found in
+  // IdealInitKeys) initializers only. Last value is used to generate
+  // diagnostics, the index is then used to sort valid initializers in
+  // initialization order.
+  SmallVector<std::pair<unsigned, CXXCtorInitializer *>, 32>
+      InitsWithIdealIndex;
+  // MissingInIdeal[I] == true iff Inits[I] is not found in ideal list.
+  llvm::SmallBitVector MissingInIdeal(Inits.size());
+
   for (unsigned InitIndex = 0; InitIndex != Inits.size(); ++InitIndex) {
     CXXCtorInitializer *Init = Inits[InitIndex];
     const void *InitKey = GetKeyForMember(SemaRef.Context, Init);
 
     // Scan forward to try to find this initializer in the idealized
     // initializers list.
     for (; IdealIndex != NumIdealInits; ++IdealIndex)
       if (InitKey == IdealInitKeys[IdealIndex])
         break;
 
-    // If we didn't find this initializer, it must be because we
-    // scanned past it on a previous iteration.  That can only
-    // happen if we're out of order;  emit a warning.
-    if (IdealIndex == NumIdealInits && PrevInit) {
-      Sema::SemaDiagnosticBuilder D =
-        SemaRef.Diag(PrevInit->getSourceLocation(),
-                     diag::warn_initializer_out_of_order);
+    const bool IsOutOfOrder = IdealIndex == NumIdealInits;
+    if (IsOutOfOrder) {
+      if (!InitsWithIdealIndex.empty()) {
+        // This is not the first initializer we process. It's either not in the
+        // ideal list at all, or it's out of order. Scan from the beginning.
+        for (IdealIndex = 0; IdealIndex != NumIdealInits; ++IdealIndex)
+          if (InitKey == IdealInitKeys[IdealIndex])
+            break;
+      }
+      if (IdealIndex == NumIdealInits) {
+        // This initializer is not in the ideal list at all. This can happen in
+        // a class with dependent base, when we cannot tell if initializer is
+        // valid or not. Just ignore it and reset IdealIndex to the previous
+        // valid initializer.
+        MissingInIdeal[InitIndex] = true;
+        IdealIndex =
+            InitsWithIdealIndex.empty() ? 0 : InitsWithIdealIndex.back().first;
+        continue;
+      }
+    }
+
+    // If we found this initializer in an idealized list, but it was before the
+    // previous (valid) initializer, emit a warning.
+    if (IsOutOfOrder && !InitsWithIdealIndex.empty()) {
+      // Emit previous diagnostic, if any.
+      if (PDiag.second.getDiagID())
+        SemaRef.Diag(PDiag.first, PDiag.second);
+
+      const auto *PrevInit = InitsWithIdealIndex.back().second;
+      PDiag = {PrevInit->getSourceLocation(),
+               SemaRef.PDiag(diag::warn_initializer_out_of_order)};
 
       if (PrevInit->isAnyMemberInitializer())
-        D << 0 << PrevInit->getAnyMember()->getDeclName();
+        PDiag.second << 0 << PrevInit->getAnyMember()->getDeclName();
       else
-        D << 1 << PrevInit->getTypeSourceInfo()->getType();
+        PDiag.second << 1 << PrevInit->getTypeSourceInfo()->getType();
 
       if (Init->isAnyMemberInitializer())
-        D << 0 << Init->getAnyMember()->getDeclName();
+        PDiag.second << 0 << Init->getAnyMember()->getDeclName();
       else
-        D << 1 << Init->getTypeSourceInfo()->getType();
+        PDiag.second << 1 << Init->getTypeSourceInfo()->getType();
+    }
 
-      // Move back to the initializer's location in the ideal list.
-      for (IdealIndex = 0; IdealIndex != NumIdealInits; ++IdealIndex)
-        if (InitKey == IdealInitKeys[IdealIndex])
-          break;
+    InitsWithIdealIndex.push_back({IdealIndex, Init});
+  }
+
+  if (!PDiag.second.getDiagID())
+    return;
 
-      assert(IdealIndex < NumIdealInits &&
-             "initializer not found in initializer list");
-    }
+  // If possible, add fix to the last diagnostic. The fix will reorder all
+  // initializers. This is preferable to attaching overlapping fixes for each
+  // initializer, which the user would then have to apply one by one.
+  auto &SrcMgr = SemaRef.getSourceManager();
+  // Things get complicated when comments, macros or multiple files are
+  // involved. Check if that's the case and if so, do not emit the fix it.
+  // Ideally we would reorder the comments as well, but that is a lot more
+  // complicated.
+  // FIXME: Support re-ordering initializers with comments.
+  bool ShouldEmitFix = true;
+  for (const auto &Init : Inits) {
+    if (Init->getSourceLocation().isMacroID() ||
+        !SrcMgr.isWrittenInSameFile(Init->getSourceLocation(),
+                                    Inits.front()->getSourceLocation())) {
+      ShouldEmitFix = false;
+      break;
+    }
+  if (ShouldEmitFix)
+    ShouldEmitFix =
+        !RangeContainsComments(SemaRef, Inits.front()->getSourceLocation(),
+                               Inits.back()->getSourceLocation());
 
-    PrevInit = Init;
+  if (ShouldEmitFix) {
+    // Construct a FixIt for re-ordering the initializers.
+    llvm::sort(InitsWithIdealIndex.begin(), InitsWithIdealIndex.end());
+    unsigned ReorderInitIndex = 0;
+    for (const auto &InitIndexPair : InitsWithIdealIndex) {
+      auto *InitToInsert = InitIndexPair.second;
+      const CXXCtorInitializer *InitToReplace = Inits[ReorderInitIndex];
+      PDiag.second << FixItHint::CreateReplacement(
+          InitToReplace->getSourceRange(),
+          clang::Lexer::getSourceText(
+              CharSourceRange(InitToInsert->getSourceRange(), true), SrcMgr,
+              SemaRef.getLangOpts()));
+
+      // Advance ReorderInitIndex to next valid initializer.
+      do {
+        ++ReorderInitIndex;
+      } while (ReorderInitIndex < Inits.size() &&
+               MissingInIdeal[ReorderInitIndex]);
+    }
   }
+
+  SemaRef.Diag(PDiag.first, PDiag.second);
 }
 
 namespace {
 bool CheckRedundantInit(Sema &S,
                         CXXCtorInitializer *Init,
                         CXXCtorInitializer *&PrevInit) {
   if (!PrevInit) {
     PrevInit = Init;

clang/test/SemaCXX/warn-reorder-ctor-initialization.cpp

 // RUN: %clang_cc1  -fsyntax-only -Wreorder -verify %s
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -Wreorder %s 2>&1 | FileCheck %s
 
 struct BB {};
 
 struct BB1 {};
 
 class complex : public BB, BB1 { 
 public: 
   complex()
union {
       struct {
         int m_int;
       };
     };
   };
 }
 
 namespace PR7179 {
+struct Foo {
+  Foo();
+};
   struct X
   {
     struct Y
     {
       template <class T> Y(T x) : X(x) { }
     };
   };
+
+  struct X2 {
+    struct Y {
+      template <class T> Y(T x) : a(), X(x) {}
+      Foo a;
+    };
+  };
 }
 
 namespace test3 {
   struct foo {
     struct {
       int a;
       int b;
     };
     foo() : b(), a() { // expected-warning {{field 'b' will be initialized after field 'a'}}
     }
   };
 }
+
+namespace fix {
+struct Foo {
+  Foo(int) {}
+};
+struct Bar {
+  Foo a, b, c;
+
+  Bar() : c(1), a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:17}:"a(2)"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:19-[[@LINE-2]]:23}:"b(3)"
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:25-[[@LINE-3]]:29}:"c(1)"
+
+  // Check that we do not generate fix it when comments are present. Ideally
+  // we would, but comments would need to be reordered as well and the current
+  // code can't do this.
+  Bar(int x) : c(x), /*foo*/ a(2), b(3) {} // expected-warning {{field 'c' will be initialized after field 'a'}}
+  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
+};
+} // namespace fix