[clang][dataflow] Replace usage of deprecated functions with the optional check

[wyt]

Summary

• Update transfer and diagnose to take const CFGElement * as input in Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.
• Update clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp accordingly.
• Rename runDataflowAnalysisOnCFG to runDataflowAnalysis and remove the deprecated runDataflowAnalysis (this was only used by the now updated optional check).

[sgatev]

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

76

Why not Ctx like above?

[wyt]

Address comment.

[wyt]

Update UncheckedOptionalAccessCheck.cpp.
Remove deprecated runDataflowAnalysis.

clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp

analyzeFunction(const FunctionDecl &FuncDecl, ASTContext &ASTCtx) {
   UncheckedOptionalAccessModel Analysis(ASTCtx);
   UncheckedOptionalAccessDiagnoser Diagnoser;
   std::vector<SourceLocation> Diagnostics;
   Expected<std::vector<
       Optional<DataflowAnalysisState<UncheckedOptionalAccessModel::Lattice>>>>
       BlockToOutputState = dataflow::runDataflowAnalysis(
           *Context, Analysis, Env,
           [&ASTCtx, &Diagnoser, &Diagnostics](
-              const CFGStmt &Stmt,
+              const CFGElement &Elt,
               const DataflowAnalysisState<UncheckedOptionalAccessModel::Lattice>
                   &State) mutable {
-            auto StmtDiagnostics =
-                Diagnoser.diagnose(ASTCtx, Stmt.getStmt(), State.Env);
-            llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
+            auto EltDiagnostics = Diagnoser.diagnose(ASTCtx, &Elt, State.Env);
+            llvm::move(EltDiagnostics, std::back_inserter(Diagnostics));
           });
   if (!BlockToOutputState)
     return llvm::None;
 
   return Diagnostics;
 }
 
 void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {

clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h

 template <typename LatticeT> struct DataflowAnalysisState {
   // Model of a program property.
   LatticeT Lattice;
 
   // Model of the state of the program (store and heap).
   Environment Env;
 };
 
-// FIXME: Rename to `runDataflowAnalysis` after usages of the overload that
-// applies to `CFGStmt` have been replaced.
-//
 /// Performs dataflow analysis and returns a mapping from basic block IDs to
 /// dataflow analysis states that model the respective basic blocks. The
 /// returned vector, if any, will have the same size as the number of CFG
 /// blocks, with indices corresponding to basic block IDs. Returns an error if
 /// the dataflow analysis cannot be performed successfully. Otherwise, calls
 /// `PostVisitCFG` on each CFG element with the final analysis results at that
 /// program point.
 template <typename AnalysisT>
 llvm::Expected<std::vector<
     llvm::Optional<DataflowAnalysisState<typename AnalysisT::Lattice>>>>
-runDataflowAnalysisOnCFG(
+runDataflowAnalysis(
     const ControlFlowContext &CFCtx, AnalysisT &Analysis,
     const Environment &InitEnv,
     std::function<void(const CFGElement &, const DataflowAnalysisState<
                                                typename AnalysisT::Lattice> &)>
         PostVisitCFG = nullptr) {
   std::function<void(const CFGElement &,
                      const TypeErasedDataflowAnalysisState &)>
       PostVisitCFGClosure = nullptr;
llvm::transform(std::move(*TypeErasedBlockStates),
                           llvm::any_cast<typename AnalysisT::Lattice>(
                               std::move(State.Lattice.Value)),
                           std::move(State.Env)};
                     });
                   });
   return BlockStates;
 }
 
