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

[alexfh]

Summary

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

[ioeric]

clang-tidy/ClangTidy.cpp

25

Also add dependency on clangFormat?

159

Do you need to update CanBeApplied and AppliedFixes in this branch as well?

202

This FIXME is not very clear, i.e. does it mean don't apply anything for all files? Or just a single file?

206

Add a FIXME here.

And I think this might be a better workaround for now:

format::FormatStyle InsertStyle = format::getStyle("file", File, "LLVM");

[hokein]

clang-tidy/ClangTidy.cpp

206

A further thought: we might not want to hard-code the code-style here. Add a FIXME to make the style customizable?

218

Shouldn't we check whether the overwrite operation is successful or not?

[alexfh]

• Addressed review comments

[ioeric]

lgtm

[malcolm.parsons]

Does clang-apply-replacements need a similar change?

clang-tidy/ClangTidy.cpp

 
 #include "ClangTidy.h"
 #include "ClangTidyDiagnosticConsumer.h"
 #include "ClangTidyModuleRegistry.h"
 #include "clang/AST/ASTConsumer.h"
 #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"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/MultiplexConsumer.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Lex/Preprocessor.h"

 
 class ErrorReporter {
 public:
   ErrorReporter(bool ApplyFixes)
       : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
         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);
   }
 
   SourceManager &getSourceManager() { return SourceMgr; }
 
   void reportDiagnostic(const ClangTidyError &Error) {
     const ClangTidyMessage &Message = Error.Message;
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
     // Contains a pair for each attempted fix: location and whether the fix was
     // applied successfully.
     SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
     {
       auto Level = static_cast<DiagnosticsEngine::Level>(Error.DiagLevel);
       std::string Name = Error.CheckName;
       if (Error.IsWarningAsError) {
         Name += ",-warnings-as-errors";
         Level = DiagnosticsEngine::Error;
         WarningsAsErrors++;
       }
       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());
-            SourceLocation FixEndLoc =
-                FixLoc.getLocWithOffset(Replacement.getLength());
-            Range = SourceRange(FixLoc, FixEndLoc);
-            Diag << FixItHint::CreateReplacement(
-                Range, Replacement.getReplacementText());
+            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));
+                } else {
+                  llvm::errs()
+                      << "Can't resolve conflict, skipping the replacement.\n";
                 }
 
-          ++TotalFixes;
-          if (ApplyFixes) {
-            bool Success =
-                Replacement.isApplicable() && Replacement.apply(Rewrite);
-            if (Success)
+              } else {
+                CanBeApplied = true;
                 ++AppliedFixes;
-            FixLocations.push_back(std::make_pair(FixLoc, Success));
               }
             }
+            FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset());
+            SourceLocation FixEndLoc =
+                FixLoc.getLocWithOffset(Repl.getLength());
+            Range = SourceRange(FixLoc, FixEndLoc);
+            Diag << FixItHint::CreateReplacement(Range,
+                                                 Repl.getReplacementText());
+          }
+
+          if (ApplyFixes)
+            FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied));
+        }
       }
     }
     for (auto Fix : FixLocations) {
       Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
                                          : diag::note_fixit_failed);
     }
     for (const ClangTidyMessage &Note : Error.Notes)
       reportNote(Note);
   }
 
   void Finish() {
     // FIXME: Run clang-format on changes.
     if (ApplyFixes && TotalFixes > 0) {
+      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 anything.
+          continue;
+        }
+        StringRef Code = Buffer.get()->getBuffer();
+        format::FormatStyle Style = format::getLLVMStyle();
+        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";
+        }
+      }
+      Rewrite.overwriteChangedFiles();
       llvm::errs() << "clang-tidy applied " << AppliedFixes << " of "
                    << TotalFixes << " suggested fixes.\n";
-      Rewrite.overwriteChangedFiles();
     }
   }
 
   unsigned getWarningsAsErrorsCount() const { return WarningsAsErrors; }
 
 private:
   SourceLocation getLocation(StringRef FilePath, unsigned Offset) {
     if (FilePath.empty())
private:
   }
 
   FileManager Files;
   LangOptions LangOpts; // FIXME: use langopts from each original file
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
   DiagnosticConsumer *DiagPrinter;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
-  Rewriter Rewrite;
+  llvm::StringMap<Replacements> FileReplacements;
   bool ApplyFixes;
   unsigned TotalFixes;
   unsigned AppliedFixes;
   unsigned WarningsAsErrors;
 };
 
 class ClangTidyASTConsumer : public MultiplexConsumer {
 public:
clang::tidy::ClangTidyContext Context(
       llvm::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(),
                                                 Options));
   ClangTidyASTConsumerFactory Factory(Context);
   return Factory.getCheckOptions();
 }
 
 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);
   clang::tidy::ClangTidyContext Context(std::move(OptionsProvider));
 
   // Add extra arguments passed by the clang-tidy command-line.
   ArgumentsAdjuster PerFileExtraArgumentsInserter =
       [&Context](const CommandLineArguments &Args, StringRef Filename) {
for (const ClangTidyError &Error : Errors) {
     FileSystem.setCurrentWorkingDirectory(InitialWorkingDir.get());
   }
   Reporter.Finish();
   WarningsAsErrorsCount += Reporter.getWarningsAsErrorsCount();
 }
 
 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(),
                               FileAndFixes.second.begin(),
                               FileAndFixes.second.end());
   }
 
   yaml::Output YAML(OS);
   YAML << TUR;
 }
 
 } // namespace tidy
 } // namespace clang

clang-tidy/ClangTidyDiagnosticConsumer.cpp

for (const auto &FixIt : Hints) {
       CharSourceRange Range = FixIt.RemoveRange;
       assert(Range.getBegin().isValid() && Range.getEnd().isValid() &&
              "Invalid range in the fix-it hint.");
       assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() &&
              "Only file locations supported in fix-it hints.");
 
       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!");
     }
   }
 
   void emitIncludeLocation(SourceLocation Loc, PresumedLoc PLoc,
                            const SourceManager &SM) override {}
 
   void emitImportLocation(SourceLocation Loc, PresumedLoc PLoc,
                           StringRef ModuleName,

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

+// 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.

test/clang-tidy/fix.cpp

 // RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
 // RUN: clang-tidy %t.cpp -checks='-*,google-explicit-constructor,llvm-namespace-comment' -fix -export-fixes=%t.yaml -- > %t.msg 2>&1
 // RUN: FileCheck -input-file=%t.cpp %s
 // RUN: FileCheck -input-file=%t.msg -check-prefix=CHECK-MESSAGES %s
 // 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
 // CHECK-YAML: ReplacementText: ' // namespace i'
 
 class A { A(int i); };
 // CHECK: class A { explicit A(int i); };
 // CHECK-MESSAGES: note: FIX-IT applied suggested code changes
 // CHECK-MESSAGES: clang-tidy applied 2 of 2 suggested fixes.
 // CHECK-YAML: ReplacementText: 'explicit '

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

 // RUN: %check_clang_tidy %s google-readability-namespace-comments %t
 
 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]
 // CHECK-MESSAGES: :[[@LINE-7]]:11: note: namespace 'n2' starts here
 // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: namespace 'n1' not terminated with
 // CHECK-MESSAGES: :[[@LINE-10]]:11: note: namespace 'n1' starts here
 }}
 // CHECK-FIXES: }  // namespace n2