[clang][dataflow] Model calls returning optionals

[sgatev]

Summary

Model calls returning optionals

[ymandel]

Thanks!

[xazax.hun]

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

166

Does alias refers to a type alias in this case? I first parsed this as if this has something to do with pointers being aliased. I wonder if possiblyOptionalAliasType would read better (considering the optional type its own alias). Feel free to ignore.

[sgatev]

Address comments.

[sgatev]

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

166

I opted for hasOptionalOrAliasType.

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

return classTemplateSpecializationDecl(
       anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
             hasName("__optional_destruct_base"), hasName("absl::optional"),
             hasName("base::Optional")),
       hasTemplateArgument(0, refersToType(type().bind("T"))));
 }
 
 auto hasOptionalType() { return hasType(optionalClass()); }
 
+auto hasOptionalOrAliasType() {
+  return hasUnqualifiedDesugaredType(
+      recordType(hasDeclaration(optionalClass())));
+}
+
 auto isOptionalMemberCallWithName(
     llvm::StringRef MemberName,
     llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
                                     : cxxThisExpr());
   return cxxMemberCallExpr(
       on(expr(Exception)),
       callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
auto isValueOrNotEqX() {
   // seen in practice.  FIXME: define a matcher that compares values across
   // 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(hasOptionalOrAliasType(),
+                    referenceType(pointee(hasOptionalOrAliasType())))))));
+}
+
 /// 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));
 }
 
transferValueOrImpl(ComparisonExpr, Result, State,
                          BoolValue &HasValueVal) -> BoolValue & {
                         // We know that if `(opt.value_or(X) != X)` then
                         // `opt.hasValue()`, even without knowing further
                         // details about the contents of `opt`.
                         return Env.makeImplication(ExprVal, HasValueVal);
                       });
 }
 
+void transferCallReturningOptional(const CallExpr *E,
+                                   const MatchFinder::MatchResult &Result,
+                                   LatticeTransferState &State) {
+  if (State.Env.getStorageLocation(*E, SkipPast::None) != nullptr)
+    return;
+
+  auto &Loc = State.Env.createStorageLocation(*E);
+  State.Env.setStorageLocation(*E, Loc);
+  State.Env.setValue(
+      Loc, createOptionalValue(State.Env, State.Env.makeAtomicBoolValue()));
+}
+
 void assignOptionalValue(const Expr &E, LatticeTransferState &State,
                          BoolValue &HasValueVal) {
   if (auto *OptionalLoc =
           State.Env.getStorageLocation(E, SkipPast::ReferenceThenPointer)) {
     State.Env.setValue(*OptionalLoc,
                        createOptionalValue(State.Env, HasValueVal));
   }
 }
return MatchSwitchBuilder<LatticeTransferState>()
       .CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
 
       // opt.value_or("").empty()
       .CaseOf<Expr>(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall)
 
       // opt.value_or(X) != X
       .CaseOf<Expr>(isValueOrNotEqX(), transferValueOrNotEqX)
 
+      // returns optional
+      .CaseOf<CallExpr>(isCallReturningOptional(),
+                        transferCallReturningOptional)
+
       .Build();
 }
 
 } // namespace
 
 ast_matchers::DeclarationMatcher
 UncheckedOptionalAccessModel::optionalClassDecl() {
   return optionalClass();

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

void target() {
       /*[[check-1]]*/
       *(*foo).opt;
       /*[[check-2]]*/
     }
   )",
       UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, CallReturningOptional) {
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    $ns::$optional<int> MakeOpt();
+
+    void target() {
+      $ns::$optional<int> opt = 0;
+      opt = MakeOpt();
+      opt.value();
+      /*[[check-1]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-1", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    const $ns::$optional<int>& MakeOpt();
+
+    void target() {
+      $ns::$optional<int> opt = 0;
+      opt = MakeOpt();
+      opt.value();
+      /*[[check-2]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-2", "unsafe: input.cc:9:7")));
+
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    using IntOpt = $ns::$optional<int>;
+    IntOpt MakeOpt();
+
+    void target() {
+      IntOpt opt = 0;
+      opt = MakeOpt();
+      opt.value();
+      /*[[check-3]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-3", "unsafe: input.cc:10:7")));
+
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    using IntOpt = $ns::$optional<int>;
+    const IntOpt& MakeOpt();
+
+    void target() {
+      IntOpt opt = 0;
+      opt = MakeOpt();
+      opt.value();
+      /*[[check-4]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-4", "unsafe: input.cc:10:7")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)
 // - invalidation (passing optional by non-const reference/pointer)
 // - nested `optional` values