[clang][dataflow] Use NoopLattice in optional model

[samestep]

Summary

Followup to D128352. This patch pulls the NoopLattice class out from the NoopAnalysis.h test file into its own NoopLattice.h source file, and uses it to replace usage of SourceLocationsLattice in UncheckedOptionalAccessModel.

[xazax.hun]

Should this PR delete SourceLocationsLattice? Are there other users left or do we anticipate new users in the future?

[samestep]

@xazax.hun After this patch, SourceLocationsLattice is still used by clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp. I'm currently working on a patch to change the way we write unit tests in that file, after which I'll make a followup patch to delete SourceLocationsLattice.

[samestep]

• Merge branch 'optional-check-diagnose' into optional-model-noop-lattice

[samestep]

• Merge branch 'optional-check-diagnose' into optional-model-noop-lattice

[samestep]

• Merge branch 'optional-check-diagnose' into optional-model-noop-lattice

[samestep]

Update UncheckedOptionalAccessCheck.cpp

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

 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/FlowSensitive/ControlFlowContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/DataflowLattice.h"
 #include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
 #include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/Any.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Error.h"
 #include <memory>
 #include <vector>
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 using ast_matchers::MatchFinder;
-using dataflow::SourceLocationsLattice;
 using dataflow::UncheckedOptionalAccessDiagnoser;
 using dataflow::UncheckedOptionalAccessModel;
 using llvm::Optional;
 
 static constexpr llvm::StringLiteral FuncID("fun");
 
 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);
   UncheckedOptionalAccessDiagnoser Diagnoser;
   std::vector<SourceLocation> Diagnostics;
-  Expected<std::vector<Optional<DataflowAnalysisState<SourceLocationsLattice>>>>
+  Expected<std::vector<
+      Optional<DataflowAnalysisState<UncheckedOptionalAccessModel::Lattice>>>>
       BlockToOutputState = dataflow::runDataflowAnalysis(
           *Context, Analysis, Env,
-          [&ASTCtx, &Diagnoser,
-           &Diagnostics](const Stmt *Stmt,
-                         const DataflowAnalysisState<SourceLocationsLattice>
-                             &State) mutable {
+          [&ASTCtx, &Diagnoser, &Diagnostics](
+              const Stmt *Stmt,
+              const DataflowAnalysisState<UncheckedOptionalAccessModel::Lattice>
             auto StmtDiagnostics = Diagnoser.diagnose(ASTCtx, Stmt, State.Env);
             llvm::move(StmtDiagnostics, std::back_inserter(Diagnostics));
           });
   if (!BlockToOutputState)
     return llvm::None;
 
   return Diagnostics;
 }

clang/docs/tools/clang-formatted-files.txt

 clang/include/clang/Analysis/FlowSensitive/ControlFlowContext.h
 clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h
 clang/include/clang/Analysis/FlowSensitive/DataflowAnalysisContext.h
 clang/include/clang/Analysis/FlowSensitive/DataflowEnvironment.h
 clang/include/clang/Analysis/FlowSensitive/DataflowLattice.h
 clang/include/clang/Analysis/FlowSensitive/DataflowWorklist.h
 clang/include/clang/Analysis/FlowSensitive/MapLattice.h
 clang/include/clang/Analysis/FlowSensitive/MatchSwitch.h
+clang/include/clang/Analysis/FlowSensitive/NoopLattice.h
 clang/include/clang/Analysis/FlowSensitive/Solver.h
 clang/include/clang/Analysis/FlowSensitive/SourceLocationsLattice.h
 clang/include/clang/Analysis/FlowSensitive/StorageLocation.h
 clang/include/clang/Analysis/FlowSensitive/Transfer.h
 clang/include/clang/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.h
 clang/include/clang/Analysis/FlowSensitive/Value.h
 clang/include/clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h
 clang/include/clang/APINotes/APINotesYAMLCompiler.h

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

 #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/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/MatchSwitch.h"
-#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.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
 // 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.
+/// Dataflow analysis that models whether optionals hold values or not.
 ///
 /// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
-///
-/// FIXME: Consider separating the models from the unchecked access analysis.
 class UncheckedOptionalAccessModel
