[clangd] Format tweak's replacements.

[hokein]

clangd/ClangdServer.cpp

369

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

369

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

366

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.

497

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

267 ↗

(On Diff #185261)

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

clangd/Format.h

29 ↗

(On Diff #185261)

NIT: add newline at the end of file.

unittests/clangd/TweakTests.cpp

80

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

107

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

[ilya-biryukov]

clangd/ClangdServer.cpp

369

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

369

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

267 ↗

(On Diff #185261)

removed, not needed any more.

unittests/clangd/TweakTests.cpp

80

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.

107

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

80

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

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

72

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

[ilya-biryukov]

clangd/refactor/Tweak.h

80

execute() LGTM!

[hokein]

Address review comments.

[hokein]

Add missing file.

[hokein]

clangd/refactor/Tweak.h

56

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

[ilya-biryukov]

LGTM! Also a few NITs.

clangd/ClangdServer.cpp

155

NIT: put . to the end of the comment.

377–378

This FIXME is stale now, remove?

clangd/Format.h

19 ↗

(On Diff #185531)

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.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();
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,
                       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));
+    return CB((*A)->apply(*Selection, InpAST->Inputs.Opts.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/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, Style, BuildDir.get(),
+        MainInput.getFile(), Content, Opts.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/SourceCode.h

llvm::Optional<std::string> getCanonicalPath(const FileEntry *F,
                                              const SourceManager &SourceMgr);
 
 bool isRangeConsecutive(const Range &Left, const Range &Right);
 
 format::FormatStyle getFormatStyleForFile(llvm::StringRef File,
                                           llvm::StringRef Content,
                                           llvm::vfs::FileSystem *FS);
 
+// Cleanup and format the given replacements.
+llvm::Expected<tooling::Replacements>
+cleanupAndFormat(StringRef Code, const tooling::Replacements &Replaces,
+                 const format::FormatStyle &Style);
+
 } // namespace clangd
 } // namespace clang
 #endif

clangd/SourceCode.cpp

format::FormatStyle getFormatStyleForFile(llvm::StringRef File,
   if (!Style) {
     log("getStyle() failed for file {0}: {1}. Fallback is LLVM style.", File,
         Style.takeError());
     Style = format::getLLVMStyle();
   }
   return *Style;
 }
 
+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

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;
-  /// Run the second stage of the action that would produce the actual changes.
+  /// Format and apply the actual changes generated from the second stage of the
+  /// action.
   /// EXPECTS: prepare() was called and returned true.
-  virtual Expected<tooling::Replacements> apply(const Selection &Sel) = 0;
+  Expected<tooling::Replacements> apply(const Selection &Sel,
+                                        const format::FormatStyle &Style);
   /// 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::apply(const Selection &Inputs) {
+Expected<tooling::Replacements>
+SwapIfBranches::execute(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(),

unittests/clangd/TweakTests.cpp

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) {
   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);
+  auto Replacements = (*T)->apply(S, 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))
       << "action id is" << ID;
 }
 
 TEST(TweakTest, SwapIfBranches) {
   llvm::StringLiteral ID = "SwapIfBranches";
 
   checkAvailable(ID, R"cpp(
     void test() {
       ^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }
     }
   )cpp");
 
   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; }
+      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;
+      }
     }
   )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; }