Revamp the "[clangd] Format tweak's replacements"

[hokein]

Summary

This patch contains two parts:

1. reverts commit r353306.
2. move the format logic out from tweaks, keep tweaks API unchanged.

[hokein]

• [clangd] Re-submit format replacement for tweaks.

[ilya-biryukov]

LGTM

clangd/ClangdServer.cpp

382

NIT: use InpAST.Inputs.FS instead of creating a new instance of the vfs.

[hokein]

Address review comment.

clangd/ClangdServer.cpp

 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                WantDiagnostics WantDiags) {
   ParseOptions Opts;
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   if (ClangTidyOptProvider)
     Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File);
-  // FIXME: cache this.
-  Opts.Style =
-      getFormatStyleForFile(File, Contents, FSProvider.getFileSystem().get());
   Opts.SuggestMissingIncludes = SuggestMissingIncludes;
   // FIXME: some build systems like Bazel will take time to preparing
   // environment to build the file, it would be nice if we could emit a
   // "PreparingBuild" status to inform users, it is non-trivial given the
   // current implementation.
   ParseInputs Inputs;
   Inputs.CompileCommand = getCompileCommand(File);
   Inputs.FS = FSProvider.getFileSystem();
void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
   };
 
   WorkScheduler.runWithAST("EnumerateTweaks", File,
                            Bind(Action, std::move(CB), File.str()));
 }
 
 void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
                               Callback<tooling::Replacements> CB) {
-  auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+  auto Action = [Sel, this](decltype(CB) CB, std::string File,
+                            std::string TweakID,
                             Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     auto Selection = tweakSelection(Sel, *InpAST);
     if (!Selection)
       return CB(Selection.takeError());
     auto A = prepareTweak(TweakID, *Selection);
     if (!A)
       return CB(A.takeError());
-    return CB((*A)->apply(*Selection, InpAST->Inputs.Opts.Style));
+    auto RawReplacements = (*A)->apply(*Selection);
+    if (!RawReplacements)
+      return CB(RawReplacements.takeError());
+    // FIXME: this function has I/O operations (find .clang-format file), figure
+    // out a way to cache the format style.
+    auto Style = getFormatStyleForFile(File, InpAST->Inputs.Contents,
+                                       InpAST->Inputs.FS.get());
+    return CB(
+        cleanupAndFormat(InpAST->Inputs.Contents, *RawReplacements, Style));
   };
   WorkScheduler.runWithAST(
       "ApplyTweak", File,
       Bind(Action, std::move(CB), File.str(), TweakID.str()));
 }
 
 void ClangdServer::dumpAST(PathRef File,
                            llvm::unique_function<void(std::string)> Callback) {

clangd/ClangdUnit.cpp

llvm::Optional<tidy::ClangTidyContext> CTContext;
     }
   }
 
   // Add IncludeFixer which can recorver diagnostics caused by missing includes
   // (e.g. incomplete type) and attach include insertion fixes to diagnostics.
   llvm::Optional<IncludeFixer> FixIncludes;
   auto BuildDir = VFS->getCurrentWorkingDirectory();
   if (Opts.SuggestMissingIncludes && Index && !BuildDir.getError()) {
+    auto Style = getFormatStyleForFile(MainInput.getFile(), Content, VFS.get());
     auto Inserter = std::make_shared<IncludeInserter>(
-        MainInput.getFile(), Content, Opts.Style, BuildDir.get(),
+        MainInput.getFile(), Content, Style, BuildDir.get(),
         Clang->getPreprocessor().getHeaderSearchInfo());
     if (Preamble) {
       for (const auto &Inc : Preamble->Includes.MainFileIncludes)
         Inserter->addExisting(Inc);
     }
     FixIncludes.emplace(MainInput.getFile(), Inserter, *Index,
                         /*IndexRequestLimit=*/5);
     ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl,

clangd/Compiler.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 
 #include "../clang-tidy/ClangTidyOptions.h"
 #include "index/Index.h"
-#include "clang/Format/Format.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Tooling/CompilationDatabase.h"
 
 namespace clang {
 namespace clangd {
 
 class IgnoreDiagnostics : public DiagnosticConsumer {
 public:
   static void log(DiagnosticsEngine::Level DiagLevel,
                   const clang::Diagnostic &Info);
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 };
 
 // Options to run clang e.g. when parsing AST.
 struct ParseOptions {
   tidy::ClangTidyOptions ClangTidyOpts;
   bool SuggestMissingIncludes = false;
-  format::FormatStyle Style;
 };
 
 /// Information required to run clang, e.g. to parse AST or do code completion.
 struct ParseInputs {
   tooling::CompileCommand CompileCommand;
   IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
   std::string Contents;
   // Used to recover from diagnostics (e.g. find missing includes for symbol).

clangd/refactor/Tweak.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_REFACTOR_ACTIONS_TWEAK_H
 
 #include "ClangdUnit.h"
 #include "Protocol.h"
 #include "Selection.h"
-#include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 namespace clang {
 namespace clangd {
 
 /// An interface base for small context-sensitive refactoring actions.
 /// To implement a new tweak use the following pattern in a .cpp file:
public:
   struct Selection {
     Selection(ParsedAST &AST, unsigned RangeBegin, unsigned RangeEnd);
     /// The text of the active document.
     llvm::StringRef Code;
     /// Parsed AST of the active file.
     ParsedAST *
     /// A location of the cursor in the editor.
     SourceLocation Cursor;
-    /// The AST nodes that were selected.
+    // The AST nodes that were selected.
     SelectionTree ASTSelection;
     // FIXME: provide a way to get sources and ASTs for other files.
   };
   virtual ~Tweak() = default;
   /// A unique id of the action, it is always equal to the name of the class
   /// defining the Tweak. Definition is provided automatically by
   /// REGISTER_TWEAK.
   virtual const char *id() const = 0;
   /// Run the first stage of the action. Returns true indicating that the
   /// action is available and should be shown to the user. Returns false if the
   /// action is not available.
   /// This function should be fast, if the action requires non-trivial work it
   /// should be moved into 'apply'.
   /// Returns true iff the action is available and apply() can be called on it.
   virtual bool prepare(const Selection &Sel) = 0;
-  /// Format and apply the actual changes generated from the second stage of the
-  /// action.
+  /// Run the second stage of the action that would produce the actual changes.
   /// EXPECTS: prepare() was called and returned true.
-  Expected<tooling::Replacements> apply(const Selection &Sel,
-                                        const format::FormatStyle &Style);
+  virtual Expected<tooling::Replacements> apply(const Selection &Sel) = 0;
   /// A one-line title of the action that should be shown to the users in the
   /// UI.
   /// EXPECTS: prepare() was called and returned true.
   virtual std::string title() const = 0;
-
-protected:
-  /// Run the second stage of the action that would produce the actual changes.
-  /// EXPECTS: prepare() was called and returned true.
-  virtual Expected<tooling::Replacements> execute(const Selection &Sel) = 0;
 };
 
 // All tweaks must be registered in the .cpp file next to their definition.
 #define REGISTER_TWEAK(Subclass)                                               \
   ::llvm::Registry<::clang::clangd::Tweak>::Add<Subclass>                      \
       TweakRegistrationFor##Subclass(#Subclass, /*Description=*/"");           \
   const char *Subclass::id() const { return #Subclass; }
 

clangd/refactor/Tweak.cpp

 Tweak::Selection::Selection(ParsedAST &AST, unsigned RangeBegin,
                             unsigned RangeEnd)
     : AST(AST), ASTSelection(AST.getASTContext(), RangeBegin, RangeEnd) {
   auto &SM = AST.getASTContext().getSourceManager();
   Code = SM.getBufferData(SM.getMainFileID());
   Cursor = SM.getComposedLoc(SM.getMainFileID(), RangeBegin);
 }
 
-Expected<tooling::Replacements> Tweak::apply(const Selection &Sel,
-                                             const format::FormatStyle &Style) {
-  auto RawReplacements = execute(Sel);
-  if (!RawReplacements)
-    return RawReplacements;
-  return cleanupAndFormat(Sel.Code, *RawReplacements, Style);
-}
-
 std::vector<std::unique_ptr<Tweak>> prepareTweaks(const Tweak::Selection &S) {
   validateRegistry();
 
   std::vector<std::unique_ptr<Tweak>> Available;
   for (const auto &E : TweakRegistry::entries()) {
     std::unique_ptr<Tweak> T = E.instantiate();
     if (!T->prepare(S))
       continue;

clangd/refactor/tweaks/SwapIfBranches.cpp

 ///   ^^^^^^^                 ^^^^
 /// After:
 ///   if (foo) { continue; } else { return 10; }
 class SwapIfBranches : public Tweak {
 public:
   const char *id() const override final;
 
   bool prepare(const Selection &Inputs) override;
+  Expected<tooling::Replacements> apply(const Selection &Inputs) override;
   std::string title() const override;
 
-protected:
-  Expected<tooling::Replacements> execute(const Selection &Inputs) override;
-
 private:
   const IfStmt *If = nullptr;
 };
 
 REGISTER_TWEAK(SwapIfBranches)
 
 bool SwapIfBranches::prepare(const Selection &Inputs) {
   for (const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
        N && !If; N = N->Parent) {
     // Stop once we hit a block, e.g. a lambda in the if condition.
     if (dyn_cast_or_null<CompoundStmt>(N->ASTNode.get<Stmt>()))
       return false;
     If = dyn_cast_or_null<IfStmt>(N->ASTNode.get<Stmt>());
   }
   // avoid dealing with single-statement brances, they require careful handling
   // to avoid changing semantics of the code (i.e. dangling else).
   return If && dyn_cast_or_null<CompoundStmt>(If->getThen()) &&
          dyn_cast_or_null<CompoundStmt>(If->getElse());
 }
 
-Expected<tooling::Replacements>
-SwapIfBranches::execute(const Selection &Inputs) {
+Expected<tooling::Replacements> SwapIfBranches::apply(const Selection &Inputs) {
   auto &Ctx = Inputs.AST.getASTContext();
   auto &SrcMgr = Ctx.getSourceManager();
 
   auto ThenRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
                                      If->getThen()->getSourceRange());
   if (!ThenRng)
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),

test/clangd/tweaks-format.test

+# RUN: clangd -lit-test < %s | FileCheck %s
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+---
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cc","languageId":"cpp","version":1,"text":"int f() { if (true) { return 1; } else {} }"}}}
+---
+{"jsonrpc":"2.0","id":5,"method":"textDocument/codeAction","params":{"textDocument":{"uri":"test:///main.cc"},"range":{"start":{"line":0,"character":11},"end":{"line":0,"character":11}},"context":{"diagnostics":[]}}}
+---
+{"jsonrpc":"2.0","id":6,"method":"workspace/executeCommand","params":{"command":"clangd.applyTweak","arguments":[{"file":"test:///main.cc","selection":{"end":{"character":11,"line":0},"start":{"character":11,"line":0}},"tweakID":"SwapIfBranches"}]}}
+#      CHECK:    "newText": "\n  ",
+# CHECK-NEXT:    "range": {
+# CHECK-NEXT:      "end": {
+# CHECK-NEXT:        "character": 10,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "start": {
+# CHECK-NEXT:        "character": 9,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      }
+# CHECK-NEXT:    }
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:    "newText": "{\n  }",
+# CHECK-NEXT:    "range": {
+# CHECK-NEXT:      "end": {
+# CHECK-NEXT:        "character": 33,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "start": {
+# CHECK-NEXT:        "character": 20,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      }
+# CHECK-NEXT:    }
+# CHECK-NEXT:  },
+# CHECK-NEXT:  {
+# CHECK-NEXT:    "newText": "{\n    return 1;\n  }\n",
+# CHECK-NEXT:    "range": {
+# CHECK-NEXT:      "end": {
+# CHECK-NEXT:        "character": 42,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      },
+# CHECK-NEXT:      "start": {
+# CHECK-NEXT:        "character": 39,
+# CHECK-NEXT:        "line": 0
+# CHECK-NEXT:      }
+# CHECK-NEXT:    }
+# CHECK-NEXT:  }
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}

unittests/clangd/TweakTests.cpp

llvm::Expected<std::string> apply(StringRef ID, llvm::StringRef Input) {
   ParsedAST AST = TU.build();
   unsigned Begin = cantFail(positionToOffset(Code.code(), SelectionRng.start));
   unsigned End = cantFail(positionToOffset(Code.code(), SelectionRng.end));
   Tweak::Selection S(AST, Begin, End);
 
   auto T = prepareTweak(ID, S);
   if (!T)
     return T.takeError();
-  auto Replacements = (*T)->apply(S, clang::format::getLLVMStyle());
+  auto Replacements = (*T)->apply(S);
   if (!Replacements)
     return Replacements.takeError();
   return applyAllReplacements(Code.code(), *Replacements);
 }
 
 void checkTransform(llvm::StringRef ID, llvm::StringRef Input,
                     llvm::StringRef Output) {
   EXPECT_THAT_EXPECTED(apply(ID, Input), HasValue(Output))
TEST(TweakTest, SwapIfBranches) {
   checkNotAvailable(ID, R"cpp(
     void test() {
       if (true) {^return ^100;^ } else { ^continue^;^ }
     }
   )cpp");
 
   llvm::StringLiteral Input = R"cpp(
     void test() {
-      ^if (true) {
-        return 100;
-      } else {
-        continue;
-      }
+      ^if (true) { return 100; } else { continue; }
     }
   )cpp";
   llvm::StringLiteral Output = R"cpp(
     void test() {
-      if (true) {
-        continue;
-      } else {
-        return 100;
-      }
-    }
-  )cpp";
-  checkTransform(ID, Input, Output);
-
-  Input = R"cpp(
-    void test() {
-      ^if () {
-        return 100;
-      } else {
-        continue;
-      }
-    }
-  )cpp";
-  Output = R"cpp(
-    void test() {
-      if () {
-        continue;
-      } else {
-        return 100;
-      }
+      if (true) { continue; } else { return 100; }
     }
   )cpp";
   checkTransform(ID, Input, Output);
 
   Input = R"cpp(
     void test() {
       ^if () { return 100; } else { continue; }
     }
   )cpp";
   Output = R"cpp(
     void test() {
-      if () {
-        continue;
-      } else {
-        return 100;
-      }
+      if () { continue; } else { return 100; }
     }
   )cpp";
   checkTransform(ID, Input, Output);
 
   // Available in subexpressions of the condition.
   checkAvailable(ID, R"cpp(
     void test() {
       if(2 + [[2]] + 2) { return 2 + 2 + 2; } else { continue; }