[clang-move] Clever on handling header file which includes itself.

[hokein]

Summary

Previously, we assume only old.cc includes "old.h", which would
introduce incorrect fixes for the cases where old.h also includes #include "old.h"

Although it should not be occurred in real projects, clang-move should handle this.

Old.h:

class Foo {};

after moving to a new old.h:

class Foo {};

[ioeric]

clang-move/ClangMove.cpp

708

This check is always true?

868

I'd pull this into a lambda.

[hokein]

address review comments.

[hokein]

clang-move/ClangMove.cpp

708

Oops, good catch, should be AbsoluteIncludeHeader.

[ioeric]

lg

clang-move/ClangMove.h

private:
   // anonymous namespaces, static function/variable definitions in global/named
   // namespaces) in old.cc. saving in an AST-visited order.
   std::vector<const NamedDecl *> HelperDeclarations;
   // The unmoved named declarations in old header.
   llvm::SmallPtrSet<const NamedDecl*, 8> UnremovedDeclsInOldHeader;
   /// The source range for the written file name in #include (i.e. "old.h" for
   /// #include "old.h") in old.cc,  including the enclosing quotes or angle
   /// brackets.
-  clang::CharSourceRange OldHeaderIncludeRange;
+  clang::CharSourceRange OldHeaderIncludeRangeInCC;
+  /// The source range for the written file name in #include (i.e. "old.h" for
+  /// #include "old.h") in old.h,  including the enclosing quotes or angle
+  /// brackets.
+  clang::CharSourceRange OldHeaderIncludeRangeInHeader;
   /// Mapping from FilePath to FileID, which can be used in post processes like
   /// cleanup around replacements.
   llvm::StringMap<FileID> FilePathToFileID;
   /// A context contains all running options. It is not owned.
   ClangMoveContext *const Context;
   /// A reporter to report all declarations from old header. It is not owned.
   DeclarationReporter *const Reporter;
   /// Builder for helper declarations reference graph.

clang-move/ClangMove.cpp

 
 void ClangMoveTool::addIncludes(llvm::StringRef IncludeHeader, bool IsAngled,
                                 llvm::StringRef SearchPath,
                                 llvm::StringRef FileName,
                                 clang::CharSourceRange IncludeFilenameRange,
                                 const SourceManager &SM) {
   SmallVector<char, 128> HeaderWithSearchPath;
   llvm::sys::path::append(HeaderWithSearchPath, SearchPath, IncludeHeader);
-  std::string AbsoluteOldHeader = makeAbsolutePath(Context->Spec.OldHeader);
-  if (AbsoluteOldHeader ==
+  std::string AbsoluteIncludeHeader =
       MakeAbsolutePath(SM, llvm::StringRef(HeaderWithSearchPath.data(),
-                                           HeaderWithSearchPath.size()))) {
-    OldHeaderIncludeRange = IncludeFilenameRange;
-    return;
-  }
-
+                                           HeaderWithSearchPath.size()));
   std::string IncludeLine =
       IsAngled ? ("#include <" + IncludeHeader + ">\n").str()
                : ("#include \"" + IncludeHeader + "\"\n").str();
 
