[clang][dataflow] Improve optional model's support for ignoring smart pointers.

[ymandel]

Summary

The optional model has an option to ignore optionals accessed through smart
pointer types (other than optional itself). This patch improves this feature in
two ways:

1. We extend support to optionals accessed directly through the smart pointer,

like ptr->value(). Previously, support was limited to cases that went through
an intermediate field.

2. We clean up the implementation by removing the option from the analysis,

leaving it only in the diagnostic phase (where it is relevant).

3. Adjusts a test which was misleading in what it was testing.

clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h

 
 namespace clang {
 namespace dataflow {
 
 // FIXME: Explore using an allowlist-approach, where constructs supported by the
 // analysis are always enabled and additional constructs are enabled through the
 // `Options`.
 struct UncheckedOptionalAccessModelOptions {
-  /// Ignore optionals reachable through overloaded `operator*` or `operator->`
-  /// (other than those of the optional type itself). The analysis does not
-  /// equate the results of such calls, so it can't identify when their results
-  /// are used safely (across calls), resulting in false positives in all such
-  /// cases. Note: this option does not cover access through `operator[]`.
+  /// In generating diagnostics, ignore optionals reachable through overloaded
+  /// `operator*` or `operator->` (other than those of the optional type
+  /// itself). The analysis does not equate the results of such calls, so it
+  /// can't identify when their results are used safely (across calls),
+  /// resulting in false positives in all such cases. Note: this option does not
+  /// cover access through `operator[]`.
   bool IgnoreSmartPointerDereference = false;
 };
 
 /// Dataflow analysis that models whether optionals hold values or not.
 ///
 /// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
 class UncheckedOptionalAccessModel
     : public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> {
 public:
-  UncheckedOptionalAccessModel(
-      ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options = {});
+  UncheckedOptionalAccessModel(ASTContext &Ctx);
 
   /// Returns a matcher for the optional classes covered by this model.
   static ast_matchers::DeclarationMatcher optionalClassDecl();
 
   static NoopLattice initialElement() { return {}; }
 
   void transfer(const CFGElement *Elt, NoopLattice &L, Environment &Env);
 

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

return hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(optionalClass())));
 }
 
 /// Matches any of the spellings of the optional types and sugar, aliases, etc.
 auto hasOptionalType() { return hasType(optionalOrAliasType()); }
 
 auto isOptionalMemberCallWithName(
     llvm::StringRef MemberName,
-    llvm::Optional<StatementMatcher> Ignorable = std::nullopt) {
+    const llvm::Optional<StatementMatcher> &Ignorable = std::nullopt) {
   auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
                                     : cxxThisExpr());
   return cxxMemberCallExpr(
       on(expr(Exception)),
       callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
 }
 
 auto isOptionalOperatorCallWithName(
     llvm::StringRef operator_name,
-    llvm::Optional<StatementMatcher> Ignorable = std::nullopt) {
+    const llvm::Optional<StatementMatcher> &Ignorable = std::nullopt) {
   return cxxOperatorCallExpr(
       hasOverloadedOperatorName(operator_name),
       callee(cxxMethodDecl(ofClass(optionalClass()))),
       Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
 }
 
 auto isMakeOptionalCall() {
   return callExpr(
if (CmpExpr->getOperator() == clang::OO_ExclaimEqual)
       CmpValue = &Env.makeNot(*CmpValue);
     Env.addToFlowCondition(evaluateEquality(Env, *CmpValue, *HasVal,
                                             Env.getBoolLiteralValue(true)));
   }
 }
 
 llvm::Optional<StatementMatcher>
 ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
-  if (Options.IgnoreSmartPointerDereference)
-    return memberExpr(hasObjectExpression(ignoringParenImpCasts(
-        cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"),
-                                  hasOverloadedOperatorName("*")),
-                            unless(hasArgument(0, expr(hasOptionalType())))))));
+  if (Options.IgnoreSmartPointerDereference) {
+    auto SmartPtrUse = expr(ignoringParenImpCasts(cxxOperatorCallExpr(
+        anyOf(hasOverloadedOperatorName("->"), hasOverloadedOperatorName("*")),
+        unless(hasArgument(0, expr(hasOptionalType()))))));
+    return expr(
+        anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse))));
+  }
   return std::nullopt;
 }
 
 StatementMatcher
-valueCall(llvm::Optional<StatementMatcher> &IgnorableOptional) {
+valueCall(const llvm::Optional<StatementMatcher> &IgnorableOptional) {
   return isOptionalMemberCallWithName("value", IgnorableOptional);
 }
 
 StatementMatcher
-valueOperatorCall(llvm::Optional<StatementMatcher> &IgnorableOptional) {
+valueOperatorCall(const llvm::Optional<StatementMatcher> &IgnorableOptional) {
   return expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional),
                     isOptionalOperatorCallWithName("->", IgnorableOptional)));
 }
 
