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

[samestep]

Summary

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.

[ymandel]

Is there a test you can add that would cover this change?

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

170

Since we're using the ast_matchers namespace, no need to qualify. Also, there's a type alias TypeMatcher that's not in the internal namespace which would be good here. That said, I'd use QualType and write it like this:

auto TypeMatcher = qualType(anyOf(...));

Or, I think even better in this case, just inline it:

callExpr(hasType(anyOf(...)))

[samestep]

A test would definitely be good, but I'm not sure how to add one to UncheckedOptionalAccessModelTest.cpp, since the issue disappears when we replace std::unique_ptr with our custom simple smart_ptr. Any ideas?

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

170

I initially tried inlining it like you suggested, but got a compile error about multiple matching overloads. Adding qualType fixed that though, thanks!

[samestep]

Simplify body using qualType

[sgatev]

I see that the declaration of operator* in std::unique_ptr is typename add_lvalue_reference<T>::type operator*() const;. I managed to reproduce the crash with the following snippet:

#include <optional>

namespace detail {
 
template <class T>
struct type_identity { using type = T; };

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>
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)) {};

template <typename T>
struct smart_ptr {
  typename add_lvalue_reference<T>::type operator*() &;
};

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

I suggest adding the definition of add_lvalue_reference to StdTypeTraitsHeader and adding a test with the rest of the code above.

[samestep]

@sgatev Oh nice, thank you for figuring that out! @ymandel and I saw that on Wednesday and tried for a while to figure out how to encode it into a test using the sample implementation from cppreference, but I think we kept forgetting the ::type part. I'll add this as a test!

[samestep]

Add a test

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

auto isValueOrNotEqX() {
   // nodes, which would let us generalize this to any `X`.
   return binaryOperation(hasOperatorName("!="),
                          anyOf(ComparesToSame(cxxNullPtrLiteralExpr()),
                                ComparesToSame(stringLiteral(hasSize(0))),
                                ComparesToSame(integerLiteral(equals(0)))));
 }
 
 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
 /// 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));
 }

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

 struct is_same<T, T> : true_type {};
 
 template <class T>
 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>
 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)) {
 };
 
 template <class T>
 typename add_rvalue_reference<T>::type declval() noexcept;
 
 namespace detail {
 
void target($ns::$optional<Foo> foo, bool b) {
       // throw away the "value" property.
       foo->value();
       /*[[merge]]*/
     }
   )",
       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)
 // - invalidation (passing optional by non-const reference/pointer)