[clangd] Format tweak's replacements.

[hokein]

clangd/ClangdServer.cpp

367

not sure the err-handling strategy here -- maybe if this is failed, we still apply replacements (without formatting), rather than stopping.

[ioeric]

clangd/ClangdServer.cpp

367

You should use getFormatStyleForFile from SourceCode.h

[ilya-biryukov]

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

clangd/ClangdServer.cpp

364

We can get the contents from InpAST.Inputs.Contents, no need to add an extra parameter.
Moving getFormatStyle() into the callback body should be ok.

504

Could this function get a vfs::FileSystem as a parameter?
Would server as a hint to the readers that it accesses the filesystem and allow to reuse vfs instances when they're already available (the apply tweaks case)

clangd/ClangdServer.h

269

NIT: could we swap the parameters for consistency with the other functions in this class (i.e. File goes first)

clangd/Format.h

29

NIT: add newline at the end of file.

unittests/clangd/TweakTests.cpp

82

NIT: remove the parameter, this should be the default.
At least until we run into the checks that deliberately don't want the formatting

110

NIT: maybe prefer LLVM style? To avoid context-switching.

[ilya-biryukov]

clangd/ClangdServer.cpp

367

not sure the err-handling strategy here -- maybe if this is failed, we still apply replacements (without formatting), rather than stopping.

Returning an error seems fine, this probably shouldn't happen under normal conditions and failing early means we're more likely to find the root-cause of the problem.

[hokein]

Adress review comments.

[hokein]

In D57739#1384844, @ilya-biryukov wrote:

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

clangd/ClangdServer.cpp

367

You should use getFormatStyleForFile from SourceCode.h

Thanks for pointing it out! Didn't notice this method. I think we need to cache it somehow rather than calling this method every time.

Returning an error seems fine, this probably shouldn't happen under normal conditions and failing early means we're more likely to find the root-cause of the problem.

I switched to use getFormatStyleForFile, it seems a reasonable API.

clangd/ClangdServer.h

269

removed, not needed any more.

unittests/clangd/TweakTests.cpp

82

I'm ok to make this change. I tempt to keep it because it would make writing test code easier -- if we always format the code, we might spend time on figuring out the format diff between Before and After, which is not worth.

110

good catch.

[ilya-biryukov]

In D57739#1385144, @hokein wrote:

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

It's totally fine, since we have just a single tweak now and changing the interface costs nothing.
The important part is making it hard to misuse the interface and forcing formatting on all the users of the interface is a significant burden. E.g. other users of ClangdServer (tests, users of clangd C++ API) shouldn't worry about formatting on their own.

[hokein]

Move format to the tweak.

[hokein]

clangd/refactor/Tweak.h

81 ↗

