[clang-tidy] New check for safe usage of `std::optional` and like types.

[ymandel]

Summary

This check verifies the safety of access to std::optional and related
types (including absl::optional). It is based on a corresponding Clang
Dataflow Analysis, which does most of the work. This check merely runs it and
converts its findings into diagnostics.

[Eugene.Zelenko]

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

86

Please don't use auto if type is not spelled in same statement or iterator.

91

Ditto.

clang-tools-extra/docs/ReleaseNotes.rst

119

Please add description. Hint: should be same as first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst

7

Please do that.

[ymandel]

Thanks, Eugene -- I'm actually still working on this, filling out the FIXMEs, etc. I uploaded it with arc as a draft and its clearly marked as such. Is there something more I should have done to preven it from being sent out?

[ymandel]

Filled out all relevant docs/comments.

[ymandel]

tweaks

[ymandel]

Rebased

[xazax.hun]

Most checks in Clang Tidy will run relatively quickly as they usually can do most/all of their work in a single traversal. I wonder whether flow sensitive checks will prove to be a bit slower. I think adding slower checks to Tidy is fine, but it would be nice to properly set the expectations to the user (e.g. if an IDE is running Tidy in the background it might want to opt out from flow sensitive checks if they turn out to be slow). In case the performance is good, I think it should be fine as is. Otherwise I wonder if we want to add something like a tag to mark flow sensitive checks and give an option to turn them off.

[ymandel]

In D121120#3494085, @xazax.hun wrote:

Most checks in Clang Tidy will run relatively quickly as they usually can do most/all of their work in a single traversal. I wonder whether flow sensitive checks will prove to be a bit slower. I think adding slower checks to Tidy is fine, but it would be nice to properly set the expectations to the user (e.g. if an IDE is running Tidy in the background it might want to opt out from flow sensitive checks if they turn out to be slow). In case the performance is good, I think it should be fine as is. Otherwise I wonder if we want to add something like a tag to mark flow sensitive checks and give an option to turn them off.

Excellent point. @aaron.ballman is there a mechanism for this?

I can check on the current performance, but I think this would be good to know regardless of the outcome, given that performance is subject to change and we expect to develop other flow-sensitive checks.

[xazax.hun]

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

39

I am a bit afraid that we have two places to update the list of supported optional types and they can get out of sync. I wonder whether creating a function in clang/Analysis/FlowSensitive/Models/UncheckedOptionalUseModel.h to return a matcher that matches the supported optional types is a better approach.

50

If we have a CXXConstructorDecl where both the initializer and the body has calls to optional types, i.e. both matchers would match, would we process the body of that twice?

61

Nit: Redundant braces.

93

Is there a way in the future to include either the name of the optional or a source range of the access? This can be confusing when we have multiple optionals in the same line. The of course, the column information helps. But the more visual guides we have the better :)

[ymandel]

address review comments

[ymandel]

added fixme

[ymandel]

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

39

Yes, it is a better approach. :) Done.

50

Yes, we would. Good catch. Fixed.

93

Yes -- we have access to the object expression at the time of detection. So both are reasonable to include. We just need to extend the SourceLocationsLattice to accomodate. Added a FIXME in the model.

[ymandel]

Factor out analysis code into separate function.

[xazax.hun]

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

85

Could the size of the vector ever be wrong? Should this be an assert instead?

85

Whoops, after the update this comment is out of place, now it supposed to be on line 60.

[xazax.hun]

Overall this looks good to me. However, I think this might not use the full potential of the check itself. With the information that the dataflow framework have it could distinguish between potentially unsafe accesses and provably unsafe accesses depending on whether the has_value property is constrained to be false. From the user point of view, it would be nice to emit different warning messages for the above two cases. This can help to gradually introduce this check to a larger codebase and focus on the higher severity diagnostics first.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst

30

I wonder if it would be easier to read if we had two top level categories, one for safe and one for unsafe accesses instead of switching back and forth between examples.

[ymandel]

reorder examples

[ymandel]

clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst

30

Sounds good, done. Let me know if that's what you had in mind.

[xazax.hun]

clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst

30

Yup, this is exactly what I had in mind, thanks!

[ymandel]

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

85

Based on my reading, it is a rare, but possible condition. Basically, we need code where the exit block is unreachable, which I believe can happen in weird cases like:

while(true) {...}

