[tooling] FixItHint Tooling refactoring

[etienneb]

Summary

This is the refactoring to lift some FixItHint into tooling.
used by: http://reviews.llvm.org/D19807

[etienneb]

If this is what you wanted Alex, I'll pursue writing unittests/docs and making a complete patch to land.

See some example on how to use it:
http://reviews.llvm.org/D19807

[alexfh]

In D19941#421735, @etienneb wrote:

If this is what you wanted Alex, I'll pursue writing unittests/docs and making a complete patch to land.

See some example on how to use it:
http://reviews.llvm.org/D19807

Yes, this looks reasonable.

include/clang/Tooling/Fixit.h

2

nit: Should be FixIt (note the capital I).

36

Do we want to expose getSourceRange as an API? If no, it should also be in the namespace internal.

56

I wonder whether we should later extend this to remove comments attached to nodes like Stmt and FunctionDecl? It's out of this patches scope, but maybe add a FIXME to sketch the further development plans.

[etienneb]

add unittests

[etienneb]

more unittests

[alexfh]

include/clang/Tooling/Fixit.h

2

nit: s/FixIt Hint/FixItHint/, since this is a reference to the type.

lib/Tooling/CMakeLists.txt

10

Please rename the files as well (s/Fixit/FixIt/).

unittests/Tooling/CMakeLists.txt

15

s/FixitTest/FixItTest/

unittests/Tooling/FixitTest.cpp

35

Automatic captures always seem suspicious to me. What exactly does this lambda need to capture? If just this, I'd rather make the capture list explicit.

54

Could you add a test where getText returns an empty string?

120

One character range is boring. Can you make the parameter longer and also verify whether the range is a token range or a character range?

[etienneb]

address alexfh comments

[etienneb]

nits

[alexfh]

LG. Thanks!

unittests/Tooling/FixItTest.cpp

37 ↗

(On Diff #56826)

Let's either put the tests in the clang::tooling::<anonymous> namespace (not common for unit tests in this specific directory, but nevertheless reasonable, imo) or add using declarations for tooling::fixit::getText and other tested functions to reduce the amount of boilerplate.

[etienneb]

fix nits

[etienneb]

I added the "using statements".
landing.

include/clang/Tooling/Fixit.h

36

Good question.
I moved it into the internal namespace.

If it's become needed, we can re-expose it.

56

I like the idea. I didn't think about comments and other syntactical elements that may be related to a Node.
But, I believe we will have a other set of functions for Node and comments.

An other concept that I believe it's missing is to provide helper to append expression (operators) and stay semantically correct with operator precedence. Some cases needed to add parentheses.

unittests/Tooling/FixitTest.cpp

54

see line 68, already there!

120

In this case, it's not one character... it's one token.
As it's a "token range", the whole token is used.

But, I can use an expression too.
Not a bad idea: added but in 'createRemoval'

There is already a reand at line 138:

EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:37>",
EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:45>",

include/clang/Tooling/Fixit.h

+//===--- FixIt.h - FixIt Hint utilities -------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements functions to ease source rewriting from AST-nodes.
+//
+//  Example swapping A and B expressions:
+//
+//    Expr *A, *B;
+//    tooling::fixit::createReplacement(*A, *B);
+//    tooling::fixit::createReplacement(*B, *A);
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_FIXIT_H
+#define LLVM_CLANG_TOOLING_FIXIT_H
+
+#include "clang/AST/ASTContext.h"
+
+namespace clang {
+namespace tooling {
+namespace fixit {
+
+namespace internal {
+StringRef getText(SourceRange Range, const ASTContext &Context);
+
+/// \brief Returns the SourceRange of a SourceRange. This identity function is
+///        used by the following template abstractions.
+inline SourceRange getSourceRange(const SourceRange &Range) { return Range; }
+
+/// \brief Returns the SourceRange of the token at Location \p Loc.
+inline SourceRange getSourceRange(const SourceLocation &Loc) {
+  return SourceRange(Loc);
+}
+
+/// \brief Returns the SourceRange of an given Node. \p Node is typically a
+///        'Stmt', 'Expr' or a 'Decl'.
+template <typename T> SourceRange getSourceRange(const T &Node) {
+  return Node.getSourceRange();
+}
+} // end namespace internal
+
+// \brief Returns a textual representation of \p Node.
+template <typename T>
+StringRef getText(const T &Node, const ASTContext &Context) {
+  return internal::getText(internal::getSourceRange(Node), Context);
+}
+
+// \brief Returns a FixItHint to remove \p Node.
+// TODO: Add support for related syntactical elements (i.e. comments, ...).
+template <typename T> FixItHint createRemoval(const T &Node) {
+  return FixItHint::CreateRemoval(internal::getSourceRange(Node));
+}
+
+// \brief Returns a FixItHint to replace \p Destination by \p Source.
+template <typename D, typename S>
+FixItHint createReplacement(const D &Destination, const S &Source,
+                            const ASTContext &Context) {
+  return FixItHint::CreateReplacement(internal::getSourceRange(Destination),
+                                      getText(Source, Context));
+}
+
+} // end namespace fixit
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_TOOLING_FIXINT_H

lib/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_subdirectory(Core)
 
 add_clang_library(clangTooling
   ArgumentsAdjusters.cpp
   CommonOptionsParser.cpp
   CompilationDatabase.cpp
   FileMatchTrie.cpp
+  Fixit.cpp
   JSONCompilationDatabase.cpp
   Refactoring.cpp
   RefactoringCallbacks.cpp
   Tooling.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangDriver
   clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangToolingCore
   )

lib/Tooling/Fixit.cpp

+//===--- FixIt.cpp - FixIt Hint utilities -----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains implementations of utitilies to ease source code rewriting
+// by providing helper functions related to FixItHint.
+//
+//===----------------------------------------------------------------------===//
+#include "clang/Tooling/Fixit.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace tooling {
+namespace fixit {
+
+namespace internal {
+StringRef getText(SourceRange Range, const ASTContext &Context) {
+  return Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
+                              Context.getSourceManager(),
+                              Context.getLangOpts());
+}
+} // end namespace internal
+
+} // end namespace fixit
+} // end namespace tooling
+} // end namespace clang

