-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[FlowSensitive] [StatusOr] [14/N] Support nested StatusOrs #170950
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
[FlowSensitive] [StatusOr] [14/N] Support nested StatusOrs #170950
Conversation
Created using spr 1.3.7 [skip ci]
Created using spr 1.3.7
|
@llvm/pr-subscribers-clang Author: Florian Mayer (fmayer) ChangesFull diff: https://github.com/llvm/llvm-project/pull/170950.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 1b68d704239e8..c917c8e8c11ba 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -1037,6 +1037,26 @@ transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr,
State.Env.setValue(*Expr, Res);
}
+static void transferDerefCall(const CallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ auto *StatusOrLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+
+ if (StatusOrLoc && State.Env.getStorageLocation(*Expr) == nullptr)
+ State.Env.setStorageLocation(*Expr,
+ StatusOrLoc->getSyntheticField("value"));
+}
+
+static void transferArrowCall(const CallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ auto *StatusOrLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+ if (!StatusOrLoc)
+ return;
+ State.Env.setValue(*Expr, State.Env.create<PointerValue>(
+ StatusOrLoc->getSyntheticField("value")));
+}
+
static RecordStorageLocation *
getSmartPtrLikeStorageLocation(const Expr &E, const Environment &Env) {
if (!E.isPRValue())
@@ -1123,6 +1143,10 @@ buildTransferMatchSwitch(ASTContext &Ctx,
transferValueAssignmentCall)
.CaseOfCFGStmt<CXXConstructExpr>(isStatusOrValueConstructor(),
transferValueConstructor)
+ .CaseOfCFGStmt<CallExpr>(isStatusOrOperatorCallWithName("->"),
+ transferArrowCall)
+ .CaseOfCFGStmt<CallExpr>(isStatusOrOperatorCallWithName("*"),
+ transferDerefCall)
.CaseOfCFGStmt<CallExpr>(isAsStatusCallWithStatus(),
transferAsStatusCallWithStatus)
.CaseOfCFGStmt<CallExpr>(isAsStatusCallWithStatusOr(),
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 48e61abf09f19..cd7353c62f537 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -3691,6 +3691,155 @@ TEST_P(UncheckedStatusOrAccessModelTest, UniquePtrReset) {
)cc");
}
+TEST_P(UncheckedStatusOrAccessModelTest, NestedStatusOrInStatusOrStruct) {
+ // Non-standard assignment with a nested StatusOr.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Inner {
+ absl::StatusOr<std::string> sor;
+ };
+
+ struct Outer {
+ absl::StatusOr<Inner> inner;
+ };
+
+ void target() {
+ Outer foo = Make<Outer>();
+ foo.inner->sor = "a"; // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(const absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) foo->sor.value();
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(const absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && (*foo).sor.ok()) (*foo).sor.value();
+ }
+ )cc");
+
+ // With assignment.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) {
+ foo->sor = Make<absl::StatusOr<std::string>>();
+ foo->sor.value(); // [[unsafe]]
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) {
+ auto n = Make<absl::StatusOr<std::string>>();
+ if (n.ok()) foo->sor = n;
+ foo->sor.value();
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) {
+ auto n = Make<absl::StatusOr<std::string>>();
+ if (n.ok()) foo->sor = std::move(n);
+ foo->sor.value();
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) *foo->sor;
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (!foo.ok()) return;
+ if (!foo->sor.ok())
+ foo->sor.value(); // [[unsafe]]
+ else
+ foo->sor.value();
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo, bool b) {
+ if (!foo.ok()) return;
+ if (b) {
+ if (!foo->sor.ok()) return;
+ foo->sor.value();
+ } else {
+ if (!foo->sor.ok()) return;
+ foo->sor.value();
+ }
+ foo->sor.value();
+ }
+ )cc");
+}
+
} // namespace
std::string
|
|
@llvm/pr-subscribers-clang-analysis Author: Florian Mayer (fmayer) ChangesFull diff: https://github.com/llvm/llvm-project/pull/170950.diff 2 Files Affected:
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
index 1b68d704239e8..c917c8e8c11ba 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedStatusOrAccessModel.cpp
@@ -1037,6 +1037,26 @@ transferAssertionResultOperatorBoolCall(const CXXMemberCallExpr *Expr,
State.Env.setValue(*Expr, Res);
}
+static void transferDerefCall(const CallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ auto *StatusOrLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+
+ if (StatusOrLoc && State.Env.getStorageLocation(*Expr) == nullptr)
+ State.Env.setStorageLocation(*Expr,
+ StatusOrLoc->getSyntheticField("value"));
+}
+
+static void transferArrowCall(const CallExpr *Expr,
+ const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+ auto *StatusOrLoc = State.Env.get<RecordStorageLocation>(*Expr->getArg(0));
+ if (!StatusOrLoc)
+ return;
+ State.Env.setValue(*Expr, State.Env.create<PointerValue>(
+ StatusOrLoc->getSyntheticField("value")));
+}
+
static RecordStorageLocation *
getSmartPtrLikeStorageLocation(const Expr &E, const Environment &Env) {
if (!E.isPRValue())
@@ -1123,6 +1143,10 @@ buildTransferMatchSwitch(ASTContext &Ctx,
transferValueAssignmentCall)
.CaseOfCFGStmt<CXXConstructExpr>(isStatusOrValueConstructor(),
transferValueConstructor)
+ .CaseOfCFGStmt<CallExpr>(isStatusOrOperatorCallWithName("->"),
+ transferArrowCall)
+ .CaseOfCFGStmt<CallExpr>(isStatusOrOperatorCallWithName("*"),
+ transferDerefCall)
.CaseOfCFGStmt<CallExpr>(isAsStatusCallWithStatus(),
transferAsStatusCallWithStatus)
.CaseOfCFGStmt<CallExpr>(isAsStatusCallWithStatusOr(),
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
index 48e61abf09f19..cd7353c62f537 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedStatusOrAccessModelTestFixture.cpp
@@ -3691,6 +3691,155 @@ TEST_P(UncheckedStatusOrAccessModelTest, UniquePtrReset) {
)cc");
}
+TEST_P(UncheckedStatusOrAccessModelTest, NestedStatusOrInStatusOrStruct) {
+ // Non-standard assignment with a nested StatusOr.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Inner {
+ absl::StatusOr<std::string> sor;
+ };
+
+ struct Outer {
+ absl::StatusOr<Inner> inner;
+ };
+
+ void target() {
+ Outer foo = Make<Outer>();
+ foo.inner->sor = "a"; // [[unsafe]]
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(const absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) foo->sor.value();
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(const absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && (*foo).sor.ok()) (*foo).sor.value();
+ }
+ )cc");
+
+ // With assignment.
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) {
+ foo->sor = Make<absl::StatusOr<std::string>>();
+ foo->sor.value(); // [[unsafe]]
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) {
+ auto n = Make<absl::StatusOr<std::string>>();
+ if (n.ok()) foo->sor = n;
+ foo->sor.value();
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) {
+ auto n = Make<absl::StatusOr<std::string>>();
+ if (n.ok()) foo->sor = std::move(n);
+ foo->sor.value();
+ }
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (foo.ok() && foo->sor.ok()) *foo->sor;
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo) {
+ if (!foo.ok()) return;
+ if (!foo->sor.ok())
+ foo->sor.value(); // [[unsafe]]
+ else
+ foo->sor.value();
+ }
+ )cc");
+
+ ExpectDiagnosticsFor(
+ R"cc(
+#include "unchecked_statusor_access_test_defs.h"
+
+ struct Foo {
+ absl::StatusOr<std::string> sor;
+ };
+
+ void target(absl::StatusOr<Foo>& foo, bool b) {
+ if (!foo.ok()) return;
+ if (b) {
+ if (!foo->sor.ok()) return;
+ foo->sor.value();
+ } else {
+ if (!foo->sor.ok()) return;
+ foo->sor.value();
+ }
+ foo->sor.value();
+ }
+ )cc");
+}
+
} // namespace
std::string
|
Created using spr 1.3.7 [skip ci]
Created using spr 1.3.7 [skip ci]
| transferValueAssignmentCall) | ||
| .CaseOfCFGStmt<CXXConstructExpr>(isStatusOrValueConstructor(), | ||
| transferValueConstructor) | ||
| .CaseOfCFGStmt<CallExpr>(isStatusOrOperatorCallWithName("->"), |
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.
nit: could use .CaseOfCFGStmt to be more specific?
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.
CXXOperatorCallExpr instead of CallExpr, I mean
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.
done
| }; | ||
| void target(absl::StatusOr<Foo>& foo) { | ||
| if (foo.ok() && foo->sor.ok()) *foo->sor; |
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.
nit: move this near the earlier if (foo.ok() && foo->sor.ok()) foo->sor.value() test case?
Seems less related to this // With assignment. block
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.
done
| } | ||
| )cc"); | ||
|
|
||
| ExpectDiagnosticsFor( |
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.
maybe add a comment to split this from the // With assignment. -- starting a new section for say more complex conditionals
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.
done
| return; | ||
| State.Env.setValue(*Expr, State.Env.create<PointerValue>( | ||
| StatusOrLoc->getSyntheticField("value"))); | ||
| } |
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.
these are probably most common for nesting, but could it be possible to have a foo.value().value() ?
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.
done
Created using spr 1.3.7 [skip ci]
Reviewers: jvoung Reviewed By: jvoung Pull Request: llvm/llvm-project#170950
No description provided.