-/// Deprecated. Use `runDataflowAnalysisOnCFG` instead.
-///
-/// Performs dataflow analysis and returns a mapping from basic block IDs to
-/// dataflow analysis states that model the respective basic blocks. The
-/// returned vector, if any, will have the same size as the number of CFG
-/// blocks, with indices corresponding to basic block IDs. Returns an error if
-/// the dataflow analysis cannot be performed successfully. Otherwise, calls
-/// `PostVisitStmt` on each statement with the final analysis results at that
-/// program point.
-template <typename AnalysisT>
-llvm::Expected<std::vector<
-    llvm::Optional<DataflowAnalysisState<typename AnalysisT::Lattice>>>>
-runDataflowAnalysis(
-    const ControlFlowContext &CFCtx, AnalysisT &Analysis,
-    const Environment &InitEnv,
-    std::function<void(const CFGStmt &, const DataflowAnalysisState<
-                                            typename AnalysisT::Lattice> &)>
-        PostVisitStmt = nullptr) {
-  std::function<void(
-      const CFGElement &,
-      const DataflowAnalysisState<typename AnalysisT::Lattice> &)>
-      PostVisitCFG = nullptr;
-  if (PostVisitStmt) {
-    PostVisitCFG =
-        [&PostVisitStmt](
-            const CFGElement &Element,
-            const DataflowAnalysisState<typename AnalysisT::Lattice> &State) {
-          if (auto Stmt = Element.getAs<CFGStmt>()) {
-            PostVisitStmt(*Stmt, State);
-          }
-        };
-  }
-  return runDataflowAnalysisOnCFG(CFCtx, Analysis, InitEnv, PostVisitCFG);
-}
-
 /// Abstract base class for dataflow "models": reusable analysis components that
 /// model a particular aspect of program semantics in the `Environment`. For
 /// example, a model may capture a type and its related functions.
 class DataflowModel : public Environment::ValueModel {
 public:
   /// Return value indicates whether the model processed the `Stmt`.
   virtual bool transfer(const Stmt *Stmt, Environment &Env) = 0;
 };
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_DATAFLOWANALYSIS_H

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

 //  values.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H
 #define CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H
 
 #include "clang/AST/ASTContext.h"
-#include "clang/AST/Stmt.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
-#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/NoopLattice.h"
 #include "clang/Basic/SourceLocation.h"
 #include <vector>
 
 namespace clang {
 namespace dataflow {
 
 // FIXME: Explore using an allowlist-approach, where constructs supported by the

 
 /// 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 &AstContext, UncheckedOptionalAccessModelOptions Options = {});
+      ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options = {});
 
   /// Returns a matcher for the optional classes covered by this model.
   static ast_matchers::DeclarationMatcher optionalClassDecl();
 
   static NoopLattice initialElement() { return {}; }
 
-  void transfer(const Stmt *Stmt, NoopLattice &State, Environment &Env);
+  void transfer(const CFGElement *Elt, NoopLattice &L, Environment &Env);
 
   bool compareEquivalent(QualType Type, const Value &Val1,
                          const Environment &Env1, const Value &Val2,
                          const Environment &Env2) override;
 
   bool merge(QualType Type, const Value &Val1, const Environment &Env1,
              const Value &Val2, const Environment &Env2, Value &MergedVal,
              Environment &MergedEnv) override;
 
 private:
-  MatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
+  CFGMatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
 };
 
 class UncheckedOptionalAccessDiagnoser {
 public:
   UncheckedOptionalAccessDiagnoser(
       UncheckedOptionalAccessModelOptions Options = {});
 
-  std::vector<SourceLocation> diagnose(ASTContext &Context, const Stmt *Stmt,
+  std::vector<SourceLocation> diagnose(ASTContext &Ctx, const CFGElement *Elt,
                                        const Environment &Env);
 
 private:
-  MatchSwitch<const Environment, std::vector<SourceLocation>>
+  CFGMatchSwitch<const Environment, std::vector<SourceLocation>>
       DiagnoseMatchSwitch;
 };
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H

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

 
 #include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/Stmt.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/CFG.h"
+#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
-#include "clang/Analysis/FlowSensitive/MatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/NoopLattice.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include <cassert>
 #include <memory>
 #include <utility>

 }
 
 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>()
+  return CFGMatchSwitchBuilder<LatticeTransferState>()
       // Attach a symbolic "has_value" state to optional values that we see for
       // the first time.
