[clang] [clang][dataflow] Fix for value constructor in class derived from optional. (PR #86942)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Mar 28 05:21:09 PDT 2024
https://github.com/martinboehme created https://github.com/llvm/llvm-project/pull/86942
The constructor `Derived(int)` in the newly added test
`ClassDerivedFromOptionalValueConstructor` is not a template, and this used to
cause an assertion failure in `valueOrConversionHasValue()` because
`F.getTemplateSpecializationArgs()` returns null.
(This is modeled after the `MaybeAlign(Align Value)` constructor, which
similarly causes an assertion failure in the analysis when assigning an `Align`
to a `MaybeAlign`.)
To fix this, we can simply look at the type of the destination type which we're
constructing or assigning to (instead of the function template argument), and
this not only fixes this specific case but actually simplifies the
implementation.
I've added some additional tests for the case of assigning to a nested optional
because we didn't have coverage for these and I wanted to make sure I didn't
break anything.
>From 28e942051cea2bd8a0df6b9b26596f60ea53fe96 Mon Sep 17 00:00:00 2001
From: Martin Braenne <mboehme at google.com>
Date: Thu, 28 Mar 2024 12:20:08 +0000
Subject: [PATCH] [clang][dataflow] Fix for value constructor in class derived
from optional.
The constructor `Derived(int)` in the newly added test
`ClassDerivedFromOptionalValueConstructor` is not a template, and this used to
cause an assertion failure in `valueOrConversionHasValue()` because
`F.getTemplateSpecializationArgs()` returns null.
(This is modeled after the `MaybeAlign(Align Value)` constructor, which
similarly causes an assertion failure in the analysis when assigning an `Align`
to a `MaybeAlign`.)
To fix this, we can simply look at the type of the destination type which we're
constructing or assigning to (instead of the function template argument), and
this not only fixes this specific case but actually simplifies the
implementation.
I've added some additional tests for the case of assigning to a nested optional
because we didn't have coverage for these and I wanted to make sure I didn't
break anything.
---
.../Models/UncheckedOptionalAccessModel.cpp | 45 ++++++------
.../UncheckedOptionalAccessModelTest.cpp | 69 +++++++++++++++++++
2 files changed, 92 insertions(+), 22 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index dbf4878622eba9..cadb1ceb2d8507 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -512,27 +512,26 @@ void constructOptionalValue(const Expr &E, Environment &Env,
/// Returns a symbolic value for the "has_value" property of an `optional<T>`
/// value that is constructed/assigned from a value of type `U` or `optional<U>`
/// where `T` is constructible from `U`.
-BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E,
+BoolValue &valueOrConversionHasValue(QualType DestType, const Expr &E,
const MatchFinder::MatchResult &MatchRes,
LatticeTransferState &State) {
- assert(F.getTemplateSpecializationArgs() != nullptr);
- assert(F.getTemplateSpecializationArgs()->size() > 0);
-
- const int TemplateParamOptionalWrappersCount =
- countOptionalWrappers(*MatchRes.Context, F.getTemplateSpecializationArgs()
- ->get(0)
- .getAsType()
- .getNonReferenceType());
+ const int DestTypeOptionalWrappersCount =
+ countOptionalWrappers(*MatchRes.Context, DestType);
const int ArgTypeOptionalWrappersCount = countOptionalWrappers(
*MatchRes.Context, E.getType().getNonReferenceType());
- // Check if this is a constructor/assignment call for `optional<T>` with
- // argument of type `U` such that `T` is constructible from `U`.
- if (TemplateParamOptionalWrappersCount == ArgTypeOptionalWrappersCount)
+ // Is this an constructor of the form `template<class U> optional(U &&)` /
+ // assignment of the form `template<class U> optional& operator=(U &&)`
+ // (where `T` is assignable / constructible from `U`)?
+ // We recognize this because the number of optionals in the optional being
+ // assigned to is different from the function argument type.
+ if (DestTypeOptionalWrappersCount != ArgTypeOptionalWrappersCount)
return State.Env.getBoolLiteralValue(true);
- // This is a constructor/assignment call for `optional<T>` with argument of
- // type `optional<U>` such that `T` is constructible from `U`.
+ // Otherwise, this must be a constructor of the form
+ // `template <class U> optional<optional<U> &&)` / assignment of the form
+ // `template <class U> optional& operator=(optional<U> &&)
+ // (where, again, `T` is assignable / constructible from `U`).
auto *Loc = State.Env.get<RecordStorageLocation>(E);
if (auto *HasValueVal = getHasValue(State.Env, Loc))
return *HasValueVal;
@@ -544,10 +543,11 @@ void transferValueOrConversionConstructor(
LatticeTransferState &State) {
assert(E->getNumArgs() > 0);
- constructOptionalValue(*E, State.Env,
- valueOrConversionHasValue(*E->getConstructor(),
- *E->getArg(0), MatchRes,
- State));
+ constructOptionalValue(
+ *E, State.Env,
+ valueOrConversionHasValue(
+ E->getConstructor()->getThisType()->getPointeeType(), *E->getArg(0),
+ MatchRes, State));
}
void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal,
@@ -566,10 +566,11 @@ void transferValueOrConversionAssignment(
const CXXOperatorCallExpr *E, const MatchFinder::MatchResult &MatchRes,
LatticeTransferState &State) {
assert(E->getNumArgs() > 1);
- transferAssignment(E,
- valueOrConversionHasValue(*E->getDirectCallee(),
- *E->getArg(1), MatchRes, State),
- State);
+ transferAssignment(
+ E,
+ valueOrConversionHasValue(E->getArg(0)->getType().getNonReferenceType(),
+ *E->getArg(1), MatchRes, State),
+ State);
}
void transferNulloptAssignment(const CXXOperatorCallExpr *E,
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 9430730004dbd2..f16472ef17141c 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2798,6 +2798,59 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueOptional) {
)");
}
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalAssignValue) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ using OptionalInt = $ns::$optional<int>;
+
+ void target($ns::$optional<OptionalInt> opt) {
+ if (!opt) return;
+
+ // Accessing the outer optional is OK now.
+ *opt;
+
+ // But accessing the nested optional is still unsafe because we haven't
+ // checked it.
+ **opt; // [[unsafe]]
+
+ *opt = 1;
+
+ // Accessing the nested optional is safe after assigning a value to it.
+ **opt;
+ }
+ )");
+}
+
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalAssignOptional) {
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ using OptionalInt = $ns::$optional<int>;
+
+ void target($ns::$optional<OptionalInt> opt) {
+ if (!opt) return;
+
+ // Accessing the outer optional is OK now.
+ *opt;
+
+ // But accessing the nested optional is still unsafe because we haven't
+ // checked it.
+ **opt; // [[unsafe]]
+
+ // Assign from `optional<short>` so that we trigger conversion assignment
+ // instead of move assignment.
+ *opt = $ns::$optional<short>();
+
+ // Accessing the nested optional is still unsafe after assigning an empty
+ // optional to it.
+ **opt; // [[unsafe]]
+ }
+ )");
+}
+
// Tests that structs can be nested. We use an optional field because its easy
// to use in a test, but the type of the field shouldn't matter.
TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
@@ -3443,6 +3496,22 @@ TEST_P(UncheckedOptionalAccessTest, ClassDerivedPrivatelyFromOptional) {
ast_matchers::hasName("Method"));
}
+TEST_P(UncheckedOptionalAccessTest, ClassDerivedFromOptionalValueConstructor) {
+ ExpectDiagnosticsFor(R"(
+ #include "unchecked_optional_access_test.h"
+
+ struct Derived : public $ns::$optional<int> {
+ Derived(int);
+ };
+
+ void target(Derived opt) {
+ *opt; // [[unsafe]]
+ opt = 1;
+ *opt;
+ }
+ )");
+}
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
More information about the cfe-commits
mailing list