[libTooling] Introduce new library of source-code builders.

[ymandel]

Summary

Introduces facilities for easily building source-code strings, including
idiomatic use of the address-of, dereference and member-access operators (dot
and arrow) and queries about need for parentheses.

[Eugene.Zelenko]

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp

77

Could return {}. Same in other places. By the word, did you run Clang-format over code?

[ymandel]

ran clang-format

[ymandel]

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp

77

No, thanks for pointing that out.

I find return std::string() more readable, because it doesn't require that I check/know the return type of the function. But, if return {} is standard, I'm fine changing it. What's most common in the clang source?

[ymandel]

Added parens operator; cleaned comments and api a bit.

[gribozavr]

clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h

10

"\file"

11

"source code"

31

s/a//

or s/a parentheses/a pair of parentheses/

34

mayEverNeedParens? (to emphasize conservativeness and lack of context)

57

Given that empty string is returned in a vanishingly small number of cases, it would be worth it returning llvm::Optional<std::string> to force the caller to handle the failure.

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp

22

No need to repeat the comment from the header.

69

buildDereference below checks for getText returning an empty string; this function does not. Why?

77

I personally find return ""; more readable. After all, we don't care about the specific type, we want to express that we return an empty string. The most natural way to write an empty string is a string literal "".

clang/unittests/Tooling/SourceCodeBuildersTest.cpp

24

"translation unit"

77

Also need tests for:

• overloaded binary operator S(10) + S(20)
• implicit conversions void takeS(S); takeS(10 + 20);

[ymandel]

Thanks for the review! Agreed on all points and then some -- next revision will have a bunch of cleanups. I think the only major issue is the return type. See below.

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp

69

Regarding this and the issue of returning llvm::Optional: the code overall is rather inconsistent in its handling of empty strings returned from getText. This is only one example.

sorry for this -- this code was mostly collected from various other places in the codebase and I should have looked it over more carefully.

With that said -- I think we need to decide whether we systematically assume that getText doesn't return an empty string, or systematically check for it and return llvm::None in that case. We have the choice becuase there is no risk of UB from ignoring the empty text. Any checks in the code seem only designed to propagate the "emptiness" for the caller's sake.

I'm inclined to state a precondition on the all of the functions that getText(E) is non empty, rather than sprinkle optionals throughout the API. My reasoning is that checking the result of these functions is "too late" in the process. A failure here is caused by passing a node that has no corresponding source code. If the caller is potentially dealing with such phantom nodes, they should know that before trying to construct code with them. Any check they can do on the result of getText they should have done earlier with a more appropriate predicate of the node.

