[clang][dataflow] Make the type of the post visit callback consistent

[sgatev]

Summary

Make the types of the post visit callbacks in transferBlock and runTypeErasedDataflowAnalysis consistent.

[sgatev]

Update commit message

[samestep]

Seems reasonable 👍 I'm curious though, why does this not require clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp to be updated?

[sgatev]

Address comments

[sgatev]

In D131014#3694852, @samestep wrote:

Seems reasonable 👍 I'm curious though, why does this not require clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp to be updated?

It does, thanks! Fixed.

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 Stmt *Stmt,
+              const CFGStmt &Stmt,
               const DataflowAnalysisState<UncheckedOptionalAccessModel::Lattice>
                   &State) mutable {
-            auto StmtDiagnostics = Diagnoser.diagnose(ASTCtx, Stmt, State.Env);
+            auto StmtDiagnostics =
+                Diagnoser.diagnose(ASTCtx, Stmt.getStmt(), State.Env);
             llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
           });
   if (!BlockToOutputState)
     return llvm::None;
 
   return Diagnostics;
 }
 

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

 /// `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 Stmt *, const DataflowAnalysisState<
+    std::function<void(const CFGStmt &, const DataflowAnalysisState<
                                             typename AnalysisT::Lattice> &)>
         PostVisitStmt = nullptr) {
-  std::function<void(const Stmt *, const TypeErasedDataflowAnalysisState &)>
+  std::function<void(const CFGStmt &, const TypeErasedDataflowAnalysisState &)>
       PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
     PostVisitStmtClosure = [&PostVisitStmt](
-                               const Stmt *Stmt,
+                               const CFGStmt &Stmt,
                                const TypeErasedDataflowAnalysisState &State) {
       auto *Lattice =
           llvm::any_cast<typename AnalysisT::Lattice>(&State.Lattice.Value);
       PostVisitStmt(Stmt, DataflowAnalysisState<typename AnalysisT::Lattice>{
                               *Lattice, State.Env});
     };
   }
 

clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h

 /// the returned vector correspond 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.
 llvm::Expected<std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>>>
 runTypeErasedDataflowAnalysis(
     const ControlFlowContext &CFCtx, TypeErasedDataflowAnalysis &Analysis,
     const Environment &InitEnv,
-    std::function<void(const Stmt *, const TypeErasedDataflowAnalysisState &)>
+    std::function<void(const CFGStmt &,
+                       const TypeErasedDataflowAnalysisState &)>
         PostVisitStmt = nullptr);
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_TYPEERASEDDATAFLOWANALYSIS_H

clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp

TypeErasedDataflowAnalysisState transferBlock(
   }
   return State;
 }
 
 llvm::Expected<std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>>>
 runTypeErasedDataflowAnalysis(
     const ControlFlowContext &CFCtx, TypeErasedDataflowAnalysis &Analysis,
     const Environment &InitEnv,
-    std::function<void(const Stmt *, const TypeErasedDataflowAnalysisState &)>
+    std::function<void(const CFGStmt &,
+                       const TypeErasedDataflowAnalysisState &)>
         PostVisitStmt) {
   PostOrderCFGView POV(&CFCtx.getCFG());
   ForwardDataflowWorklist Worklist(CFCtx.getCFG(), &POV);
 
   std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>> BlockStates;
   BlockStates.resize(CFCtx.getCFG().size(), llvm::None);
 
   // The entry basic block doesn't contain statements so it can be skipped.
runTypeErasedDataflowAnalysis(
   // FIXME: Consider evaluating unreachable basic blocks (those that have a
   // state set to `llvm::None` at this point) to also analyze dead code.
 
   if (PostVisitStmt) {
     for (const CFGBlock *Block : CFCtx.getCFG()) {
       // Skip blocks that were not evaluated.
       if (!BlockStates[Block->getBlockID()])
         continue;
-      transferBlock(
-          CFCtx, BlockStates, *Block, InitEnv, Analysis,
-          [&PostVisitStmt](const clang::CFGStmt &Stmt,
-                           const TypeErasedDataflowAnalysisState &State) {
-            PostVisitStmt(Stmt.getStmt(), State);
-          });
+
+      transferBlock(CFCtx, BlockStates, *Block, InitEnv, Analysis,
+                    PostVisitStmt);
     }
   }
 
   return BlockStates;
 }
 
 } // namespace dataflow
 } // namespace clang

