From a33d0c02f8a31fdabf2a678fd02c7e6b4ca4a274 Mon Sep 17 00:00:00 2001 From: mtx Date: Sun, 9 Nov 2025 17:01:11 +0800 Subject: [PATCH 1/3] [clang-tidy] Add `IgnoreValueCalls` option to bugprone-unchecked-optional-access --- .../bugprone/UncheckedOptionalAccessCheck.h | 4 ++- clang-tools-extra/docs/ReleaseNotes.rst | 5 +++- .../bugprone/unchecked-optional-access.rst | 19 ++++++++++++++ ...unchecked-optional-access-ignore-value.cpp | 25 +++++++++++++++++++ .../Models/UncheckedOptionalAccessModel.h | 7 ++++++ .../Models/UncheckedOptionalAccessModel.cpp | 15 +++++++++++ 6 files changed, 73 insertions(+), 2 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-value.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h index 11086fb4bfda1..62bf42da4f9f9 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 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck { 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 666865cfb2fcd..7e1e1802ade22 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -351,7 +351,10 @@ Changes in existing checks - Improved :doc:`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()`` while still diagnosing UB-prone dereferences via + ``operator*`` and ``operator->``. - Improved :doc:`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 552e6db699696..76078571f8c2d 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`` (default is ``false``), 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. Note: This does not cover access through ``operator[]``. + +.. option:: IgnoreValueCalls + + If set to ``true`` (default is ``false``), 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. 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 0000000000000..f54621269f8c0 --- /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 &opt) { + opt.value(); // no-warning +} + +void unchecked_deref_operator_access(const absl::optional &opt) { + *opt; + // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unchecked access to optional value +} + +void unchecked_arrow_operator_access(const absl::optional &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 696c9f4a6cf5c..07fa95fe81059 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -46,6 +46,13 @@ 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()`. + /// + /// Projects that intentionally use `value()` as a guarded access pattern may + /// set this to true to suppress diagnostics for `value()` while continuing to + /// diagnose UB-prone operator accesses (`operator*`, `operator->`). + bool IgnoreValueCalls = false; }; using UncheckedOptionalAccessLattice = CachedConstAccessorsLattice; diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 0fa333eedcfdd..f4dacfb0bcff4 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -1154,6 +1154,21 @@ auto buildDiagnoseMatchSwitch( // lot of duplicated work (e.g. string comparisons), consider providing APIs // that avoid it through memoization. auto IgnorableOptional = ignorableOptional(Options); + if (Options.IgnoreValueCalls) { + // Only diagnose operator-based unwraps. Value calls are ignored. + return CFGMatchSwitchBuilder< + const Environment, + llvm::SmallVector>() + // optional::operator*, optional::operator-> + .CaseOfCFGStmt(valueOperatorCall(IgnorableOptional), + [](const CallExpr *E, + const MatchFinder::MatchResult &, + const Environment &Env) { + return diagnoseUnwrapCall(E->getArg(0), Env); + }) + .Build(); + } + return CFGMatchSwitchBuilder< const Environment, llvm::SmallVector>() From 7ee9c58b701bfe30c5b2a6f161776ec496cb810b Mon Sep 17 00:00:00 2001 From: mtx Date: Mon, 10 Nov 2025 10:19:17 +0800 Subject: [PATCH 2/3] Fix --- clang-tools-extra/docs/ReleaseNotes.rst | 7 +-- .../bugprone/unchecked-optional-access.rst | 8 +-- .../Models/UncheckedOptionalAccessModel.cpp | 53 ++++++++----------- 3 files changed, 29 insertions(+), 39 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 7e1e1802ade22..c0804e1555c9a 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -352,9 +352,10 @@ Changes in existing checks ` check by supporting ``NullableValue::makeValue`` and ``NullableValue::makeValueInplace`` to prevent false-positives for ``BloombergLP::bdlb::NullableValue`` type, and by - adding the ``IgnoreValueCalls`` option to suppress diagnostics for - ``optional::value()`` while still diagnosing UB-prone dereferences via - ``operator*`` and ``operator->``. + 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 ` 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 76078571f8c2d..ebed79e339d4b 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 @@ -314,16 +314,16 @@ Options .. option:: IgnoreSmartPointerDereference - If set to ``true`` (default is ``false``), the check ignores optionals that + 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. Note: This does not cover access through ``operator[]``. + calls. This does not cover access through ``operator[]``. Default is `false`. .. option:: IgnoreValueCalls - If set to ``true`` (default is ``false``), the check does not diagnose calls + 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. + flagging UB-prone operator dereferences. Default is `false`. diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index f4dacfb0bcff4..956d59c572076 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -1153,41 +1153,30 @@ 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); - if (Options.IgnoreValueCalls) { - // Only diagnose operator-based unwraps. Value calls are ignored. - return CFGMatchSwitchBuilder< - const Environment, - llvm::SmallVector>() - // optional::operator*, optional::operator-> - .CaseOfCFGStmt(valueOperatorCall(IgnorableOptional), - [](const CallExpr *E, - const MatchFinder::MatchResult &, - const Environment &Env) { - return diagnoseUnwrapCall(E->getArg(0), Env); - }) + const auto IgnorableOptional = ignorableOptional(Options); + + auto Builder = CFGMatchSwitchBuilder< + const Environment, + llvm::SmallVector>() + .CaseOfCFGStmt( + valueOperatorCall(IgnorableOptional), + [](const CallExpr *E, const MatchFinder::MatchResult &, + const Environment &Env) { + return diagnoseUnwrapCall(E->getArg(0), Env); + }); + + if (!Options.IgnoreValueCalls) { + return std::move(Builder) + .CaseOfCFGStmt( + valueCall(IgnorableOptional), + [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, + const Environment &Env) { + return diagnoseUnwrapCall(E->getImplicitObjectArgument(), Env); + }) .Build(); } - return CFGMatchSwitchBuilder< - const Environment, - llvm::SmallVector>() - // optional::value - .CaseOfCFGStmt( - valueCall(IgnorableOptional), - [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, - const Environment &Env) { - return diagnoseUnwrapCall(E->getImplicitObjectArgument(), Env); - }) - - // optional::operator*, optional::operator-> - .CaseOfCFGStmt(valueOperatorCall(IgnorableOptional), - [](const CallExpr *E, - const MatchFinder::MatchResult &, - const Environment &Env) { - return diagnoseUnwrapCall(E->getArg(0), Env); - }) - .Build(); + return std::move(Builder).Build(); } } // namespace From ede97cbb21e600640f4625d556115b88205218dd Mon Sep 17 00:00:00 2001 From: mtx Date: Mon, 10 Nov 2025 19:46:27 +0800 Subject: [PATCH 3/3] Fix --- .../Models/UncheckedOptionalAccessModel.h | 4 -- .../Models/UncheckedOptionalAccessModel.cpp | 44 ++++++++++--------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h index 07fa95fe81059..c547d6ce2e387 100644 --- a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h +++ b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h @@ -48,10 +48,6 @@ struct UncheckedOptionalAccessModelOptions { bool IgnoreSmartPointerDereference = false; /// In generating diagnostics, ignore calls to `optional::value()`. - /// - /// Projects that intentionally use `value()` as a guarded access pattern may - /// set this to true to suppress diagnostics for `value()` while continuing to - /// diagnose UB-prone operator accesses (`operator*`, `operator->`). bool IgnoreValueCalls = false; }; diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 956d59c572076..d90f5d4eaf7bb 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -1155,26 +1155,30 @@ auto buildDiagnoseMatchSwitch( // that avoid it through memoization. const auto IgnorableOptional = ignorableOptional(Options); - auto Builder = CFGMatchSwitchBuilder< - const Environment, - llvm::SmallVector>() - .CaseOfCFGStmt( - valueOperatorCall(IgnorableOptional), - [](const CallExpr *E, const MatchFinder::MatchResult &, - const Environment &Env) { - return diagnoseUnwrapCall(E->getArg(0), Env); - }); - - if (!Options.IgnoreValueCalls) { - return std::move(Builder) - .CaseOfCFGStmt( - valueCall(IgnorableOptional), - [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &, - const Environment &Env) { - return diagnoseUnwrapCall(E->getImplicitObjectArgument(), Env); - }) - .Build(); - } + auto DiagBuilder = + CFGMatchSwitchBuilder< + const Environment, + llvm::SmallVector>() + // optional::operator*, optional::operator-> + .CaseOfCFGStmt( + 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( + valueCall(IgnorableOptional), + [](const CXXMemberCallExpr *E, + const MatchFinder::MatchResult &, + const Environment &Env) { + return diagnoseUnwrapCall( + E->getImplicitObjectArgument(), Env); + }); return std::move(Builder).Build(); }