That said, I understand the preference for defensiveness (don't assume the caller will get it right). I'm also fine with a compromise of asserting the non-emptiness of any internal call to getText although that might be almost as messy as just handling it.

WDYT?

[ymandel]

address comments and some general cleanup

[gribozavr]

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp

69

It is true that there is no risk of UB (in terms of C++ spec), but there is a garbage-in/garbage-out issue. I don't think users will appreciate garbage out, because I don't think they have the ability to check the precondition properly -- after all, they don't know how exactly getText will be called.

Even if we figure out how to specify for users to check the precondition, they would always have to check it in order to use these APIs properly. After all, the source transformation tools that users write can't generally assume what code *their users* will throw at those tools. And if someone wants to throw together a simple tool prototype, they can always force-unwrap the optional, it is just an extra *.

Therefore, I believe embedding the check in these APIs will create a significantly better usage experience, and does not create much of a burden for users who don't want to handle errors.

[ymandel]

converted builders to return optional

clang/include/clang/Tooling/Refactoring/SourceCodeBuilders.h

+//===--- SourceCodeBuilders.h - Source-code building facilities -*- 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
+//
+//===----------------------------------------------------------------------===//
+///
+/// /file
+/// This file collects facilities for generating source-code strings.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_SOURCE_CODE_BUILDERS_H_
+#define LLVM_CLANG_TOOLING_REFACTOR_SOURCE_CODE_BUILDERS_H_
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
+#include <string>
+
+namespace clang {
+namespace tooling {
+
+/// \name Code analysis utilities.
+/// @{
+/// Ignores implicit object-construction expressions in addition to the normal
+/// implicit expressions that are ignored.
+const Expr* reallyIgnoreImplicit(const Expr &ExprNode);
+
+/// Determines whether printing this expression in *any* expression requires a
+/// parentheses to preserve its meaning. This analyses is necessarily
+/// conservative because it lacks information about the target context.
+bool mayNeedParens(const Expr &ExprNode);
+
+/// Determines whether printing this expression to the left of a dot or arrow
+/// operator requires a parentheses to preserve its meaning. Given that
+/// dot/arrow are (effectively) the highest precedence, this is equivalent to
+/// asking whether it ever needs parens.
+inline bool needParensBeforeDotOrArrow(const Expr &ExprNode) {
+  return mayNeedParens(ExprNode);
+}
+
+/// Determines whether printing this expression to the right of a unary operator
+/// requires a parentheses to preserve its meaning.
+bool needParensAfterUnaryOperator(const Expr &ExprNode);
+/// @}
+
+/// \name Basic code-string generation utilities.
+/// @{
+/// Builds a pointer to an expression: prefix with `*` but simplify when it
+/// already begins with `&`.  \returns empty string on failure.
+std::string buildDereference(const ASTContext &Context, const Expr &Expr);
+
+/// Builds a pointer to an expression: prefix with `&` but simplify when it
+/// already begins with `*`.  \returns empty string on failure.
+std::string buildAddressOf(const ASTContext &Context, const Expr &Expr);
+
+/// Adds a dot to the end of the given expression, but adds parentheses when
+/// needed by the syntax, and simplifies to `->` when possible, e.g.:
+///
+///  `x` becomes `x.`
+///  `*a` becomes `a->`
+///  `a+b` becomes `(a+b).`
+std::string buildDot(const ASTContext &Context, const Expr &Expr);
+
+/// Adds an arrow to the end of the given expression, but adds parentheses
+/// when needed by the syntax, and simplifies to `.` when possible, e.g.:
+///
+///  `x` becomes `x->`
+///  `&a` becomes `a.`
+///  `a+b` becomes `(a+b)->`
+std::string buildArrow(const ASTContext &Context, const Expr &Expr);
+/// @}
+
+} // namespace tooling
+} // namespace clang
+#endif // LLVM_CLANG_TOOLING_REFACTOR_SOURCE_CODE_BUILDERS_H_

clang/lib/Tooling/Refactoring/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangToolingRefactoring
   ASTSelection.cpp
   ASTSelectionRequirements.cpp
   AtomicChange.cpp
   Extract/Extract.cpp
   Extract/SourceExtraction.cpp
   RangeSelector.cpp
   RefactoringActions.cpp
   Rename/RenamingAction.cpp
   Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
   SourceCode.cpp
+  SourceCodeBuilders.cpp
   Stencil.cpp
   Transformer.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangIndex
   clangLex
   clangRewrite
   clangToolingCore
   )

clang/lib/Tooling/Refactoring/SourceCodeBuilders.cpp

