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

Summary: This patch contains two parts: 1) reverts commit r353306. 2) move the format logic out from tweaks, keep tweaks API unchanged. Reviewers: sammccall, ilya-biryukov Reviewed By: ilya-biryukov Subscribers: ilya-biryukov, ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D58051 llvm-svn: 353712
llvm · Feb 11, 2019 · e64ee7c · e64ee7c
1 parent 9a857d2
commit e64ee7c
Show file tree

Hide file tree

Showing 8 changed files with 73 additions and 69 deletions.
diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -152,9 +152,6 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
   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
@@ -365,8 +362,9 @@ void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
 
 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,
-                      Expected<InputsAndAST> InpAST) {
+  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);
@@ -375,7 +373,15 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, StringRef TweakID,
     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,

diff --git a/clang-tools-extra/clangd/ClangdUnit.cpp b/clang-tools-extra/clangd/ClangdUnit.cpp
@@ -308,8 +308,9 @@ ParsedAST::build(std::unique_ptr<CompilerInvocation> CI,
   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)

diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h
@@ -17,7 +17,6 @@
 
 #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"
@@ -39,7 +38,6 @@ class IgnoreDiagnostics : public DiagnosticConsumer {
 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.

diff --git a/clang-tools-extra/clangd/refactor/Tweak.cpp b/clang-tools-extra/clangd/refactor/Tweak.cpp
@@ -46,14 +46,6 @@ Tweak::Selection::Selection(ParsedAST &AST, unsigned RangeBegin,
   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();
 

diff --git a/clang-tools-extra/clangd/refactor/Tweak.h b/clang-tools-extra/clangd/refactor/Tweak.h
@@ -22,7 +22,6 @@
 #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"
@@ -48,7 +47,7 @@ class Tweak {
     ParsedAST &AST;
     /// 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.
   };
@@ -64,20 +63,13 @@ class Tweak {
   /// 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.

diff --git a/clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp b/clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp
@@ -37,11 +37,9 @@ class SwapIfBranches : public Tweak {
   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;
 };
@@ -62,8 +60,7 @@ bool SwapIfBranches::prepare(const Selection &Inputs) {
          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();
 

diff --git a/clang-tools-extra/test/clangd/tweaks-format.test b/clang-tools-extra/test/clangd/tweaks-format.test
@@ -0,0 +1,50 @@
+# 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"}
diff --git a/clang-tools-extra/unittests/clangd/TweakTests.cpp b/clang-tools-extra/unittests/clangd/TweakTests.cpp
@@ -98,7 +98,7 @@ llvm::Expected<std::string> apply(StringRef ID, llvm::StringRef Input) {
   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);
@@ -127,40 +127,12 @@ TEST(TweakTest, SwapIfBranches) {
 
   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);
@@ -172,11 +144,7 @@ TEST(TweakTest, SwapIfBranches) {
   )cpp";
   Output = R"cpp(
     void test() {
-      if () {
-        continue;
-      } else {
-        return 100;
-      }
+      if () { continue; } else { return 100; }
     }
   )cpp";
   checkTransform(ID, Input, Output);