[clang][dataflow] Fix for value constructor in class derived from optional.

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,

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)