+//===--- SourceCodeBuilder.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 "clang/Tooling/Refactoring/SourceCodeBuilders.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/Tooling/Refactoring/SourceCode.h"
+#include "llvm/ADT/Twine.h"
+#include <string>
+
+using namespace clang;
+using namespace tooling;
+
+// Ignores implicit object-construction expressions in addition to the normal
+// implicit expressions that are ignored.
+const Expr *tooling::reallyIgnoreImplicit(const Expr &ExprNode) {
+  const Expr* E = ExprNode.IgnoreImplicit();
+  if (const auto* CE = dyn_cast<CXXConstructExpr>(E)) {
+    if (CE->getNumArgs() == 1 &&
+        CE->getArg(0)->getSourceRange() == E->getSourceRange())
+      return CE->getArg(0)->IgnoreImplicit();
+  }
+  return E;
+}
+
+bool tooling::mayNeedParens(const Expr &ExprNode) {
+  const Expr* Expr = reallyIgnoreImplicit(ExprNode);
+  // We always want parens around unary, binary, and ternary operators, because
+  // they are lower precedence.
+  if (isa<UnaryOperator>(Expr) || isa<BinaryOperator>(Expr) ||
+      isa<AbstractConditionalOperator>(Expr)) {
+    return true;
+  }
+
+  // We need parens around calls to all overloaded operators except: function
+  // calls, subscripts, and expressions that are already part of an (implicit)
+  // call to operator->. These latter are all in the same precedence level as
+  // dot/arrow and that level is left associative, so they don't need parens
+  // when appearing on the left.
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(Expr)) {
+    return Op->getOperator() != OO_Call && Op->getOperator() != OO_Subscript &&
+           Op->getOperator() != OO_Arrow;
+  }
+
+  return false;
+}
+
+bool tooling::needParensAfterUnaryOperator(const Expr &ExprNode) {
+  const Expr* Expr = reallyIgnoreImplicit(ExprNode);
+  if (isa<BinaryOperator>(Expr) || isa<ConditionalOperator>(Expr)) {
+    return true;
+  }
+  if (const auto *Op = dyn_cast<CXXOperatorCallExpr>(Expr)) {
+    return Op->getNumArgs() == 2 && Op->getOperator() != OO_PlusPlus &&
+           Op->getOperator() != OO_MinusMinus && Op->getOperator() != OO_Call &&
+           Op->getOperator() != OO_Subscript;
+  }
+  return false;
+}
+
+std::string tooling::buildDereference(const ASTContext &Context,
+                                      const Expr &ExprNode) {
+  if (const auto *Op = dyn_cast<UnaryOperator>(&ExprNode)) {
+    if (Op->getOpcode() == UO_AddrOf) {
+      // Strip leading '&'.
+      return getText(*Op->getSubExpr()->IgnoreParenImpCasts(), Context);
+    }
+  }
+  StringRef Text = getText(ExprNode, Context);
+
+  if (Text.empty()) return std::string();
+  // Add leading '*'.
+  if (needParensAfterUnaryOperator(ExprNode)) {
+    return (llvm::Twine("*(") + Text + ")").str();
+  }
+  return (llvm::Twine("*") + Text).str();
+}
+
+std::string tooling::buildAddressOf(const ASTContext &Context,
+                                    const Expr &ExprNode) {
+  if (const auto *Op = dyn_cast<UnaryOperator>(&ExprNode)) {
+    if (Op->getOpcode() == UO_Deref) {
+      // Strip leading '*'.
+      return getText(*Op->getSubExpr()->IgnoreParenImpCasts(), Context);
+    }
+  }
+  // Add leading '&'.
+  const std::string Text = getText(ExprNode, Context);
+  if (Text.empty()) return std::string();
+  if (needParensAfterUnaryOperator(ExprNode)) {
+    return (llvm::Twine("&(") + Text + ")").str();
+  }
+  return (llvm::Twine("&") + Text).str();
+}
+
+std::string tooling::buildDot(const ASTContext &Context, const Expr &ExprNode) {
+  if (const auto *Op = llvm::dyn_cast<UnaryOperator>(&ExprNode)) {
+    if (Op->getOpcode() == UO_Deref) {
+      // Strip leading '*', add following '->'.
+      const Expr *SubExpr = Op->getSubExpr()->IgnoreParenImpCasts();
+      const std::string DerefText = getText(*SubExpr, Context);
+      if (DerefText.empty()) return std::string();
+      if (needParensBeforeDotOrArrow(*SubExpr)) {
+        return (llvm::Twine("(") + DerefText + ")->").str();
+      }
+      return (llvm::Twine(DerefText) + "->").str();
+    }
+  }
+  // Add following '.'.
+  const std::string Text = getText(ExprNode, Context);
+  if (Text.empty()) return std::string();
+  if (needParensBeforeDotOrArrow(ExprNode)) {
+    return (llvm::Twine("(") + Text + ").").str();
+  }
+  return (llvm::Twine(Text) + ".").str();
+}
+
+std::string tooling::buildArrow(const ASTContext &Context,
+                                const Expr &ExprNode) {
+  if (const auto *Op = llvm::dyn_cast<UnaryOperator>(&ExprNode)) {
+    if (Op->getOpcode() == UO_AddrOf) {
+      // Strip leading '&', add following '.'.
+      const Expr *SubExpr = Op->getSubExpr()->IgnoreParenImpCasts();
+      const std::string DerefText = getText(*SubExpr, Context);
+      if (DerefText.empty()) return std::string();
+      if (needParensBeforeDotOrArrow(*SubExpr)) {
+        return (llvm::Twine("(") + DerefText + ").").str();
+      }
+      return (llvm::Twine(DerefText) + ".").str();
+    }
+  }
+  // Add following '->'.
+  const std::string Text = getText(ExprNode, Context);
+  if (Text.empty()) return std::string();
+  if (needParensBeforeDotOrArrow(ExprNode)) {
+    return (llvm::Twine("(") + Text + ")->").str();
+  }
+  return (llvm::Twine(Text) + "->").str();
+}

