[clang] cd0d526 - [clang][dataflow] In `optional` model, match call return via hasType

[Yitzhak Mandelbaum via cfe-commits]

Author: Sam Estep
Date: 2022-06-10T14:52:05Z
New Revision: cd0d52610d80116324f19983320ec6db2048450f

URL: https://github.com/llvm/llvm-project/commit/cd0d52610d80116324f19983320ec6db2048450f
DIFF: https://github.com/llvm/llvm-project/commit/cd0d52610d80116324f19983320ec6db2048450f.diff

LOG: [clang][dataflow] In `optional` model, match call return via hasType

Currently the unchecked-optional-access model fails on this example:

    #include <memory>
    #include <optional>

    void foo() {
      std::unique_ptr<std::optional<float>> x;
      *x = std::nullopt;
    }

You can verify the failure by saving the file as `foo.cpp` and running this command:

    clang-tidy -checks='-*,bugprone-unchecked-optional-access' foo.cpp -- -std=c++17

The failing `assert` is in the `transferAssignment` function of the `UncheckedOptionalAccessModel.cpp` file:

    assert(OptionalLoc != nullptr);

The symptom can be treated by replacing that `assert` with an early `return`:

    if (OptionalLoc == nullptr)
      return;

That would be better anyway since we cannot expect to always cover all possible LHS expressions, but it is out of scope for this patch and left as a followup.

Note that the failure did not occur on this very similar example:

    #include <optional>

    template <typename T>
    struct smart_ptr {
      T& operator*() &;
      T* operator->();
    };

    void foo() {
      smart_ptr<std::optional<float>> x;
      *x = std::nullopt;
    }

The difference is caused by the `isCallReturningOptional` matcher, which was previously checking the `functionDecl` of the `callee`. This patch changes it to instead use `hasType` directly on the call expression, fixing the failure for the `std::unique_ptr` example above.

Reviewed By: sgatev

Differential Revision: https://reviews.llvm.org/D127434

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
    clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 00d22fbe8bb80..993d56e44a87d 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -165,8 +165,8 @@ auto isValueOrNotEqX() {
 }
 
 auto isCallReturningOptional() {
-  return callExpr(callee(functionDecl(returns(anyOf(
-      optionalOrAliasType(), referenceType(pointee(optionalOrAliasType())))))));
+  return callExpr(hasType(qualType(anyOf(
+      optionalOrAliasType(), referenceType(pointee(optionalOrAliasType()))))));
 }
 
 /// Creates a symbolic value for an `optional` value using `HasValueVal` as the

diff  --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 72efc724776b3..3c37bec82139c 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -179,6 +179,11 @@ struct is_void : is_same<void, typename remove_cv<T>::type> {};
 
 namespace detail {
 
+template <class T>
+auto try_add_lvalue_reference(int) -> type_identity<T&>;
+template <class T>
+auto try_add_lvalue_reference(...) -> type_identity<T>;
+
 template <class T>
 auto try_add_rvalue_reference(int) -> type_identity<T&&>;
 template <class T>
@@ -186,6 +191,10 @@ auto try_add_rvalue_reference(...) -> type_identity<T>;
 
 }  // namespace detail
 
+template <class T>
+struct add_lvalue_reference : decltype(detail::try_add_lvalue_reference<T>(0)) {
+};
+
 template <class T>
 struct add_rvalue_reference : decltype(detail::try_add_rvalue_reference<T>(0)) {
 };
@@ -2318,6 +2327,26 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
       UnorderedElementsAre(Pair("merge", "unsafe: input.cc:19:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, AssignThroughLvalueReferencePtr) {
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    template <typename T>
+    struct smart_ptr {
+      typename std::add_lvalue_reference<T>::type operator*() &;
+    };
+
+    void target() {
+      smart_ptr<$ns::$optional<float>> x;
+      *x = $ns::nullopt;
+      (*x).value();
+      /*[[check]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check", "unsafe: input.cc:12:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)