[clang][dataflow] Model the behavior of optional and std swap

[ymandel]

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

1961

nit: use unique identifiers for each check?

[sgatev]

Address comments.

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

 
 auto isOptionalNulloptAssignment() {
   return cxxOperatorCallExpr(hasOverloadedOperatorName("="),
                              callee(cxxMethodDecl(ofClass(optionalClass()))),
                              argumentCountIs(2),
                              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) {
   auto OptionalVal = std::make_unique<StructValue>();
   OptionalVal->setProperty("has_value", HasValueVal);
   return Env.takeOwnership(std::move(OptionalVal));
 }
 

 }
 
 void transferNulloptAssignment(const CXXOperatorCallExpr *E,
                                const MatchFinder::MatchResult &,
                                LatticeTransferState &State) {
   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
   // that avoid it through memoization.
   return MatchSwitchBuilder<LatticeTransferState>()
       // Attach a symbolic "has_value" state to optional values that we see for
       // the first time.
       .CaseOf<Expr>(expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
return MatchSwitchBuilder<LatticeTransferState>()
       .CaseOf<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("reset"),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             assignOptionalValue(*E->getImplicitObjectArgument(), State,
                                 State.Env.getBoolLiteralValue(false));
           })
 
+      // optional::swap
+      .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
+                                 transferSwapCall)
+
+      // std::swap
+      .CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+
       .Build();
 }
 
 } // namespace
 
 UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
     : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
           Ctx),

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

 
 #include "std_type_traits.h"
 
 namespace std {
 
 template <typename T>
 constexpr remove_reference_t<T>&& move(T&& x);
 
+template <typename T>
+void swap(T& a, T& b) noexcept;
+
 } // namespace std
 
 #endif // UTILITY_H
 )";
 
 static constexpr char StdInitializerListHeader[] = R"(
 #ifndef INITIALIZER_LIST_H
 #define INITIALIZER_LIST_H
public:
 
   template <typename U, typename... Args>
   _Tp& emplace(std::initializer_list<U> ilist, Args&&... args);
 
   using __base::reset;
 
   constexpr explicit operator bool() const noexcept;
   using __base::has_value;
+
+  constexpr void swap(optional& __opt) noexcept;
 };
 
 template <typename T>
 constexpr optional<typename std::decay<T>::type> make_optional(T&& v);
 
 template <typename T, typename... Args>
 constexpr optional<T> make_optional(Args&&... args);
 
public:
 
   template <typename U, typename... Args>
   T& emplace(std::initializer_list<U> ilist, Args&&... args);
 
   void reset() noexcept;
 
   constexpr explicit operator bool() const noexcept;
   constexpr bool has_value() const noexcept;
+
+  void swap(optional& rhs) noexcept;
 };
 
 template <typename T>
 constexpr optional<typename std::decay<T>::type> make_optional(T&& v);
 
 template <typename T, typename... Args>
 constexpr optional<T> make_optional(Args&&... args);
 
public:
 
   template <typename U, typename... Args>
   T& emplace(std::initializer_list<U> ilist, Args&&... args);
 
   void reset() noexcept;
 
   constexpr explicit operator bool() const noexcept;
   constexpr bool has_value() const noexcept;
+
+  void swap(Optional& other);
 };
 
 template <typename T>
 constexpr Optional<typename std::decay<T>::type> make_optional(T&& v);
 
 template <typename T, typename... Args>
 constexpr Optional<T> make_optional(Args&&... args);
 
void target() {
       $ns::$optional<int> opt = 3;
       (opt = $ns::nullopt).value();
       /*[[check]]*/
     }
   )",
       UnorderedElementsAre(Pair("check", "unsafe: input.cc:6:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, OptionalSwap) {
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    void target() {
+      $ns::$optional<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> 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<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> 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<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> 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<int> opt1 = $ns::nullopt;
+      $ns::$optional<int> 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