[clang][dataflow] Use diagnosis API in optional checker

[samestep]

Summary

Followup to D127898. This patch updates bugprone-unchecked-optional-access to use the new diagnoseCFG function instead of just looking at the exit block.

A followup to this will update the optional model itself to use a noop lattice rather than redundantly computing the diagnostics in both phases of the analysis.

[samestep]

Remove unnecessary ExitBlock code

[sgatev]

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

59–68

I think the typed API should still be preferred unless we make a call to drop it. The type-erased API is only used internally in the dataflow analysis framework. I suggest taking a DataflowAnalysisState<Lattice> in diagnoseCFG instead of TypeErasedDataflowAnalysisState for now. Would that work?

[samestep]

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

59–68

That wouldn't be easy to do, because checkDataflow in TestingSupport.h uses the type-erased API. But if you know how to change that to use the non-type-erased API, then we could probably change the signature of diagnoseCFG. I don't know how to do that, though.

[samestep]

• Merge branch 'diagnose-api' into optional-check-diagnose

[samestep]

• Merge branch 'diagnose-api' into optional-check-diagnose

[samestep]

• Merge branch 'diagnose-api' into optional-check-diagnose

[samestep]

• Merge branch 'diagnose-api' into optional-check-diagnose
• Use the updated API

[ymandel]

Nice!

[gribozavr2]

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

67

Please use the range-based llvm;:move() from llvm-project/llvm/include/llvm/ADT/STLExtras.h.

[sgatev]

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

126–129

[samestep]

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

126–129

I tried this but it doesn't seem to work; I get this error:

llvm-project/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h:130:13: error: non-const lvalue reference to type 'clang::dataflow::SourceLocationsLattice' cannot bind to a temporary of type 'clang::dataflow::SourceLocationsLattice'
      auto &Lattice =

[samestep]

• Merge branch 'diagnose-api' into optional-check-diagnose
• Update using review comments

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

 #include <memory>
 #include <vector>
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 using ast_matchers::MatchFinder;
 using dataflow::SourceLocationsLattice;
+using dataflow::UncheckedOptionalAccessDiagnosis;
 using dataflow::UncheckedOptionalAccessModel;
 using llvm::Optional;
 
 static constexpr llvm::StringLiteral FuncID("fun");
 
-static Optional<SourceLocationsLattice>
+static Optional<std::vector<SourceLocation>>
 analyzeFunction(const FunctionDecl &FuncDecl, ASTContext &ASTCtx) {
   using dataflow::ControlFlowContext;
   using dataflow::DataflowAnalysisState;
   using llvm::Expected;
 
   Expected<ControlFlowContext> Context =
       ControlFlowContext::build(&FuncDecl, FuncDecl.getBody(), &ASTCtx);
   if (!Context)
     return llvm::None;
 
   dataflow::DataflowAnalysisContext AnalysisContext(
       std::make_unique<dataflow::WatchedLiteralsSolver>());
   dataflow::Environment Env(AnalysisContext, FuncDecl);
   UncheckedOptionalAccessModel Analysis(ASTCtx);
+  UncheckedOptionalAccessDiagnosis Diagnosis(ASTCtx);
+  std::vector<SourceLocation> Diagnostics;
   Expected<std::vector<Optional<DataflowAnalysisState<SourceLocationsLattice>>>>
-      BlockToOutputState =
-          dataflow::runDataflowAnalysis(*Context, Analysis, Env);
+      BlockToOutputState = dataflow::runDataflowAnalysis(
+          *Context, Analysis, Env,
+          [&Diagnosis,
+           &Diagnostics](const Stmt *Stmt,
+                         const DataflowAnalysisState<SourceLocationsLattice>
+                             &State) mutable {
+            auto StmtDiagnostics = Diagnosis.diagnose(Stmt, State.Env);
+            std::move(StmtDiagnostics.begin(), StmtDiagnostics.end(),
+                      std::back_inserter(Diagnostics));
+          });
   if (!BlockToOutputState)
     return llvm::None;
-  assert(Context->getCFG().getExit().getBlockID() < BlockToOutputState->size());
 
-  const Optional<DataflowAnalysisState<SourceLocationsLattice>>
-      &ExitBlockState =
-          (*BlockToOutputState)[Context->getCFG().getExit().getBlockID()];
-  // `runDataflowAnalysis` doesn't guarantee that the exit block is visited;
-  // for example, when it is unreachable.
-  // FIXME: Diagnose violations even when the exit block is unreachable.
-  if (!ExitBlockState)
-    return llvm::None;
-
-  return std::move(ExitBlockState->Lattice);
+  return Diagnostics;
 }
 
 void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
   using namespace ast_matchers;
 
   auto HasOptionalCallDescendant = hasDescendant(callExpr(callee(cxxMethodDecl(
       ofClass(UncheckedOptionalAccessModel::optionalClassDecl())))));
   Finder->addMatcher(
void UncheckedOptionalAccessCheck::check(
     const MatchFinder::MatchResult &Result) {
   if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
     return;
 
   const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>(FuncID);
   if (FuncDecl->isTemplated())
     return;
 
-  if (Optional<SourceLocationsLattice> Errors =
+  if (Optional<std::vector<SourceLocation>> Errors =
           analyzeFunction(*FuncDecl, *Result.Context))
-    for (const SourceLocation &Loc : Errors->getSourceLocations())
+    for (const SourceLocation &Loc : *Errors)
       diag(Loc, "unchecked access to optional value");
 }
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang

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

 /// 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.
 template <typename AnalysisT>
 llvm::Expected<std::vector<
     llvm::Optional<DataflowAnalysisState<typename AnalysisT::Lattice>>>>
-runDataflowAnalysis(const ControlFlowContext &CFCtx, AnalysisT &Analysis,
-                    const Environment &InitEnv) {
-  auto TypeErasedBlockStates =
-      runTypeErasedDataflowAnalysis(CFCtx, Analysis, InitEnv);
+runDataflowAnalysis(
+    const ControlFlowContext &CFCtx, AnalysisT &Analysis,
+    const Environment &InitEnv,
+    std::function<void(const Stmt *, const DataflowAnalysisState<
+                                         typename AnalysisT::Lattice> &)>
+        PostVisitStmt = nullptr) {
+  auto TypeErasedBlockStates = runTypeErasedDataflowAnalysis(
+      CFCtx, Analysis, InitEnv,
+      [&PostVisitStmt](const Stmt *Stmt,
+                       const TypeErasedDataflowAnalysisState &State) {
+        auto *Lattice =
+            llvm::any_cast<typename AnalysisT::Lattice>(&State.Lattice.Value);
+        PostVisitStmt(Stmt, DataflowAnalysisState<typename AnalysisT::Lattice>{
+                                *Lattice, State.Env});
+      });
   if (!TypeErasedBlockStates)
     return TypeErasedBlockStates.takeError();
 
   std::vector<
       llvm::Optional<DataflowAnalysisState<typename AnalysisT::Lattice>>>
       BlockStates;
   BlockStates.reserve(TypeErasedBlockStates->size());