diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -80,11 +80,20 @@ hasOptionalType()); } -auto hasNulloptType() { - return hasType(namedDecl( - hasAnyName("std::nullopt_t", "absl::nullopt_t", "base::nullopt_t"))); +auto nulloptTypeDecl() { + return namedDecl( + hasAnyName("std::nullopt_t", "absl::nullopt_t", "base::nullopt_t")); } +auto hasNulloptType() { return hasType(nulloptTypeDecl()); } + +// `optional` or `nullopt_t` +auto hasAnyOptionalType() { + return hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(anyOf(nulloptTypeDecl(), optionalClass()))))); +} + + auto inPlaceClass() { return recordDecl( hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t")); @@ -117,6 +126,11 @@ argumentCountIs(2), hasArgument(1, unless(hasNulloptType()))); } +auto isNulloptConstructor() { + return cxxConstructExpr(hasNulloptType(), argumentCountIs(1), + hasArgument(0, hasNulloptType())); +} + auto isOptionalNulloptAssignment() { return cxxOperatorCallExpr(hasOverloadedOperatorName("="), callee(cxxMethodDecl(ofClass(optionalClass()))), @@ -172,6 +186,27 @@ optionalOrAliasType(), referenceType(pointee(optionalOrAliasType())))))); } +template +auto isComparisonOperatorCall(L lhs_arg_matcher, R rhs_arg_matcher) { + return cxxOperatorCallExpr( + anyOf(hasOverloadedOperatorName("=="), hasOverloadedOperatorName("!=")), + argumentCountIs(2), hasArgument(0, lhs_arg_matcher), + hasArgument(1, rhs_arg_matcher)); +} + +// Ensures that `Expr` is mapped to a `BoolValue` and returns it. +BoolValue &forceBoolValue(Environment &Env, const Expr &Expr) { + auto *Value = cast_or_null(Env.getValue(Expr, SkipPast::None)); + if (Value != nullptr) + return *Value; + + auto &Loc = Env.createStorageLocation(Expr); + Value = &Env.makeAtomicBoolValue(); + Env.setValue(Loc, *Value); + Env.setStorageLocation(Expr, Loc); + return *Value; +} + /// Sets `HasValueVal` as the symbolic value that represents the "has_value" /// property of the optional value `OptionalVal`. void setHasValue(Value &OptionalVal, BoolValue &HasValueVal) { @@ -357,16 +392,8 @@ if (HasValueVal == nullptr) return; - auto *ExprValue = cast_or_null( - State.Env.getValue(*ValueOrPredExpr, SkipPast::None)); - if (ExprValue == nullptr) { - auto &ExprLoc = State.Env.createStorageLocation(*ValueOrPredExpr); - ExprValue = &State.Env.makeAtomicBoolValue(); - State.Env.setValue(ExprLoc, *ExprValue); - State.Env.setStorageLocation(*ValueOrPredExpr, ExprLoc); - } - - Env.addToFlowCondition(ModelPred(Env, *ExprValue, *HasValueVal)); + Env.addToFlowCondition( + ModelPred(Env, forceBoolValue(Env, *ValueOrPredExpr), *HasValueVal)); } void transferValueOrStringEmptyCall(const clang::Expr *ComparisonExpr, @@ -423,9 +450,9 @@ /// 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 &value_orConversionHasValue(const FunctionDecl &F, const Expr &E, - const MatchFinder::MatchResult &MatchRes, - LatticeTransferState &State) { +BoolValue &valueOrConversionHasValue(const FunctionDecl &F, const Expr &E, + const MatchFinder::MatchResult &MatchRes, + LatticeTransferState &State) { assert(F.getTemplateSpecializationArgs()->size() > 0); const int TemplateParamOptionalWrappersCount = countOptionalWrappers( @@ -453,9 +480,9 @@ assert(E->getNumArgs() > 0); assignOptionalValue(*E, State, - value_orConversionHasValue(*E->getConstructor(), - *E->getArg(0), MatchRes, - State)); + valueOrConversionHasValue(*E->getConstructor(), + *E->getArg(0), MatchRes, + State)); } void transferAssignment(const CXXOperatorCallExpr *E, BoolValue &HasValueVal, @@ -478,8 +505,8 @@ LatticeTransferState &State) { assert(E->getNumArgs() > 1); transferAssignment(E, - value_orConversionHasValue(*E->getDirectCallee(), - *E->getArg(1), MatchRes, State), + valueOrConversionHasValue(*E->getDirectCallee(), + *E->getArg(1), MatchRes, State), State); } @@ -533,6 +560,56 @@ transferSwap(*OptionalLoc1, *OptionalLoc2, State); } +BoolValue &evaluateEquality(Environment &Env, BoolValue &EqVal, BoolValue &LHS, + BoolValue &RHS) { + // Logically, an optional object is composed of two values - a `has_value` + // bit and a value of type T. Equality of optional objects compares both + // values. Therefore, merely comparing the `has_value` bits isn't sufficient: + // when two optional objects are engaged, the equality of their respective + // values of type T matters. Since we only track the `has_value` bits, we + // can't make any conclusions about equality when we know that two optional + // objects are engaged. + // + // We express this as two facts about the equality: + // a) EqVal => (LHS & RHS) v (!RHS & !LHS) + // If they are equal, then either both are set or both are unset. + // b) (!LHS & !RHS) => EqVal + // If neither is set, then they are equal. + // We rewrite b) as !EqVal => (LHS v RHS), for a more compact formula. + return Env.makeAnd( + Env.makeImplication( + EqVal, Env.makeOr(Env.makeAnd(LHS, RHS), + Env.makeAnd(Env.makeNot(LHS), Env.makeNot(RHS)))), + Env.makeImplication(Env.makeNot(EqVal), Env.makeOr(LHS, RHS))); +} + +void transferOptionalAndOptionalCmp(const clang::CXXOperatorCallExpr *CmpExpr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + Environment &Env = State.Env; + auto *CmpValue = &forceBoolValue(Env, *CmpExpr); + if (auto *LHasVal = getHasValue( + Env, Env.getValue(*CmpExpr->getArg(0), SkipPast::Reference))) + if (auto *RHasVal = getHasValue( + Env, Env.getValue(*CmpExpr->getArg(1), SkipPast::Reference))) { + if (CmpExpr->getOperator() == clang::OO_ExclaimEqual) + CmpValue = &State.Env.makeNot(*CmpValue); + Env.addToFlowCondition( + evaluateEquality(Env, *CmpValue, *LHasVal, *RHasVal)); + } +} + +void transferOptionalAndValueCmp(const clang::CXXOperatorCallExpr *CmpExpr, + const clang::Expr *E, Environment &Env) { + auto *CmpValue = &forceBoolValue(Env, *CmpExpr); + if (auto *HasVal = getHasValue(Env, Env.getValue(*E, SkipPast::Reference))) { + if (CmpExpr->getOperator() == clang::OO_ExclaimEqual) + CmpValue = &Env.makeNot(*CmpValue); + Env.addToFlowCondition(evaluateEquality(Env, *CmpValue, *HasVal, + Env.getBoolLiteralValue(true))); + } +} + llvm::Optional ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) { if (Options.IgnoreSmartPointerDereference) @@ -578,12 +655,24 @@ assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(true)); }) .CaseOfCFGStmt( - isOptionalNulloptConstructor(), + isNulloptConstructor(), [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, LatticeTransferState &State) { assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(false)); }) + .CaseOfCFGStmt( + isOptionalNulloptConstructor(), + [](const CXXConstructExpr *E, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + // Shares a temporary with the underlying `nullopt_t` instance. + if (auto *OptionalLoc = + State.Env.getStorageLocation(*E, SkipPast::None)) { + State.Env.setValue( + *OptionalLoc, + *State.Env.getValue(*E->getArg(0), SkipPast::None)); + } + }) .CaseOfCFGStmt(isOptionalValueOrConversionConstructor(), transferValueOrConversionConstructor) @@ -652,6 +741,25 @@ // opt.value_or(X) != X .CaseOfCFGStmt(isValueOrNotEqX(), transferValueOrNotEqX) + // Comparisons (==, !=): + .CaseOfCFGStmt( + isComparisonOperatorCall(hasAnyOptionalType(), hasAnyOptionalType()), + transferOptionalAndOptionalCmp) + .CaseOfCFGStmt( + isComparisonOperatorCall(hasOptionalType(), + unless(hasAnyOptionalType())), + [](const clang::CXXOperatorCallExpr *Cmp, + const MatchFinder::MatchResult &, LatticeTransferState &State) { + transferOptionalAndValueCmp(Cmp, Cmp->getArg(0), State.Env); + }) + .CaseOfCFGStmt( + isComparisonOperatorCall(unless(hasAnyOptionalType()), + hasOptionalType()), + [](const clang::CXXOperatorCallExpr *Cmp, + const MatchFinder::MatchResult &, LatticeTransferState &State) { + transferOptionalAndValueCmp(Cmp, Cmp->getArg(1), State.Env); + }) + // returns optional .CaseOfCFGStmt(isCallReturningOptional(), transferCallReturningOptional) diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp --- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp +++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp @@ -762,6 +762,29 @@ constexpr optional make_optional(std::initializer_list il, Args&&... args); +template +constexpr bool operator==(const optional &lhs, const optional &rhs); +template +constexpr bool operator!=(const optional &lhs, const optional &rhs); + +template +constexpr bool operator==(const optional &opt, nullopt_t); +template +constexpr bool operator==(nullopt_t, const optional &opt); +template +constexpr bool operator!=(const optional &opt, nullopt_t); +template +constexpr bool operator!=(nullopt_t, const optional &opt); + +template +constexpr bool operator==(const optional &opt, const U &value); +template +constexpr bool operator==(const T &value, const optional &opt); +template +constexpr bool operator!=(const optional &opt, const U &value); +template +constexpr bool operator!=(const T &value, const optional &opt); + } // namespace std )"; @@ -984,6 +1007,29 @@ constexpr optional make_optional(std::initializer_list il, Args&&... args); +template +constexpr bool operator==(const optional &lhs, const optional &rhs); +template +constexpr bool operator!=(const optional &lhs, const optional &rhs); + +template +constexpr bool operator==(const optional &opt, nullopt_t); +template +constexpr bool operator==(nullopt_t, const optional &opt); +template +constexpr bool operator!=(const optional &opt, nullopt_t); +template +constexpr bool operator!=(nullopt_t, const optional &opt); + +template +constexpr bool operator==(const optional &opt, const U &value); +template +constexpr bool operator==(const T &value, const optional &opt); +template +constexpr bool operator!=(const optional &opt, const U &value); +template +constexpr bool operator!=(const T &value, const optional &opt); + } // namespace absl )"; @@ -1177,6 +1223,29 @@ constexpr Optional make_optional(std::initializer_list il, Args&&... args); +template +constexpr bool operator==(const Optional &lhs, const Optional &rhs); +template +constexpr bool operator!=(const Optional &lhs, const Optional &rhs); + +template +constexpr bool operator==(const Optional &opt, nullopt_t); +template +constexpr bool operator==(nullopt_t, const Optional &opt); +template +constexpr bool operator!=(const Optional &opt, nullopt_t); +template +constexpr bool operator!=(nullopt_t, const Optional &opt); + +template +constexpr bool operator==(const Optional &opt, const U &value); +template +constexpr bool operator==(const T &value, const Optional &opt); +template +constexpr bool operator!=(const Optional &opt, const U &value); +template +constexpr bool operator!=(const T &value, const Optional &opt); + } // namespace base )"; @@ -2117,6 +2186,325 @@ )"); } + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckLeftSet) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = 3; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt1 == opt2) { + opt2.value(); + } else { + opt2.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckRightSet) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = 3; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt2 == opt1) { + opt2.value(); + } else { + opt2.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckVerifySetAfterEq) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = Make<$ns::$optional>(); + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt1 == opt2) { + if (opt1.has_value()) + opt2.value(); + if (opt2.has_value()) + opt1.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckLeftUnset) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt1 == opt2) { + opt2.value(); // [[unsafe]] + } else { + opt2.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckRightUnset) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt2 == opt1) { + opt2.value(); // [[unsafe]] + } else { + opt2.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckRightNullopt) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if (opt == $ns::nullopt) { + opt.value(); // [[unsafe]] + } else { + opt.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckLeftNullopt) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if ($ns::nullopt == opt) { + opt.value(); // [[unsafe]] + } else { + opt.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckRightValue) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if (opt == 3) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, EqualityCheckLeftValue) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if (3 == opt) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckLeftSet) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = 3; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt1 != opt2) { + opt2.value(); // [[unsafe]] + } else { + opt2.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckRightSet) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = 3; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt2 != opt1) { + opt2.value(); // [[unsafe]] + } else { + opt2.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckVerifySetAfterEq) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = Make<$ns::$optional>(); + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt1 != opt2) { + if (opt1.has_value()) + opt2.value(); // [[unsafe]] + if (opt2.has_value()) + opt1.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckLeftUnset) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt1 != opt2) { + opt2.value(); + } else { + opt2.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckRightUnset) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = Make<$ns::$optional>(); + + if (opt2 != opt1) { + opt2.value(); + } else { + opt2.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckRightNullopt) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if (opt != $ns::nullopt) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckLeftNullopt) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if ($ns::nullopt != opt) { + opt.value(); + } else { + opt.value(); // [[unsafe]] + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckRightValue) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if (opt != 3) { + opt.value(); // [[unsafe]] + } else { + opt.value(); + } + } + )"); +} + +TEST_P(UncheckedOptionalAccessTest, InequalityCheckLeftValue) { + ExpectDiagnosticsFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt = Make<$ns::$optional>(); + + if (3 != opt) { + opt.value(); // [[unsafe]] + } else { + opt.value(); + } + } + )"); +} + // Verifies that the model sees through aliases. TEST_P(UncheckedOptionalAccessTest, WithAlias) { ExpectDiagnosticsFor(