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 @@ -107,6 +107,12 @@ hasArgument(1, hasNulloptType())); } +auto isStdSwapCall() { + return callExpr(callee(functionDecl(hasName("std::swap"))), + argumentCountIs(2), hasArgument(0, hasOptionalType()), + hasArgument(1, hasOptionalType())); +} + /// Creates a symbolic value for an `optional` value using `HasValueVal` as the /// symbolic value of its "has_value" property. StructValue &createOptionalValue(Environment &Env, BoolValue &HasValueVal) { @@ -283,6 +289,50 @@ transferAssignment(E, State.Env.getBoolLiteralValue(false), State); } +void transferSwap(const StorageLocation &OptionalLoc1, + const StorageLocation &OptionalLoc2, + LatticeTransferState &State) { + auto *OptionalVal1 = State.Env.getValue(OptionalLoc1); + assert(OptionalVal1 != nullptr); + + auto *OptionalVal2 = State.Env.getValue(OptionalLoc2); + assert(OptionalVal2 != nullptr); + + State.Env.setValue(OptionalLoc1, *OptionalVal2); + State.Env.setValue(OptionalLoc2, *OptionalVal1); +} + +void transferSwapCall(const CXXMemberCallExpr *E, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(E->getNumArgs() == 1); + + auto *OptionalLoc1 = State.Env.getStorageLocation( + *E->getImplicitObjectArgument(), SkipPast::ReferenceThenPointer); + assert(OptionalLoc1 != nullptr); + + auto *OptionalLoc2 = + State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference); + assert(OptionalLoc2 != nullptr); + + transferSwap(*OptionalLoc1, *OptionalLoc2, State); +} + +void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &, + LatticeTransferState &State) { + assert(E->getNumArgs() == 2); + + auto *OptionalLoc1 = + State.Env.getStorageLocation(*E->getArg(0), SkipPast::Reference); + assert(OptionalLoc1 != nullptr); + + auto *OptionalLoc2 = + State.Env.getStorageLocation(*E->getArg(1), SkipPast::Reference); + assert(OptionalLoc2 != nullptr); + + transferSwap(*OptionalLoc1, *OptionalLoc2, State); +} + auto buildTransferMatchSwitch() { // FIXME: Evaluate the efficiency of matchers. If using matchers results in a // lot of duplicated work (e.g. string comparisons), consider providing APIs @@ -361,6 +411,13 @@ State.Env.getBoolLiteralValue(false)); }) + // optional::swap + .CaseOf(isOptionalMemberCallWithName("swap"), + transferSwapCall) + + // std::swap + .CaseOf(isStdSwapCall(), transferStdSwapCall) + .Build(); } 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 @@ -507,6 +507,9 @@ template constexpr remove_reference_t&& move(T&& x); +template +void swap(T& a, T& b) noexcept; + } // namespace std #endif // UTILITY_H @@ -718,6 +721,8 @@ constexpr explicit operator bool() const noexcept; using __base::has_value; + + constexpr void swap(optional& __opt) noexcept; }; template @@ -938,6 +943,8 @@ constexpr explicit operator bool() const noexcept; constexpr bool has_value() const noexcept; + + void swap(optional& rhs) noexcept; }; template @@ -1129,6 +1136,8 @@ constexpr explicit operator bool() const noexcept; constexpr bool has_value() const noexcept; + + void swap(Optional& other); }; template @@ -1911,10 +1920,93 @@ UnorderedElementsAre(Pair("check", "unsafe: input.cc:6:7"))); } +TEST_P(UncheckedOptionalAccessTest, OptionalSwap) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = 3; + + opt1.swap(opt2); + + opt1.value(); + /*[[check-1]]*/ + + opt2.value(); + /*[[check-2]]*/ + } + )", + UnorderedElementsAre(Pair("check-1", "safe"), + Pair("check-2", "unsafe: input.cc:13:7"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = 3; + + opt2.swap(opt1); + + opt1.value(); + /*[[check-3]]*/ + + opt2.value(); + /*[[check-4]]*/ + } + )", + UnorderedElementsAre(Pair("check-3", "safe"), + Pair("check-4", "unsafe: input.cc:13:7"))); +} + +TEST_P(UncheckedOptionalAccessTest, StdSwap) { + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = 3; + + std::swap(opt1, opt2); + + opt1.value(); + /*[[check-1]]*/ + + opt2.value(); + /*[[check-2]]*/ + } + )", + UnorderedElementsAre(Pair("check-1", "safe"), + Pair("check-2", "unsafe: input.cc:13:7"))); + + ExpectLatticeChecksFor( + R"( + #include "unchecked_optional_access_test.h" + + void target() { + $ns::$optional opt1 = $ns::nullopt; + $ns::$optional opt2 = 3; + + std::swap(opt2, opt1); + + opt1.value(); + /*[[check-3]]*/ + + opt2.value(); + /*[[check-4]]*/ + } + )", + UnorderedElementsAre(Pair("check-3", "safe"), + Pair("check-4", "unsafe: input.cc:13:7"))); +} + // FIXME: Add support for: // - constructors (copy, move) // - assignment operators (default, copy, move) -// - swap // - invalidation (passing optional by non-const reference/pointer) // - `value_or(nullptr) != nullptr`, `value_or(0) != 0`, `value_or("").empty()` // - nested `optional` values