[clang] 7e63a0d - [clang-tidy] New check for safe usage of `std::optional` and like types.
Yitzhak Mandelbaum via cfe-commits
cfe-commits at lists.llvm.org
Fri May 6 11:59:44 PDT 2022
Author: Yitzhak Mandelbaum
Date: 2022-05-06T18:50:36Z
New Revision: 7e63a0d479dd3ccce20de5cddb0f138b537c08bb
URL: https://github.com/llvm/llvm-project/commit/7e63a0d479dd3ccce20de5cddb0f138b537c08bb
DIFF: https://github.com/llvm/llvm-project/commit/7e63a0d479dd3ccce20de5cddb0f138b537c08bb.diff
LOG: [clang-tidy] New check for safe usage of `std::optional` and like types.
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.
Differential Revision: https://reviews.llvm.org/D121120
Added:
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst
clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp
Modified:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 7b1a2c14ed3c1..2d8bfffc01268 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -63,6 +63,7 @@
#include "TerminatingContinueCheck.h"
#include "ThrowKeywordMissingCheck.h"
#include "TooSmallLoopVariableCheck.h"
+#include "UncheckedOptionalAccessCheck.h"
#include "UndefinedMemoryManipulationCheck.h"
#include "UndelegatedConstructorCheck.h"
#include "UnhandledExceptionAtNewCheck.h"
@@ -185,6 +186,8 @@ class BugproneModule : public ClangTidyModule {
"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>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index b33072ddf01b8..50f6862bc2f44 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -59,6 +59,7 @@ add_clang_library(clangTidyBugproneModule
TerminatingContinueCheck.cpp
ThrowKeywordMissingCheck.cpp
TooSmallLoopVariableCheck.cpp
+ UncheckedOptionalAccessCheck.cpp
UndefinedMemoryManipulationCheck.cpp
UndelegatedConstructorCheck.cpp
UnhandledExceptionAtNewCheck.cpp
@@ -80,6 +81,8 @@ add_clang_library(clangTidyBugproneModule
clang_target_link_libraries(clangTidyBugproneModule
PRIVATE
clangAnalysis
+ clangAnalysisFlowSensitive
+ clangAnalysisFlowSensitiveModels
clangAST
clangASTMatchers
clangBasic
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
new file mode 100644
index 0000000000000..ab6b172014324
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -0,0 +1,108 @@
+//===--- 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
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
new file mode 100644
index 0000000000000..64ab6005560b8
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -0,0 +1,39 @@
+//===--- 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
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 00cf38647b75f..00124b0cad75d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -113,6 +113,12 @@ New checks
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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst
new file mode 100644
index 0000000000000..40ca8eeb7ff86
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst
@@ -0,0 +1,273 @@
+.. 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
diff erences
+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
diff erent
+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
diff erent 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.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 6b3a6d83a361c..cfab8a01e42d3 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -105,6 +105,7 @@ Clang-Tidy Checks
`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>`_,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h
new file mode 100644
index 0000000000000..154cc262ab7cd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h
@@ -0,0 +1,73 @@
+#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_
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp
new file mode 100644
index 0000000000000..d28af7a37f4ea
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-unchecked-optional-access.cpp
@@ -0,0 +1,112 @@
+// 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_;
+};
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 235121b2e5759..8cc3fa73de9df 100644
--- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -49,6 +49,9 @@ class UncheckedOptionalAccessModel
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();
}
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index aaf8289c48674..c797e02cec990 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -13,6 +13,7 @@
#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"
@@ -32,10 +33,9 @@ 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"),
@@ -230,6 +230,8 @@ void transferUnwrapCall(const Expr *UnwrapExpr, const Expr *ObjectExpr,
}
// 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());
}
@@ -550,6 +552,11 @@ auto buildTransferMatchSwitch(
} // namespace
+ast_matchers::DeclarationMatcher
+UncheckedOptionalAccessModel::optionalClassDecl() {
+ return optionalClass();
+}
+
UncheckedOptionalAccessModel::UncheckedOptionalAccessModel(
ASTContext &Ctx, UncheckedOptionalAccessModelOptions Options)
: DataflowAnalysis<UncheckedOptionalAccessModel, SourceLocationsLattice>(
More information about the cfe-commits
mailing list