https://godbolt.org/z/rfEnfaWTv -- notice the lack of predecessors for the exit block.

See the code here, which follows the ordering of the blocks and doesn't force blocks to be processed:

https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L337-L364

[xazax.hun]

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

85

Interesting. Since we already have optionals in the vector, I assumed we will always have matching size. I think we might want to change this so there is only one way for the analysis to not provide a state for a basic block to make this a bit less confusing,

[xazax.hun]

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

85

Actually, in the linked code I see BlockStates.resize(CFCtx.getCFG().size(), llvm::None);. So I would expect the size to be always right with possibly some Nones for the nodes that were not processed.

[ymandel]

add assert

[ymandel]

In D121120#3494427, @xazax.hun wrote:

Overall this looks good to me. However, I think this might not use the full potential of the check itself. With the information that the dataflow framework have it could distinguish between potentially unsafe accesses and provably unsafe accesses depending on whether the has_value property is constrained to be false. From the user point of view, it would be nice to emit different warning messages for the above two cases. This can help to gradually introduce this check to a larger codebase and focus on the higher severity diagnostics first.

Agreed. I've filed this in our issue tracker (which I'm still planning to port over to github...).

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

85

Actually, in the linked code I see BlockStates.resize(CFCtx.getCFG().size(), llvm::None);. So I would expect the size to be always right with possibly some Nones for the nodes that were not processed.

Ah, my mistake! I thought resize only allocated the space. #TIL

Changed to an assert. Thanks.

[xazax.hun]

It looks like all of my comments are resolved now, thanks!

[xazax.hun]

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

85

But this discussion shed light on an interesting detail. If the exit block is unreachable, we will not diagnose the unsafe accesses. I wonder if this worth a FIXME.

[sgatev]

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

40–46

Do we really need all these using declarations? There seems to be one reference for each of these types. I think we can simply qualify the references.

55

Call this Env to disambiguate from the name of the type.

85

The documentation of runDataflowAnalysis already hints that the size of the vector matches the size of the CFG. Do you think we should make this more clear?

88

Let's add a constant for this so that we can reuse it in UncheckedOptionalAccessCheck::check.

clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h

70

clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp

25

Call this checked_access?

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

39

40

This should be a class member, no? clang::dataflow::optionalClass seems underspecified.

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

234

[ymandel]

Thanks for the review!

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

40–46

No, we don't need them, but IMO they clarify the code. We're really heavy with the types, given that auto is discouraged, so I think pulling these out improves readability in some critical places (lines 56 and 63). I figured I'd do all for consistency. I've removed those not related to lines 56 and 63, or used more than once. WDYT?

85

But this discussion shed light on an interesting detail. If the exit block is unreachable, we will not diagnose the unsafe accesses. I wonder if this worth a FIXME.

The documentation of runDataflowAnalysis already hints that the size of the vector matches the size of the CFG. Do you think we should make this more clear?

Yes to both. I'll send a separate patch on that header with the doc changes.

[ymandel]

address review comments

[ymandel]

add fixme

[ymandel]

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

85

On second thought, the FIXME is for the check, not the framework, so I've added it here. I'll still send the comment tweak separately.

[sgatev]

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

40–46

Ah, I see, we're using all of these types in a single line :D Perhaps std::vector<Optional<DataflowAnalysisState<T>>> could be encapsulated in a DataflowResult<T> type or something like that. I suggest we keep the patch as it currently is and consider such a change separately.

[ymandel]

In D121120#3494137, @ymandel wrote:

In D121120#3494085, @xazax.hun wrote:

Most checks in Clang Tidy will run relatively quickly as they usually can do most/all of their work in a single traversal. I wonder whether flow sensitive checks will prove to be a bit slower. I think adding slower checks to Tidy is fine, but it would be nice to properly set the expectations to the user (e.g. if an IDE is running Tidy in the background it might want to opt out from flow sensitive checks if they turn out to be slow). In case the performance is good, I think it should be fine as is. Otherwise I wonder if we want to add something like a tag to mark flow sensitive checks and give an option to turn them off.

Excellent point. @aaron.ballman is there a mechanism for this?

I can check on the current performance, but I think this would be good to know regardless of the outcome, given that performance is subject to change and we expect to develop other flow-sensitive checks.

