[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

1 ↗

(On Diff #56214)

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

35 ↗

(On Diff #56214)

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

55 ↗

(On Diff #56214)

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

1 ↗

(On Diff #56790)

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

34 ↗

(On Diff #56790)

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.

53 ↗

(On Diff #56790)

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

119 ↗

(On Diff #56790)

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

38

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

35 ↗

(On Diff #56214)

Good question.
I moved it into the internal namespace.

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

55 ↗

(On Diff #56214)

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

53 ↗

(On Diff #56790)

see line 68, already there!

119 ↗

(On Diff #56790)

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;
+
+using tooling::fixit::getText;
+using tooling::fixit::createRemoval;
+using tooling::fixit::createReplacement;
+
+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)", getText(*CE, *Context));
+    EXPECT_EQ("foo(x, y)", getText(CE->getSourceRange(), *Context));
+
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getText(*P0, *Context));
+    EXPECT_EQ("y", 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)", 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, getTextWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("F OO", getText(*CE, *Context));
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("", getText(*P0, *Context));
+    EXPECT_EQ("", getText(*P1, *Context));
+  };
+  Visitor.runOver("#define F foo(\n"
+                  "#define OO x, y)\n"
+                  "void foo(int x, int y) { F OO ; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    EXPECT_EQ("", getText(*CE, *Context));
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    EXPECT_EQ("x", getText(*P0, *Context));
+    EXPECT_EQ("y", getText(*P1, *Context));
+  };
+  Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
+                  "void foo(int x, int y) { FOO(x,y) }");
+}
+
+TEST(FixItTest, createRemoval) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = createRemoval(*CE);
+    EXPECT_EQ("foo(x, y)", getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = createRemoval(*P0);
+    EXPECT_EQ("x", getText(Hint0.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint0.CodeToInsert.empty());
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = createRemoval(*P1);
+    EXPECT_EQ("y", 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); }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = createRemoval(*P0);
+    EXPECT_EQ("x + y", getText(Hint0.RemoveRange.getAsRange(), *Context));
+
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint1 = createRemoval(*P1);
+    EXPECT_EQ("y + x", getText(Hint1.RemoveRange.getAsRange(), *Context));
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x + y, y + x); }");
+}
+
+TEST(FixItTest, createRemovalWithMacro) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = createRemoval(*CE);
+    EXPECT_EQ("FOO", getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+
+    Expr *P0 = CE->getArg(0);
+    FixItHint Hint0 = 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 = 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; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    FixItHint Hint = createRemoval(*CE);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:37>",
+              LocationToString(Hint.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:1:45>",
+              LocationToString(Hint.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_TRUE(Hint.CodeToInsert.empty());
+  };
+  Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
+                  "void foo(int x, int y) { FOO(x,y) }");
+}
+
+TEST(FixItTest, createReplacement) {
+  CallsVisitor Visitor;
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint0 = createReplacement(*P0, *P1, *Context);
+    FixItHint Hint1 = createReplacement(*P1, *P0, *Context);
+
+    // Validate Hint0 fields.
+    EXPECT_EQ("x", getText(Hint0.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint0.InsertFromRange.isInvalid());
+    EXPECT_EQ(Hint0.CodeToInsert, "y");
+
+    // Validate Hint1 fields.
+    EXPECT_EQ("y", 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 = 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; }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint = createReplacement(*P0, *P1, *Context);
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:2:30>",
+              LocationToString(Hint.RemoveRange.getBegin(), Context));
+    EXPECT_EQ("input.cc:2:26 <Spelling=input.cc:2:30>",
+              LocationToString(Hint.RemoveRange.getEnd(), Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_EQ("y", Hint.CodeToInsert);
+  };
+  Visitor.runOver("#define FOO(x, y) (void)x; (void)y; foo(x, y);\n"
+                  "void foo(int x, int y) { FOO(x,y) }");
+
+  Visitor.OnCall = [](CallExpr *CE, ASTContext *Context) {
+    Expr *P0 = CE->getArg(0);
+    Expr *P1 = CE->getArg(1);
+    FixItHint Hint = createReplacement(*P0, *P1, *Context);
+    EXPECT_EQ("x + y", getText(Hint.RemoveRange.getAsRange(), *Context));
+    EXPECT_TRUE(Hint.InsertFromRange.isInvalid());
+    EXPECT_EQ("y + x", Hint.CodeToInsert);
+  };
+  Visitor.runOver("void foo(int x, int y) { foo(x + y, y + x); }");
+}
+
+} // end anonymous namespace