-auto buildTransferMatchSwitch(
-    const UncheckedOptionalAccessModelOptions &Options) {
+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.
-  auto IgnorableOptional = ignorableOptional(Options);
   return CFGMatchSwitchBuilder<LatticeTransferState>()
       // Attach a symbolic "has_value" state to optional values that we see for
       // the first time.
       .CaseOfCFGStmt<Expr>(
           expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
           initializeOptionalReference)
 
       // make_optional
return CFGMatchSwitchBuilder<LatticeTransferState>()
       .CaseOfCFGStmt<CXXOperatorCallExpr>(
           isOptionalValueOrConversionAssignment(),
           transferValueOrConversionAssignment)
       .CaseOfCFGStmt<CXXOperatorCallExpr>(isOptionalNulloptAssignment(),
                                           transferNulloptAssignment)
 
       // optional::value
       .CaseOfCFGStmt<CXXMemberCallExpr>(
-          valueCall(IgnorableOptional),
+          valueCall(std::nullopt),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
           })
 
       // optional::operator*, optional::operator->
-      .CaseOfCFGStmt<CallExpr>(valueOperatorCall(IgnorableOptional),
+      .CaseOfCFGStmt<CallExpr>(valueOperatorCall(std::nullopt),
                                [](const CallExpr *E,
                                   const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
                                  transferUnwrapCall(E, E->getArg(0), State);
                                })
 
       // optional::has_value
       .CaseOfCFGStmt<CXXMemberCallExpr>(

 
 } // namespace
 
 ast_matchers::DeclarationMatcher
 UncheckedOptionalAccessModel::optionalClassDecl() {
   return optionalClass();
 }
 
-UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
-    ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
     : DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
-      TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
+      TransferMatchSwitch(buildTransferMatchSwitch()) {}
 
 void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt,
                                             NoopLattice &L, Environment &Env) {
   LatticeTransferState State(L, Env);
   TransferMatchSwitch(*Elt, getASTContext(), State);
 }
 
 ComparisonResult UncheckedOptionalAccessModel::compare(

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Headers.emplace_back("unchecked_optional_access_test.h", R"(
       T Make();
     )");
     UncheckedOptionalAccessModelOptions Options{
         /*IgnoreSmartPointerDereference=*/true};
     std::vector<SourceLocation> Diagnostics;
     llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
         AnalysisInputs<UncheckedOptionalAccessModel>(
             SourceCode, std::move(FuncMatcher),
-            [Options](ASTContext &Ctx, Environment &) {
-              return UncheckedOptionalAccessModel(Ctx, Options);
+            [](ASTContext &Ctx, Environment &) {
+              return UncheckedOptionalAccessModel(Ctx);
             })
             .withPostVisitCFG(
                 [&Diagnostics,
                  Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
                     ASTContext &Ctx, const CFGElement &Elt,
                     const TransferStateForDiagnostics<NoopLattice>
                         &State) mutable {
                   auto EltDiagnostics =
void target() {
 
       opt1.value();
 
       opt2.value(); // [[unsafe]]
     }
   )");
 }
 
+TEST_P(UncheckedOptionalAccessTest, UniquePtrToOptional) {
+  // We suppress diagnostics for optionals in smart pointers (other than
+  // `optional` itself).
+  ExpectDiagnosticsFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    template <typename T>
+    struct smart_ptr {
+      T& operator*() &;
+      T* operator->();
+    };
+
+    void target() {
+      smart_ptr<$ns::$optional<bool>> foo;
+      foo->value();
+      (*foo).value();
+    }
+  )");
+}
+
 TEST_P(UncheckedOptionalAccessTest, UniquePtrToStructWithOptionalField) {
-  // We suppress diagnostics for values reachable from smart pointers (other
-  // than `optional` itself).
+  // We suppress diagnostics for optional fields reachable from smart pointers
+  // (other than `optional` itself) through (exactly) one member access.
   ExpectDiagnosticsFor(
       R"(
     #include "unchecked_optional_access_test.h"
 
     template <typename T>
     struct smart_ptr {
       T& operator*() &;
       T* operator->();
void target($ns::$optional<Foo> foo, bool b) {
       }
       // Now we merge the two values. UncheckedOptionalAccessModel::merge() will
       // throw away the "value" property.
       foo->value(); // [[unsafe]]
     }
   )");
 }
 
+// This test is aimed at the core model, not the diagnostic. It is a regression
+// test against a crash when using non-trivial smart pointers, like
+// `std::unique_ptr`. As such, it doesn't test the access itself, which would be
+// ignored regardless because of `IgnoreSmartPointerDereference = true`, above.
 TEST_P(UncheckedOptionalAccessTest, AssignThroughLvalueReferencePtr) {
   ExpectDiagnosticsFor(
       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(); // [[unsafe]]
+      smart_ptr<$ns::$optional<int>> x;
+      // Verify that this assignment does not crash.
+      *x = 3;
     }
   )");
 }
 
 TEST_P(UncheckedOptionalAccessTest, CorrelatedBranches) {
   ExpectDiagnosticsFor(R"code(
     #include "unchecked_optional_access_test.h"