Just realized we left this open. How about a disclaimer at the top of the doc (.rst) file noting the potential cost? Anyone using clang-tidy should be explicitly configuring which checks to run, so that may be sufficient. If we want to allow users to enable/disable flow-sensitive checks across the board, though, it seems like we would need to add a new option to ClangTidyOptions (https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidyOptions.h#L50).

[xazax.hun]

In D121120#3496597, @ymandel wrote:

Just realized we left this open. How about a disclaimer at the top of the doc (.rst) file noting the potential cost? Anyone using clang-tidy should be explicitly configuring which checks to run, so that may be sufficient. If we want to allow users to enable/disable flow-sensitive checks across the board, though, it seems like we would need to add a new option to ClangTidyOptions (https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidyOptions.h#L50).

Sounds good to me.

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

 #include "SuspiciousMemsetUsageCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
+#include "UncheckedOptionalAccessCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnhandledExceptionAtNewCheck.h"
 #include "UnhandledSelfAssignmentCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "bugprone-swapped-arguments");
     CheckFactories.registerCheck<TerminatingContinueCheck>(
         "bugprone-terminating-continue");
     CheckFactories.registerCheck<ThrowKeywordMissingCheck>(
         "bugprone-throw-keyword-missing");
     CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
         "bugprone-too-small-loop-variable");
+    CheckFactories.registerCheck<UncheckedOptionalAccessCheck>(
+        "bugprone-unchecked-optional-access");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
         "bugprone-unhandled-self-assignment");
     CheckFactories.registerCheck<UnhandledExceptionAtNewCheck>(
         "bugprone-unhandled-exception-at-new");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   SuspiciousMemsetUsageCheck.cpp
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   TerminatingContinueCheck.cpp
   ThrowKeywordMissingCheck.cpp
   TooSmallLoopVariableCheck.cpp
+  UncheckedOptionalAccessCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnhandledExceptionAtNewCheck.cpp
   UnhandledSelfAssignmentCheck.cpp
   UnusedRaiiCheck.cpp
   UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangTidy
   clangTidyCppCoreGuidelinesModule
   clangTidyUtils
 
   DEPENDS
   omp_gen
   )
 
 clang_target_link_libraries(clangTidyBugproneModule
   PRIVATE
   clangAnalysis
+  clangAnalysisFlowSensitive
+  clangAnalysisFlowSensitiveModels
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTooling
   clangTransformer
   )

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

