diff options
6 files changed, 81 insertions, 20 deletions
diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h index 11086fb..62bf42d 100644 --- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h @@ -25,7 +25,8 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck { public: UncheckedOptionalAccessCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), - ModelOptions{Options.get("IgnoreSmartPointerDereference", false)} {} + ModelOptions{Options.get("IgnoreSmartPointerDereference", false), + Options.get("IgnoreValueCalls", false)} {} void registerMatchers(ast_matchers::MatchFinder *Finder) override; void check(const ast_matchers::MatchFinder::MatchResult &Result) override; bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { @@ -34,6 +35,7 @@ public: void storeOptions(ClangTidyOptions::OptionMap &Opts) override { Options.store(Opts, "IgnoreSmartPointerDereference", ModelOptions.IgnoreSmartPointerDereference); + Options.store(Opts, "IgnoreValueCalls", ModelOptions.IgnoreValueCalls); } private: diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 2628a26..4283fe0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -367,7 +367,11 @@ Changes in existing checks - Improved :doc:`bugprone-unchecked-optional-access <clang-tidy/checks/bugprone/unchecked-optional-access>` check by supporting ``NullableValue::makeValue`` and ``NullableValue::makeValueInplace`` to - prevent false-positives for ``BloombergLP::bdlb::NullableValue`` type. + prevent false-positives for ``BloombergLP::bdlb::NullableValue`` type, and by + adding the `IgnoreValueCalls` option to suppress diagnostics for + ``optional::value()`` and the `IgnoreSmartPointerDereference` option to + ignore optionals reached via smart-pointer-like dereference, while still + diagnosing UB-prone dereferences via ``operator*`` and ``operator->``. - Improved :doc:`bugprone-unhandled-self-assignment <clang-tidy/checks/bugprone/unhandled-self-assignment>` check by adding 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 index 552e6db..ebed79e 100644 --- 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 @@ -308,3 +308,22 @@ advantages: * Performance. A single check can cover many or even all accesses within scope. This gives the user the best of both worlds -- the safety of a dynamic check, but without incurring redundant costs. + +Options +------- + +.. option:: IgnoreSmartPointerDereference + + If set to `true`, the check ignores optionals that + are reached through overloaded smart-pointer-like dereference (``operator*``, + ``operator->``) on classes other than the optional type itself. This helps + avoid false positives where the analysis cannot equate results across such + calls. This does not cover access through ``operator[]``. Default is `false`. + +.. option:: IgnoreValueCalls + + If set to `true`, the check does not diagnose calls + to ``optional::value()``. Diagnostics for ``operator*()`` and + ``operator->()`` remain enabled. This is useful for codebases that + intentionally rely on ``value()`` for defined, guarded access while still + flagging UB-prone operator dereferences. Default is `false`. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-value.cpp new file mode 100644 index 0000000..f546212 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-value.cpp @@ -0,0 +1,25 @@ +// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- \ +// RUN: -config="{CheckOptions: \ +// RUN: {bugprone-unchecked-optional-access.IgnoreValueCalls: true}}" -- \ +// RUN: -I %S/Inputs/unchecked-optional-access + +#include "absl/types/optional.h" + +struct Foo { + void foo() const {} +}; + +void unchecked_value_access(const absl::optional<int> &opt) { + opt.value(); // no-warning +} + +void unchecked_deref_operator_access(const absl::optional<int> &opt) { + *opt; + // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unchecked access to optional value +} + +void unchecked_arrow_operator_access(const absl::optional<Foo> &opt) { + opt->foo(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value +} + diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index 696c9f4..c547d6c 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -46,6 +46,9 @@ struct UncheckedOptionalAccessModelOptions { /// are confident in this const accessor caching, we shouldn't need the /// IgnoreSmartPointerDereference option anymore. bool IgnoreSmartPointerDereference = false; + + /// In generating diagnostics, ignore calls to `optional::value()`. + bool IgnoreValueCalls = false; }; using UncheckedOptionalAccessLattice = CachedConstAccessorsLattice<NoopLattice>; diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 0fa333e..d90f5d4 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -1153,26 +1153,34 @@ auto buildDiagnoseMatchSwitch( // FIXME: Evaluate the efficiency of matchers. If using matchers results in a // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. - auto IgnorableOptional = ignorableOptional(Options); - return CFGMatchSwitchBuilder< - const Environment, - llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>() - // optional::value - .CaseOfCFGStmt<CXXMemberCallExpr>( - valueCall(IgnorableOptional), - [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, - const Environment &Env) { - return diagnoseUnwrapCall(E->getImplicitObjectArgument(), Env); - }) - - // optional::operator*, optional::operator-> - .CaseOfCFGStmt<CallExpr>(valueOperatorCall(IgnorableOptional), - [](const CallExpr *E, + const auto IgnorableOptional = ignorableOptional(Options); + + auto DiagBuilder = + CFGMatchSwitchBuilder< + const Environment, + llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>() + // optional::operator*, optional::operator-> + .CaseOfCFGStmt<CallExpr>( + valueOperatorCall(IgnorableOptional), + [](const CallExpr *E, const MatchFinder::MatchResult &, + const Environment &Env) { + return diagnoseUnwrapCall(E->getArg(0), Env); + }); + + auto Builder = Options.IgnoreValueCalls + ? std::move(DiagBuilder) + : std::move(DiagBuilder) + // optional::value + .CaseOfCFGStmt<CXXMemberCallExpr>( + valueCall(IgnorableOptional), + [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, const Environment &Env) { - return diagnoseUnwrapCall(E->getArg(0), Env); - }) - .Build(); + return diagnoseUnwrapCall( + E->getImplicitObjectArgument(), Env); + }); + + return std::move(Builder).Build(); } } // namespace |