(On Diff #185529)

I think the current name is slightly better than doApply, I also have a few candidates in mind perform, invoke.

[ilya-biryukov]

clangd/refactor/Tweak.h

56 ↗

(On Diff #185529)

NIT: Maybe make this a second argument of apply?
This would convey the idea that execute() should not do formatting on its own.

72 ↗

(On Diff #185529)

Could you duplicate the EXPECTS: comments here? It's an important part of the public API.

[ilya-biryukov]

clangd/refactor/Tweak.h

81 ↗

(On Diff #185529)

execute() LGTM!

[hokein]

Address review comments.

[hokein]

Add missing file.

[hokein]

clangd/refactor/Tweak.h

56 ↗

(On Diff #185529)

SG, this also makes the format intention of apply more explicitly.

[ilya-biryukov]

LGTM! Also a few NITs.

clangd/ClangdServer.cpp

156

NIT: put . to the end of the comment.

376–377

This FIXME is stale now, remove?

clangd/Format.h

20

NIT: Maybe move to SourceCode.h? We have helpers to turn replacements into edits there, having this would be a good addition.
Or inline now that this function has only one call-site?

The extra file does not seem to carry its weight.

[hokein]

Address comments.

[hokein]

Remove accident change.

[hokein]

Committed in rL353306.

[sammccall]

In D57739#1385144, @hokein wrote:

In D57739#1384844, @ilya-biryukov wrote:

Could we move the code to the Tweak itself, so that all the callers would get the formatting? I.e.

class Tweak {
   Replacements apply() {  // This is now non-virtual.
     auto Replacements = doApply();
     return cleanupAndFormat(Replacements);
   }

protected:
   virtual Replacements doApply() = 0; // inheritors should implement this now. Note: the name is terrible, suggestions are welcome.
};

This would ensure the callers don't need to deal with the formatting on their own.

This seems introduce intrusive changes to the Tweak interface (Tweak will need to know the FormatStyle somehow), I think this might be done in another patch.

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

[ilya-biryukov]

In D57739#1390252, @sammccall wrote:

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

[sammccall]

In D57739#1390315, @ilya-biryukov wrote:

In D57739#1390252, @sammccall wrote:

I agree with this concern, and don't think this is an appropriate layer to be aware of styling or failing on format errors. Can we consider moving the formatting to clangdserver as originally planned?

clang-format seems to be somewhat stable, do we actually expect that to be a large problem in practice?
On the other side, I can't imagine any clients that don't need formatting? E.g. it's nice when tests give the same results as one would see in the final clangd and tests don't go through ClangdServer.

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

[ilya-biryukov]

In D57739#1390321, @sammccall wrote:

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.
However, I also think tests should format by default, not sure we agree on this.
WDYT?

[sammccall]

In D57739#1390374, @ilya-biryukov wrote:

In D57739#1390321, @sammccall wrote:

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.

I'd be happy with applyAndFormat as a free function - my main concern is that formatting not be part of the scope of the class.

However, I also think tests should format by default, not sure we agree on this.
WDYT?

I'd rather they didn't format, but I don't think it will matter much and I'm happy either way.

(Where it does matter, I'd rather have the differences between input/output be a result of the tweak replacements, not of different formatting triggered by a different token sequence.
I don't think there's much point in testing clang-format here, though we should have one test that verifies we're calling it at all.)

[hokein]

In D57739#1392453, @sammccall wrote:

In D57739#1390374, @ilya-biryukov wrote:

In D57739#1390321, @sammccall wrote:

It's not about stability or whether the functionality is desired, but layering.
Unit tests having narrow scope is a good thing - if we want system tests that check clangdserver's behavior, they should test clangdserver.
Clients that don't go through clangdserver probably want formatting, but an immediate cleanupAndFormat on each generated change isn't necessarily the right way to do that.

My argument is that it's better to provide formatting by default in the public interface for applying a single tweak.
I might be missing some use-cases, e.g. one that comes to mind is applying multiple tweaks in a row, in which case we'd want to format once and not for every tweak.

If providing formatting was free, I'd be fine with this, but it leaks into the public interface in two places: a) it requires the caller to plumb through a formatting style, and b) it is another source of errors.

For this particular interface it's more important that it's conceptually clear and easy to implement than it is that it's easy to call - this is an extension point.

My concerns are code duplication and ease of use for the clients. Having both apply and applyAndFormat (the latter being a non-virtual or a free-standing utility function) in the public interface of the Tweak would totally do it for me.

I'd be happy with applyAndFormat as a free function - my main concern is that formatting not be part of the scope of the class.

Given that clangdServer is the only client of Tweaks (if we don't do format in tests), I think it is fine to move out applyAndFormat functionality from Tweaks. I somehow agree that applyAndFormat is a standalone function.

However, I also think tests should format by default, not sure we agree on this.
WDYT?

I'd rather they didn't format, but I don't think it will matter much and I'm happy either way.

(Where it does matter, I'd rather have the differences between input/output be a result of the tweak replacements, not of different formatting triggered by a different token sequence.
I don't think there's much point in testing clang-format here, though we should have one test that verifies we're calling it at all.)

Personally, I prefer not doing format in tests -- it would make writing testcases easier, it is annoying to spend time on figuring out format-only differences between actual code and expected code.

clangd/ClangdServer.h

 #include "GlobalCompilationDatabase.h"
 #include "Protocol.h"
 #include "TUScheduler.h"
 #include "XRefs.h"
 #include "index/Background.h"
 #include "index/FileIndex.h"
 #include "index/Index.h"
 #include "refactor/Tweak.h"
+#include "clang/Format/Format.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include <functional>
 #include <future>
private:
   /// FIXME: This stats several files to find a .clang-format file. I/O can be
   /// slow. Think of a way to cache this.
   llvm::Expected<tooling::Replacements>
   formatCode(llvm::StringRef Code, PathRef File,
              ArrayRef<tooling::Range> Ranges);
 
   tooling::CompileCommand getCompileCommand(PathRef File);
 
   const GlobalCompilationDatabase &CDB;
   const FileSystemProvider &FSProvider;
 
   Path ResourceDir;
   // The index used to look up symbols. This could be:
   //   - null (all index functionality is optional)
   //   - the dynamic index owned by ClangdServer (DynamicIdx)
   //   - the static index passed to the constructor
   //   - a merged view of a static and dynamic index (MergedIndex)

clangd/ClangdServer.cpp

 //===--- ClangdServer.cpp - Main clangd server code --------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===-------------------------------------------------------------------===//
 
 #include "ClangdServer.h"
 #include "ClangdUnit.h"
 #include "CodeComplete.h"
 #include "FindSymbols.h"
+#include "Format.h"
 #include "Headers.h"
 #include "SourceCode.h"
 #include "Trace.h"
 #include "index/CanonicalIncludes.h"
 #include "index/FileIndex.h"
 #include "index/Merge.h"
 #include "refactor/Tweak.h"
 #include "clang/Format/Format.h"

 }
 
 void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents,
                                WantDiagnostics WantDiags) {
   ParseOptions Opts;
   Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults();
   if (ClangTidyOptProvider)
     Opts.ClangTidyOpts = ClangTidyOptProvider->getOptions(File);
   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();
   Inputs.Contents = Contents;
for (auto &T : prepareTweaks(*Selection))
       Res.push_back({T->id(), T->title()});
     CB(std::move(Res));
   };
 
   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());
-    // FIXME: run formatter on top of resulting replacements.
-    return CB((*A)->apply(*Selection));
+    auto ResultReplacements = (*A)->apply(*Selection);
+    if (!ResultReplacements)
+      return CB(ResultReplacements.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,
+                                       FSProvider.getFileSystem().get());
+    return CB(
+        cleanupAndFormat(InpAST->Inputs.Contents, *ResultReplacements, 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) {
ClangdServer::formatCode(llvm::StringRef Code, PathRef File,
 
   return IncludeReplaces.merge(format::reformat(
       Style.get(), *Changed,
       tooling::calculateRangesAfterReplacements(IncludeReplaces, Ranges),
       File));
 }
 
 void ClangdServer::findDocumentHighlights(
     PathRef File, Position Pos, Callback<std::vector<DocumentHighlight>> CB) {
   auto Action = [Pos](Callback<std::vector<DocumentHighlight>> CB,
                       llvm::Expected<InputsAndAST> InpAST) {
     if (!InpAST)
       return CB(InpAST.takeError());
     CB(clangd::findDocumentHighlights(InpAST->AST, Pos));
   };
 
   WorkScheduler.runWithAST("Highlights", File, Bind(Action, std::move(CB)));

clangd/Format.h

+//===--- Format.h ------------------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+
+// Cleanup and format the given replacements.
+inline llvm::Expected<tooling::Replacements>
+cleanupAndFormat(StringRef Code, const tooling::Replacements &Replaces,
+                 const format::FormatStyle &Style) {
+  auto CleanReplaces = cleanupAroundReplacements(Code, Replaces, Style);
+  if (!CleanReplaces)
+    return CleanReplaces;
+  return formatReplacements(Code, std::move(*CleanReplaces), Style);
+}
+
+} // namespace clangd
+} // namespace clang

unittests/clangd/TweakTests.cpp

 //===-- TweakTests.cpp ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Annotations.h"
 #include "SourceCode.h"
+#include "Format.h"
 #include "TestTU.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/Expr.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Testing/Support/Error.h"

 void checkAvailable(StringRef ID, llvm::StringRef Input) {
   return checkAvailable(ID, Input, /*Available=*/true);
 }
 
 /// Same as checkAvailable, but checks the action is not available.
 void checkNotAvailable(StringRef ID, llvm::StringRef Input) {
   return checkAvailable(ID, Input, /*Available=*/false);
 }
-llvm::Expected<std::string> apply(StringRef ID, llvm::StringRef Input) {
+llvm::Expected<std::string> apply(StringRef ID, llvm::StringRef Input,
+                                  bool Format) {
   Annotations Code(Input);
   Range SelectionRng;
   if (Code.points().size() != 0) {
     assert(Code.ranges().size() == 0 &&
            "both a cursor point and a selection range were specified");
     SelectionRng = Range{Code.point(), Code.point()};
   } else {
     SelectionRng = Code.range();
   }
   TestTU TU;
   TU.Filename = "foo.cpp";
   TU.Code = Code.code();
 
   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);
   if (!Replacements)
     return Replacements.takeError();
+  if (Format) {
+    Replacements = cleanupAndFormat(
+        Code.code(), *Replacements,
+        clang::format::getLLVMStyle());
+    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))
+                    llvm::StringRef Output, bool Format = false) {
+  EXPECT_THAT_EXPECTED(apply(ID, Input, Format), HasValue(Output))
       << "action id is" << ID;
 }
 
 TEST(TweakTest, SwapIfBranches) {
   llvm::StringLiteral ID = "SwapIfBranches";
 
   checkAvailable(ID, R"cpp(
     void test() {
TEST(TweakTest, SwapIfBranches) {
   )cpp";
   Output = R"cpp(
     void test() {
       if () { 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;
+      }
+    }
+  )cpp";
+  checkTransform(ID, Input, Output, /*Format=*/true);
+
   // Available in subexpressions of the condition.
   checkAvailable(ID, R"cpp(
     void test() {
       if(2 + [[2]] + 2) { return 2 + 2 + 2; } else { continue; }
     }
   )cpp");
   // But not as part of the branches.
   checkNotAvailable(ID, R"cpp(