-      .CaseOf<Expr>(
+      .CaseOfCFGStmt<Expr>(
           expr(anyOf(declRefExpr(), memberExpr()), hasOptionalType()),
           initializeOptionalReference)
 
       // make_optional
-      .CaseOf<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
+      .CaseOfCFGStmt<CallExpr>(isMakeOptionalCall(), transferMakeOptionalCall)
 
       // optional::optional
-      .CaseOf<CXXConstructExpr>(
+      .CaseOfCFGStmt<CXXConstructExpr>(
           isOptionalInPlaceConstructor(),
           [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             assignOptionalValue(*E, State, State.Env.getBoolLiteralValue(true));
           })
-      .CaseOf<CXXConstructExpr>(
+      .CaseOfCFGStmt<CXXConstructExpr>(
           isOptionalNulloptConstructor(),
           [](const CXXConstructExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             assignOptionalValue(*E, State,
                                 State.Env.getBoolLiteralValue(false));
           })
-      .CaseOf<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
+      .CaseOfCFGStmt<CXXConstructExpr>(isOptionalValueOrConversionConstructor(),
                                        transferValueOrConversionConstructor)
 
       // optional::operator=
-      .CaseOf<CXXOperatorCallExpr>(isOptionalValueOrConversionAssignment(),
-                                   transferValueOrConversionAssignment)
-      .CaseOf<CXXOperatorCallExpr>(isOptionalNulloptAssignment(),
+      .CaseOfCFGStmt<CXXOperatorCallExpr>(
+          isOptionalValueOrConversionAssignment(),
+      .CaseOfCFGStmt<CXXOperatorCallExpr>(isOptionalNulloptAssignment(),
                                           transferNulloptAssignment)
 
       // optional::value
-      .CaseOf<CXXMemberCallExpr>(
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
           valueCall(IgnorableOptional),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             transferUnwrapCall(E, E->getImplicitObjectArgument(), State);
           })
 
       // optional::operator*, optional::operator->
-      .CaseOf<CallExpr>(valueOperatorCall(IgnorableOptional),
-                        [](const CallExpr *E, const MatchFinder::MatchResult &,
+      .CaseOfCFGStmt<CallExpr>(valueOperatorCall(IgnorableOptional),
+                               [](const CallExpr *E,
+                                  const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
                                  transferUnwrapCall(E, E->getArg(0), State);
                                })
 
       // optional::has_value
-      .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("has_value"),
-                                 transferOptionalHasValueCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithName("has_value"),
 
       // optional::operator bool
-      .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("operator bool"),
-                                 transferOptionalHasValueCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithName("operator bool"),
 
       // optional::emplace
-      .CaseOf<CXXMemberCallExpr>(
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("emplace"),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             assignOptionalValue(*E->getImplicitObjectArgument(), State,
                                 State.Env.getBoolLiteralValue(true));
           })
 
       // optional::reset
-      .CaseOf<CXXMemberCallExpr>(
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
           isOptionalMemberCallWithName("reset"),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             assignOptionalValue(*E->getImplicitObjectArgument(), State,
                                 State.Env.getBoolLiteralValue(false));
           })
 
       // optional::swap
-      .CaseOf<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
+      .CaseOfCFGStmt<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
                                         transferSwapCall)
 
       // std::swap
-      .CaseOf<CallExpr>(isStdSwapCall(), transferStdSwapCall)
+      .CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
 
       // opt.value_or("").empty()
-      .CaseOf<Expr>(isValueOrStringEmptyCall(), transferValueOrStringEmptyCall)
+      .CaseOfCFGStmt<Expr>(isValueOrStringEmptyCall(),
+                           transferValueOrStringEmptyCall)
 
       // opt.value_or(X) != X
-      .CaseOf<Expr>(isValueOrNotEqX(), transferValueOrNotEqX)
+      .CaseOfCFGStmt<Expr>(isValueOrNotEqX(), transferValueOrNotEqX)
 
       // returns optional
-      .CaseOf<CallExpr>(isCallReturningOptional(),
+      .CaseOfCFGStmt<CallExpr>(isCallReturningOptional(),
                                transferCallReturningOptional)
 
       .Build();
 }
 
 std::vector<SourceLocation> diagnoseUnwrapCall(const Expr *UnwrapExpr,
                                                const Expr *ObjectExpr,
                                                const Environment &Env) {
   if (auto *OptionalVal =

 }
 
 auto buildDiagnoseMatchSwitch(
     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<const Environment, std::vector<SourceLocation>>()
+  return CFGMatchSwitchBuilder<const Environment, std::vector<SourceLocation>>()
       // optional::value
-      .CaseOf<CXXMemberCallExpr>(
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
           valueCall(IgnorableOptional),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              const Environment &Env) {
             return diagnoseUnwrapCall(E, E->getImplicitObjectArgument(), Env);
           })
 
       // optional::operator*, optional::operator->
-      .CaseOf<CallExpr>(
+      .CaseOfCFGStmt<CallExpr>(
           valueOperatorCall(IgnorableOptional),
           [](const CallExpr *E, const MatchFinder::MatchResult &,
              const Environment &Env) {
             return diagnoseUnwrapCall(E, E->getArg(0), Env);
           })
       .Build();
 }
 
 } // namespace
 
 ast_matchers::DeclarationMatcher
 UncheckedOptionalAccessModel::optionalClassDecl() {
   return optionalClass();
 }
 
 UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
     ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
     : DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
       TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
 
-void UncheckedOptionalAccessModel::transfer(const Stmt *S, NoopLattice &L,
-                                            Environment &Env) {
+void UncheckedOptionalAccessModel::transfer(const CFGElement *Elt,
+                                            NoopLattice &L, Environment &Env) {
   LatticeTransferState State(L, Env);
-  TransferMatchSwitch(*S, getASTContext(), State);
+  TransferMatchSwitch(*Elt, getASTContext(), State);
 }
 
 bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
                                                      const Value &Val1,
                                                      const Environment &Env1,
                                                      const Value &Val2,
                                                      const Environment &Env2) {
   return isNonEmptyOptional(Val1, Env1) == isNonEmptyOptional(Val2, Env2);
bool UncheckedOptionalAccessModel::merge(QualType Type, const Value &Val1,
   return true;
 }
 
 UncheckedOptionalAccessDiagnoser::UncheckedOptionalAccessDiagnoser(
     UncheckedOptionalAccessModelOptions Options)
     : DiagnoseMatchSwitch(buildDiagnoseMatchSwitch(Options)) {}
 
 std::vector<SourceLocation> UncheckedOptionalAccessDiagnoser::diagnose(
-    ASTContext &Context, const Stmt *Stmt, const Environment &Env) {
-  return DiagnoseMatchSwitch(*Stmt, Context, Env);
+    ASTContext &Ctx, const CFGElement *Elt, const Environment &Env) {
+  return DiagnoseMatchSwitch(*Elt, Ctx, Env);
 }
 
 } // namespace dataflow
 } // namespace clang

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

 //===- UncheckedOptionalAccessModelTest.cpp -------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 // FIXME: Move this to clang/unittests/Analysis/FlowSensitive/Models.
 
 #include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
 #include "TestingSupport.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Tooling.h"
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
-#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Error.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <string>
 #include <utility>
 #include <vector>
 
 using namespace clang;
llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
             [Options](ASTContext &Ctx, Environment &) {
               return UncheckedOptionalAccessModel(Ctx, Options);
             })
             .withPostVisitCFG(
                 [&Diagnostics,
                  Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
                     ASTContext &Ctx, const CFGElement &Elt,
                     const TypeErasedDataflowAnalysisState &State) mutable {
-                  auto Stmt = Elt.getAs<CFGStmt>();
-                  if (!Stmt) {
-                    return;
-                  }
-                  auto StmtDiagnostics =
-                      Diagnoser.diagnose(Ctx, Stmt->getStmt(), State.Env);
-                  llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
+                  auto EltDiagnostics =
+                      Diagnoser.diagnose(Ctx, &Elt, State.Env);
+                  llvm::move(EltDiagnostics, std::back_inserter(Diagnostics));
                 })
             .withASTBuildArgs(
                 {"-fsyntax-only", "-std=c++17", "-Wno-undefined-inline"})
             .withASTBuildVirtualMappedFiles(
                 tooling::FileContentMappings(Headers.begin(), Headers.end())),
         /*VerifyResults=*/[&Diagnostics](
                               const llvm::DenseMap<unsigned, std::string>
                                   &Annotations,