+  std::string AbsoluteOldHeader = makeAbsolutePath(Context->Spec.OldHeader);
   std::string AbsoluteCurrentFile = MakeAbsolutePath(SM, FileName);
   if (AbsoluteOldHeader == AbsoluteCurrentFile) {
+    // Find old.h includes "old.h".
+    if (AbsoluteOldHeader == AbsoluteIncludeHeader) {
+      OldHeaderIncludeRangeInHeader = IncludeFilenameRange;
+      return;
+    }
     HeaderIncludes.push_back(IncludeLine);
   } else if (makeAbsolutePath(Context->Spec.OldCC) == AbsoluteCurrentFile) {
+    // Find old.cc includes "old.h".
+    if (AbsoluteOldHeader == AbsoluteIncludeHeader) {
+      OldHeaderIncludeRangeInCC = IncludeFilenameRange;
+      return;
+    }
     CCIncludes.push_back(IncludeLine);
   }
 }
 
 void ClangMoveTool::removeDeclsInOldFiles() {
   if (RemovedDecls.empty()) return;
 
   // If old_header is not specified (only move declarations from old.cc), remain
void ClangMoveTool::moveAll(SourceManager &SM, StringRef OldFile,
   std::string FilePath = RemoveAll.getFilePath().str();
   Context->FileToReplacements[FilePath] =
       clang::tooling::Replacements(RemoveAll);
 
   StringRef Code = SM.getBufferData(ID);
   if (!NewFile.empty()) {
     auto AllCode = clang::tooling::Replacements(
         clang::tooling::Replacement(NewFile, 0, 0, Code));
-    // If we are moving from old.cc, an extra step is required: excluding
-    // the #include of "old.h", instead, we replace it with #include of "new.h".
-    if (Context->Spec.NewCC == NewFile && OldHeaderIncludeRange.isValid()) {
-      AllCode = AllCode.merge(
-          clang::tooling::Replacements(clang::tooling::Replacement(
-              SM, OldHeaderIncludeRange, '"' + Context->Spec.NewHeader + '"')));
-    }
+    auto ReplaceOldInclude = [&](clang::CharSourceRange OldHeaderIncludeRange) {
+      AllCode = AllCode.merge(clang::tooling::Replacements(
+          clang::tooling::Replacement(SM, OldHeaderIncludeRange,
+                                      '"' + Context->Spec.NewHeader + '"')));
+    };
+    // Fix the case where old.h/old.cc includes "old.h", we replace the
+    // `#include "old.h"` with `#include "new.h"`.
+    if (Context->Spec.NewCC == NewFile && OldHeaderIncludeRangeInCC.isValid())
+      ReplaceOldInclude(OldHeaderIncludeRangeInCC);
+    else if (Context->Spec.NewHeader == NewFile &&
+             OldHeaderIncludeRangeInHeader.isValid())
+      ReplaceOldInclude(OldHeaderIncludeRangeInHeader);
     Context->FileToReplacements[NewFile] = std::move(AllCode);
   }
 }
 
 void ClangMoveTool::onEndOfTranslationUnit() {
   if (Context->DumpDeclarations) {
     assert(Reporter);
     for (const auto *Decl : UnremovedDeclsInOldHeader) {

unittests/clang-move/ClangMoveTests.cpp

TEST(ClangMove, MoveNonExistClass) {
   Spec.OldHeader = "foo.h";
   Spec.OldCC = "foo.cc";
   Spec.NewHeader = "new_foo.h";
   Spec.NewCC = "new_foo.cc";
   auto Results = runClangMoveOnCode(Spec);
   EXPECT_EQ(0u, Results.size());
 }
 
+TEST(ClangMove, HeaderIncludeSelf) {
+  move::MoveDefinitionSpec Spec;
+  Spec.Names = {std::string("Foo")};
+  Spec.OldHeader = "foo.h";
+  Spec.OldCC = "foo.cc";
+  Spec.NewHeader = "new_foo.h";
+  Spec.NewCC = "new_foo.cc";
+
+  const char TestHeader[] = "#ifndef FOO_H\n"
+                            "#define FOO_H\n"
+                            "#include \"foo.h\"\n"
+                            "class Foo {};\n"
+                            "#endif\n";
+  const char TestCode[] = "#include \"foo.h\"";
+  const char ExpectedNewHeader[] = "#ifndef FOO_H\n"
+                                   "#define FOO_H\n"
+                                   "#include \"new_foo.h\"\n"
+                                   "class Foo {};\n"
+                                   "#endif\n";
+  const char ExpectedNewCC[] = "#include \"new_foo.h\"";
+  auto Results = runClangMoveOnCode(Spec, TestHeader, TestCode);
+  EXPECT_EQ("", Results[Spec.OldHeader]);
+  EXPECT_EQ(ExpectedNewHeader, Results[Spec.NewHeader]);
+  EXPECT_EQ(ExpectedNewCC, Results[Spec.NewCC]);
+}
+
 TEST(ClangMove, MoveAll) {
   std::vector<std::string> TestHeaders = {
     "class A {\npublic:\n  int f();\n};",
     // forward declaration.
     "class B;\nclass A {\npublic:\n  int f();\n};",
     // template forward declaration.
     "template <typename T> class B;\nclass A {\npublic:\n  int f();\n};",
     "namespace a {}\nclass A {\npublic:\n  int f();\n};",