-    : public DataflowAnalysis<UncheckedOptionalAccessModel,
-                              SourceLocationsLattice> {
+    : public DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice> {
 public:
   UncheckedOptionalAccessModel(
       ASTContext &AstContext, UncheckedOptionalAccessModelOptions Options = {});
 
   /// Returns a matcher for the optional classes covered by this model.
   static ast_matchers::DeclarationMatcher optionalClassDecl();
 
-  static SourceLocationsLattice initialElement() {
-    return SourceLocationsLattice();
-  }
+  static NoopLattice initialElement() { return {}; }
 
-  void transfer(const Stmt *Stmt, SourceLocationsLattice &State,
-                Environment &Env);
+  void transfer(const Stmt *Stmt, NoopLattice &State, 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<SourceLocationsLattice>> TransferMatchSwitch;
+  MatchSwitch<TransferState<NoopLattice>> TransferMatchSwitch;
 };
 
 class UncheckedOptionalAccessDiagnoser {
 public:
   UncheckedOptionalAccessDiagnoser(
       UncheckedOptionalAccessModelOptions Options = {});
 
   std::vector<SourceLocation> diagnose(ASTContext &Context, const Stmt *Stmt,

clang/include/clang/Analysis/FlowSensitive/NoopLattice.h

+//===-- NoopLattice.h -------------------------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file defines the lattice with exactly one element.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_NOOP_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_NOOP_LATTICE_H
+
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include <ostream>
+
+namespace clang {
+namespace dataflow {
+
+/// Trivial lattice for dataflow analysis with exactly one element.
+///
+/// Useful for analyses that only need the Environment and nothing more.
+class NoopLattice {
+public:
+  bool operator==(const NoopLattice &Other) const { return true; }
+
+  LatticeJoinEffect join(const NoopLattice &Other) {
+    return LatticeJoinEffect::Unchanged;
+  }
+};
+
+inline std::ostream &operator<<(std::ostream &OS, const NoopLattice &) {
+  return OS << "noop";
+}
+
+} // namespace dataflow
+} // namespace clang
+
+#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_NOOP_LATTICE_H

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

 #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/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/MatchSwitch.h"
-#include "clang/Analysis/FlowSensitive/SourceLocationsLattice.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>
 #include <vector>
 
 namespace clang {
 namespace dataflow {
 namespace {
 
 using namespace ::clang::ast_matchers;
-using LatticeTransferState = TransferState<SourceLocationsLattice>;
+using LatticeTransferState = TransferState<NoopLattice>;
 
 DeclarationMatcher optionalClass() {
   return classTemplateSpecializationDecl(
       anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"),
             hasName("__optional_destruct_base"), hasName("absl::optional"),
             hasName("base::Optional")),
       hasTemplateArgument(0, refersToType(type().bind("T"))));
 }

 void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
                         LatticeTransferState &State) {
   if (auto *OptionalVal =
           State.Env.getValue(*ObjectExpr, SkipPast::ReferenceThenPointer)) {
     if (State.Env.getStorageLocation(*UnwrapExpr, SkipPast::None) == nullptr)
       if (auto *Loc = maybeInitializeOptionalValueMember(
               UnwrapExpr->getType(), *OptionalVal, State.Env))
         State.Env.setStorageLocation(*UnwrapExpr, *Loc);
-
-    auto *Prop = OptionalVal->getProperty("has_value");
-    if (auto *HasValueVal = cast_or_null<BoolValue>(Prop)) {
-      if (State.Env.flowConditionImplies(*HasValueVal))
-        return;
-    }
   }
-
-  // Record that this unwrap is *not* provably safe.
-  // FIXME: include either the name of the optional (if applicable) or a source
-  // range of the access for easier interpretation of the result.
-  State.Lattice.getSourceLocations().insert(ObjectExpr->getBeginLoc());
 }
 
 void transferMakeOptionalCall(const CallExpr *E,
                               const MatchFinder::MatchResult &,
                               LatticeTransferState &State) {
   auto &Loc = State.Env.createStorageLocation(*E);
   State.Env.setStorageLocation(*E, Loc);
   State.Env.setValue(

 
 ast_matchers::DeclarationMatcher
 UncheckedOptionalAccessModel::optionalClassDecl() {
   return optionalClass();
 }
 
 UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
     ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
-    : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
-          Ctx),
+    : DataflowAnalysis<UncheckedOptionalAccessModel, NoopLattice>(Ctx),
       TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
 
-void UncheckedOptionalAccessModel::transfer(const Stmt *S,
-                                            SourceLocationsLattice &L,
+void UncheckedOptionalAccessModel::transfer(const Stmt *S, NoopLattice &L,
                                             Environment &Env) {
   LatticeTransferState State(L, Env);
   TransferMatchSwitch(*S, getASTContext(), State);
 }
 
 bool UncheckedOptionalAccessModel::compareEquivalent(QualType Type,
                                                      const Value &Val1,
                                                      const Environment &Env1,

clang/unittests/Analysis/FlowSensitive/NoopAnalysis.h

 
 #ifndef LLVM_CLANG_UNITTESTS_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
 #define LLVM_CLANG_UNITTESTS_ANALYSIS_FLOWSENSITIVE_NOOPANALYSIS_H
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
 #include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
-#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
-#include <ostream>
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
 
 namespace clang {
 namespace dataflow {
 
-class NoopLattice {
-public:
-  bool operator==(const NoopLattice &) const { return true; }
-
-  LatticeJoinEffect join(const NoopLattice &) {
-    return LatticeJoinEffect::Unchanged;
-  }
-};
-
-inline std::ostream &operator<<(std::ostream &OS, const NoopLattice &) {
-  return OS << "noop";
-}
-
 class NoopAnalysis : public DataflowAnalysis<NoopAnalysis, NoopLattice> {
 public:
   /// `ApplyBuiltinTransfer` controls whether to run the built-in transfer
   /// functions that model memory during the analysis. Their results are not
   /// used by `NoopAnalysis`, but tests that need to inspect the environment
   /// should enable them.
   NoopAnalysis(ASTContext &Context, bool ApplyBuiltinTransfer)
       : DataflowAnalysis<NoopAnalysis, NoopLattice>(Context,