[clang-tidy] Clean up code after applying replacements.

Summary:
Remove empty namespaces and initializer list commas / colons in
affected ranges. Initial patch: proper options for enabling the cleanup and
specifying the format style are needed.

Reviewers: hokein, ioeric

Subscribers: beanz, mgorny, cfe-commits

Differential Revision: https://reviews.llvm.org/D24572

llvm-svn: 284399

clang-tools-extra/clang-tidy/CMakeLists.txt

@@ -15,6 +15,7 @@ add_clang_library(clangTidy
   clangAST
   clangASTMatchers
   clangBasic
+  clangFormat
   clangFrontend
   clangLex
   clangRewrite

clang-tools-extra/clang-tidy/ClangTidy.cpp

@@ -22,6 +22,7 @@
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Format/Format.h"
 #include "clang/Frontend/ASTConsumers.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendActions.h"
@@ -101,9 +102,8 @@ public:
         DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
-        SourceMgr(Diags, Files), Rewrite(SourceMgr, LangOpts),
-        ApplyFixes(ApplyFixes), TotalFixes(0), AppliedFixes(0),
-        WarningsAsErrors(0) {
+        SourceMgr(Diags, Files), ApplyFixes(ApplyFixes), TotalFixes(0),
+        AppliedFixes(0), WarningsAsErrors(0) {
     DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
     DiagPrinter->BeginSourceFile(LangOpts);
   }
@@ -127,31 +127,58 @@ public:
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
                   << Message.Message << Name;
       for (const auto &FileAndReplacements : Error.Fix) {
-        for (const auto &Replacement : FileAndReplacements.second) {
+        for (const auto &Repl : FileAndReplacements.second) {
           // Retrieve the source range for applicable fixes. Macro definitions
           // on the command line have locations in a virtual buffer and don't
           // have valid file paths and are therefore not applicable.
           SourceRange Range;
           SourceLocation FixLoc;
-          if (Replacement.isApplicable()) {
-            SmallString<128> FixAbsoluteFilePath = Replacement.getFilePath();
+          ++TotalFixes;
+          bool CanBeApplied = false;
+          if (Repl.isApplicable()) {
+            SmallString<128> FixAbsoluteFilePath = Repl.getFilePath();
             Files.makeAbsolutePath(FixAbsoluteFilePath);
-            FixLoc = getLocation(FixAbsoluteFilePath, Replacement.getOffset());
+            if (ApplyFixes) {
+              tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(),
+                                     Repl.getLength(),
+                                     Repl.getReplacementText());
+              Replacements &Replacements = FileReplacements[R.getFilePath()];
+              llvm::Error Err = Replacements.add(R);
+              if (Err) {
+                // FIXME: Implement better conflict handling.
+                llvm::errs() << "Trying to resolve conflict: "
+                             << llvm::toString(std::move(Err)) << "\n";
+                unsigned NewOffset =
+                    Replacements.getShiftedCodePosition(R.getOffset());
+                unsigned NewLength = Replacements.getShiftedCodePosition(
+                                         R.getOffset() + R.getLength()) -
+                                     NewOffset;
+                if (NewLength == R.getLength()) {
+                  R = Replacement(R.getFilePath(), NewOffset, NewLength,
+                                  R.getReplacementText());
+                  Replacements = Replacements.merge(tooling::Replacements(R));
+                  CanBeApplied = true;
+                  ++AppliedFixes;
+                } else {
+                  llvm::errs()
+                      << "Can't resolve conflict, skipping the replacement.\n";
+                }
+
+              } else {
+                CanBeApplied = true;
+                ++AppliedFixes;
+              }
+            }
+            FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
             SourceLocation FixEndLoc =
-                FixLoc.getLocWithOffset(Replacement.getLength());
+                FixLoc.getLocWithOffset(Repl.getLength());
             Range = SourceRange(FixLoc, FixEndLoc);
-            Diag << FixItHint::CreateReplacement(
-                Range, Replacement.getReplacementText());
+            Diag << FixItHint::CreateReplacement(Range,
+                                                 Repl.getReplacementText());
           }
 
-          ++TotalFixes;
-          if (ApplyFixes) {
-            bool Success =
-                Replacement.isApplicable() && Replacement.apply(Rewrite);
-            if (Success)
-              ++AppliedFixes;
-            FixLocations.push_back(std::make_pair(FixLoc, Success));
-          }
+          if (ApplyFixes)
+            FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
         }
       }
     }
@@ -166,9 +193,37 @@ public:
   void Finish() {
     // FIXME: Run clang-format on changes.
     if (ApplyFixes && TotalFixes > 0) {
-      llvm::errs() << "clang-tidy applied " << AppliedFixes << " of "
-                   << TotalFixes << " suggested fixes.\n";
-      Rewrite.overwriteChangedFiles();
+      Rewriter Rewrite(SourceMgr, LangOpts);
+      for (const auto &FileAndReplacements : FileReplacements) {
+        StringRef File = FileAndReplacements.first();
+        llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer =
+            SourceMgr.getFileManager().getBufferForFile(File);
+        if (!Buffer) {
+          llvm::errs() << "Can't get buffer for file " << File << ": "
+                       << Buffer.getError().message() << "\n";
+          // FIXME: Maybe don't apply fixes for other files as well.
+          continue;
+        }
+        StringRef Code = Buffer.get()->getBuffer();
+        // FIXME: Make the style customizable.
+        format::FormatStyle Style = format::getStyle("file", File, "LLVM");
+        llvm::Expected<Replacements> CleanReplacements =
+            format::cleanupAroundReplacements(Code, FileAndReplacements.second,
+                                              Style);
+        if (!CleanReplacements) {
+          llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
+          continue;
+        }
+        if (!tooling::applyAllReplacements(CleanReplacements.get(), Rewrite)) {
+          llvm::errs() << "Can't apply replacements for file " << File << "\n";
+        }
+      }
+      if (Rewrite.overwriteChangedFiles()) {
+        llvm::errs() << "clang-tidy failed to apply suggested fixes.\n";
+      } else {
+        llvm::errs() << "clang-tidy applied " << AppliedFixes << " of "
+                     << TotalFixes << " suggested fixes.\n";
+      }
     }
   }
 
@@ -197,7 +252,7 @@ private:
   DiagnosticConsumer *DiagPrinter;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
-  Rewriter Rewrite;
+  llvm::StringMap<Replacements> FileReplacements;
   bool ApplyFixes;
   unsigned TotalFixes;
   unsigned AppliedFixes;
@@ -416,7 +471,7 @@ ClangTidyOptions::OptionMap getCheckOptions(const ClangTidyOptions &Options) {
 
 ClangTidyStats
 runClangTidy(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
-             const tooling::CompilationDatabase &Compilations,
+             const CompilationDatabase &Compilations,
              ArrayRef<std::string> InputFiles,
              std::vector<ClangTidyError> *Errors, ProfileData *Profile) {
   ClangTool Tool(Compilations, InputFiles);
@@ -519,7 +574,7 @@ void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
 
 void exportReplacements(const std::vector<ClangTidyError> &Errors,
                         raw_ostream &OS) {
-  tooling::TranslationUnitReplacements TUR;
+  TranslationUnitReplacements TUR;
   for (const ClangTidyError &Error : Errors) {
     for (const auto &FileAndFixes : Error.Fix)
       TUR.Replacements.insert(TUR.Replacements.end(),

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

@@ -79,12 +79,13 @@ protected:
 
       tooling::Replacement Replacement(SM, Range, FixIt.CodeToInsert);
       llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
-      // FIXME: better error handling.
+      // FIXME: better error handling (at least, don't let other replacements be
+      // applied).
       if (Err) {
         llvm::errs() << "Fix conflicts with existing fix! "
                     << llvm::toString(std::move(Err)) << "\n";
+        assert(false && "Fix conflicts with existing fix!");
       }
-      assert(!Err && "Fix conflicts with existing fix!");
     }
   }
 

clang-tools-extra/test/clang-tidy/clean-up-code.cpp

@@ -0,0 +1,12 @@
+// RUN: %check_clang_tidy %s misc-unused-using-decls %t
+namespace a { class A {}; }
+namespace b {
+using a::A;
+}
+namespace c {}
+// CHECK-MESSAGES: :[[@LINE-3]]:10: warning: using decl 'A' is unused [misc-unused-using-decls]
+// CHECK-FIXES: {{^namespace a { class A {}; }$}}
+// CHECK-FIXES-NOT: namespace
+// CHECK-FIXES: {{^namespace c {}$}}
+// FIXME: cleanupAroundReplacements leaves whitespace. Otherwise we could just
+// check the next line.

clang-tools-extra/test/clang-tidy/fix.cpp

@@ -5,6 +5,7 @@
 // RUN: FileCheck -input-file=%t.yaml -check-prefix=CHECK-YAML %s
 
 namespace i {
+void f(); // So that the namespace isn't empty.
 }
 // CHECK: } // namespace i
 // CHECK-MESSAGES: note: FIX-IT applied suggested code changes

clang-tools-extra/test/clang-tidy/google-readability-namespace-comments.cpp

@@ -4,7 +4,7 @@ namespace n1 {
 namespace n2 {
 
 
-
+void f(); // So that the namespace isn't empty.
 
 
 // CHECK-MESSAGES: :[[@LINE+4]]:2: warning: namespace 'n2' not terminated with a closing comment [google-readability-namespace-comments]