[LibTooling] Add NodeId, a strong type for AST-matcher node identifiers.

[ymandel]

Summary

The standard matcher API uses StringRefs to identify bound nodes. This patch
introduces a strong type thats allows distinguishing ids from arbitrary text in
APIs. It additionally adds a templated version which can indicate the AST type
to which the id is bound.

This patch is the second in a series intended to improve the abstractions
available to users for writing source-to-source transformations. A full
discussion of the end goal can be found on the cfe-dev list with subject "[RFC]
Easier source-to-source transformations with clang tooling".

[ymandel]

This comment was removed by ymandel.

[ymandel]

"Revert" to original diff. This undoes the previous diff, which associated with the wrong revision.

[ymandel]

gentle ping...

[ilya-biryukov]

clang/include/clang/Tooling/Refactoring/NodeId.h

29

What are the use-cases for passing a custom id to this class?
If the purpose is to hide the IDs from the user, exposing this constructor seems to be against this goal.

clang/lib/Tooling/Refactoring/NodeId.cpp

12

NIT: change to using namespace for consistency

24

Suggestion from @klimek :

could we get the unique string by printing a pointer of this instead?
(And disallow copies and moves of this class for this to be correct)

[ilya-biryukov]

Could you provide the rationale for having NodeID vs just using strings for the binds?
Is this just a more type-safe way to do the same thing or is that actually required to solve a particular problem?

[ymandel]

clang/include/clang/Tooling/Refactoring/NodeId.h

29

If there are errors relating to the id (say, referencing it in a stencil when it wasn't bound in the match), the errors are hard to understand when the id is auto-generated. In general, I'd say that best practice is to explicitly specify the id so that dynamic failures print a meaningful id. The default constructor is a compromise and perhaps shouldn't be exported.

that said, if we could find another way to give good error messages (for example, if we could somehow grab the line number of the declaration), then the default construction would be the preferred approach.

clang/include/clang/Tooling/Refactoring/NodeId.h

+//===--- NodeId.h - NodeId class ------------------------------*- 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 defines abstractions for the bound identifiers used in AST
+/// matchers.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLING_REFACTOR_NODE_ID_H_
+#define LLVM_CLANG_TOOLING_REFACTOR_NODE_ID_H_
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "llvm/ADT/StringRef.h"
+#include <string>
+
+namespace clang {
+namespace tooling {
+
+/// A strong type for AST node identifiers.  The standard API uses StringRefs
+/// for identifiers.  The strong type allows us to distinguish ids from
+/// arbitrary text snippets in various APIs.
+class NodeId {
+public:
+  explicit NodeId(std::string Id) : Id(std::move(Id)) {}
+
+  /// Creates a NodeId whose name is based on \p Id. Guarantees that unique ids
+  /// map to unique NodeIds.
+  explicit NodeId(size_t Id) : Id("id" + std::to_string(Id)) {}
+
+  /// Creates a NodeId with a generated name. The name is guaranteed to be
+  /// unique with respect to other generated names.
+  NodeId();
+
+  llvm::StringRef id() const { return Id; }
+
+  /// Gets the AST node in \p Result corresponding to this NodeId, if
+  /// any. Otherwise, returns null.
+  template <typename Node>
+  const Node *
+  getNodeAs(const ast_matchers::MatchFinder::MatchResult &Result) const {
+    return Result.Nodes.getNodeAs<Node>(Id);
+  }
+
+private:
+  std::string Id;
+};
+
+/// Refinement of NodeId that identifies the intended node type for the id. This
+/// additional information allows us to select appropriate overloads or
+/// constrain use of various combinators. It also allows us to distinguish when
+/// a \c Expr node is intended as a \c Stmt, which influences the intended
+/// source range for the node.  \p Node is the AST node type corresponding to
+/// this id.
+template <typename Node> class TypedNodeId : public NodeId {
+public:
+  using NodeId::NodeId;
+  using MatcherType = ast_matchers::internal::Matcher<Node>;
+
+  /// Creates a matcher corresponding to the AST-node type of this id and bound
+  /// to this id. Intended for settings where the type of matcher is
+  /// obvious/uninteresting. For example,
+  ///
+  ///   ExprId Arg;
+  ///   auto Matcher = callExpr(callee(isFunctionNamed("foo")),
+  ///                           hasArgument(0, Arg.bind()));
+  MatcherType bind() const {
+    return ast_matchers::internal::BindableMatcher<Node>(
+               ast_matchers::internal::TrueMatcher())
+        .bind(id());
+  }
+};
+
+using ExprId = TypedNodeId<Expr>;
+using StmtId = TypedNodeId<Stmt>;
+using DeclId = TypedNodeId<Decl>;
+using TypeId = TypedNodeId<Type>;
+
+} // namespace tooling
+} // namespace clang
+#endif // LLVM_CLANG_TOOLING_REFACTOR_NODE_ID_H_

clang/lib/Tooling/Refactoring/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangToolingRefactor
   ASTSelection.cpp
   ASTSelectionRequirements.cpp
   AtomicChange.cpp
   Extract/Extract.cpp
   Extract/SourceExtraction.cpp
   RefactoringActions.cpp
   Rename/RenamingAction.cpp
   Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
+  NodeId.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangFormat
   clangIndex
   clangLex
   clangRewrite
   clangToolingCore
   )

clang/lib/Tooling/Refactoring/NodeId.cpp

+//===--- NodeId.cpp - NodeId implementation ---------------------*- 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/NodeId.h"
+#include <atomic>
+
+namespace clang {
+namespace tooling {
+
+// For guaranteeing unique ids on NodeId creation.
+static size_t nextId() {
+  // Start with a relatively high number to avoid bugs if the user mixes
+  // explicitly-numbered ids with those generated with `NextId()`. Similarly, we
+  // choose a number that allows generated ids to be easily recognized.
+  static std::atomic<size_t> Next(2222);
+  return Next.fetch_add(1, std::memory_order_relaxed);
+}
+
+NodeId::NodeId() : NodeId(nextId()) {}
+
+} // namespace tooling
+} // namespace clang