+//===--- UncheckedOptionalAccessCheck.h - clang-tidy ------------*- 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNCHECKEDOPTIONALACCESSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNCHECKEDOPTIONALACCESSCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Warns when the code is unwrapping a `std::optional<T>`, `absl::optional<T>`,
+/// or `base::Optional<T>` object without assuring that it contains a value.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unchecked-optional-access.html
+class UncheckedOptionalAccessCheck : public ClangTidyCheck {
+public:
+  UncheckedOptionalAccessCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNCHECKEDOPTIONALACCESSCHECK_H

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

+//===--- UncheckedOptionalAccessCheck.cpp - clang-tidy --------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "UncheckedOptionalAccessCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#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/Support/Error.h"
+#include <memory>
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+using ast_matchers::MatchFinder;
+using dataflow::SourceLocationsLattice;
+using dataflow::UncheckedOptionalAccessModel;
+using llvm::Optional;
+
+static constexpr llvm::StringLiteral FuncID("fun");
+
+static Optional<SourceLocationsLattice>
+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);
+  Expected<std::vector<Optional<DataflowAnalysisState<SourceLocationsLattice>>>>
+      BlockToOutputState =
+          dataflow::runDataflowAnalysis(*Context, Analysis, Env);
+  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.hasValue())
+    return llvm::None;
+
+  return std::move(ExitBlockState->Lattice);
+}
+
+void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
+  using namespace ast_matchers;
+
+  auto HasOptionalCallDescendant = hasDescendant(callExpr(callee(cxxMethodDecl(
+      ofClass(UncheckedOptionalAccessModel::optionalClassDecl())))));
+  Finder->addMatcher(
+      decl(anyOf(functionDecl(unless(isExpansionInSystemHeader()),
+                              // FIXME: Remove the filter below when lambdas are
+                              // well supported by the check.
+                              unless(hasDeclContext(cxxRecordDecl(isLambda()))),
+                              hasBody(HasOptionalCallDescendant)),
+                 cxxConstructorDecl(hasAnyConstructorInitializer(
+                     withInitializer(HasOptionalCallDescendant)))))
+          .bind(FuncID),
+      this);
+}
+
+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 =
+          analyzeFunction(*FuncDecl, *Result.Context))
+    for (const SourceLocation &Loc : Errors->getSourceLocations())
+      diag(Loc, "unchecked access to optional value");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 
 New checks
 ^^^^^^^^^^
 
 - New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone-shared-ptr-array-mismatch>` check.
 
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
+- New :doc:`bugprone-unchecked-optional-access
+  <clang-tidy/checks/bugprone-unchecked-optional-access>` check.
+
+   Warns when the code is unwrapping a `std::optional<T>`, `absl::optional<T>`,
+   or `base::Optional<T>` object without assuring that it contains a value.
+
 - New :doc:`modernize-macro-to-enum
   <clang-tidy/checks/modernize-macro-to-enum>` check.
 
   Replaces groups of adjacent macros with an unscoped anonymous enum.
 
 - New :doc:`portability-std-allocator-const <clang-tidy/checks/portability-std-allocator-const>` check.
 
   Report use of ``std::vector<const T>`` (and similar containers of const

clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst

+.. title:: clang-tidy - bugprone-unchecked-optional-access
+
+bugprone-unchecked-optional-access
+==================================
+
+*Note*: This check uses a flow-sensitive static analysis to produce its
+ results. Therefore, it may be more resource intensive (RAM, CPU) than the
+ average clang-tidy check.
+
+This check identifies unsafe accesses to values contained in
+``std::optional<T>``, ``absl::optional<T>``, or ``base::Optional<T>``
+objects. Below we will refer to all these types collectively as
+``optional<T>``.
+
+An access to the value of an ``optional<T>`` occurs when one of its
+``value``, ``operator*``, or ``operator->`` member functions is invoked.
+To align with common misconceptions, the check considers these member
+functions as equivalent, even though there are subtle differences
+related to exceptions versus undefined behavior. See
+go/optional-style-recommendations for more information on that topic.
+
+An access to the value of an ``optional<T>`` is considered safe if and only if
+code in the local scope (for example, a function body) ensures that the
+``optional<T>`` has a value in all possible execution paths that can reach the
+access. That should happen either through an explicit check, using the
+``optional<T>::has_value`` member function, or by constructing the
+``optional<T>`` in a way that shows that it unambiguously holds a value (e.g
+using ``std::make_optional`` which always returns a populated
+``std::optional<T>``).
+
+Below we list some examples, starting with unsafe optional access patterns,
+followed by safe access patterns.
+
+Unsafe access patterns
+~~~~~~~~~~~~~~~~~~~~~~
+
+Access the value without checking if it exists
+----------------------------------------------
+
+The check flags accesses to the value that are not locally guarded by
+existence check:
+
+.. code-block:: c++
+
+   void f(absl::optional<int> opt) {
+     use(*opt); // unsafe: it is unclear whether `opt` has a value.
+   }
+
+Access the value in the wrong branch
+------------------------------------
+
+The check is aware of the state of an optional object in different
+branches of the code. For example:
+
+.. code-block:: c++
+
+   void f(absl::optional<int> opt) {
+     if (opt.has_value()) {
+     } else {
+       use(opt.value()); // unsafe: it is clear that `opt` does *not* have a value.
+     }
+   }
+
+Assume a function result to be stable
+-------------------------------------
+
+The check is aware that function results might not be stable. That is,
+consecutive calls to the same function might return different values.
+For example:
+
+.. code-block:: c++
+
+   void f(Foo foo) {
+     if (foo.opt().has_value()) {
+       use(*foo.opt()); // unsafe: it is unclear whether `foo.opt()` has a value.
+     }
+   }
+
+Rely on invariants of uncommon APIs
+-----------------------------------
+
+The check is unaware of invariants of uncommon APIs. For example:
+
+   void f(Foo foo) {
+     if (foo.HasProperty("bar")) {
+       use(*foo.GetProperty("bar")); // unsafe: it is unclear whether `foo.GetProperty("bar")` has a value.
+     }
+   }
+
+Check if a value exists, then pass the optional to another function
+-------------------------------------------------------------------
+
+The check relies on local reasoning. The check and value access must
+both happen in the same function. An access is considered unsafe even if
+the caller of the function performing the access ensures that the
+optional has a value. For example:
+
+.. code-block:: c++
+
+   void g(absl::optional<int> opt) {
+     use(*opt); // unsafe: it is unclear whether `opt` has a value.
+   }
+
+   void f(absl::optional<int> opt) {
+     if (opt.has_value()) {
+       g(opt);
+     }
+   }
+
+Safe access patterns
+~~~~~~~~~~~~~~~~~~~~
+
+Check if a value exists, then access the value
+----------------------------------------------
+
+The check recognizes all straightforward ways for checking if a value
+exists and accessing the value contained in an optional object. For
+example:
+
+.. code-block:: c++
+
+   void f(absl::optional<int> opt) {
+     if (opt.has_value()) {
+       use(*opt);
+     }
+   }
+
+
+Check if a value exists, then access the value from a copy
+----------------------------------------------------------
+
+The criteria that the check uses is semantic, not syntactic. It
+recognizes when a copy of the optional object being accessed is known to
+have a value. For example:
+
+.. code-block:: c++
+
+   void f(absl::optional<int> opt1) {
+     if (opt1.has_value()) {
+       absl::optional<int> opt2 = opt1;
+       use(*opt2);
+     }
+   }
+
+
+Ensure that a value exists using common macros
+----------------------------------------------
+
+The check is aware of common macros like ``CHECK``, ``DCHECK``, and
+``ASSERT_THAT``. Those can be used to ensure that an optional object has
+a value. For example:
+
+.. code-block:: c++
+
+   void f(absl::optional<int> opt) {
+     DCHECK(opt.has_value());
+     use(*opt);
+   }
+
+Ensure that a value exists, then access the value in a correlated branch
+------------------------------------------------------------------------
+
+The check is aware of correlated branches in the code and can figure out
+when an optional object is ensured to have a value on all execution
+paths that lead to an access. For example:
+
+.. code-block:: c++
+
+   void f(absl::optional<int> opt) {
+     bool safe = false;
+     if (opt.has_value() && SomeOtherCondition()) {
+       safe = true;
+     }
+     // ... more code...
+     if (safe) {
+       use(*opt);
+     }
+   }
+
+Stabilize function results
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Since function results are not assumed to be stable across calls, it is best to
+store the result of the function call in a local variable and use that variable
+to access the value. For example:
+
+.. code-block:: c++
+
+   void f(Foo foo) {
+     if (const auto& foo_opt = foo.opt(); foo_opt.has_value()) {
+       use(*foo_opt);
+     }
+   }
+
+Do not rely on uncommon-API invariants
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+When uncommon APIs guarantee that an optional has contents, do not rely on it --
+instead, check explicitly that the optional object has a value. For example:
+
+.. code-block:: c++
+
+   void f(Foo foo) {
+     if (const auto& property = foo.GetProperty("bar")) {
+       use(*property);
+     }
+   }
+
+instead of the `HasProperty`, `GetProperty` pairing we saw above.
+
+Do not rely on caller-performed checks
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you know that all of a function's callers have checked that an optional
+argument has a value, either change the function to take the value directly or
+check the optional again in the local scope of the callee. For example:
+
+.. code-block:: c++
+
+   void g(int val) {
+     use(val);
+   }
+
+   void f(absl::optional<int> opt) {
+     if (opt.has_value()) {
+       g(*opt);
+     }
+   }
+
+and
+
+.. code-block:: c++
+
+   struct S {
+     absl::optional<int> opt;
+     int x;
+   };
+
+   void g(const S &s) {
+     if (s.opt.has_value() && s.x > 10) {
+       use(*s.opt);
+   }
+
+   void f(S s) {
+     if (s.opt.has_value()) {
+       g(s);
+     }
+   }
+
+Additional notes
+~~~~~~~~~~~~~~~~
+
+Aliases created via ``using`` declarations
+------------------------------------------
+
+The check is aware of aliases of optional types that are created via
+``using`` declarations. For example:
+
+.. code-block:: c++
+
+   using OptionalInt = absl::optional<int>;
+
+   void f(OptionalInt opt) {
+     use(opt.value()); // unsafe: it is unclear whether `opt` has a value.
+   }
+
+Lambdas
+-------
+
+The check does not currently report unsafe optional acceses in lambdas.
+A future version will expand the scope to lambdas, following the rules
+outlined above. It is best to follow the same principles when using
+optionals in lambdas.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `bugprone-suspicious-memset-usage <bugprone-suspicious-memset-usage.html>`_, "Yes"
    `bugprone-suspicious-missing-comma <bugprone-suspicious-missing-comma.html>`_,
    `bugprone-suspicious-semicolon <bugprone-suspicious-semicolon.html>`_, "Yes"
    `bugprone-suspicious-string-compare <bugprone-suspicious-string-compare.html>`_, "Yes"
    `bugprone-swapped-arguments <bugprone-swapped-arguments.html>`_, "Yes"
    `bugprone-terminating-continue <bugprone-terminating-continue.html>`_, "Yes"
    `bugprone-throw-keyword-missing <bugprone-throw-keyword-missing.html>`_,
    `bugprone-too-small-loop-variable <bugprone-too-small-loop-variable.html>`_,
+   `bugprone-unchecked-optional-access <bugprone-unchecked-optional-access.html>`_, "Yes"
    `bugprone-undefined-memory-manipulation <bugprone-undefined-memory-manipulation.html>`_,
    `bugprone-undelegated-constructor <bugprone-undelegated-constructor.html>`_,
    `bugprone-unhandled-exception-at-new <bugprone-unhandled-exception-at-new.html>`_,
    `bugprone-unhandled-self-assignment <bugprone-unhandled-self-assignment.html>`_,
    `bugprone-unused-raii <bugprone-unused-raii.html>`_, "Yes"
    `bugprone-unused-return-value <bugprone-unused-return-value.html>`_,
    `bugprone-use-after-move <bugprone-use-after-move.html>`_,
    `bugprone-virtual-near-miss <bugprone-virtual-near-miss.html>`_, "Yes"

clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h

+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_ABSL_TYPES_OPTIONAL_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_ABSL_TYPES_OPTIONAL_H_
+
+/// Mock of `absl::optional`.
+namespace absl {
+
+// clang-format off
+template <typename T> struct remove_reference      { using type = T; };
+template <typename T> struct remove_reference<T&>  { using type = T; };
+template <typename T> struct remove_reference<T&&> { using type = T; };
+// clang-format on
+
+template <typename T>
+using remove_reference_t = typename remove_reference<T>::type;
+
+template <typename T>
+constexpr T &&forward(remove_reference_t<T> &t) noexcept;
+
+template <typename T>
+constexpr T &&forward(remove_reference_t<T> &&t) noexcept;
+
+template <typename T>
+constexpr remove_reference_t<T> &&move(T &&x);
+
+struct nullopt_t {
+  constexpr explicit nullopt_t() {}
+};
+
+constexpr nullopt_t nullopt;
+
+template <typename T>
+class optional {
+public:
+  constexpr optional() noexcept;
+
+  constexpr optional(nullopt_t) noexcept;
+
+  optional(const optional &) = default;
+
+  optional(optional &&) = default;
+
+  const T &operator*() const &;
+  T &operator*() &;
+  const T &&operator*() const &&;
+  T &&operator*() &&;
+
+  const T *operator->() const;
+  T *operator->();
+
+  const T &value() const &;
+  T &value() &;
+  const T &&value() const &&;
+  T &&value() &&;
+
+  constexpr explicit operator bool() const noexcept;
+  constexpr bool has_value() const noexcept;
+
+  template <typename U>
+  constexpr T value_or(U &&v) const &;
+  template <typename U>
+  T value_or(U &&v) &&;
+
+  template <typename... Args>
+  T &emplace(Args &&...args);
+
+  void reset() noexcept;
+
+  void swap(optional &rhs) noexcept;
+};
+
+} // namespace absl
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_ABSL_TYPES_OPTIONAL_H_

clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp

+// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I %S/Inputs/
+
+#include "absl/types/optional.h"
+
+void unchecked_value_access(const absl::optional<int> &opt) {
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
+}
+
+void unchecked_deref_operator_access(const absl::optional<int> &opt) {
+  *opt;
+  // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unchecked access to optional value
+}
+
+struct Foo {
+  void foo() const {}
+};
+
+void unchecked_arrow_operator_access(const absl::optional<Foo> &opt) {
+  opt->foo();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value
+}
+
+void checked_access(const absl::optional<int> &opt) {
+  if (opt.has_value()) {
+    opt.value();
+  }
+}
+
+template <typename T>
+void function_template_without_user(const absl::optional<T> &opt) {
+  opt.value(); // no-warning
+}
+
+template <typename T>
+void function_template_with_user(const absl::optional<T> &opt) {
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value
+}
+
+void function_template_user(const absl::optional<int> &opt) {
+  // Instantiate the f3 function template so that it gets matched by the check.
+  function_template_with_user(opt);
+}
+
+template <typename T>
+void function_template_with_specialization(const absl::optional<int> &opt) {
+  opt.value(); // no-warning
+}
+
+template <>
+void function_template_with_specialization<int>(
+    const absl::optional<int> &opt) {
+  opt.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value
+}
+
+template <typename T>
+class ClassTemplateWithSpecializations {
+  void f(const absl::optional<int> &opt) {
+    opt.value(); // no-warning
+  }
+};
+
+template <typename T>
+class ClassTemplateWithSpecializations<T *> {
+  void f(const absl::optional<int> &opt) {
+    opt.value(); // no-warning
+  }
+};
+
+template <>
+class ClassTemplateWithSpecializations<int> {
+  void f(const absl::optional<int> &opt) {
+    opt.value();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional
+  }
+};
+
+// The templates below are not instantiated and CFGs can not be properly built
+// for them. They are here to make sure that the checker does not crash, but
+// instead ignores non-instantiated templates.
+
+template <typename T>
+struct C1 {};
+
+template <typename T>
+struct C2 : public C1<T> {
+  ~C2() {}
+};
+
+template <typename T, template <class> class B>
+struct C3 : public B<T> {
+  ~C3() {}
+};
+
+void multiple_unchecked_accesses(absl::optional<int> opt1,
+                                 absl::optional<int> opt2) {
+  for (int i = 0; i < 10; i++) {
+    opt1.value();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional
+  }
+  opt2.value();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value
+}
+
+class C4 {
+  explicit C4(absl::optional<int> opt) : foo_(opt.value()) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:47: warning: unchecked access to optional
+  }
+  int foo_;
+};

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

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.
 ///
 /// 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:
   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();
   }
 
   void transfer(const Stmt *Stmt, SourceLocationsLattice &State,
                 Environment &Env);
 
 private:
   MatchSwitch<TransferState<SourceLocationsLattice>> TransferMatchSwitch;
 };
 
 } // namespace dataflow
 } // namespace clang
 
 #endif // CLANG_ANALYSIS_FLOWSENSITIVE_MODELS_UNCHECKEDOPTIONALACCESSMODEL_H

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

 //===-- UncheckedOptionalAccessModel.cpp ------------------------*- 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 a dataflow analysis that detects unsafe uses of optional
 //  values.
 //
 //===----------------------------------------------------------------------===//
 
 #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/FlowSensitive/DataflowEnvironment.h"
 #include "clang/Analysis/FlowSensitive/MatchSwitch.h"
 #include "clang/Analysis/FlowSensitive/SourceLocationsLattice.h"
 #include "clang/Analysis/FlowSensitive/Value.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include <cassert>
 #include <memory>
 #include <utility>
 
 namespace clang {
 namespace dataflow {
 namespace {
 
 using namespace ::clang::ast_matchers;
-
 using LatticeTransferState = TransferState<SourceLocationsLattice>;
 
-auto optionalClass() {
+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"))));
 }
 
 auto hasOptionalType() { return hasType(optionalClass()); }
if (auto *OptionalVal = cast_or_null<StructValue>(
     auto *HasValueVal = getHasValue(OptionalVal);
     assert(HasValueVal != nullptr);
 
     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);
return MatchSwitchBuilder<LatticeTransferState>()
       // opt.value_or(X) != X
       .CaseOf<Expr>(isValueOrNotEqX(), transferValueOrNotEqX)
 
       .Build();
 }
 
 } // namespace
 
+ast_matchers::DeclarationMatcher
+UncheckedOptionalAccessModel::optionalClassDecl() {
+  return optionalClass();
+}
+
 UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
     ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
     : DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
           Ctx),
       TransferMatchSwitch(buildTransferMatchSwitch(Options)) {}
 
 void UncheckedOptionalAccessModel::transfer(const Stmt *S,
                                             SourceLocationsLattice &L,
                                             Environment &Env) {
   LatticeTransferState State(L, Env);
   TransferMatchSwitch(*S, getASTContext(), State);
 }
 
 } // namespace dataflow
 } // namespace clang