[clang-tidy] Format code around applied fixes

[alexfh]

Summary

Add -format option (disabled by default for now) to trigger formatting
of replacements.

[malcolm.parsons]

test/clang-tidy/readability-braces-around-statements-format.cpp

1

Will clang-tidy look for a .clang-format file?
Does this test rely on a .clang-format file being found / not found?

[alexfh]

Pacify llvm::Expected<> debug checks.

[alexfh]

test/clang-tidy/readability-braces-around-statements-format.cpp

1

Good question. No idea ;) Lemme check...

[kimgr]

docs/clang-tidy/index.rst

184

This looks like a separate patch?

[alexfh]

docs/clang-tidy/index.rst

184

I've just pasted up-to-date -help output here. This should have been done when the -quiet option was added.

[ioeric]

test/clang-tidy/readability-braces-around-statements-format.cpp

1

Yes, it will look for .clang-format file in the working directory and its parents. If there happens to be a .clang-format with different style, the test might fail. We might want to explicitly set the style or add a .clang-format file in the test directory.

[alexfh]

Replace the separate -format and -style options with -format-style (default is
'none', which means no formatting).

[malcolm.parsons]

In D30564#691441, @alexfh wrote:

Replace the separate -format and -style options with -format-style (default is
'none', which means no formatting).

Is there a style that means use my .clang-format file?

[alexfh]

Expanded -format-style option description. Run cleanup tests with different format styles, just in case.

[alexfh]

In D30564#691446, @malcolm.parsons wrote:

In D30564#691441, @alexfh wrote:

Replace the separate -format and -style options with -format-style (default is
'none', which means no formatting).

Is there a style that means use my .clang-format file?

Yes, -format-style=file, see the updated docs.

[alexfh]

Clarify the 'file' option a bit.

[alexfh]

PTAL

[malcolm.parsons]

LGTM.

[ioeric]

Lg

[ioeric]

clang-tidy/ClangTidy.cpp

225

Maybe still apply the replacements if formatting fails?

[alexfh]

Apply changes even when formatting fails.

clang-tidy/ClangTidy.h

 
 // FIXME: This interface will need to be significantly extended to be useful.
 // FIXME: Implement confidence levels for displaying/fixing errors.
 //
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
 /// Errors containing fixes are automatically applied and reformatted. If no
 /// clang-format configuration file is found, the given \P FormatStyle is used.
 void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
-                  StringRef FormatStyle, unsigned &WarningsAsErrorsCount);
+                  bool FormatFixes, StringRef FormatStyle,
+                  unsigned &WarningsAsErrorsCount);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.
 void exportReplacements(StringRef MainFilePath,
                         const std::vector<ClangTidyError> &Errors,
                         raw_ostream &OS);
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDY_H

clang-tidy/ClangTidy.cpp

public:
   bool supportsCrossFileDiagnostics() const override { return true; }
 
 private:
   ClangTidyContext &Context;
 };
 
 class ErrorReporter {
 public:
-  ErrorReporter(bool ApplyFixes, StringRef FormatStyle)
+  ErrorReporter(bool ApplyFixes, bool FormatFixes, StringRef FormatStyle)
       : Files(FileSystemOptions()), DiagOpts(new DiagnosticOptions()),
         DiagPrinter(new TextDiagnosticPrinter(llvm::outs(), &*DiagOpts)),
         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
-        SourceMgr(Diags, Files), ApplyFixes(ApplyFixes), TotalFixes(0),
-        AppliedFixes(0), WarningsAsErrors(0), FormatStyle(FormatStyle) {
+        SourceMgr(Diags, Files), ApplyFixes(ApplyFixes),
+        FormatFixes(FormatFixes), TotalFixes(0), AppliedFixes(0),
+        WarningsAsErrors(0), FormatStyle(FormatStyle) {
     DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
     DiagPrinter->BeginSourceFile(LangOpts);
   }
 
   SourceManager &getSourceManager() { return SourceMgr; }
 
   void reportDiagnostic(const ClangTidyError &Error) {
     const tooling::DiagnosticMessage &Message = Error.Message;
for (auto Fix : FixLocations) {
       Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
                                          : diag::note_fixit_failed);
     }
     for (const auto &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 << ": "
if (ApplyFixes && TotalFixes > 0) {
         }
         llvm::Expected<Replacements> CleanReplacements =
             format::cleanupAroundReplacements(Code, FileAndReplacements.second,
                                               *Style);
         if (!CleanReplacements) {
           llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
           continue;
         }
+        if (FormatFixes) {
+          llvm::Expected<Replacements> FormattedReplacements =
+              format::formatReplacements(Code, *CleanReplacements, *Style);
+          if (FormattedReplacements) {
+            CleanReplacements = std::move(FormattedReplacements);
+          } else {
+            llvm::errs() << llvm::toString(FormattedReplacements.takeError())
+                         << ". Continuing without formatting.\n";
+          }
+        }
         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";
       }
     }
   }
