-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[AST matchers] matchers for absl Status / StatusOr #160953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[AST matchers] matchers for absl Status / StatusOr #160953
Conversation
Created using spr 1.3.4
@llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) ChangesThis is part of the upstreaming of abseil-unchecked-statusor-use. These matchers are used in the upcoming model, but also in a downstream Full diff: https://github.com/llvm/llvm-project/pull/160953.diff 5 Files Affected:
diff --git a/clang/include/clang/ASTMatchers/AbslMatchers.h b/clang/include/clang/ASTMatchers/AbslMatchers.h
new file mode 100644
index 0000000000000..18cc6dd07c9af
--- /dev/null
+++ b/clang/include/clang/ASTMatchers/AbslMatchers.h
@@ -0,0 +1,29 @@
+//===- AbslMatchers.h - AST Matchers for Abseil -----------------*- 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
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements matchers specific to structures in Abseil
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ASTMATCHERS_ABSLMATCHERS_H
+#define LLVM_CLANG_ASTMATCHERS_ABSLMATCHERS_H
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang {
+namespace ast_matchers {
+namespace absl_matchers {
+
+DeclarationMatcher statusOrClass();
+DeclarationMatcher statusClass();
+
+} // end namespace absl_matchers
+} // end namespace ast_matchers
+} // end namespace clang
+
+#endif // LLVM_CLANG_ASTMATCHERS_ABSLMATCHERS_H
diff --git a/clang/lib/ASTMatchers/AbslMatchers.cpp b/clang/lib/ASTMatchers/AbslMatchers.cpp
new file mode 100644
index 0000000000000..f666733f340ac
--- /dev/null
+++ b/clang/lib/ASTMatchers/AbslMatchers.cpp
@@ -0,0 +1,26 @@
+//===- AbslMatchers.cpp - AST Matchers for Abseil --------------*- 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/ASTMatchers/AbslMatchers.h"
+
+namespace clang {
+namespace ast_matchers {
+namespace absl_matchers {
+DeclarationMatcher statusOrClass() {
+ return classTemplateSpecializationDecl(
+ hasName("::absl::StatusOr"),
+ hasTemplateArgument(0, refersToType(type().bind("StatusOrValueType"))));
+}
+
+DeclarationMatcher statusClass() {
+ return cxxRecordDecl(hasName("::absl::Status"));
+}
+
+} // end namespace absl_matchers
+} // end namespace ast_matchers
+} // end namespace clang
diff --git a/clang/lib/ASTMatchers/CMakeLists.txt b/clang/lib/ASTMatchers/CMakeLists.txt
index 7769fd656ac06..e7c6b90a3a781 100644
--- a/clang/lib/ASTMatchers/CMakeLists.txt
+++ b/clang/lib/ASTMatchers/CMakeLists.txt
@@ -8,6 +8,7 @@ set(LLVM_LINK_COMPONENTS
add_clang_library(clangASTMatchers
ASTMatchFinder.cpp
ASTMatchersInternal.cpp
+ AbslMatchers.cpp
GtestMatchers.cpp
LowLevelHelpers.cpp
diff --git a/clang/unittests/ASTMatchers/AbslMatchersTest.cpp b/clang/unittests/ASTMatchers/AbslMatchersTest.cpp
new file mode 100644
index 0000000000000..9327a6a3befd7
--- /dev/null
+++ b/clang/unittests/ASTMatchers/AbslMatchersTest.cpp
@@ -0,0 +1,59 @@
+//===- unittests/ASTMatchers/GTestMatchersTest.cpp - GTest matcher unit tests //
+//
+// 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/ASTMatchers/AbslMatchers.h"
+#include "ASTMatchersTest.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang {
+namespace ast_matchers {
+namespace absl_matchers {
+
+constexpr llvm::StringLiteral abslMockDecls = R"cc(
+namespace absl {
+template <typename T> class StatusOr {};
+class Status {};
+}
+
+)cc";
+
+static auto wrapAbsl(llvm::StringRef Input) { return abslMockDecls + Input; }
+
+TEST(AbslMatchersTest, StatusOrDecl) {
+ DeclarationMatcher StatusOrDecl =
+ varDecl(hasType(qualType(hasDeclaration(statusOrClass()))));
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ wrapAbsl("void Test() { absl::StatusOr<int> X; }"), StatusOrDecl,
+ std::make_unique<VerifyIdIsBoundTo<Type>>("StatusOrValueType")));
+ EXPECT_TRUE(notMatches(wrapAbsl("void Test() { int X; }"), StatusOrDecl));
+ EXPECT_TRUE(notMatches(wrapAbsl(R"cc(
+ namespace foo { namespace absl {
+ template <typename T> class StatusOr {};
+ }}
+ void Test() { foo::absl::StatusOr<int> X; }
+ )cc"),
+ StatusOrDecl));
+}
+
+TEST(AbslMatchersTest, StatusDecl) {
+ DeclarationMatcher StatusDecl =
+ varDecl(hasType(recordType(hasDeclaration(statusClass()))));
+ EXPECT_TRUE(matches(wrapAbsl("void Test() { absl::Status X; }"), StatusDecl));
+ EXPECT_TRUE(notMatches(wrapAbsl("void Test() { int X; }"), StatusDecl));
+ EXPECT_TRUE(notMatches(wrapAbsl(R"cc(
+ namespace foo { namespace absl {
+ class Status {};
+ }}
+ void Test() { foo::absl::Status X; }
+ )cc"),
+ StatusDecl));
+}
+
+} // end namespace absl_matchers
+} // end namespace ast_matchers
+} // end namespace clang
diff --git a/clang/unittests/ASTMatchers/CMakeLists.txt b/clang/unittests/ASTMatchers/CMakeLists.txt
index 47bd5c108bb5a..8ecf9dd0445eb 100644
--- a/clang/unittests/ASTMatchers/CMakeLists.txt
+++ b/clang/unittests/ASTMatchers/CMakeLists.txt
@@ -3,6 +3,7 @@ add_clang_unittest(ASTMatchersTests
ASTMatchersNodeTest.cpp
ASTMatchersNarrowingTest.cpp
ASTMatchersTraversalTest.cpp
+ AbslMatchersTest.cpp
GtestMatchersTest.cpp
CLANG_LIBS
clangAST
|
@BaLiKfromUA CC |
I think that, rather than referencing an existing downstream version, you might want to motivate as a generaly feature. For that, what is the downstream version using them, etc? IIUC, they support writing extensions to the upstream model, which could equally well apply in other upstream uses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about location. Otherwise, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be interested in other clang member opinions as to whether this is the right location for these matchers. As much as they are general, their intended use/need is still the StatusOr model, so co-locating them with the model code might be more appropriate. I suspect AST matchers maintainers are not looking to add matcher support for a wide variety of (common?) library types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW there is also a file for Gtest (next to this one). But I have no real opinion about the namespace, and am happy to just colocate it with the model.
This is part of the upstreaming of abseil-unchecked-statusor-use.
See https://discourse.llvm.org/t/rfc-abseil-unchecked-statusor-use-check/87998
These matchers are used in the upcoming model, but also in a downstream
version which extends the model to support some internal constructs.
This is why they cannot be internal to the model.