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

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: llvm-commits, mgorny, ioeric, MaskRay, jkorous, arphaman, kadircet, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D56611

llvm-svn: 352796

Author: ilya-biryukov

clang-tools-extra/clangd/SourceCode.cpp

@@ -11,6 +11,8 @@
 #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"
@@ -141,6 +143,69 @@ Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc) {
   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();
@@ -169,8 +234,7 @@ std::pair<size_t, size_t> offsetToClangLineColumn(llvm::StringRef Code,
   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};

clang-tools-extra/clangd/SourceCode.h

@@ -14,6 +14,7 @@
 #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"
@@ -61,6 +62,46 @@ Position sourceLocToPosition(const SourceManager &SM, SourceLocation Loc);
 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 - one can use
+/// SourceManager::{getExpansion|getFile|getSpelling}Loc. This function
+/// calls SourceManager::getFileLoc on both ends of \p R to do the conversion.
+///
+/// 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);

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

@@ -8,6 +8,5 @@ include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
 # $<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

@@ -0,0 +1,132 @@
+//===--- 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/clangd/refactor/tweaks/SwapIfBranches.cpp

@@ -45,8 +45,11 @@ add_extra_unittest(ClangdTests
   TestTU.cpp
   ThreadingTests.cpp
   TraceTests.cpp
+  TweakTests.cpp
   URITests.cpp
   XRefsTests.cpp
+
+  $<TARGET_OBJECTS:obj.clangDaemonTweaks>
   )
 
 target_link_libraries(ClangdTests