unittests/Tooling/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   ${LLVM_TARGETS_TO_BUILD}
   Support
   )
 
 # By default MSVC has a 2^16 limit on the number of sections in an object file,
 # and this needs more than that.
 if (MSVC)
   set_source_files_properties(RecursiveASTVisitorTestExprVisitor.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_unittest(ToolingTests
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  FixitTest.cpp  
   LookupTest.cpp
-  ToolingTest.cpp
+  QualTypeNamesTest.cpp
   RecursiveASTVisitorTest.cpp
   RecursiveASTVisitorTestCallVisitor.cpp
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestExprVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
-  RefactoringTest.cpp
-  RewriterTest.cpp
   RefactoringCallbacksTest.cpp
+  RefactoringTest.cpp
   ReplacementsYamlTest.cpp
-  QualTypeNamesTest.cpp
+  RewriterTest.cpp
+  ToolingTest.cpp
   )
 
 target_link_libraries(ToolingTests
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangFrontend
   clangLex
   clangRewrite
   clangTooling
   clangToolingCore
   )

unittests/Tooling/FixitTest.cpp

+//===- unittest/Tooling/FixitTest.cpp ------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestVisitor.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Tooling/Fixit.h"
+
+using namespace clang;
+
+namespace {
+
+struct CallsVisitor : TestVisitor<CallsVisitor> {
+  bool VisitCallExpr(CallExpr *Expr) {
+    OnCall(Expr, Context);
+    return true;
+  }
+
+  std::function<void(CallExpr *, ASTContext *Context)> OnCall;
+};
+
+std::string LocationToString(SourceLocation Loc, ASTContext *Context) {
+  return Loc.printToString(Context->getSourceManager());
+}
+
+TEST(FixitTest, getText) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("foo(x, y)", tooling::fixit::getText(*CE, *Context));
+    EXPECT_EQ("foo(x, y)",
+              tooling::fixit::getText(CE->getSourceRange(), *Context));
+
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", tooling::fixit::getText(*P0, *Context));
+    EXPECT_EQ("y", tooling::fixit::getText(*P1, *Context));
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+
+  Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("APPLY(foo, x, y)", tooling::fixit::getText(*CE, *Context));
+  };
+  Visitor.runOver("#define APPLY(f, x, y) f(x, y)\n"
+                  "void foo(int x, int y) { APPLY(foo, x, y); }");
+}
+
+TEST(FixitTest, createRemoval) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = tooling::fixit::createRemoval(*CE);
+    EXPECT_EQ("foo(x, y)",
+              tooling::fixit::getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = tooling::fixit::createRemoval(*P0);
+    EXPECT_EQ(
+        "x", tooling::fixit::getText(Hint0.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint0.CodeToInsert.empty());
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = tooling::fixit::createRemoval(*P1);
+    EXPECT_EQ(
+        "y", tooling::fixit::getText(Hint1.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint1.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint1.CodeToInsert.empty());
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+}
+
+TEST(FixitTest, createRemovalWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = tooling::fixit::createRemoval(*CE);
+    EXPECT_EQ("FOO",
+              tooling::fixit::getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = tooling::fixit::createRemoval(*P0);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint0.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint0.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint0.CodeToInsert.empty());
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = tooling::fixit::createRemoval(*P1);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:20>",
+              LocationToString(Hint1.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:20>",
+              LocationToString(Hint1.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint1.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint1.CodeToInsert.empty());
+  };
+  Visitor.runOver("#define FOO foo(1, 1)\n"
+                  "void foo(int x, int y) { FOO; }");
+}
+
+TEST(FixitTest, createReplacement) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint0 = tooling::fixit::createReplacement(*P0, *P1, *Context);
+    FixItHint Hint1 = tooling::fixit::createReplacement(*P1, *P0, *Context);
+
+    // Validate Hint0 fields.
+    EXPECT_EQ(
+        "x", tooling::fixit::getText(Hint0.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_EQ(Hint0.CodeToInsert, "y");
+
+    // Validate Hint1 fields.
+    EXPECT_EQ(
+        "y", tooling::fixit::getText(Hint1.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint1.InsertFromRange.isInvalid());
+    EXPECT_EQ(Hint1.CodeToInsert, "x");
+  };
+
+  Visitor.runOver("void foo(int x, int y) { foo(x, y); }");
+
+  Visitor.runOver("#define APPLY(f, x, y) f(x, y)\n"
+                  "void foo(int x, int y) { APPLY(foo, x, y); }");
+
+  Visitor.runOver("#define APPLY(f, P) f(P)\n"
+                  "#define PAIR(x, y) x, y\n"
+                  "void foo(int x, int y) { APPLY(foo, PAIR(x, y)); }\n");
+}
+
+TEST(FixitTest, createReplacementWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [&](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint = tooling::fixit::createReplacement(*P0, *P1, *Context);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:17>",
+              LocationToString(Hint.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+  };
+
+  Visitor.runOver("#define FOO foo(1, 1)\n"
+                  "void foo(int x, int y) { FOO; }");
+}
+
+} // end anonymous namespace