[FlowSensitive] [StatusOr] [2/N] Add minimal model #162932
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) ChangesThis model implements a dataflow analysis for reporting instances of This is an example of code that will be flagged by the analysis: This is an example of code that will not be flagged by the analysis: This model has successfully been used by Google for some time now. This is the initial commit that adds the simplest possible model, that The test setup is notable in that it has an extra indirection. This is RFC: https://discourse.llvm.org/t/rfc-abseil-unchecked-statusor-use-check/87998 Patch is 180.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/162932.diff 9 Files Affected:
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
new file mode 100644
index 0000000000000..ed82907f4867d
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h
@@ -0,0 +1,114 @@
+//===- UncheckedStatusOrAccessModel.h -------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDSTATUSORACCESSMODEL_H
+#define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDSTATUSORACCESSMODEL_H
+
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchers.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/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang::dataflow::statusor_model {
+
+// The helper functions exported here are for use of downstream vendor
+// extensions of this model.
+
+// Match declation of `absl::StatusOr<T>` and bind `T` to "T".
+clang::ast_matchers::DeclarationMatcher statusOrClass();
+// Match declation of `absl::Status`.
+clang::ast_matchers::DeclarationMatcher statusClass();
+// Match declaration of `absl::internal_statusor::OperatorBase`.
+clang::ast_matchers::DeclarationMatcher statusOrOperatorBaseClass();
+clang::ast_matchers::TypeMatcher possiblyAliasedStatusType();
+clang::ast_matchers::TypeMatcher possiblyAliasedStatusOrType();
+clang::ast_matchers::TypeMatcher statusOrType();
+
+// Get RecordStorageLocation for the `Status` contained in the `StatusOr`
+RecordStorageLocation &locForStatus(RecordStorageLocation &StatusOrLoc);
+// Get the StorageLocation OK boolean in the `Status`
+StorageLocation &locForOk(RecordStorageLocation &StatusLoc);
+// Get the OK boolean in the `Status`, and initialize it if necessary.
+BoolValue &valForOk(RecordStorageLocation &StatusLoc, Environment &Env);
+// Get synthetic fields for the types modelled by
+// `UncheckedStatusOrAccessModel`.
+llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType,
+ const CXXRecordDecl &RD);
+
+// Initialize the synthetic fields of the `StatusOr`.
+// N.B. if it is already initialized, the value gets reset.
+BoolValue &initializeStatusOr(RecordStorageLocation &StatusOrLoc,
+ Environment &Env);
+// Initialize the synthetic fields of the `Status`.
+// N.B. if it is already initialized, the value gets reset.
+BoolValue &initializeStatus(RecordStorageLocation &StatusLoc, Environment &Env);
+
+bool isRecordTypeWithName(QualType Type, llvm::StringRef TypeName);
+// Return true if `Type` is instantiation of `absl::StatusOr<T>`
+bool isStatusOrType(QualType Type);
+// Return true if `Type` is `absl::Status`
+bool isStatusType(QualType Type);
+
+// Get `QualType` for `absl::Status`, or default-constructed
+// QualType if it does not exist.
+QualType findStatusType(const ASTContext &Ctx);
+
+struct UncheckedStatusOrAccessModelOptions {};
+
+// Dataflow analysis that discovers unsafe uses of StatusOr values.
+class UncheckedStatusOrAccessModel
+ : public DataflowAnalysis<UncheckedStatusOrAccessModel, NoopLattice> {
+public:
+ explicit UncheckedStatusOrAccessModel(ASTContext &Ctx, Environment &Env);
+
+ static Lattice initialElement() { return {}; }
+
+ void transfer(const CFGElement &Elt, Lattice &L, Environment &Env);
+
+private:
+ CFGMatchSwitch<TransferState<Lattice>> TransferMatchSwitch;
+};
+
+using LatticeTransferState =
+ TransferState<UncheckedStatusOrAccessModel::Lattice>;
+
+// Extend the Builder with the transfer functions for
+// `UncheckedStatusOrAccessModel`. This is useful to write downstream models
+// that extend the model.
+CFGMatchSwitch<LatticeTransferState>
+buildTransferMatchSwitch(ASTContext &Ctx,
+ CFGMatchSwitchBuilder<LatticeTransferState> Builder);
+
+class UncheckedStatusOrAccessDiagnoser {
+public:
+ explicit UncheckedStatusOrAccessDiagnoser(
+ UncheckedStatusOrAccessModelOptions Options = {});
+
+ llvm::SmallVector<SourceLocation> operator()(
+ const CFGElement &Elt, ASTContext &Ctx,
+ const TransferStateForDiagnostics<UncheckedStatusOrAccessModel::Lattice>
+ &State);
+
+private:
+ CFGMatchSwitch<const Environment, llvm::SmallVector<SourceLocation>>
+ DiagnoseMatchSwitch;
+};
+
+} // namespace clang::dataflow::statusor_model
+
+#endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDSTATUSORACCESSMODEL_H
diff --git a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
index 89bbe8791eb2c..d1236f5714881 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
+++ b/clang/lib/Analysis/FlowSensitive/Models/CMakeLists.txt
@@ -1,6 +1,7 @@
add_clang_library(clangAnalysisFlowSensitiveModels
ChromiumCheckModel.cpp
UncheckedOptionalAccessModel.cpp
+ UncheckedStatusOrAccessModel.cpp
LINK_LIBS
clangAnalysis
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
new file mode 100644
index 0000000000000..eab1db8330d5e
--- /dev/null
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -0,0 +1,284 @@
+//===- UncheckedStatusOrAccessModel.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/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h"
+
+#include <cassert>
+#include <utility>
+
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/TypeBase.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/ASTMatchers/ASTMatchersInternal.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/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "llvm/ADT/StringMap.h"
+
+namespace clang::dataflow::statusor_model {
+namespace {
+
+using ::clang::ast_matchers::MatchFinder;
+using ::clang::ast_matchers::StatementMatcher;
+
+} // namespace
+
+static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ RecordStorageLocation *StatusOrLoc =
+ getImplicitObjectLocation(*Expr, State.Env);
+ if (StatusOrLoc == nullptr)
+ return;
+
+ auto &OkVal = valForOk(locForStatus(*StatusOrLoc), State.Env);
+ State.Env.setValue(*Expr, OkVal);
+}
+
+static bool isStatusOrOperatorBaseType(QualType type) {
+ return isRecordTypeWithName(type, "absl::internal_statusor::OperatorBase");
+}
+
+static bool isSafeUnwrap(RecordStorageLocation *StatusOrLoc,
+ const Environment &Env) {
+ if (!StatusOrLoc)
+ return false;
+ auto &StatusLoc = locForStatus(*StatusOrLoc);
+ auto *OkVal = Env.get<BoolValue>(locForOk(StatusLoc));
+ return OkVal != nullptr && Env.proves(OkVal->formula());
+}
+
+static ClassTemplateSpecializationDecl *
+getStatusOrBaseClass(const QualType &Ty) {
+ auto *RD = Ty->getAsCXXRecordDecl();
+ if (RD == nullptr)
+ return nullptr;
+ if (isStatusOrType(Ty) ||
+ // In case we are analyzing code under OperatorBase itself that uses
+ // operator* (e.g. to implement operator->).
+ isStatusOrOperatorBaseType(Ty))
+ return cast<ClassTemplateSpecializationDecl>(RD);
+ if (!RD->hasDefinition())
+ return nullptr;
+ for (const auto &Base : RD->bases())
+ if (auto *QT = getStatusOrBaseClass(Base.getType()))
+ return QT;
+ return nullptr;
+}
+
+static QualType getStatusOrValueType(ClassTemplateSpecializationDecl *TRD) {
+ return TRD->getTemplateArgs().get(0).getAsType();
+}
+
+static auto isStatusOrMemberCallWithName(llvm::StringRef member_name) {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxMemberCallExpr(
+ on(expr(unless(cxxThisExpr()))),
+ callee(cxxMethodDecl(
+ hasName(member_name),
+ ofClass(anyOf(statusOrClass(), statusOrOperatorBaseClass())))));
+}
+
+static auto isStatusOrOperatorCallWithName(llvm::StringRef operator_name) {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxOperatorCallExpr(
+ hasOverloadedOperatorName(operator_name),
+ callee(cxxMethodDecl(
+ ofClass(anyOf(statusOrClass(), statusOrOperatorBaseClass())))));
+}
+
+static auto valueCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return anyOf(isStatusOrMemberCallWithName("value"),
+ isStatusOrMemberCallWithName("ValueOrDie"));
+}
+
+static auto valueOperatorCall() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return expr(anyOf(isStatusOrOperatorCallWithName("*"),
+ isStatusOrOperatorCallWithName("->")));
+}
+
+static auto
+buildDiagnoseMatchSwitch(const UncheckedStatusOrAccessModelOptions &Options) {
+ return CFGMatchSwitchBuilder<const Environment,
+ llvm::SmallVector<SourceLocation>>()
+ // StatusOr::value, StatusOr::ValueOrDie
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ valueCall(),
+ [](const CXXMemberCallExpr *E,
+ const ast_matchers::MatchFinder::MatchResult &,
+ const Environment &Env) {
+ if (!isSafeUnwrap(getImplicitObjectLocation(*E, Env), Env))
+ return llvm::SmallVector<SourceLocation>({E->getExprLoc()});
+ return llvm::SmallVector<SourceLocation>();
+ })
+
+ // StatusOr::operator*, StatusOr::operator->
+ .CaseOfCFGStmt<CXXOperatorCallExpr>(
+ valueOperatorCall(),
+ [](const CXXOperatorCallExpr *E,
+ const ast_matchers::MatchFinder::MatchResult &,
+ const Environment &Env) {
+ RecordStorageLocation *StatusOrLoc =
+ Env.get<RecordStorageLocation>(*E->getArg(0));
+ if (!isSafeUnwrap(StatusOrLoc, Env))
+ return llvm::SmallVector<SourceLocation>({E->getOperatorLoc()});
+ return llvm::SmallVector<SourceLocation>();
+ })
+ .Build();
+}
+
+UncheckedStatusOrAccessDiagnoser::UncheckedStatusOrAccessDiagnoser(
+ UncheckedStatusOrAccessModelOptions Options)
+ : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {}
+
+llvm::SmallVector<SourceLocation> UncheckedStatusOrAccessDiagnoser::operator()(
+ const CFGElement &Elt, ASTContext &Ctx,
+ const TransferStateForDiagnostics<UncheckedStatusOrAccessModel::Lattice>
+ &State) {
+ return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
+}
+
+BoolValue &initializeStatus(RecordStorageLocation &StatusLoc,
+ Environment &Env) {
+ auto &OkVal = Env.makeAtomicBoolValue();
+ Env.setValue(locForOk(StatusLoc), OkVal);
+ return OkVal;
+}
+
+BoolValue &initializeStatusOr(RecordStorageLocation &StatusOrLoc,
+ Environment &Env) {
+ return initializeStatus(locForStatus(StatusOrLoc), Env);
+}
+
+clang::ast_matchers::DeclarationMatcher statusOrClass() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return classTemplateSpecializationDecl(
+ hasName("absl::StatusOr"),
+ hasTemplateArgument(0, refersToType(type().bind("T"))));
+}
+
+clang::ast_matchers::DeclarationMatcher statusClass() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return cxxRecordDecl(hasName("absl::Status"));
+}
+
+clang::ast_matchers::DeclarationMatcher statusOrOperatorBaseClass() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return classTemplateSpecializationDecl(
+ hasName("absl::internal_statusor::OperatorBase"));
+}
+
+clang::ast_matchers::TypeMatcher possiblyAliasedStatusOrType() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(statusOrClass())));
+}
+
+clang::ast_matchers::TypeMatcher possiblyAliasedStatusType() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return hasUnqualifiedDesugaredType(recordType(hasDeclaration(statusClass())));
+}
+
+clang::ast_matchers::TypeMatcher statusOrType() {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return hasCanonicalType(qualType(hasDeclaration(statusOrClass())));
+}
+
+bool isRecordTypeWithName(QualType Type, llvm::StringRef TypeName) {
+ return Type->isRecordType() &&
+ Type->getAsCXXRecordDecl()->getQualifiedNameAsString() == TypeName;
+}
+
+bool isStatusOrType(QualType Type) {
+ return isRecordTypeWithName(Type, "absl::StatusOr");
+}
+
+bool isStatusType(QualType Type) {
+ return isRecordTypeWithName(Type, "absl::Status");
+}
+
+llvm::StringMap<QualType> getSyntheticFields(QualType Ty, QualType StatusType,
+ const CXXRecordDecl &RD) {
+ if (auto *TRD = getStatusOrBaseClass(Ty))
+ return {{"status", StatusType}, {"value", getStatusOrValueType(TRD)}};
+ if (isStatusType(Ty) || (RD.hasDefinition() &&
+ RD.isDerivedFrom(StatusType->getAsCXXRecordDecl())))
+ return {{"ok", RD.getASTContext().BoolTy}};
+ return {};
+}
+
+RecordStorageLocation &locForStatus(RecordStorageLocation &StatusOrLoc) {
+ return cast<RecordStorageLocation>(StatusOrLoc.getSyntheticField("status"));
+}
+
+StorageLocation &locForOk(RecordStorageLocation &StatusLoc) {
+ return StatusLoc.getSyntheticField("ok");
+}
+
+BoolValue &valForOk(RecordStorageLocation &StatusLoc, Environment &Env) {
+ if (auto *Val = Env.get<BoolValue>(locForOk(StatusLoc)))
+ return *Val;
+ return initializeStatus(StatusLoc, Env);
+}
+
+CFGMatchSwitch<LatticeTransferState>
+buildTransferMatchSwitch(ASTContext &Ctx,
+ CFGMatchSwitchBuilder<LatticeTransferState> Builder) {
+ using namespace ::clang::ast_matchers; // NOLINT: Too many names
+ return std::move(Builder)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(isStatusOrMemberCallWithName("ok"),
+ transferStatusOrOkCall)
+ .Build();
+}
+
+QualType findStatusType(const ASTContext &Ctx) {
+ for (Type *Ty : Ctx.getTypes())
+ if (isStatusType(QualType(Ty, 0)))
+ return QualType(Ty, 0);
+
+ return QualType();
+}
+
+UncheckedStatusOrAccessModel::UncheckedStatusOrAccessModel(ASTContext &Ctx,
+ Environment &Env)
+ : DataflowAnalysis<UncheckedStatusOrAccessModel,
+ UncheckedStatusOrAccessModel::Lattice>(Ctx),
+ TransferMatchSwitch(buildTransferMatchSwitch(Ctx, {})) {
+ QualType StatusType = findStatusType(Ctx);
+ Env.getDataflowAnalysisContext().setSyntheticFieldCallback(
+ [StatusType](QualType Ty) -> llvm::StringMap<QualType> {
+ CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
+ if (RD == nullptr)
+ return {};
+
+ if (auto Fields = getSyntheticFields(Ty, StatusType, *RD);
+ !Fields.empty())
+ return Fields;
+ return {};
+ });
+}
+
+void UncheckedStatusOrAccessModel::transfer(const CFGElement &Elt, Lattice &L,
+ Environment &Env) {
+ LatticeTransferState State(L, Env);
+ TransferMatchSwitch(Elt, getASTContext(), State);
+}
+
+} // namespace clang::dataflow::statusor_model
diff --git a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
index 3bd4a6e21bee7..0fd136fa77ca9 100644
--- a/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/unittests/Analysis/FlowSensitive/CMakeLists.txt
@@ -24,6 +24,8 @@ add_clang_unittest(ClangAnalysisFlowSensitiveTests
TransferTest.cpp
TypeErasedDataflowAnalysisTest.cpp
UncheckedOptionalAccessModelTest.cpp
+ UncheckedStatusOrAccessModelTest.cpp
+ UncheckedStatusOrAccessModelTestFixture.cpp
ValueTest.cpp
WatchedLiteralsSolverTest.cpp
CLANG_LIBS
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTest.cpp
new file mode 100644
index 0000000000000..07d3f2412e842
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTest.cpp
@@ -0,0 +1,34 @@
+//===- UncheckedStatusOrAccessModelTest.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 <utility>
+
+#include "UncheckedStatusOrAccessModelTestFixture.h"
+#include "clang/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow::statusor_model {
+namespace {
+
+INSTANTIATE_TEST_SUITE_P(
+ UncheckedStatusOrAccessModelTest, UncheckedStatusOrAccessModelTest,
+ testing::Values(
+ std::make_pair(new UncheckedStatusOrAccessModelTestExecutor<
+ UncheckedStatusOrAccessModel>(),
+ UncheckedStatusOrAccessModelTestAliasKind::kUnaliased),
+ std::make_pair(
+ new UncheckedStatusOrAccessModelTestExecutor<
+ UncheckedStatusOrAccessModel>(),
+ UncheckedStatusOrAccessModelTestAliasKind::kPartiallyAliased),
+ std::make_pair(
+ new UncheckedStatusOrAccessModelTestExecutor<
+ UncheckedStatusOrAccessModel>(),
+ UncheckedStatusOrAccessModelTestAliasKind::kFullyAliased)));
+} // namespace
+
+} // namespace clang::dataflow::statusor_model
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
new file mode 100644
index 0000000000000..af30e98c51c56
--- /dev/null
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -0,0 +1,5231 @@
+//===- UncheckedStatusOrAccessModelTestFixture.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 "UncheckedStatusOrAccessModelTestFixture.h"
+#include "llvm/Support/ErrorHandling.h"
+
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "gtest/gtest.h"
+
+namespace clang::dataflow::statusor_model {
+namespace {
+
+static constexpr const char *kAbslDefsHeader = R"cc(
+ // Contains minimal mock declarations of common entities from //base, //util
+ // and //testing.
+
+#pragma clang system_header
+
+#ifndef BASE_DEFS_H_
+#define BASE_DEFS_H_
+
+#include "absl_type_traits.h"
+#include "std_type...
[truncated]
|
Created using spr 1.3.4
Created using spr 1.3.4
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4 [skip ci]
fmayer
changed the base branch from
main
to
users/fmayer/spr/main.flowsensitive-statusor-2n-add-minimal-model-2
fmayer
changed the title
|
@BaLiKfromUA CC |
jvoung
reviewed
Oct 15, 2025
| #include "unchecked_statusor_use_test_defs.h" | ||
|
|
||
| void target(STATUSOR_INT sor) { | ||
| sor.value(); // [[unsafe]] |
There was a problem hiding this comment.
this looks like UnwrapWithoutCheck_Lvalue_CallToValue
did you mean to force a newline somewhere between the sor and value() to see which line the [[unsafe]] lands on?
| )cc"); | ||
| } | ||
|
|
||
| TEST_P(UncheckedStatusOrAccessModelTest, While_TerminatingBranch_Continue) { |
There was a problem hiding this comment.
this looks like TerminatingIfThenBranchInLoop (unless i'm missing something?)
| )cc"); | ||
| } | ||
|
|
||
| TEST_P(UncheckedStatusOrAccessModelTest, UsingDeclaration) { |
There was a problem hiding this comment.
The "using" might be missing now after the refactor to have parameterized tests for aliases?
There was a problem hiding this comment.
yeah. the name is a leftover from the refactor. renamed to test this (kind of trivial) case
| )cc"); | ||
| } | ||
|
|
||
| TEST_P(UncheckedStatusOrAccessModelTest, VarDeclInitExprWithoutValue) { |
There was a problem hiding this comment.
Can you clarify what "WithoutValue" means?
|
|
||
| class Bar; | ||
|
|
||
| class Foo : public absl::StatusOr<Bar> {}; |
There was a problem hiding this comment.
This one doesn't need the parameterization on aliases, unless you want to subclass one of the macros?
Similar below with SubclassOk and SubclassOperator
There was a problem hiding this comment.
good catch, we can just use STATUSOR_INT here, we don't really need the Bar
| )cc"); | ||
| } | ||
|
|
||
| TEST_P(UncheckedStatusOrAccessModelTest, GoodLambda) { |
There was a problem hiding this comment.
nit: maybe put this near the other lambda tests?
|
|
||
| } // namespace | ||
|
|
||
| static void transferStatusOrOkCall(const CXXMemberCallExpr *Expr, |
There was a problem hiding this comment.
nit wonder if once you have more transfer functions it would make more sense to cluster those later, closer to the switch builder?
Created using spr 1.3.7
jvoung
approved these changes
Oct 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This model implements a dataflow analysis for reporting instances of
unchecked use of absl::StatusOr values. It makes sure that every use
the value of a StatusOr object is dominated by a check that the
StatusOr object is ok.
This is an example of code that will be flagged by the analysis:
This is an example of code that will not be flagged by the analysis:
This model has successfully been used by Google for some time now.
This is the initial commit that adds the simplest possible model, that
only models calls to
ok()and checks for unsafe accesses. I will addmore fidelity to the model in follow up changes.
The test setup is notable in that it has an extra indirection. This is
because we have an internal model that extends the model we intend to
upstream, in order to model special constructs only found in our code
base. The parametrized test allows us (and anyone who chooses to do
this) to make sure our extensions do not break the base functionality.
RFC: https://discourse.llvm.org/t/rfc-abseil-unchecked-statusor-use-check/87998