[clangd] A code action to swap branches of an if statement

[ilya-biryukov]

A deliberately simple syntactic transformation. Missing tests, but should work very reliably. To serve as an reference point for writing similar actions.

[jkorous]

Hi Ilya, this seems really useful for people learning how to implement their custom actions!

clangd/refactor/actions/SwapIfBranches.cpp

36 ↗

(On Diff #181313)

It seems to me we don't find If token whenever
CursorLoc == location of 'f' of the If token

For example if there's "if" starting at location 1:1 I think we proceed like this (hope my pseudocode is clear):

1. If->getIfLoc() returns SourceLocation{1:1}
2. we construct SourceRange{begin = 1:1, end = 1:1}
3. toHalfOpenFileRange() returns SourceRange{1:1, 1:2}
4. the condition for SourceLocation L in halfOpenRangeContains() is 1:1 <= LOffset && LOffset < 1:2 which is only ever true for L == 1:1

Do I understand it right?

62 ↗

(On Diff #181313)

Just a typo in comment s/brances/branches/

[ilya-biryukov]

clangd/refactor/actions/SwapIfBranches.cpp

36 ↗

(On Diff #181313)

In step (3) the constructed range is intended to be SourceRange{1:1, 1:3}, i.e. it should cover both chars of the if keyword.
I haven't checked it, though, will make sure that's the case when actually testing this.

[ilya-biryukov]

• Update after changes to parent revision

[ilya-biryukov]

• Fix a typo in the id of the SwapIfBranches

[ilya-biryukov]

• Rebase after parent change

[ilya-biryukov]

• Update to reflect changes in parent revision

[ilya-biryukov]

• Remove the header file

[sammccall]

Cool! Adopting this one as the simplest tweak for "how should tweaks do X" questions.

This depends on helpers not (yet) present in this patch, so not commenting on them now.

Need unit tests for tweaks. Something like this should work:

Annotations Test(R"cpp(void foo() {
  [[if]] (1) { return; } else { continue; }
})cpp");
auto AST = TestTU(Test.code()).build();
auto Sel = Tweak::Selection::create(Test.Code(), AST, Test.range());
auto T = prepareTweak("SwapIfBranches", Sel);
ASSERT_TRUE(T) << T.takeError();
auto Edits = T.apply(Sel);
ASSERT_TRUE(Edits) << Edits.takeError();
auto After = applyAllReplacements(Test.code(), *Edits);
EXPECT_EQ(After, R"cpp(void foo() {
  if (1) { continue; } else { return; }
})cpp");

Probably want to wrap it up into a table driven test once we have a few.

clangd/refactor/tweaks/SwapIfBranches.cpp

34 ↗

(On Diff #182321)

The before/after is useful, we should probably have it for all tweaks if possible.
It'd also be useful to notate where the cursor can be to trigger the action. Partly because it forces us to consider this!

e.g. (not sure if this actually matches the logic you want, just an example)

Before:
  if (foo) { return 10; } else { continue; }
  ^^^^^^^^^^            ^^^^^^^^           ^
After:
  ...

46 ↗

(On Diff #182321)

I think prepare() should just verify:

• cursor is in the right place
• else statement exists and isn't an else if
• both branches are compound statements (for now)
• (maybe) relevant locations (if keyword, else keyword, braces) are not macro expansions

and then record the relevant source locations (or just the IfStmt*)

We may be able to get away with doing all the work in prepare(), but it's not a good model for future tweaks. (And it is at least somewhat wasteful on a hot path).

50 ↗

(On Diff #182321)

(Mostly this is food for thought for later - we shouldn't try to solve everything in this patch)

Two efficiency problems here:

• doing one traversal per tweak is wasteful (when we have >1 tweaks, but worth at least *thinking* about)
• traversing the entire AST rather than just the nodes over the cursor is bad news (though "over the cursor" may not be trivial to define)

Idea for how to "framework" this problem away:
Add vector<DynTypedNode> SelectedNodes to the inputs, the idea being that this is the stack from the narrowest Expr under the cursor to the TranslationUnitDecl at the top.
This can be produced by a RecursiveASTVisitor (that either traverses the whole AST, or just the bits under the cursor if that's simple enough).
Tweaks would iterate over the nodes from narrowest to broadest, deciding whether to select this node for processing, continue iterating, or bail out.

Matching in checks are then pretty easy to write, we haven't removed too much flexibility in flow control, and it's pretty hard to write a slow check.

There are some complications:

• we need access to parents somehow (e.g. consider the case of adding NS qualifiers, we want to match on names but then traverse up to the containing declrefexpr to get the nested namespace loc)
• the interesting nodes may be a tree rather than a stack, because nodes overlap in the source. We could store a postorder traversal... but this makes e.g. "bail out when you hit a functiondecl" harder - you probably want to bail out of the current branch only.
• things get complicated when we think about macros - depending on the semantics we want, it may be hard for the framework to prune parts of the tree that aren't under the cursor withouth traversing the whole AST.

84 ↗

(On Diff #182321)

isa<CompoundStmt>

[ilya-biryukov]

• Move source code helpers to this patch

[ilya-biryukov]

• Add tests.

[ilya-biryukov]

• Update id of swap branches in tests
• Rebase

[ilya-biryukov]

• Use helper to avoid creating RewriteBuffer
• Use std::string to fix stack corruption

[ilya-biryukov]

• Use a helper to avoid creating RewriteBuffer

[ilya-biryukov]

• Fix a typo in a comment: isValidRange -> isValidFileRange
• Make action available under 'else' keywords and conditions
• Move the logic of creating replacements to apply
• Use llvm::isa instead of dyn_cast_or_null

[ilya-biryukov]

clangd/refactor/tweaks/SwapIfBranches.cpp

34 ↗

(On Diff #182321)

LG, updated the comment and ranges per suggestion.
I still not sure if the action should span the whole condition, but going with the suggested ranges anyway. We can tweak them later if the resulting ranges turn out to be problematic.

However, note that I excluded { and } from available ranges, but only because I'm lazy and this requires declaring some more local vars, etc.

46 ↗

(On Diff #182321)

Done.
The only reason I thought applying replacements was a sensible idea is because it would fail in some weird macro cases (only when resulting ranges overlap, though). And it seemed better to avoid showing the action rather than showing it and failing later.

50 ↗

(On Diff #182321)

I mostly agree, however I still wonder whether that would be inefficient in some cases. E.g. consider a corner if the input selection from LSP contains the whole file? I see two options there: (1) putting all nodes of a file into vector<DynTypedNode>, (2) putting only the top-level TranslationUnitDecl there.

It seems easy to end up with (1) to make the interface useful, however we probably prefer (2) because otherwise we're back into the worst-case scenario, i.e. "every tweak traverses all the nodes"

we need access to parents somehow (e.g. consider the case of adding NS qualifiers, we want to match on names but then traverse up to the containing declrefexpr to get the nested namespace loc)

I wonder if it's possible to instead write the checks so that they only look at the children of the nodes in a vector? My bet is that almost all of the checks only need to look one or two levels down, so this shouldn't turn into inefficiency.

I initially thought about providing a very limited set of AST nodes as inputs for the actions that are computed efficiently. Specifically, I thought about having:

struct Selection {
  Decl *DeclUnderCursor; // the innermost declaration under CursorLoc.
  Stmt *StmtUnderCursor; // the innermost statement under CursorLoc.
  Expr *ExprUnderCursor; // the innermost expression under CursorLoc.

  // For some tweaks (extract var, extract method) we will also need these:
  Expr *SelectedExpression; // expression exactly matching selection.
  vector<Stmt*> SelectedStatements; // a list of statements exactly matching 
selection.
};

The checks would only look at those few statements, e.g. "Swap If" would try dyn_cast_or_null<Stmt*>(S.StmtUnderCursor) and do the same checks for cursor locations.

We could expand the set of inputs as needed. This would mean that we don't have to design the framework, but that also mean writing some checks would involve some work in the shared code that computes these inputs.

I bet that complexity-wise we would end up with simpler checks and more complicated shared code to compute the inputs. OTOH, we won't need to struggle with the limitations of the AST to design the framework that would properly handle all interesting cases without doing extra work.

84 ↗

(On Diff #182321)

Done.
I used dyn_cast_or_null, because isa expectes the arguments to be non-null, therefore we need an extra check before calling it. So the former looked better to me, but I'm actually ok either way.

[ilya-biryukov]

• Update the license header

[sammccall]

clang-tools-extra/clangd/SourceCode.h

65

I think the semantics of the locations (expansion vs spelling) need to be spelled out here, at least in the comment.

clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp

61

what's the case where the condition is null?

64

the explicit checks for equality at the endpoint seem... odd
do you need a halfOpenRangeTouches() or so?

102

do this in apply()?

This is an example refactoring, if we have to add a bunch of checks because it fires too often then we should try to solve that systematically.

clang-tools-extra/unittests/clangd/TweakTests.cpp

47

fixture has no state, can these be free functions?

117

why StringLiteral here?

141

checkTransform(ID, Input, Output), to avoid mixing styles?

clangd/refactor/tweaks/SwapIfBranches.cpp

50 ↗

(On Diff #182321)

It seems easy to end up with (1) to make the interface useful, however we probably prefer (2) because otherwise we're back into the worst-case scenario, i.e. "every tweak traverses all the nodes"

I think the principled version of 2) is something like: for every node, find its own range, and its expanded range until the next non-comment token. e.g.:

                 #######################                
foo(); /* bar */ if (x) { y } else { z } /*baz*/ abort()
      ###########################################

The directly interesting nodes are those whose range intersects with the selection, and whose expanded range contains the selection, and none of whose children contains the selection.

Or something like that. I think you'd be able to invert the fuzziness here (do range-expansion on the selection endpoints instead of the nodes) to make the checks cheap.

[ilya-biryukov]

• Remove Dummy.cpp
• Add halfOpenRangeTouches
• Add a comment about file vs expansion locations
• Move range manipulations with else and then to apply()
• Remove test fixture, turn test member functions into free functions
• Add checkTransform
• Replace a null check for getCond() with an isValid() check for the corresponding location.

[ilya-biryukov]

clang-tools-extra/clangd/SourceCode.h

65

Added a small comment, but it could probably be improved. Let me know what's missing.

clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp

61

I initially thought of invalid parse trees (i.e. if() {}), but it turns out this never happens.

From experience, it's better to assume everything can be null in the AST. This saves a lot of time chasing rare null dereferences later and a lot of time investigating whether something is null in a particular case we're facing.

In our particular case IfStmt seems to always have a condition, but the condition will have an invalid location when it's empty. I've added a test that the action still works (I see no reason why it shouldn't if we assume the parser recovery is good). Happy to discuss and adjust, though, let me know what you think.

64

Done. Thanks for the suggestion, this does make the code much clearer.

102

Done.

This goes back to a similar comment about precomputing the replacements... Doing this in advance means we're not showing the check in some cases where it would later fail because of macros.

However, macros support is poor in the current state of the checks anyway, we'll need to invest some more time to make it better.

clang-tools-extra/unittests/clangd/TweakTests.cpp

117

Seems like a good default for a constant string, i.e. it can compute the size at compile time.

[nridge]

This commit is causing clangd to fail to link, with errors like this:

tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o:SwapIfBranches.cpp:function clang::RecursiveASTVisitor<clang::clangd::(anonymous namespace)::FindIfUnderCursor>::TraverseParmVarDecl(clang::ParmVarDecl*): error: undefined reference to 'clang::ParmVarDecl::hasDefaultArg() const'

(and dozens more that are similar).

[sammccall]

In D56611#1379785, @nridge wrote:

This commit is causing clangd to fail to link, with errors like this:

tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o:SwapIfBranches.cpp:function clang::RecursiveASTVisitor<clang::clangd::(anonymous namespace)::FindIfUnderCursor>::TraverseParmVarDecl(clang::ParmVarDecl*): error: undefined reference to 'clang::ParmVarDecl::hasDefaultArg() const'

(and dozens more that are similar).

Hi Nathan,
What platform is this on? Not seeing it on the buildbots.
Anything unusual in build setup (standalone build, building with shared libraries, etc)?

[nridge]

In D56611#1379876, @sammccall wrote:

Hi Nathan,
What platform is this on? Not seeing it on the buildbots.
Anything unusual in build setup (standalone build, building with shared libraries, etc)?

I'm on Linux, building with shared libraries. Not sure what standalone means in this context, I'm building the monorepo with -DLLVM_ENABLE_PROJECTS="clang".

[nridge]

The complete command that's failing is:

/usr/bin/clang++-8  -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O0 -g3  -fuse-ld=gold -Wl,-allow-shlib-undefined tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/SwapIfBranches.cpp.o tools/clang/tools/extra/clangd/tool/CMakeFiles/clangd.dir/ClangdMain.cpp.o  -o bin/clangd  -Wl,-rpath,"\$ORIGIN/../lib" lib/libLLVMSupport.so.9svn -lpthread lib/libclangBasic.so.9svn lib/libclangTidy.so.9svn lib/libclangDaemon.so.9svn lib/libclangFormat.so.9svn lib/libclangFrontend.so.9svn lib/libclangSema.so.9svn lib/libclangTooling.so.9svn lib/libclangToolingCore.so.9svn

[nridge]

Fix here: https://reviews.llvm.org/D57560

clang-tools-extra/clangd/SourceCode.h

 //===--- SourceCode.h - Manipulating source code as strings -----*- 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
 //
 //===----------------------------------------------------------------------===//
 //
 // Various code that examines C++ source code without using heavy AST machinery
 // (and often not even the lexer). To be used sparingly!
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SOURCECODE_H
 #include "Protocol.h"
 #include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SHA1.h"
 
 namespace clang {

 /// FIXME: This should return an error if the location is invalid.
 Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
 
 /// Return the file location, corresponding to \p P. Note that one should take
 /// care to avoid comparing the result with expansion locations.
 llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM,
                                                         Position P);
 
+/// Turns a token range into a half-open range and checks its correctness.
+/// The resulting range will have only valid source location on both sides, both
+/// of which are file locations.
+///
+/// File locations always point to a particular offset in a file, i.e. they
+/// never refer to a location inside a macro expansion. Turning locations from
+/// macro expansions into file locations is ambiguous, this function calls
+/// SourceManager::getFileLoc on both ends of \p R.
+///
+/// User input (e.g. cursor position) is expressed as a file location, so this
+/// function can be viewed as a way to normalize the ranges used in the clang
+/// AST so that they are comparable with ranges coming from the user input.
+llvm::Optional<SourceRange> toHalfOpenFileRange(const SourceManager &Mgr,
+                                                const LangOptions &LangOpts,
+                                                SourceRange R);
+
+/// Returns true iff all of the following conditions hold:
+///   - start and end locations are valid,
+///   - start and end locations are file locations from the same file
+///     (i.e. expansion locations are not taken into account).
+///   - start offset <= end offset.
+/// FIXME: introduce a type for source range with this invariant.
+bool isValidFileRange(const SourceManager &Mgr, SourceRange R);
+
+/// Returns true iff \p L is contained in \p R.
+/// EXPECTS: isValidFileRange(R) == true, L is a file location.
+bool halfOpenRangeContains(const SourceManager &Mgr, SourceRange R,
+                           SourceLocation L);
+
+/// Returns true iff \p L is contained in \p R or \p L is equal to the end point
+/// of \p R.
+/// EXPECTS: isValidFileRange(R) == true, L is a file location.
+bool halfOpenRangeTouches(const SourceManager &Mgr, SourceRange R,
+                          SourceLocation L);
+
+/// Returns the source code covered by the source range.
+/// EXPECTS: isValidFileRange(R) == true.
+llvm::StringRef toSourceCode(const SourceManager &SM, SourceRange R);
+
 // Converts a half-open clang source range to an LSP range.
 // Note that clang also uses closed source ranges, which this can't handle!
 Range halfOpenToRange(const SourceManager &SM, CharSourceRange R);
 
 // Converts an offset to a clang line/column (1-based, columns are bytes).
 // The offset must be in range [0, Code.size()].
 // Prefer to use SourceManager if one is available.
 std::pair<size_t, size_t> offsetToClangLineColumn(llvm::StringRef Code,

clang-tools-extra/clangd/SourceCode.cpp

 //===--- SourceCode.h - Manipulating source code as strings -----*- 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 "SourceCode.h"
 
 #include "Logger.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 
 // Here be dragons. LSP positions use columns measured in *UTF-16 code units*!
Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc) {
   if (!Invalid) {
     auto ColumnInBytes = SM.getColumnNumber(FID, Offset) - 1;
     auto LineSoFar = Code.substr(Offset - ColumnInBytes, ColumnInBytes);
     P.character = lspLength(LineSoFar);
   }
   return P;
 }
 
+bool isValidFileRange(const SourceManager &Mgr, SourceRange R) {
+  if (!R.getBegin().isValid() || !R.getEnd().isValid())
+    return false;
+
+  FileID BeginFID;
+  size_t BeginOffset = 0;
+  std::tie(BeginFID, BeginOffset) = Mgr.getDecomposedLoc(R.getBegin());
+
+  FileID EndFID;
+  size_t EndOffset = 0;
+  std::tie(EndFID, EndOffset) = Mgr.getDecomposedLoc(R.getEnd());
+
+  return BeginFID.isValid() && BeginFID == EndFID && BeginOffset <= EndOffset;
+}
+
+bool halfOpenRangeContains(const SourceManager &Mgr, SourceRange R,
+                           SourceLocation L) {
+  assert(isValidFileRange(Mgr, R));
+
+  FileID BeginFID;
+  size_t BeginOffset = 0;
+  std::tie(BeginFID, BeginOffset) = Mgr.getDecomposedLoc(R.getBegin());
+  size_t EndOffset = Mgr.getFileOffset(R.getEnd());
+
+  FileID LFid;
+  size_t LOffset;
+  std::tie(LFid, LOffset) = Mgr.getDecomposedLoc(L);
+  return BeginFID == LFid && BeginOffset <= LOffset && LOffset < EndOffset;
+}
+
+bool halfOpenRangeTouches(const SourceManager &Mgr, SourceRange R,
+                          SourceLocation L) {
+  return L == R.getEnd() || halfOpenRangeContains(Mgr, R, L);
+}
+
+llvm::Optional<SourceRange> toHalfOpenFileRange(const SourceManager &Mgr,
+                                                const LangOptions &LangOpts,
+                                                SourceRange R) {
+  auto Begin = Mgr.getFileLoc(R.getBegin());
+  if (Begin.isInvalid())
+    return llvm::None;
+  auto End = Mgr.getFileLoc(R.getEnd());
+  if (End.isInvalid())
+    return llvm::None;
+  End = Lexer::getLocForEndOfToken(End, 0, Mgr, LangOpts);
+
+  SourceRange Result(Begin, End);
+  if (!isValidFileRange(Mgr, Result))
+    return llvm::None;
+  return Result;
+}
+
+llvm::StringRef toSourceCode(const SourceManager &SM, SourceRange R) {
+  assert(isValidFileRange(SM, R));
+  bool Invalid = false;
+  auto *Buf = SM.getBuffer(SM.getFileID(R.getBegin()), &Invalid);
+  assert(!Invalid);
+
+  size_t BeginOffset = SM.getFileOffset(R.getBegin());
+  size_t EndOffset = SM.getFileOffset(R.getEnd());
+  return Buf->getBuffer().substr(BeginOffset, EndOffset - BeginOffset);
+}
+
 llvm::Expected<SourceLocation> sourceLocationInMainFile(const SourceManager &SM,
                                                         Position P) {
   llvm::StringRef Code = SM.getBuffer(SM.getMainFileID())->getBuffer();
   auto Offset =
       positionToOffset(Code, P, /*AllowColumnBeyondLineLength=*/false);
   if (!Offset)
     return Offset.takeError();
   return SM.getLocForStartOfFile(SM.getMainFileID()).getLocWithOffset(*Offset);
std::pair<size_t, size_t> offsetToClangLineColumn(llvm::StringRef Code,
   Offset = std::min(Code.size(), Offset);
   llvm::StringRef Before = Code.substr(0, Offset);
   int Lines = Before.count('\n');
   size_t PrevNL = Before.rfind('\n');
   size_t StartOfLine = (PrevNL == llvm::StringRef::npos) ? 0 : (PrevNL + 1);
   return {Lines + 1, Offset - StartOfLine + 1};
 }
 
-std::pair<llvm::StringRef, llvm::StringRef>
-splitQualifiedName(llvm::StringRef QName) {
+std::pair<StringRef, StringRef> splitQualifiedName(StringRef QName) {
   size_t Pos = QName.rfind("::");
   if (Pos == llvm::StringRef::npos)
     return {llvm::StringRef(), QName};
   return {QName.substr(0, Pos + 2), QName.substr(Pos + 2)};
 }
 
 TextEdit replacementToEdit(llvm::StringRef Code,
                            const tooling::Replacement &R) {

clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt

 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
 
 # A target containing all code tweaks (i.e. mini-refactorings) provided by
 # clangd.
 # Built as an object library to make sure linker does not remove global
 # constructors that register individual tweaks in a global registry.
 # To enable these tweaks in exectubales or shared libraries, add
 # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
-  Dummy.cpp # FIXME: to avoid CMake errors due to empty inputs, remove when a
-            #        first tweak lands.
+  SwapIfBranches.cpp
   )

clang-tools-extra/clangd/refactor/tweaks/Dummy.cpp

-//===--- Dummy.cpp -----------------------------------------------*- C++-*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-// Does nothing, only here to avoid cmake errors for empty libraries.
 

clang-tools-extra/clangd/refactor/tweaks/SwapIfBranches.cpp

+//===--- SwapIfBranches.cpp --------------------------------------*- 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 "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/None.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+/// Swaps the 'then' and 'else' branch of the if statement.
+/// Before:
+///   if (foo) { return 10; } else { continue; }
+///   ^^^^^^^                 ^^^^
+/// After:
+///   if (foo) { continue; } else { return 10; }
+class SwapIfBranches : public Tweak {
+public:
+  TweakID id() const override final;
+
+  bool prepare(const Selection &Inputs) override;
+  Expected<tooling::Replacements> apply(const Selection &Inputs) override;
+  std::string title() const override;
+
+private:
+  IfStmt *If = nullptr;
+};
+
+REGISTER_TWEAK(SwapIfBranches);
+
+class FindIfUnderCursor : public RecursiveASTVisitor<FindIfUnderCursor> {
+public:
+  FindIfUnderCursor(ASTContext &Ctx, SourceLocation CursorLoc, IfStmt *&Result)
+      : Ctx(Ctx), CursorLoc(CursorLoc), Result(Result) {}
+
+  bool VisitIfStmt(IfStmt *If) {
+    // Check if the cursor is in the range of 'if (cond)'.
+    // FIXME: this does not contain the closing paren, add it too.
+    auto R = toHalfOpenFileRange(
+        Ctx.getSourceManager(), Ctx.getLangOpts(),
+        SourceRange(If->getIfLoc(), If->getCond()->getEndLoc().isValid()
+                                        ? If->getCond()->getEndLoc()
+                                        : If->getIfLoc()));
+    if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) {
+      Result = If;
+      return false;
+    }
+    // Check the range of 'else'.
+    R = toHalfOpenFileRange(Ctx.getSourceManager(), Ctx.getLangOpts(),
+                            SourceRange(If->getElseLoc()));
+    if (R && halfOpenRangeTouches(Ctx.getSourceManager(), *R, CursorLoc)) {
+      Result = If;
+      return false;
+    }
+
+    return true;
+  }
+
+private:
+  ASTContext &Ctx;
+  SourceLocation CursorLoc;
+  IfStmt *&Result;
+};
+} // namespace
+
+bool SwapIfBranches::prepare(const Selection &Inputs) {
+  auto &Ctx = Inputs.AST.getASTContext();
+  FindIfUnderCursor(Ctx, Inputs.Cursor, If).TraverseAST(Ctx);
+  if (!If)
+    return false;
+
+  // avoid dealing with single-statement brances, they require careful handling
+  // to avoid changing semantics of the code (i.e. dangling else).
+  if (!If->getThen() || !llvm::isa<CompoundStmt>(If->getThen()) ||
+      !If->getElse() || !llvm::isa<CompoundStmt>(If->getElse()))
+    return false;
+  return true;
+}
+
+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(),
+        "Could not obtain range of the 'then' branch. Macros?");
+  auto ElseRng = toHalfOpenFileRange(SrcMgr, Ctx.getLangOpts(),
+                                     If->getElse()->getSourceRange());
+  if (!ElseRng)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "Could not obtain range of the 'else' branch. Macros?");
+
+  auto ThenCode = toSourceCode(SrcMgr, *ThenRng);
+  auto ElseCode = toSourceCode(SrcMgr, *ElseRng);
+
+  tooling::Replacements Result;
+  if (auto Err = Result.add(tooling::Replacement(Ctx.getSourceManager(),
+                                                 ThenRng->getBegin(),
+                                                 ThenCode.size(), ElseCode)))
+    return std::move(Err);
+  if (auto Err = Result.add(tooling::Replacement(Ctx.getSourceManager(),
+                                                 ElseRng->getBegin(),
+                                                 ElseCode.size(), ThenCode)))
+    return std::move(Err);
+  return Result;
+}
+
+std::string SwapIfBranches::title() const { return "Swap if branches"; }
+} // namespace clangd
+} // namespace clang

clang-tools-extra/unittests/clangd/CMakeLists.txt

add_extra_unittest(ClangdTests
   SymbolInfoTests.cpp
   SyncAPI.cpp
   TUSchedulerTests.cpp
   TestFS.cpp
   TestIndex.cpp
   TestTU.cpp
   ThreadingTests.cpp
   TraceTests.cpp
+  TweakTests.cpp
   URITests.cpp
   XRefsTests.cpp
+
+  $<TARGET_OBJECTS:obj.clangDaemonTweaks>
   )
 
 target_link_libraries(ClangdTests
   PRIVATE
   clangAST
   clangBasic
   clangDaemon
   clangFormat

clang-tools-extra/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 "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"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <cassert>
+
+using llvm::Failed;
+using llvm::HasValue;
+using llvm::Succeeded;
+using ::testing::IsEmpty;
+using ::testing::Not;
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::string markRange(llvm::StringRef Code, Range R) {
+  size_t Begin = llvm::cantFail(positionToOffset(Code, R.start));
+  size_t End = llvm::cantFail(positionToOffset(Code, R.end));
+  assert(Begin <= End);
+  if (Begin == End) // Mark a single point.
+    return (Code.substr(0, Begin) + "^" + Code.substr(Begin)).str();
+  // Mark a range.
+  return (Code.substr(0, Begin) + "[[" + Code.substr(Begin, End - Begin) +
+          "]]" + Code.substr(End))
+      .str();
+}
+
+void checkAvailable(TweakID ID, llvm::StringRef Input, bool Available) {
+  Annotations Code(Input);
+  ASSERT_TRUE(0 < Code.points().size() || 0 < Code.ranges().size())
+      << "no points of interest specified";
+  TestTU TU;
+  TU.Filename = "foo.cpp";
+  TU.Code = Code.code();
+
+  ParsedAST AST = TU.build();
+
+  auto CheckOver = [&](Range Selection) {
+    auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+        AST.getASTContext().getSourceManager(), Selection.start));
+    auto T = prepareTweak(ID, Tweak::Selection{Code.code(), AST, CursorLoc});
+    if (Available)
+      EXPECT_THAT_EXPECTED(T, Succeeded())
+          << "code is " << markRange(Code.code(), Selection);
+    else
+      EXPECT_THAT_EXPECTED(T, Failed())
+          << "code is " << markRange(Code.code(), Selection);
+  };
+  for (auto P : Code.points())
+    CheckOver(Range{P, P});
+  for (auto R : Code.ranges())
+    CheckOver(R);
+}
+
+/// Checks action is available at every point and range marked in \p Input.
+void checkAvailable(TweakID ID, llvm::StringRef Input) {
+  return checkAvailable(ID, Input, /*Available=*/true);
+}
+
+/// Same as checkAvailable, but checks the action is not available.
+void checkNotAvailable(TweakID ID, llvm::StringRef Input) {
+  return checkAvailable(ID, Input, /*Available=*/false);
+}
+llvm::Expected<std::string> apply(TweakID 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();
+  auto CursorLoc = llvm::cantFail(sourceLocationInMainFile(
+      AST.getASTContext().getSourceManager(), SelectionRng.start));
+  Tweak::Selection S = {Code.code(), AST, CursorLoc};
+
+  auto T = prepareTweak(ID, S);
+  if (!T)
+    return T.takeError();
+  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))
+      << "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; }
+    }
+  )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; }
+    }
+  )cpp";
+  checkTransform(ID, Input, Output);
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang