diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- 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 @@ "bugprone-throw-keyword-missing"); CheckFactories.registerCheck( "bugprone-too-small-loop-variable"); + CheckFactories.registerCheck( + "bugprone-unchecked-optional-access"); CheckFactories.registerCheck( "bugprone-undefined-memory-manipulation"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -59,6 +59,7 @@ TerminatingContinueCheck.cpp ThrowKeywordMissingCheck.cpp TooSmallLoopVariableCheck.cpp + UncheckedOptionalAccessCheck.cpp UndefinedMemoryManipulationCheck.cpp UndelegatedConstructorCheck.cpp UnhandledExceptionAtNewCheck.cpp @@ -80,6 +81,8 @@ clang_target_link_libraries(clangTidyBugproneModule PRIVATE clangAnalysis + clangAnalysisFlowSensitive + clangAnalysisFlowSensitiveModels clangAST clangASTMatchers clangBasic diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h new file mode 100644 --- /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`, `absl::optional`, +/// or `base::Optional` 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/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp @@ -0,0 +1,100 @@ +//===--- 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 +#include + +namespace clang { +namespace tidy { +namespace bugprone { +using ast_matchers::MatchFinder; + +void UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) { + using namespace ast_matchers; + + auto HasOptionalCallDescendant = hasDescendant( + callExpr(callee(cxxMethodDecl(ofClass(dataflow::optionalClass()))))); + 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("f"), + this); +} + +void UncheckedOptionalAccessCheck::check( + const MatchFinder::MatchResult &Result) { + using dataflow::ControlFlowContext; + using dataflow::DataflowAnalysisContext; + using dataflow::DataflowAnalysisState; + using dataflow::Environment; + using dataflow::SourceLocationsLattice; + using dataflow::UncheckedOptionalAccessModel; + using dataflow::WatchedLiteralsSolver; + using llvm::Expected; + using llvm::Optional; + + if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred()) + return; + + const auto *FuncDecl = Result.Nodes.getNodeAs("f"); + if (FuncDecl->isTemplated()) + return; + + Expected Context = + ControlFlowContext::build(FuncDecl, FuncDecl->getBody(), Result.Context); + if (!Context) + return; + + DataflowAnalysisContext AnalysisContext( + std::make_unique()); + Environment Environment(AnalysisContext, *FuncDecl); + UncheckedOptionalAccessModel Analysis(*Result.Context); + Expected>>> + BlockToOutputState = + dataflow::runDataflowAnalysis(*Context, Analysis, Environment); + if (!BlockToOutputState || + BlockToOutputState->size() <= Context->getCFG().getExit().getBlockID()) + return; + + const Optional> + &ExitBlockState = + (*BlockToOutputState)[Context->getCFG().getExit().getBlockID()]; + if (!ExitBlockState.hasValue()) + return; + + const SourceLocationsLattice &ExitBlockLattice = ExitBlockState->Lattice; + for (const SourceLocation &Loc : ExitBlockLattice.getSourceLocations()) + diag(Loc, "unchecked access to optional value"); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -113,6 +113,12 @@ Finds initializations of C++ shared pointers to non-array type that are initialized with an array. +- New :doc:`bugprone-unchecked-optional-access + ` check. + + Warns when the code is unwrapping a `std::optional`, `absl::optional`, + or `base::Optional` object without assuring that it contains a value. + - New :doc:`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 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-unchecked-optional-access.rst @@ -0,0 +1,228 @@ +.. title:: clang-tidy - bugprone-unchecked-optional-access + +bugprone-unchecked-optional-access +================================== + +This check identifies unsafe accesses to values contained in +``std::optional``, ``absl::optional``, or ``base::Optional`` +objects. Below we will refer to all these types collectively as +``optional``. + +An access to the value of an ``optional`` 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`` is considered safe if and only if +code in the local scope (for example, a function body) ensures that the +``optional`` has a value in all possible execution paths that can reach the +access. That should happen either through an explicit check, using the +``optional::has_value`` member function, or by constructing the +``optional`` in a way that shows that it unambiguously holds a value (e.g +using ``std::make_optional`` which always returns a populated +``std::optional``). + +Below we list some examples of safe and unsafe optional access patterns. + +Checking if a value exists, then accessing 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 opt) { + if (opt.has_value()) { + use(*opt); + } + } + + +Checking if a value exists, then accessing 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 opt1) { + if (opt1.has_value()) { + absl::optional opt2 = opt1; + use(*opt2); + } + } + + +Ensuring 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 opt) { + DCHECK(opt.has_value()); + use(*opt); + } + +Ensuring that a value exists, then accessing 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 opt) { + bool safe = false; + if (opt.has_value() && SomeOtherCondition()) { + safe = true; + } + // ... more code... + if (safe) { + use(*opt); + } + } + +Accessing 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 opt) { + use(*opt); // unsafe: it is unclear whether `opt` has a value. + } + + +Accessing 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 opt) { + if (opt.has_value()) { + } else { + use(opt.value()); // unsafe: it is clear that `opt` does *not* have a value. + } + } + +Assuming 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. + } + } + +In such cases it is best to store the result of the function call in a +local variable and use it 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); + } + } + +Relying on invariants of uncommon APIs +-------------------------------------- + +The check is unaware of invariants of uncommon APIs. For example: + + void f(Foo foo) { + if (foo.HasProprty("bar")) { + use(*foo.GetProperty("bar")); // unsafe: it is unclear whether `foo.GetProperty("bar")` has a value. + } + } + +In such cases it is best to 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); + } + } + +Checking if a value exists, then passing 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 opt) { + use(*opt); // unsafe: it is unclear whether `opt` has a value. + } + + void f(absl::optional opt) { + if (opt.has_value()) { + g(opt); + } + } + +In such cases it is best to either pass the value directly when calling a +function or check that the optional contains a value in the local scope of the +callee. For example: + +.. code-block:: c++ + + void g(int val) { + use(val); + } + + void f(absl::optional opt) { + if (opt.has_value()) { + g(*opt); + } + } + +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; + + 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 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -105,6 +105,7 @@ `bugprone-terminating-continue `_, "Yes" `bugprone-throw-keyword-missing `_, `bugprone-too-small-loop-variable `_, + `bugprone-unchecked-optional-access `_, "Yes" `bugprone-undefined-memory-manipulation `_, `bugprone-undelegated-constructor `_, `bugprone-unhandled-exception-at-new `_, 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 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/absl/types/optional.h @@ -0,0 +1,72 @@ +#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 struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +// clang-format on + +template +using remove_reference_t = typename remove_reference::type; + +template +constexpr T &&forward(remove_reference_t &t) noexcept; + +template +constexpr T &&forward(remove_reference_t &&t) noexcept; + +template +constexpr remove_reference_t &&move(T &&x); + +struct nullopt_t { + constexpr explicit nullopt_t() {} +}; + +constexpr nullopt_t nullopt; + +template +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 + constexpr T value_or(U &&v) const &; + template + T value_or(U &&v) &&; + + template + 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 --- /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 &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 &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 &opt) { + opt->foo(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value +} + +void f2(const absl::optional &opt) { + if (opt.has_value()) { + opt.value(); + } +} + +template +void function_template_without_user(const absl::optional &opt) { + opt.value(); // no-warning +} + +template +void function_template_with_user(const absl::optional &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value +} + +void function_template_user(const absl::optional &opt) { + // Instantiate the f3 function template so that it gets matched by the check. + function_template_with_user(opt); +} + +template +void function_template_with_specialization(const absl::optional &opt) { + opt.value(); // no-warning +} + +template <> +void function_template_with_specialization( + const absl::optional &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value +} + +template +class ClassTemplateWithSpecializations { + void f(const absl::optional &opt) { + opt.value(); // no-warning + } +}; + +template +class ClassTemplateWithSpecializations { + void f(const absl::optional &opt) { + opt.value(); // no-warning + } +}; + +template <> +class ClassTemplateWithSpecializations { + void f(const absl::optional &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 +struct C1 {}; + +template +struct C2 : public C1 { + ~C2() {} +}; + +template class B> +struct C3 : public B { + ~C3() {} +}; + +void multiple_unchecked_accesses(absl::optional opt1, + absl::optional 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 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 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -36,6 +36,9 @@ bool IgnoreSmartPointerDereference = false; }; +/// A matcher for the optional classes covered by this model. +ast_matchers::DeclarationMatcher optionalClass(); + /// Dataflow analysis that discovers unsafe accesses of optional values and /// adds the respective source locations to the lattice. /// diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp --- 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" @@ -29,13 +30,9 @@ namespace clang { namespace dataflow { -namespace { - -using namespace ::clang::ast_matchers; -using LatticeTransferState = TransferState; - -auto optionalClass() { +ast_matchers::DeclarationMatcher optionalClass() { + using namespace ::clang::ast_matchers; return classTemplateSpecializationDecl( anyOf(hasName("std::optional"), hasName("std::__optional_storage_base"), hasName("__optional_destruct_base"), hasName("absl::optional"), @@ -43,6 +40,12 @@ hasTemplateArgument(0, refersToType(type().bind("T")))); } +namespace { + +using namespace ::clang::ast_matchers; + +using LatticeTransferState = TransferState; + auto hasOptionalType() { return hasType(optionalClass()); } auto isOptionalMemberCallWithName( @@ -230,6 +233,8 @@ } // 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 intepretation of the result. State.Lattice.getSourceLocations().insert(ObjectExpr->getBeginLoc()); }