[clang][dataflow] Add support for disabling warnings on smart pointers.

This patch provides the user with the ability to disable all checked of accesses
to optionals that are the pointees of smart pointers. Since smart pointers are
not modeled (yet), the system cannot distinguish safe from unsafe accesses to
optionals through smart pointers. This results in false positives whenever
optionals are used through smart pointers. The patch gives the user the choice
of ignoring all positivess in these cases.

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

diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 05a9165..235121b 100644 (file)
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -24,6 +24,18 @@
 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[]`.
+  bool IgnoreSmartPointerDereference = false;
+};
+
 /// Dataflow analysis that discovers unsafe accesses of optional values and
 /// adds the respective source locations to the lattice.
 ///
@@ -34,7 +46,8 @@ class UncheckedOptionalAccessModel
     : public DataflowAnalysis<UncheckedOptionalAccessModel,
                               SourceLocationsLattice> {
 public:
-  explicit UncheckedOptionalAccessModel(ASTContext &AstContext);
+  UncheckedOptionalAccessModel(
+      ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {});
 
   static SourceLocationsLattice initialElement() {
     return SourceLocationsLattice();

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index bf325b0..b775698 100644 (file)
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -45,15 +45,23 @@ auto optionalClass() {
 
 auto hasOptionalType() { return hasType(optionalClass()); }
 
-auto isOptionalMemberCallWithName(llvm::StringRef MemberName) {
+auto isOptionalMemberCallWithName(
+    llvm::StringRef MemberName,
+    llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
+  auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
+                                    : cxxThisExpr());
   return cxxMemberCallExpr(
-      on(expr(unless(cxxThisExpr()))),
+      on(expr(Exception)),
       callee(cxxMethodDecl(hasName(MemberName), ofClass(optionalClass()))));
 }
 
-auto isOptionalOperatorCallWithName(llvm::StringRef OperatorName) {
-  return cxxOperatorCallExpr(hasOverloadedOperatorName(OperatorName),
-                             callee(cxxMethodDecl(ofClass(optionalClass()))));
+auto isOptionalOperatorCallWithName(
+    llvm::StringRef operator_name,
+    llvm::Optional<StatementMatcher> Ignorable = llvm::None) {
+  return cxxOperatorCallExpr(
+      hasOverloadedOperatorName(operator_name),
+      callee(cxxMethodDecl(ofClass(optionalClass()))),
+      Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
 }
 
 auto isMakeOptionalCall() {
@@ -333,10 +341,22 @@ void transferStdSwapCall(const CallExpr *E, const MatchFinder::MatchResult &,
   transferSwap(*OptionalLoc1, *OptionalLoc2, State);
 }
 
-auto buildTransferMatchSwitch() {
+llvm::Optional<StatementMatcher>
+ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
+  if (Options.IgnoreSmartPointerDereference)
+    return memberExpr(hasObjectExpression(ignoringParenImpCasts(
+        cxxOperatorCallExpr(anyOf(hasOverloadedOperatorName("->"),
+                                  hasOverloadedOperatorName("*")),
+                            unless(hasArgument(0, expr(hasOptionalType())))))));
+  return llvm::None;
+}
+
+auto buildTransferMatchSwitch(
+    const UncheckedOptionalAccessModelOptions &Options) {
   // 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 MatchSwitchBuilder<LatticeTransferState>()
       // Attach a symbolic "has_value" state to optional values that we see for
       // the first time.
@@ -371,19 +391,20 @@ auto buildTransferMatchSwitch() {
 
       // optional::value
       .CaseOf<CXXMemberCallExpr>(
-          isOptionalMemberCallWithName("value"),
+          isOptionalMemberCallWithName("value", IgnorableOptional),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
           })
 
       // optional::operator*, optional::operator->
-      .CaseOf<CallExpr>(expr(anyOf(isOptionalOperatorCallWithName("*"),
-                                   isOptionalOperatorCallWithName("->"))),
-                        [](const CallExpr *E, const MatchFinder::MatchResult &,
-                           LatticeTransferState &State) {
-                          transferUnwrapCall(E, E->getArg(0), State);
-                        })
+      .CaseOf<CallExpr>(
+          expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional),
+                     isOptionalOperatorCallWithName("->", IgnorableOptional))),
+          [](const CallExpr *E, const MatchFinder::MatchResult &,
+             LatticeTransferState &State) {
+            transferUnwrapCall(E, E->getArg(0), State);
+          })
 
       // optional::has_value
       .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("has_value"),
@@ -423,10 +444,11 @@ auto buildTransferMatchSwitch() {
 
 } // namespace
 
-UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(ASTContext &Ctx)
+UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
+    ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
     : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
           Ctx),
-      TransferMatchSwitch(buildTransferMatchSwitch()) {}
+      TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
 
 void UncheckedOptionalAccessModel::transfer(const Stmt *S,
                                             SourceLocationsLattice &L,

diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 76340aa..6ef4b2a 100644 (file)
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1219,7 +1219,9 @@ private:
     llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
         SourceCode, FuncMatcher,
         [](ASTContext &Ctx, Environment &) {
-          return UncheckedOptionalAccessModel(Ctx);
+          return UncheckedOptionalAccessModel(
+              Ctx, UncheckedOptionalAccessModelOptions{
+                       /*IgnoreSmartPointerDereference=*/true});
         },
         [&MatchesLatticeChecks](
             llvm::ArrayRef<std::pair<
@@ -2004,6 +2006,34 @@ TEST_P(UncheckedOptionalAccessTest, StdSwap) {
                            Pair("check-4", "unsafe: input.cc:13:7")));
 }
 
+TEST_P(UncheckedOptionalAccessTest, UniquePtrToStructWithOptionalField) {
+  // We suppress diagnostics for values reachable from smart pointers (other
+  // than `optional` itself).
+  ExpectLatticeChecksFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    template <typename T>
+    struct smart_ptr {
+      T& operator*() &;
+      T* operator->();
+    };
+
+    struct Foo {
+      $ns::$optional<int> opt;
+    };
+
+    void target() {
+      smart_ptr<Foo> foo;
+      *foo->opt;
+      /*[[check-1]]*/
+      *(*foo).opt;
+      /*[[check-2]]*/
+    }
+  )",
+      UnorderedElementsAre(Pair("check-1", "safe"), Pair("check-2", "safe")));
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)