diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index dbf4878622eba..cadb1ceb2d850 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` /// value that is constructed/assigned from a value of type `U` or `optional` /// 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` with - // argument of type `U` such that `T` is constructible from `U`. - if (TemplateParamOptionalWrappersCount == ArgTypeOptionalWrappersCount) + // Is this an constructor of the form `template optional(U &&)` / + // assignment of the form `template 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` with argument of - // type `optional` such that `T` is constructible from `U`. + // Otherwise, this must be a constructor of the form + // `template optional &&)` / assignment of the form + // `template optional& operator=(optional &&) + // (where, again, `T` is assignable / constructible from `U`). auto *Loc = State.Env.get(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 9430730004dbd..f16472ef17141 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; + + void target($ns::$optional 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; + + void target($ns::$optional 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` so that we trigger conversion assignment + // instead of move assignment. + *opt = $ns::$optional(); + + // 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 { + Derived(int); + }; + + void target(Derived opt) { + *opt; // [[unsafe]] + opt = 1; + *opt; + } + )"); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move)