private:
   FileManager Files;
   LangOptions LangOpts; // FIXME: use langopts from each original file
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
   DiagnosticConsumer *DiagPrinter;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
   llvm::StringMap<Replacements> FileReplacements;
   bool ApplyFixes;
+  bool FormatFixes;
   unsigned TotalFixes;
   unsigned AppliedFixes;
   unsigned WarningsAsErrors;
   StringRef FormatStyle;
 };
 
 class ClangTidyASTConsumer : public MultiplexConsumer {
 public:
runClangTidy(std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
 
   ActionFactory Factory(Context);
   Tool.run(&Factory);
   *Errors = Context.getErrors();
   return Context.getStats();
 }
 
 void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix,
-                  StringRef FormatStyle, unsigned &WarningsAsErrorsCount) {
-  ErrorReporter Reporter(Fix, FormatStyle);
+                  bool FormatFixes, StringRef FormatStyle,
+                  unsigned &WarningsAsErrorsCount) {
+  ErrorReporter Reporter(Fix, FormatFixes, FormatStyle);
   vfs::FileSystem &FileSystem =
       *Reporter.getSourceManager().getFileManager().getVirtualFileSystem();
   auto InitialWorkingDir = FileSystem.getCurrentWorkingDirectory();
   if (!InitialWorkingDir)
     llvm::report_fatal_error("Cannot get current working path.");
 
   for (const ClangTidyError &Error : Errors) {
     if (!Error.BuildDirectory.empty()) {

clang-tidy/tool/ClangTidyMain.cpp

 
 static cl::opt<std::string> Checks("checks", cl::desc(R"(
 Comma-separated list of globs with optional '-'
 prefix. Globs are processed in order of
 appearance in the list. Globs without '-'
 prefix add checks with matching names to the
 set, globs with the '-' prefix remove checks
 with matching names from the set of enabled
-checks.  This option's value is appended to the
+checks. This option's value is appended to the
 value of the 'Checks' option in .clang-tidy
 file, if any.
 )"),
                                    cl::init(""), cl::cat(ClangTidyCategory));
 
 static cl::opt<std::string> WarningsAsErrors("warnings-as-errors", cl::desc(R"(
 Upgrades warnings to errors. Same format as
 '-checks'.

 static cl::opt<bool> FixErrors("fix-errors", cl::desc(R"(
 Apply suggested fixes even if compilation
 errors were found. If compiler errors have
 attached fix-its, clang-tidy will apply them as
 well.
 )"),
                                cl::init(false), cl::cat(ClangTidyCategory));
 
+static cl::opt<bool> Format("format", cl::desc(R"(
+Format code around applied fixes.
+)"),
+                            cl::init(false), cl::cat(ClangTidyCategory));
+
 static cl::opt<std::string> FormatStyle("style", cl::desc(R"(
 Fallback style for reformatting after inserting fixes
 if there is no clang-format config file found.
 )"),
                                         cl::init("llvm"),
                                         cl::cat(ClangTidyCategory));
 
 static cl::opt<bool> ListChecks("list-checks", cl::desc(R"(

 YAML file to store suggested fixes in. The
 stored fixes can be applied to the input source
 code with clang-apply-replacements.
 )"),
                                         cl::value_desc("filename"),
                                         cl::cat(ClangTidyCategory));
 
 static cl::opt<bool> Quiet("quiet", cl::desc(R"(
-Run clang-tidy in quiet mode.  This suppresses
+Run clang-tidy in quiet mode. This suppresses
 printing statistics about ignored warnings and
 warnings treated as errors if the respective
 options are specified.
 )"),
                            cl::init(false),
                            cl::cat(ClangTidyCategory));
 
 namespace clang {
bool FoundErrors =
         return E.DiagLevel == ClangTidyError::Error;
       }) != Errors.end();
 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;
 
   unsigned WErrorCount = 0;
 
   // -fix-errors implies -fix.
-  handleErrors(Errors, (FixErrors || Fix) && !DisableFixes, FormatStyle,
+  handleErrors(Errors, (FixErrors || Fix) && !DisableFixes, Format, FormatStyle,
                WErrorCount);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
     std::error_code EC;
     llvm::raw_fd_ostream OS(ExportFixes, EC, llvm::sys::fs::F_None);
     if (EC) {
       llvm::errs() << "Error opening output file: " << EC.message() << '\n';
       return 1;

docs/ReleaseNotes.rst

- New `safety-no-assembler
   Finds uses of inline assembler.
 
 - New `modernize-return-braced-init-list
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-return-braced-init-list.html>`_ check
 
   Finds and replaces explicit calls to the constructor in a return statement by
   a braced initializer list so that the return type is not needlessly repeated.
 
+- Support clang-formatting of the code around applied fixes (``-format``
+  command-line option).
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/index.rst

-fix                         -
                                    Apply suggested fixes. Without -fix-errors
                                    clang-tidy will bail out if any compilation
                                    errors were found.
     -fix-errors                  -
                                    Apply suggested fixes even if compilation
                                    errors were found. If compiler errors have
                                    attached fix-its, clang-tidy will apply them as
                                    well.
+    -format                      -
+                                   Format code around applied fixes.
     -header-filter=<string>      -
                                    Regular expression matching the names of the
                                    headers to output diagnostics from. Diagnostics
                                    from the main file of each translation unit are
                                    always displayed.
                                    Can be used together with -line-filter.
                                    This option overrides the 'HeaderFilter' option
                                    in .clang-tidy file, if any.
     -line-filter=<string>        -
                                    List of files with line ranges to filter the
                                    warnings. Can be used together with
                                    -header-filter. The format of the list is a
                                    JSON array of objects:
                                      [
                                        {"name":"file1.cpp","lines":[[1,3],[5,7]]},
                                        {"name":"file2.h"}
                                      ]
     -list-checks                 -
                                    List all enabled checks and exit. Use with
                                    -checks=* to list all available checks.
     -p=<string>                  - Build path
+    -quiet                       -
+                                   Run clang-tidy in quiet mode. This suppresses
+                                   printing statistics about ignored warnings and
+                                   warnings treated as errors if the respective
+                                   options are specified.
     -style=<string>              -
                                    Fallback style for reformatting after inserting fixes
                                    if there is no clang-format config file found.
     -system-headers              - Display the errors from system headers.
     -warnings-as-errors=<string> -
                                    Upgrades warnings to errors. Same format as
                                    '-checks'.
                                    This option's value is appended to the value of

 diagnostic messages and fix-its. It filters out ``CHECK`` lines from the test
 file, runs :program:`clang-tidy` and verifies messages and fixes with two
 separate `FileCheck`_ invocations: once with FileCheck's directive
 prefix set to ``CHECK-MESSAGES``, validating the diagnostic messages,
 and once with the directive prefix set to ``CHECK-FIXES``, running
 against the fixed code (i.e., the code after generated fix-its are
 applied). In particular, ``CHECK-FIXES:`` can be used to check
 that code was not modified by fix-its, by checking that it is present
-unchanged in the fixed code.  The full set of `FileCheck`_ directives
+unchanged in the fixed code. The full set of `FileCheck`_ directives
 is available (e.g., ``CHECK-MESSAGES-SAME:``, ``CHECK-MESSAGES-NOT:``), though
 typically the basic ``CHECK`` forms (``CHECK-MESSAGES`` and ``CHECK-FIXES``)
-are sufficient for clang-tidy tests.  Note that the `FileCheck`_
+are sufficient for clang-tidy tests. Note that the `FileCheck`_
 documentation mostly assumes the default prefix (``CHECK``), and hence
 describes the directive as ``CHECK:``, ``CHECK-SAME:``, ``CHECK-NOT:``, etc.
 Replace ``CHECK`` by either ``CHECK-FIXES`` or ``CHECK-MESSAGES`` for
 clang-tidy tests.
 
 An additional check enabled by ``check_clang_tidy.py`` ensures that
 if `CHECK-MESSAGES:` is used in a file then every warning or error
 must have an associated CHECK in that file.

test/clang-tidy/readability-braces-around-statements-format.cpp

+// RUN: %check_clang_tidy %s readability-braces-around-statements %t -- -format --
+
+void do_something(const char *) {}
+
+bool cond(const char *) {
+  return false;
+}
+
+void test() {
+  if (cond("if0") /*comment*/) do_something("same-line");
+  // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: statement should be inside braces
+  // CHECK-FIXES: {{^}}  if (cond("if0") /*comment*/) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    do_something("same-line");{{$}}
+  // CHECK-FIXES-NEXT: {{^}}  }{{$}}
+
+  if (1) while (2) if (3) for (;;) do ; while(false) /**/;/**/
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: statement should be inside braces
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: statement should be inside braces
+  // CHECK-MESSAGES: :[[@LINE-3]]:26: warning: statement should be inside braces
+  // CHECK-MESSAGES: :[[@LINE-4]]:35: warning: statement should be inside braces
+  // CHECK-MESSAGES: :[[@LINE-5]]:38: warning: statement should be inside braces
+  // CHECK-FIXES:      {{^}}  if (1) {{{$}}
+  // CHECK-FIXES-NEXT: {{^}}    while (2) {
+  // CHECK-FIXES-NEXT: {{^}}      if (3) {
+  // CHECK-FIXES-NEXT: {{^}}        for (;;) {
+  // CHECK-FIXES-NEXT: {{^}}          do {
+  // CHECK-FIXES-NEXT: {{^}}            ;
+  // CHECK-FIXES-NEXT: {{^}}          } while (false) /**/; /**/
+  // CHECK-FIXES-NEXT: {{^}}        }
+  // CHECK-FIXES-NEXT: {{^}}      }
+  // CHECK-FIXES-NEXT: {{^}}    }
+  // CHECK-FIXES-NEXT: {{^}}  }
+}