[clang-tidy] Add IgnoreValueCalls option to bugprone-unchecked-optional-access
#167209
+73
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang-analysis @llvm/pr-subscribers-clang-tidy Author: mitchell (zeyi2) ChangesAdd a new option Closes #163831 Full diff: https://github.com/llvm/llvm-project/pull/167209.diff 6 Files Affected:
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
<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()`` 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 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<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 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<NoopLattice>;
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<UncheckedOptionalAccessDiagnostic>>()
+ // optional::operator*, optional::operator->
+ .CaseOfCFGStmt<CallExpr>(valueOperatorCall(IgnorableOptional),
+ [](const CallExpr *E,
+ const MatchFinder::MatchResult &,
+ const Environment &Env) {
+ return diagnoseUnwrapCall(E->getArg(0), Env);
+ })
+ .Build();
+ }
+
return CFGMatchSwitchBuilder<
const Environment,
llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>()
|
Member
|
@llvm/pr-subscribers-clang Author: mitchell (zeyi2) ChangesAdd a new option Closes #163831 Full diff: https://github.com/llvm/llvm-project/pull/167209.diff 6 Files Affected:
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
<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()`` 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 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<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 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<NoopLattice>;
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<UncheckedOptionalAccessDiagnostic>>()
+ // optional::operator*, optional::operator->
+ .CaseOfCFGStmt<CallExpr>(valueOperatorCall(IgnorableOptional),
+ [](const CallExpr *E,
+ const MatchFinder::MatchResult &,
+ const Environment &Env) {
+ return diagnoseUnwrapCall(E->getArg(0), Env);
+ })
+ .Build();
+ }
+
return CFGMatchSwitchBuilder<
const Environment,
llvm::SmallVector<UncheckedOptionalAccessDiagnostic>>()
|
| ``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 |
Contributor
There was a problem hiding this comment.
Suggested change
| adding the ``IgnoreValueCalls`` option to suppress diagnostics for | |
| adding the `IgnoreValueCalls` option to suppress diagnostics for |
|
|
||
| .. option:: IgnoreSmartPointerDereference | ||
|
|
||
| If set to ``true`` (default is ``false``), the check ignores optionals that |
Contributor
There was a problem hiding this comment.
Suggested change
| If set to ``true`` (default is ``false``), the check ignores optionals that | |
| If set to `true` (default is `false`), the check ignores optionals that |
Default value should be last sentence. Same in other option.
|
|
||
| .. option:: IgnoreValueCalls | ||
|
|
||
| If set to ``true`` (default is ``false``), the check does not diagnose calls |
Contributor
There was a problem hiding this comment.
Suggested change
| If set to ``true`` (default is ``false``), the check does not diagnose calls | |
| If set to `true` (default is `false`), the check does not diagnose calls |
| 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->``. |
Contributor
There was a problem hiding this comment.
Second added option is not mentioned.
EugeneZelenko
requested review from
HerrCai0907,
carlosgalvezp,
localspook and
vbvictor
vbvictor
reviewed
Nov 9, 2025
| const MatchFinder::MatchResult &, | ||
| const Environment &Env) { | ||
| return diagnoseUnwrapCall(E->getArg(0), Env); | ||
| }) |
Contributor
There was a problem hiding this comment.
We shouldn't duplicate code here.
We should first add case with *, -> and save temporary builder variable. Then if (!IgnoreValueCalls) add case with value. In the end, call .Build().
See that build returns himself
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new option
IgnoreValueCallstobugprone-unchecked-optional-accessCloses #163831