clang/unittests/Analysis/FlowSensitive/TestingSupport.h

struct AnalysisData {
   std::vector<llvm::Optional<TypeErasedDataflowAnalysisState>> &BlockStates;
 };
 
 template <typename AnalysisT>
 llvm::Error checkDataflow(
     llvm::StringRef Code,
     ast_matchers::internal::Matcher<FunctionDecl> TargetFuncMatcher,
     std::function<AnalysisT(ASTContext &, Environment &)> MakeAnalysis,
-    std::function<void(ASTContext &, const Stmt *,
+    std::function<void(ASTContext &, const CFGStmt &,
                        const TypeErasedDataflowAnalysisState &)>
         PostVisitStmt,
     std::function<void(AnalysisData)> VerifyResults, ArrayRef<std::string> Args,
     const tooling::FileContentMappings &VirtualMappedFiles = {}) {
   llvm::Annotations AnnotatedCode(Code);
   auto Unit = tooling::buildASTFromCodeWithArgs(
       AnnotatedCode.code(), Args, "input.cc", "clang-dataflow-test",
       std::make_shared<PCHContainerOperations>(),
llvm::Error checkDataflow(
   auto CFCtx = ControlFlowContext::build(F, F->getBody(), &F->getASTContext());
   if (!CFCtx)
     return CFCtx.takeError();
 
   DataflowAnalysisContext DACtx(std::make_unique<WatchedLiteralsSolver>());
   Environment Env(DACtx, *F);
   auto Analysis = MakeAnalysis(Context, Env);
 
-  std::function<void(const Stmt *, const TypeErasedDataflowAnalysisState &)>
+  std::function<void(const CFGStmt &, const TypeErasedDataflowAnalysisState &)>
       PostVisitStmtClosure = nullptr;
   if (PostVisitStmt != nullptr) {
     PostVisitStmtClosure = [&PostVisitStmt, &Context](
-                               const Stmt *Stmt,
+                               const CFGStmt &Stmt,
                                const TypeErasedDataflowAnalysisState &State) {
       PostVisitStmt(Context, Stmt, State);
     };
   }
 
   llvm::Expected<llvm::DenseMap<const clang::Stmt *, std::string>>
       StmtToAnnotations = buildStatementToAnnotationMapping(F, AnnotatedCode);
   if (!StmtToAnnotations)

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

UncheckedOptionalAccessModelOptions Options{
         /*IgnoreSmartPointerDereference=*/true};
     std::vector<SourceLocation> Diagnostics;
     llvm::Error Error = checkDataflow<UncheckedOptionalAccessModel>(
         SourceCode, FuncMatcher,
         [Options](ASTContext &Ctx, Environment &) {
           return UncheckedOptionalAccessModel(Ctx, Options);
         },
         [&Diagnostics, Diagnoser = UncheckedOptionalAccessDiagnoser(Options)](
-            ASTContext &Ctx, const Stmt *Stmt,
+            ASTContext &Ctx, const CFGStmt &Stmt,
             const TypeErasedDataflowAnalysisState &State) mutable {
-          auto StmtDiagnostics = Diagnoser.diagnose(Ctx, Stmt, State.Env);
+          auto StmtDiagnostics =
+              Diagnoser.diagnose(Ctx, Stmt.getStmt(), State.Env);
           llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
         },
         [&Diagnostics](AnalysisData AnalysisData) {
           auto &SrcMgr = AnalysisData.ASTCtx.getSourceManager();
 
           llvm::DenseSet<unsigned> AnnotationLines;
           for (const auto &Pair : AnalysisData.Annotations) {
             auto *Stmt = Pair.getFirst();