[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 ↗

(On Diff #71380)

Also add dependency on clangFormat?

159 ↗

(On Diff #71380)

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

202 ↗

(On Diff #71380)

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

206 ↗

(On Diff #71380)

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 ↗

(On Diff #71380)

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

218 ↗

(On Diff #71380)

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-tools-extra/trunk/clang-tidy/CMakeLists.txt

add_clang_library(clangTidy
 
   DEPENDS
   ClangSACheckers
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
+  clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangSema
   clangStaticAnalyzerCore
   clangStaticAnalyzerFrontend
   clangTooling
   clangToolingCore

clang-tools-extra/trunk/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));
+                  CanBeApplied = true;
+                  ++AppliedFixes;
+                } 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 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";
-      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-tools-extra/trunk/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,

clang-tools-extra/trunk/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.

clang-tools-extra/trunk/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 '

clang-tools-extra/trunk/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