Simplify tests, drop analyze_target_for_test
analyze_target_for_test forces the AST-consuming code into a callback, and doesn't simplify much.
infer_nullability_constraints_test: use clang::TestAST and findValueDecl directly
Also fix use-after-free: pointers returned by inferAnnotationsForTargetFunction dangled.
while here, Target => target.
safety_constraint_generator_test: use dataflow::test::checkDataflow.
Avoid difficult-to-debug matchers-containing-callbacks by expressing the constraints as strings, assigning names to the variables we care about.
While here, Target => target.
safety_constraint_generator: pass Lattice/Env separately instead of in a pair type, to avoid ugly conversions (and copies!) between different pair types in tests
PiperOrigin-RevId: 544127985
diff --git a/nullability/inference/BUILD b/nullability/inference/BUILD
index 5071479..74f274c 100644
--- a/nullability/inference/BUILD
+++ b/nullability/inference/BUILD
@@ -28,14 +28,15 @@
name = "infer_nullability_constraints_test",
srcs = ["infer_nullability_constraints_test.cc"],
deps = [
- ":analyze_target_for_test",
":infer_nullability_constraints",
":inference_cc_proto",
"//nullability:proto_matchers",
"@absl//absl/log:check",
"@llvm-project//clang:analysis",
"@llvm-project//clang:ast",
- "@llvm-project//clang:ast_matchers",
+ "@llvm-project//clang:basic",
+ "@llvm-project//clang:testing",
+ "@llvm-project//clang/unittests:dataflow_testing_support",
"@llvm-project//llvm:Support",
"@llvm-project//third-party/unittest:gmock",
"@llvm-project//third-party/unittest:gtest",
@@ -70,20 +71,6 @@
],
)
-cc_library(
- name = "analyze_target_for_test",
- testonly = True,
- srcs = ["analyze_target_for_test.cc"],
- hdrs = ["analyze_target_for_test.h"],
- deps = [
- "@absl//absl/log:check",
- "@llvm-project//clang:ast",
- "@llvm-project//clang:ast_matchers",
- "@llvm-project//clang:testing",
- "@llvm-project//llvm:Support",
- ],
-)
-
cc_test(
name = "resolve_constraints_test",
srcs = ["resolve_constraints_test.cc"],
@@ -105,7 +92,6 @@
name = "safety_constraint_generator_test",
srcs = ["safety_constraint_generator_test.cc"],
deps = [
- ":analyze_target_for_test",
":safety_constraint_generator",
"//nullability:pointer_nullability",
"//nullability:pointer_nullability_analysis",
@@ -115,6 +101,8 @@
"@llvm-project//clang:ast",
"@llvm-project//clang:ast_matchers",
"@llvm-project//clang:basic",
+ "@llvm-project//clang:testing",
+ "@llvm-project//clang/unittests:dataflow_testing_support",
"@llvm-project//llvm:Support",
"@llvm-project//third-party/unittest:gmock",
"@llvm-project//third-party/unittest:gtest",
diff --git a/nullability/inference/analyze_target_for_test.cc b/nullability/inference/analyze_target_for_test.cc
deleted file mode 100644
index ecfff1b..0000000
--- a/nullability/inference/analyze_target_for_test.cc
+++ /dev/null
@@ -1,73 +0,0 @@
-// Part of the Crubit project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#include "nullability/inference/analyze_target_for_test.h"
-
-#include <functional>
-#include <utility>
-
-#include "absl/log/check.h"
-#include "clang/AST/Decl.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/ASTMatchers/ASTMatchers.h"
-#include "clang/Testing/TestAST.h"
-#include "llvm/ADT/STLFunctionalExtras.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace clang::tidy::nullability {
-namespace {
-using ::clang::ast_matchers::compoundStmt;
-using ::clang::ast_matchers::functionDecl;
-using ::clang::ast_matchers::hasBody;
-using ::clang::ast_matchers::hasName;
-using ::clang::ast_matchers::MatchFinder;
-
-// Simplifies creation of MatchCallbacks.
-class CallbackAdapter : public clang::ast_matchers::MatchFinder::MatchCallback {
- public:
- explicit CallbackAdapter(
- std::function<void(const clang::ast_matchers::MatchFinder::MatchResult &)>
- Callback)
- : StoredCallback(std::move(Callback)) {
- CHECK(StoredCallback);
- }
-
- void run(
- const clang::ast_matchers::MatchFinder::MatchResult &result) override {
- StoredCallback(result);
- };
-
- private:
- std::function<void(const clang::ast_matchers::MatchFinder::MatchResult &)>
- StoredCallback;
-};
-} // namespace
-
-void analyzeTargetForTest(
- llvm::StringRef Source,
- llvm::function_ref<void(const clang::FunctionDecl &,
- const MatchFinder::MatchResult &)>
- AnalysisCallback) {
- int MatchesForFunctionDeclarationsNamedTarget = 0;
- CallbackAdapter adapter([&, AnalysisCallback](
- const MatchFinder::MatchResult &result) mutable {
- ++MatchesForFunctionDeclarationsNamedTarget;
- const auto *MaybeFunc = result.Nodes.getNodeAs<clang::FunctionDecl>("func");
- CHECK(MaybeFunc);
- AnalysisCallback(*MaybeFunc, result);
- });
- MatchFinder Finder;
- Finder.addMatcher(
- functionDecl(hasName("Target"), hasBody(compoundStmt())).bind("func"),
- &adapter);
- clang::TestInputs Inputs(Source);
- Inputs.FileName = "main.cc";
- clang::TestAST Ast(Inputs);
- Finder.matchAST(Ast.context());
- QCHECK_EQ(MatchesForFunctionDeclarationsNamedTarget, 1)
- << "Source must match exactly 1 function named 'Target' that has a "
- "definition";
-}
-
-} // namespace clang::tidy::nullability
diff --git a/nullability/inference/analyze_target_for_test.h b/nullability/inference/analyze_target_for_test.h
deleted file mode 100644
index 8c2de84..0000000
--- a/nullability/inference/analyze_target_for_test.h
+++ /dev/null
@@ -1,24 +0,0 @@
-// Part of the Crubit project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#ifndef CRUBIT_NULLABILITY_INFERENCE_ANALYZE_TARGET_FOR_TEST_H_
-#define CRUBIT_NULLABILITY_INFERENCE_ANALYZE_TARGET_FOR_TEST_H_
-
-#include "clang/AST/Decl.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "llvm/ADT/STLFunctionalExtras.h"
-#include "llvm/ADT/StringRef.h"
-
-namespace clang::tidy::nullability {
-
-void analyzeTargetForTest(
- llvm::StringRef Source,
- llvm::function_ref<
- void(const clang::FunctionDecl &,
- const clang::ast_matchers::MatchFinder::MatchResult &)>
- AnalysisCallback);
-
-} // namespace clang::tidy::nullability
-
-#endif // CRUBIT_NULLABILITY_INFERENCE_ANALYZE_TARGET_FOR_TEST_H_
diff --git a/nullability/inference/infer_nullability_constraints.cc b/nullability/inference/infer_nullability_constraints.cc
index 9d8c0c1..afc368c 100644
--- a/nullability/inference/infer_nullability_constraints.cc
+++ b/nullability/inference/infer_nullability_constraints.cc
@@ -74,7 +74,7 @@
return false;
}
-llvm::Expected<llvm::DenseMap<const clang::Decl *, NullabilityConstraint>>
+llvm::Expected<llvm::DenseMap<const clang::NamedDecl *, NullabilityConstraint>>
inferNullabilityConstraints(const clang::FunctionDecl &Func,
clang::ASTContext &Context) {
// We want to make sure we use the declaration that the body comes from,
@@ -87,7 +87,7 @@
}
CHECK(DeclWithBody);
- llvm::DenseMap<const clang::Decl *, NullabilityConstraint> Results;
+ llvm::DenseMap<const clang::NamedDecl *, NullabilityConstraint> Results;
llvm::Expected<clang::dataflow::ControlFlowContext> ControlFlowContext =
clang::dataflow::ControlFlowContext::build(*DeclWithBody);
if (!ControlFlowContext) return Results;
@@ -106,8 +106,8 @@
const clang::CFGElement &Element,
const clang::dataflow::DataflowAnalysisState<
PointerNullabilityLattice> &State) {
- SafetyConstraintGenerator.collectConstraints(Element, State,
- Context);
+ SafetyConstraintGenerator.collectConstraints(Element, State.Lattice,
+ State.Env, Context);
});
if (!BlockToOutputStateOrError) {
return Results;
diff --git a/nullability/inference/infer_nullability_constraints.h b/nullability/inference/infer_nullability_constraints.h
index 64b9d0e..7556215 100644
--- a/nullability/inference/infer_nullability_constraints.h
+++ b/nullability/inference/infer_nullability_constraints.h
@@ -17,7 +17,7 @@
// Collects constraints on nullability annotations that could be added to the
// types of Func's parameters based on the function's behavior and our
// definition of null-safety.
-llvm::Expected<llvm::DenseMap<const clang::Decl *, NullabilityConstraint>>
+llvm::Expected<llvm::DenseMap<const clang::NamedDecl *, NullabilityConstraint>>
inferNullabilityConstraints(const clang::FunctionDecl &Func,
clang::ASTContext &Context);
diff --git a/nullability/inference/infer_nullability_constraints_test.cc b/nullability/inference/infer_nullability_constraints_test.cc
index ce0543b..70b4090 100644
--- a/nullability/inference/infer_nullability_constraints_test.cc
+++ b/nullability/inference/infer_nullability_constraints_test.cc
@@ -6,149 +6,131 @@
#include <string>
#include <utility>
+#include <vector>
#include "absl/log/check.h"
-#include "nullability/inference/analyze_target_for_test.h"
#include "nullability/inference/inference.proto.h"
#include "nullability/proto_matchers.h"
#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/CFG.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Testing/TestAST.h"
+#include "third_party/llvm/llvm-project/clang/unittests/Analysis/FlowSensitive/TestingSupport.h"
#include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/STLFunctionalExtras.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
#include "third_party/llvm/llvm-project/third-party/unittest/googlemock/include/gmock/gmock.h"
#include "third_party/llvm/llvm-project/third-party/unittest/googletest/include/gtest/gtest.h"
namespace clang::tidy::nullability {
namespace {
-using ::clang::ast_matchers::MatchFinder;
using ::testing::IsEmpty;
using ::testing::Pair;
using ::testing::UnorderedElementsAre;
using InferredNullabilityConstraints =
- llvm::DenseMap<const clang::Decl *, NullabilityConstraint>;
+ std::vector<std::pair<std::string, NullabilityConstraint>>;
InferredNullabilityConstraints inferAnnotationsForTargetFunction(
llvm::StringRef Source) {
- InferredNullabilityConstraints Constraints;
- analyzeTargetForTest(
- Source, [&Constraints](const clang::FunctionDecl &Func,
- const MatchFinder::MatchResult &Result) mutable {
- auto Results = inferNullabilityConstraints(Func, *Result.Context);
- CHECK(Results);
- Constraints = std::move(*Results);
- });
- return Constraints;
-}
-
-MATCHER_P(Parameter, Name, std::string("is a parameter named ") + Name) {
- const auto *Param = clang::dyn_cast_or_null<clang::ParmVarDecl>(arg);
- if (!Param) {
- return false;
- }
-
- const auto *Identifier = Param->getIdentifier();
- if (!Identifier) {
- return false;
- }
-
- return testing::ExplainMatchResult(Name, Identifier->getName(),
- result_listener);
+ clang::TestAST AST(Source);
+ auto DeclResults = inferNullabilityConstraints(
+ cast<FunctionDecl>(
+ *dataflow::test::findValueDecl(AST.context(), "target")),
+ AST.context());
+ CHECK(DeclResults) << toString(DeclResults.takeError());
+ InferredNullabilityConstraints StringResults;
+ for (const auto &Entry : *DeclResults)
+ StringResults.emplace_back(Entry.first->getName(), Entry.second);
+ return StringResults;
}
TEST(InferAnnotationsTest, NoParams) {
static constexpr llvm::StringRef Src = R"cc(
- void Target() {}
+ void target() {}
)cc";
EXPECT_THAT(inferAnnotationsForTargetFunction(Src), IsEmpty());
}
TEST(InferAnnotationsTest, OneParamUnused) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0) {}
+ void target(int *p0) {}
)cc";
EXPECT_THAT(inferAnnotationsForTargetFunction(Src), IsEmpty());
}
TEST(InferAnnotationsTest, OneParamUsedWithoutRestriction) {
static constexpr llvm::StringRef Src = R"cc(
- void TakesUnknown(int *unknown) {}
+ void takesUnknown(int *unknown) {}
- void Target(int *p0) { TakesUnknown(p0); }
+ void target(int *p0) { takesUnknown(p0); }
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: false)pb"))));
}
TEST(InferAnnotationsTest, Deref) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0, int *p1) {
+ void target(int *p0, int *p1) {
int a = *p0;
if (p1 != nullptr) {
int b = *p1;
}
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: true)pb")),
- Pair(Parameter("p1"), EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: true)pb")),
+ Pair("p1", EqualsProto(R"pb(must_be_nonnull: false)pb"))));
}
TEST(InferAnnotationsTest, DereferenceBeforeAssignment) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p) {
+ void target(int *p) {
*p;
int i = 1;
p = &i;
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p"), EqualsProto(R"pb(must_be_nonnull: true)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(Pair("p", EqualsProto(R"pb(must_be_nonnull:
+ true)pb"))));
}
TEST(InferAnnotationsTest, DereferenceAfterAssignment) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p) {
+ void target(int *p) {
int i = 1;
p = &i;
*p;
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p"), EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p", EqualsProto(R"pb(must_be_nonnull: false)pb"))));
}
TEST(InferAnnotationsTest, DerefOfPtrRef) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *&p0, int *&p1) {
+ void target(int *&p0, int *&p1) {
int a = *p0;
if (p1 != nullptr) {
int b = *p1;
}
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: true)pb")),
- Pair(Parameter("p1"), EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: true)pb")),
+ Pair("p1", EqualsProto(R"pb(must_be_nonnull: false)pb"))));
}
TEST(InferAnnotationsTest, UnrelatedCondition) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0, int *p1, int *p2, bool b) {
+ void target(int *p0, int *p1, int *p2, bool b) {
if (b) {
int a = *p0;
int b = *p1;
@@ -158,17 +140,16 @@
}
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: true)pb")),
- Pair(Parameter("p1"), EqualsProto(R"pb(must_be_nonnull: true)pb")),
- Pair(Parameter("p2"), EqualsProto(R"pb(must_be_nonnull: true)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: true)pb")),
+ Pair("p1", EqualsProto(R"pb(must_be_nonnull: true)pb")),
+ Pair("p2", EqualsProto(R"pb(must_be_nonnull: true)pb"))));
}
TEST(InferAnnotationsTest, LaterDeref) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0) {
+ void target(int *p0) {
if (p0 == nullptr) {
(void)0;
} else {
@@ -177,45 +158,42 @@
int a = *p0;
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: true)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: true)pb"))));
}
TEST(InferAnnotationsTest, DerefBeforeGuardedDeref) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0) {
+ void target(int *p0) {
int a = *p0;
if (p0 != nullptr) {
int b = *p0;
}
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: true)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: true)pb"))));
}
TEST(InferAnnotationsTest, EarlyReturn) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0) {
+ void target(int *p0) {
if (!p0) {
return;
}
int a = *p0;
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: false)pb"))));
}
TEST(InferAnnotationsTest, UnreachableCode) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p0, int *p1, int *p2, int *p3) {
+ void target(int *p0, int *p1, int *p2, int *p3) {
if (true) {
int a = *p0;
} else {
@@ -230,30 +208,28 @@
int a = *p3;
}
)cc";
- EXPECT_THAT(
- inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("p0"), EqualsProto(R"pb(must_be_nonnull: true)pb"))));
+ EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("p0", EqualsProto(R"pb(must_be_nonnull: true)pb"))));
}
TEST(InferAnnotationsTest,
RequiresNonNullWhenAnnotatedWithClangNullabilityAttributeAtDefinition) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *unannotated, int *_Nonnull non_null,
+ void target(int *unannotated, int *_Nonnull non_null,
int *_Nonnull *only_inner_layer_nonnull) {
unannotated;
non_null;
only_inner_layer_nonnull;
}
)cc";
- EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("unannotated"),
- EqualsProto(R"pb(must_be_nonnull: false)pb")),
- Pair(Parameter("non_null"), EqualsProto(R"pb(must_be_nonnull:
- true)pb")),
- Pair(Parameter("only_inner_layer_nonnull"),
- EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(
+ inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("unannotated", EqualsProto(R"pb(must_be_nonnull: false)pb")),
+ Pair("non_null", EqualsProto(R"pb(must_be_nonnull: true)pb")),
+ Pair("only_inner_layer_nonnull", EqualsProto(R"pb(must_be_nonnull:
+ false)pb"))));
}
TEST(InferAnnotationsTest,
@@ -264,21 +240,20 @@
using NonNull [[clang::annotate("Nonnull")]] = T;
} // namespace custom
- void Target(int *unannotated, custom::NonNull<int *> non_null,
+ void target(int *unannotated, custom::NonNull<int *> non_null,
custom::NonNull<int *> *only_inner_layer_nonnull) {
unannotated;
non_null;
only_inner_layer_nonnull;
}
)cc";
- EXPECT_THAT(inferAnnotationsForTargetFunction(Src),
- UnorderedElementsAre(
- Pair(Parameter("unannotated"),
- EqualsProto(R"pb(must_be_nonnull: false)pb")),
- Pair(Parameter("non_null"), EqualsProto(R"pb(must_be_nonnull:
- true)pb")),
- Pair(Parameter("only_inner_layer_nonnull"),
- EqualsProto(R"pb(must_be_nonnull: false)pb"))));
+ EXPECT_THAT(
+ inferAnnotationsForTargetFunction(Src),
+ UnorderedElementsAre(
+ Pair("unannotated", EqualsProto(R"pb(must_be_nonnull: false)pb")),
+ Pair("non_null", EqualsProto(R"pb(must_be_nonnull: true)pb")),
+ Pair("only_inner_layer_nonnull", EqualsProto(R"pb(must_be_nonnull:
+ false)pb"))));
}
} // namespace
diff --git a/nullability/inference/safety_constraint_generator.cc b/nullability/inference/safety_constraint_generator.cc
index 231f5a0..e87d6d2 100644
--- a/nullability/inference/safety_constraint_generator.cc
+++ b/nullability/inference/safety_constraint_generator.cc
@@ -11,7 +11,7 @@
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
-#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
#include "clang/Analysis/FlowSensitive/Value.h"
#include "llvm/ADT/DenseSet.h"
@@ -65,11 +65,10 @@
: ConstraintCollector(buildConstraintCollector()) {}
void SafetyConstraintGenerator::collectConstraints(
- const clang::CFGElement &Element,
- const clang::dataflow::DataflowAnalysisState<LatticeType> &State,
- clang::ASTContext &Context) {
+ const clang::CFGElement &Element, const LatticeType &Lattice,
+ const clang::dataflow::Environment &Env, clang::ASTContext &Context) {
if (auto *Constraint =
- ConstraintCollector(Element, Context, {State.Lattice, State.Env})) {
+ ConstraintCollector(Element, Context, {Lattice, Env})) {
Constraints.insert(Constraint);
}
}
diff --git a/nullability/inference/safety_constraint_generator.h b/nullability/inference/safety_constraint_generator.h
index a866c38..2789b2f 100644
--- a/nullability/inference/safety_constraint_generator.h
+++ b/nullability/inference/safety_constraint_generator.h
@@ -38,10 +38,10 @@
// Intended for use as a PostVisitCFG after running
// PointerNullabilityAnalysis. Assumes that `State` includes pointer
// nullability state as set by PointerNullabilityAnalysis.
- void collectConstraints(
- const clang::CFGElement &Element,
- const clang::dataflow::DataflowAnalysisState<LatticeType> &State,
- clang::ASTContext &Context);
+ void collectConstraints(const clang::CFGElement &Element,
+ const LatticeType &Lattice,
+ const dataflow::Environment &Env,
+ clang::ASTContext &Context);
// Retrieves constraints gathered thus far. Until all analyzed CFGElements
// have been processed by `collectConstraints`, the return value will not
diff --git a/nullability/inference/safety_constraint_generator_test.cc b/nullability/inference/safety_constraint_generator_test.cc
index 4011296..11ec6af 100644
--- a/nullability/inference/safety_constraint_generator_test.cc
+++ b/nullability/inference/safety_constraint_generator_test.cc
@@ -6,23 +6,24 @@
#include <memory>
#include <optional>
+#include <string>
+#include <utility>
#include <vector>
#include "absl/log/check.h"
-#include "nullability/inference/analyze_target_for_test.h"
#include "nullability/pointer_nullability.h"
#include "nullability/pointer_nullability_analysis.h"
-#include "nullability/pointer_nullability_lattice.h"
#include "clang/AST/Decl.h"
-#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Analysis/CFG.h"
-#include "clang/Analysis/FlowSensitive/ControlFlowContext.h"
-#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DebugSupport.h"
#include "clang/Analysis/FlowSensitive/Value.h"
-#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
#include "clang/Basic/LLVM.h"
+#include "third_party/llvm/llvm-project/clang/unittests/Analysis/FlowSensitive/TestingSupport.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "third_party/llvm/llvm-project/third-party/unittest/googlemock/include/gmock/gmock.h"
@@ -31,149 +32,115 @@
namespace clang::tidy::nullability {
namespace {
-using ::testing::AllOf;
+using ::std::replace;
+using ::testing::ElementsAre;
using ::testing::IsEmpty;
using ::testing::Not;
-using ::testing::SizeIs;
using ::testing::UnorderedElementsAre;
-// Analyzes the to-be-matched code snippet with PointerNullabilityAnalysis, and
-// then matches against the constraints produced by SafetyConstraintGenerator
-// for the snippet.
-//
-// `ConstraintsMatcherProducer` should be a function taking the Environment
-// and a vector of PointerValue* representing the Target function's parameters
-// and returning a matcher for the generated constraints. This allows for
-// matching of the constraints against individual parameter null state
-// properties.
-MATCHER_P(ProducesSafetyConstraints, ConstraintsMatcherProducer, "") {
- bool Success = false;
- auto MatcherProducer = ConstraintsMatcherProducer;
- analyzeTargetForTest(
- arg, [&Success, &MatcherProducer, &result_listener](
- const clang::FunctionDecl& Func,
- const clang::ast_matchers::MatchFinder::MatchResult& Result) {
- QCHECK(Func.hasBody()) << "Matched function has no body.";
- QCHECK(Result.Context) << "Missing ASTContext from match result.";
- llvm::Expected<clang::dataflow::ControlFlowContext> ControlFlowContext =
- clang::dataflow::ControlFlowContext::build(Func);
- clang::dataflow::DataflowAnalysisContext AnalysisContext(
- std::make_unique<clang::dataflow::WatchedLiteralsSolver>());
- clang::dataflow::Environment Environment(AnalysisContext, Func);
- PointerNullabilityAnalysis Analysis(*Result.Context);
- SafetyConstraintGenerator Generator;
- llvm::Expected<std::vector<std::optional<
- clang::dataflow::DataflowAnalysisState<PointerNullabilityLattice>>>>
- BlockToOutputStateOrError = clang::dataflow::runDataflowAnalysis(
- *ControlFlowContext, Analysis, Environment,
- [&Generator, &Result](
- const clang::CFGElement& Element,
- const clang::dataflow::DataflowAnalysisState<
- PointerNullabilityLattice>& State) {
- Generator.collectConstraints(Element, State, *Result.Context);
- });
+// Returns names for nullability atoms of a function's params.
+// Given a function `void foo(int *x)`, returns: {
+// Env.getPointerNullState(x).first => "x.is_known",
+// Env.getPointerNullState(x).second => "x.is_null",
+// }
+llvm::DenseMap<const dataflow::AtomicBoolValue *, std::string>
+getNullabilityVariableNames(const FunctionDecl &Func,
+ const dataflow::Environment &Env) {
+ llvm::DenseMap<const dataflow::AtomicBoolValue *, std::string> Result;
+ for (unsigned I = 0; I < Func.param_size(); ++I) {
+ auto &Param = *Func.getParamDecl(I);
+ if (auto *Val = dyn_cast_or_null<clang::dataflow::PointerValue>(
+ Env.getValue(Param));
+ Val && hasPointerNullState(*Val)) {
+ std::string Name = Param.getName().str();
- QCHECK(BlockToOutputStateOrError)
- << "No output state from dataflow analysis.";
+ auto [Known, Null] = getPointerNullState(*Val);
+ Result[&Known] = Name + ".is_known";
+ Result[&Null] = Name + ".is_null";
+ }
+ }
+ return Result;
+}
- // TODO(b/268440048) When we can retrieve the improved atoms
- // representing annotations, stop using the Environment to retrieve the
- // initial Environment PointerValues, which won't work for local
- // variables down the road.
- std::vector<clang::dataflow::PointerValue*> ParamDeclPointerValues;
- for (const auto* P : Func.parameters()) {
- CHECK(P != nullptr);
- auto* Val = clang::dyn_cast_or_null<clang::dataflow::PointerValue>(
- Environment.getValue(*P));
- if (Val) {
- ParamDeclPointerValues.push_back(Val);
- }
+// Analyzes the "target" function in the code, and returns safety constraints.
+// These are expressed as strings (AtomicBoolValue* won't live long enough).
+std::vector<std::string> getSafetyConstraints(llvm::StringRef Code) {
+ using namespace ast_matchers;
+ std::vector<std::string> Result;
+ SafetyConstraintGenerator Generator;
+ auto Inputs =
+ dataflow::test::AnalysisInputs<PointerNullabilityAnalysis>(
+ Code,
+ functionDecl(hasName("target"), hasBody(compoundStmt()))
+ .bind("target"),
+ [&](ASTContext &Ctx, const dataflow::Environment &E) {
+ return PointerNullabilityAnalysis(Ctx);
+ })
+ .withPostVisitCFG([&](ASTContext &AST, const CFGElement &Elt,
+ auto &&State) {
+ Generator.collectConstraints(Elt, State.Lattice, State.Env, AST);
+ });
+ auto Err = dataflow::test::checkDataflow(
+ std::move(Inputs), [&](const dataflow::test::AnalysisOutputs &Out) {
+ auto Names = getNullabilityVariableNames(*Out.Target, Out.InitEnv);
+ for (const auto *Constraint : Generator.constraints()) {
+ Result.push_back(dataflow::debugString(*Constraint, Names));
+ // Debug representation is ugly, drop newlines and excess spaces.
+ replace(Result.back().begin(), Result.back().end(), '\n', ' ');
+ llvm::erase_if(Result.back(),
+ [](char &C) { return C == ' ' && *(&C + 1) == ' '; });
}
- Success = ExplainMatchResult(
- MatcherProducer(Environment, ParamDeclPointerValues),
- Generator.constraints(), result_listener);
});
- return Success;
+ CHECK(!Err) << toString(std::move(Err));
+ return Result;
}
TEST(SafetyConstraintGenerator, GeneratesNoConstraintsForEmptyFunctionDefn) {
- static constexpr llvm::StringRef Src = R"cc(
- void Target() {}
- )cc";
- EXPECT_THAT(Src, ProducesSafetyConstraints(
- [](const dataflow::Environment& Environment,
- auto ParamPointerValues) { return IsEmpty(); }));
+ EXPECT_THAT(getSafetyConstraints("void target() {}"), IsEmpty());
}
TEST(SafetyConstraintGenerator, GeneratesNoConstraintsForUnusedParam) {
- static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p) {}
- )cc";
- EXPECT_THAT(Src, ProducesSafetyConstraints(
- [](const dataflow::Environment& Environment,
- auto ParamPointerValues) { return IsEmpty(); }));
+ EXPECT_THAT(getSafetyConstraints("void target(int *p) {}"), IsEmpty());
}
TEST(SafetyConstraintGenerator, GeneratesNotIsNullConstraintForDeref) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p) { *p; }
+ void target(int *p) { *p; }
)cc";
- EXPECT_THAT(
- Src,
- ProducesSafetyConstraints([](const dataflow::Environment& Environment,
- auto ParamPointerValues) {
- return UnorderedElementsAre(&Environment.makeNot(
- getPointerNullState(*ParamPointerValues[0]).second));
- }));
+ EXPECT_THAT(getSafetyConstraints(Src), ElementsAre("(not p.is_null)"));
}
TEST(SafetyConstraintGenerator,
GeneratesNotIsNullConstraintForImproperlyGuardedDeref) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p) {
+ void target(int *p) {
if (p == nullptr) *p;
}
)cc";
- EXPECT_THAT(
- Src,
- ProducesSafetyConstraints([](const dataflow::Environment& Environment,
- auto ParamPointerValues) {
- return UnorderedElementsAre(&Environment.makeNot(
- getPointerNullState(*ParamPointerValues[0]).second));
- }));
+ EXPECT_THAT(getSafetyConstraints(Src), ElementsAre("(not p.is_null)"));
}
TEST(SafetyConstraintGenerator, GeneratesConstraintsForAllParams) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p, int *q, int *r) {
+ void target(int *p, int *q, int *r) {
*p;
*q;
*r;
}
)cc";
- EXPECT_THAT(Src, ProducesSafetyConstraints([](const dataflow::Environment&
- Environment,
- auto ParamPointerValues) {
- return UnorderedElementsAre(
- &Environment.makeNot(
- getPointerNullState(*ParamPointerValues[0]).second),
- &Environment.makeNot(
- getPointerNullState(*ParamPointerValues[1]).second),
- &Environment.makeNot(
- getPointerNullState(*ParamPointerValues[2]).second));
- }));
+ EXPECT_THAT(getSafetyConstraints(Src),
+ UnorderedElementsAre("(not p.is_null)", "(not q.is_null)",
+ "(not r.is_null)"));
}
TEST(SafetyConstraintGenerator, DoesntGenerateConstraintForNullCheckedPtr) {
static constexpr llvm::StringRef Src = R"cc(
- void Target(int *p) {
+ void target(int *p) {
if (p) *p;
if (p != nullptr) *p;
}
)cc";
- EXPECT_THAT(Src, ProducesSafetyConstraints(
- [](const dataflow::Environment& Environment,
- auto ParamPointerValues) { return IsEmpty(); }));
+ EXPECT_THAT(getSafetyConstraints(Src), IsEmpty());
}
TEST(SafetyConstraintGenerator,
@@ -181,18 +148,12 @@
static constexpr llvm::StringRef Src = R"cc(
int *getPtr();
- void Target(int *p) {
+ void target(int *p) {
*p;
p = getPtr();
}
)cc";
- EXPECT_THAT(
- Src,
- ProducesSafetyConstraints([](const dataflow::Environment& Environment,
- auto ParamPointerValues) {
- return UnorderedElementsAre(&Environment.makeNot(
- getPointerNullState(*ParamPointerValues[0]).second));
- }));
+ EXPECT_THAT(getSafetyConstraints(Src), ElementsAre("(not p.is_null)"));
}
TEST(SafetyConstraintGenerator,
@@ -200,21 +161,15 @@
static constexpr llvm::StringRef Src = R"cc(
int *getPtr();
- void Target(int *p) {
+ void target(int *p) {
p = getPtr();
*p;
}
)cc";
- EXPECT_THAT(
- Src,
- ProducesSafetyConstraints([](const dataflow::Environment& Environment,
- auto ParamPointerValues) {
- return AllOf(SizeIs(1),
- // TODO(b/268440048) Figure out how to access and assert
- // equality for the constraint that this is.
- Not(UnorderedElementsAre(&Environment.makeNot(
- getPointerNullState(*ParamPointerValues[0]).second))));
- }));
+ // TODO(b/268440048) Figure out how to access and assert
+ // equality for the constraint that this is.
+ // (We require the value that models the getPtr() result to be non-null)
+ EXPECT_THAT(getSafetyConstraints(Src), ElementsAre(Not("(not p.is_null)")));
}
} // namespace
} // namespace clang::tidy::nullability