clang/unittests/Tooling/CMakeLists.txt

add_clang_unittest(ToolingTests
   RecursiveASTVisitorTestDeclVisitor.cpp
   RecursiveASTVisitorTestPostOrderVisitor.cpp
   RecursiveASTVisitorTestTypeLocVisitor.cpp
   RefactoringActionRulesTest.cpp
   RefactoringCallbacksTest.cpp
   RefactoringTest.cpp
   ReplacementsYamlTest.cpp
   RewriterTest.cpp
+  SourceCodeBuildersTest.cpp
   SourceCodeTest.cpp
   StencilTest.cpp
   ToolingTest.cpp
   TransformerTest.cpp
   )
 
 clang_target_link_libraries(ToolingTests
   PRIVATE

clang/unittests/Tooling/SourceCodeBuildersTest.cpp

+//===- unittest/Tooling/SourceCodeBuildersTest.cpp ------------------------===//
+//
+// 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 "clang/Tooling/Refactoring/SourceCodeBuilders.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+using namespace tooling;
+using namespace ast_matchers;
+
+namespace {
+using MatchResult = MatchFinder::MatchResult;
+
+// Create a valid translation-unit from a statement.
+static std::string wrapSnippet(StringRef StatementCode) {
+  return ("struct S { int field; }; "
+          "S operator+(const S &a, const S &b); "
+          "auto test_snippet = []{" + StatementCode +
+          "};")
+      .str();
+}
+
+static DeclarationMatcher wrapMatcher(const StatementMatcher &Matcher) {
+  return varDecl(hasName("test_snippet"),
+                 hasDescendant(compoundStmt(hasAnySubstatement(Matcher))));
+}
+
+struct TestMatch {
+  // The AST unit from which `result` is built. We bundle it because it backs
+  // the result. Users are not expected to access it.
+  std::unique_ptr<ASTUnit> AstUnit;
+  // The result to use in the test. References `ast_unit`.
+  MatchResult Result;
+};
+
+// Matches `Matcher` against the statement `StatementCode` and returns the
+// result. Handles putting the statement inside a function and modifying the
+// matcher correspondingly. `Matcher` should match one of the statements in
+// `StatementCode` exactly -- that is, produce exactly one match. However,
+// `StatementCode` may contain other statements not described by `Matcher`.
+static llvm::Optional<TestMatch> matchStmt(StringRef StatementCode,
+                                           StatementMatcher Matcher) {
+  auto AstUnit = buildASTFromCode(wrapSnippet(StatementCode));
+  if (AstUnit == nullptr) {
+    ADD_FAILURE() << "AST construction failed";
+    return llvm::None;
+  }
+  ASTContext &Context = AstUnit->getASTContext();
+  auto Matches = ast_matchers::match(wrapMatcher(Matcher), Context);
+  // We expect a single, exact match for the statement.
+  if (Matches.size() != 1) {
+    ADD_FAILURE() << "Wrong number of matches: " << Matches.size();
+    return llvm::None;
+  }
+  return TestMatch{std::move(AstUnit), MatchResult(Matches[0], &Context)};
+}
+
+static void testPredicate(bool (*Pred)(const Expr &ExprNode),
+                        StringRef Snippet, bool Expected) {
+  auto StmtMatch = matchStmt(Snippet, expr().bind("expr"));
+  ASSERT_TRUE(StmtMatch);
+  EXPECT_EQ(Expected, Pred(*StmtMatch->Result.Nodes.getNodeAs<Expr>("expr")));
+}
+
+TEST(SourceCodeBuildersTest, needParensAfterUnaryOperator) {
+  testPredicate(needParensAfterUnaryOperator, "3 + 5;", true);
+  testPredicate(needParensAfterUnaryOperator, "true ? 3 : 5;", true);
+
+  testPredicate(needParensAfterUnaryOperator, "int x; x;", false);
+  testPredicate(needParensAfterUnaryOperator, "int(3.0);", false);
+  testPredicate(needParensAfterUnaryOperator, "void f(); f();", false);
+  testPredicate(needParensAfterUnaryOperator, "int a[3]; a[0];", false);
+  testPredicate(needParensAfterUnaryOperator, "S x; x.field;", false);
+  testPredicate(needParensAfterUnaryOperator, "int x = 1; --x;", false);
+  testPredicate(needParensAfterUnaryOperator, "int x = 1; -x;", false);
+}
+
+TEST(SourceCodeBuildersTest, mayNeedParens) {
+  testPredicate(mayNeedParens, "3 + 5;", true);
+  testPredicate(mayNeedParens, "true ? 3 : 5;", true);
+  testPredicate(mayNeedParens, "int x = 1; --x;", true);
+  testPredicate(mayNeedParens, "int x = 1; -x;", true);
+
+  testPredicate(mayNeedParens, "int x; x;", false);
+  testPredicate(mayNeedParens, "int(3.0);", false);
+  testPredicate(mayNeedParens, "void f(); f();", false);
+  testPredicate(mayNeedParens, "int a[3]; a[0];", false);
+  testPredicate(mayNeedParens, "S x; x.field;", false);
+}
+
+static void testBuilder(std::string (*Builder)(const ASTContext &Context,
+                                               const Expr &ExprNode),
+                        StringRef Snippet, StringRef Expected) {
+  auto StmtMatch = matchStmt(Snippet, expr().bind("expr"));
+  ASSERT_TRUE(StmtMatch);
+  EXPECT_EQ(Expected,
+            Builder(*StmtMatch->Result.Context,
+                    *StmtMatch->Result.Nodes.getNodeAs<Expr>("expr")));
+}
+
+TEST(SourceCodeBuildersTest, BuildAddressOfValue) {
+  testBuilder(buildAddressOf, "S x; x;", "&x");
+}
+
+TEST(SourceCodeBuildersTest, BuildAddressOfPointerDereference) {
+  testBuilder(buildAddressOf, "S *x; *x;", "x");
+}
+
+TEST(SourceCodeBuildersTest, BuildAddressOfPointerDereferenceIgnoresParens) {
+  testBuilder(buildAddressOf, "S *x; *(x);", "x");
+}
+
+TEST(SourceCodeBuildersTest, BuildAddressOfBinaryOperation) {
+  testBuilder(buildAddressOf, "S x; x + x;", "&(x + x)");
+}
+
+TEST(SourceCodeBuildersTest, BuildDereferencePointer) {
+  testBuilder(buildDereference, "S *x; x;", "*x");
+}
+
+TEST(SourceCodeBuildersTest, BuildDereferenceValueAddress) {
+  testBuilder(buildDereference, "S x; &x;", "x");
+}
+
+TEST(SourceCodeBuildersTest, BuildDereferenceValueAddressIgnoresParens) {
+  testBuilder(buildDereference, "S x; &(x);", "x");
+}
+
+TEST(SourceCodeBuildersTest, BuildDereferenceBinaryOperation) {
+  testBuilder(buildDereference, "S *x; x + 1;", "*(x + 1)");
+}
+
+TEST(SourceCodeBuildersTest, BuildDotValue) {
+  testBuilder(buildDot, "S x; x;", "x.");
+}
+
+TEST(SourceCodeBuildersTest, BuildDotPointerDereference) {
+  testBuilder(buildDot, "S *x; *x;", "x->");
+}
+
+TEST(SourceCodeBuildersTest, BuildDotPointerDereferenceIgnoresParens) {
+  testBuilder(buildDot, "S *x; *(x);", "x->");
+}
+
+TEST(SourceCodeBuildersTest, BuildDotBinaryOperation) {
+  testBuilder(buildDot, "S x; x + x;", "(x + x).");
+}
+
+TEST(SourceCodeBuildersTest, BuildDotPointerDereferenceExprWithParens) {
+  testBuilder(buildDot, "S *x; *(x + 1);", "(x + 1)->");
+}
+
+TEST(SourceCodeBuildersTest, BuildArrowPointer) {
+  testBuilder(buildArrow, "S *x; x;", "x->");
+}
+
+TEST(SourceCodeBuildersTest, BuildArrowValueAddress) {
+  testBuilder(buildArrow, "S x; &x;", "x.");
+}
+
+TEST(SourceCodeBuildersTest, BuildArrowValueAddressIgnoresParens) {
+  testBuilder(buildArrow, "S x; &(x);", "x.");
+}
+
+TEST(SourceCodeBuildersTest, BuildArrowBinaryOperation) {
+  testBuilder(buildArrow, "S *x; x + 1;", "(x + 1)->");
+}
+
+TEST(SourceCodeBuildersTest, BuildArrowValueAddressWithParens) {
+  testBuilder(buildArrow, "S x; &(true ? x : x);", "(true ? x : x).");
+}
+} // namespace