[clang] [clang][dataflow] Fix bugprone-unchecked-optional-access for recent changes to libcxx (PR #188044)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 23 08:41:46 PDT 2026
https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/188044
>From d4c13121bfe49b763e6287cadd1e11fbf64a9d16 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Mar 2026 14:07:36 +0000
Subject: [PATCH 1/5] [clang][dataflow] Fix bugprone-unchecked-optional-access
for recent changes to libcxx
Mainly, when checking the receiver with an UncheckedDerivedToBase cast,
just check each component of the chain if we've hit the public optional
class, rather than rely on public vs private/protected inheritance,
or check if the SubExpr of the cast is the public optional type.
See comment about why there is public inheritance to a private base
class at the moment:
https://github.com/llvm/llvm-project/issues/187788#issuecomment-4106543794
The performance doesn't seem much different in several benchmarks.
Perhaps because we first see if the method name matches, which filters a
bunch out.
Also, make the operator* and operator-> calls do this search, since
https://github.com/llvm/llvm-project/pull/185252 moved the methods to
the internal base class.
Update the test mock headers slightly to reflect the operator* and
operator-> change.
---
.../Models/UncheckedOptionalAccessModel.cpp | 90 +++++++++----------
.../Analysis/FlowSensitive/MockHeaders.cpp | 30 ++++---
.../UncheckedOptionalAccessModelTest.cpp | 30 +++++++
3 files changed, 88 insertions(+), 62 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index b6eb8f0228932..51038ff51b1c7 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -157,52 +157,27 @@ auto hasOptionalOrDerivedType() {
return hasType(desugarsToOptionalOrDerivedType());
}
-QualType getPublicType(const Expr *E) {
- auto *Cast = dyn_cast<ImplicitCastExpr>(E->IgnoreParens());
- if (Cast == nullptr || Cast->getCastKind() != CK_UncheckedDerivedToBase) {
- QualType Ty = E->getType();
- if (Ty->isPointerType())
- return Ty->getPointeeType();
- return Ty;
- }
-
- // Is the derived type that we're casting from the type of `*this`? In this
- // special case, we can upcast to the base class even if the base is
- // non-public.
- bool CastingFromThis = isa<CXXThisExpr>(Cast->getSubExpr());
-
- // Find the least-derived type in the path (i.e. the last entry in the list)
- // that we can access.
- const CXXBaseSpecifier *PublicBase = nullptr;
- for (const CXXBaseSpecifier *Base : Cast->path()) {
- if (Base->getAccessSpecifier() != AS_public && !CastingFromThis)
- break;
- PublicBase = Base;
- CastingFromThis = false;
- }
-
- if (PublicBase != nullptr)
- return PublicBase->getType();
-
- // We didn't find any public type that we could cast to. There may be more
- // casts in `getSubExpr()`, so recurse. (If there aren't any more casts, this
- // will return the type of `getSubExpr()`.)
- return getPublicType(Cast->getSubExpr());
+bool isDesugaredTypeOptionalOrPointerToOptional(QualType Ty) {
+ if (Ty->isPointerType())
+ Ty = Ty->getPointeeType();
+ const Type &DesugaredTy = *Ty->getUnqualifiedDesugaredType();
+ return DesugaredTy.isRecordType() &&
+ hasOptionalClassName(*DesugaredTy.getAsCXXRecordDecl());
}
-// Returns the least-derived type for the receiver of `MCE` that
-// `MCE.getImplicitObjectArgument()->IgnoreParentImpCasts()` can be downcast to.
-// Effectively, we upcast until we reach a non-public base class, unless that
-// base is a base of `*this`.
+// Returns true if `E` is intended to refer to an optional type, but may refer
+// to an internal base class of the optional type, and involve an
+// UncheckedDerivedToBase cast.
//
// This is needed to correctly match methods called on types derived from
-// `std::optional`.
+// `std::optional`, as methods may be defined in a private base class like
+// `std::__optional_storage_base`.
//
// Say we have a `struct Derived : public std::optional<int> {} d;` For a call
// `d.has_value()`, the `getImplicitObjectArgument()` looks like this:
//
// ImplicitCastExpr 'const std::__optional_storage_base<int>' lvalue
-// | <UncheckedDerivedToBase (optional -> __optional_storage_base)>
+// | <UncheckedDerivedToBase (optional -> ... -> __optional_storage_base)>
// `-DeclRefExpr 'Derived' lvalue Var 'd' 'Derived'
//
// The type of the implicit object argument is `__optional_storage_base`
@@ -213,23 +188,42 @@ QualType getPublicType(const Expr *E) {
// calling a method on `optional`.
//
// Instead, starting with the most derived type, we need to follow the chain of
-// casts
-QualType getPublicReceiverType(const CXXMemberCallExpr &MCE) {
- return getPublicType(MCE.getImplicitObjectArgument());
+// casts, until we reach a class that matches `isDesugaredTypeOptional`
+// (if at all).
+bool hasReceiverTypeDesugaringToOptional(const Expr *E) {
+ auto *Cast = dyn_cast<ImplicitCastExpr>(E->IgnoreParens());
+ if (Cast == nullptr || Cast->getCastKind() != CK_UncheckedDerivedToBase)
+ return isDesugaredTypeOptionalOrPointerToOptional(E->getType());
+
+ // See if we hit an optional type in the cast path, going from derived to base
+ // (we could stop earlier, without going into the internal
+ // `std::__optional_storage_base` type).
+ for (const CXXBaseSpecifier *Base : Cast->path()) {
+ if (isDesugaredTypeOptionalOrPointerToOptional(Base->getType()))
+ return true;
+ }
+
+ // We didn't find a optional in the cast path. There may be more casts in
+ // `getSubExpr()`, so recurse. If there aren't any more casts, just check the
+ // type of `getSubExpr()`.
+ return hasReceiverTypeDesugaringToOptional(Cast->getSubExpr());
+}
+
+AST_MATCHER(CXXMemberCallExpr, hasOptionalReceiverType) {
+ return hasReceiverTypeDesugaringToOptional(Node.getImplicitObjectArgument());
}
-AST_MATCHER_P(CXXMemberCallExpr, publicReceiverType,
- ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
- return InnerMatcher.matches(getPublicReceiverType(Node), Finder, Builder);
+AST_MATCHER(CXXOperatorCallExpr, hasOptionalOperatorObjectType) {
+ return hasReceiverTypeDesugaringToOptional(Node.getArg(0));
}
auto isOptionalMemberCallWithNameMatcher(
ast_matchers::internal::Matcher<NamedDecl> matcher,
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
- return cxxMemberCallExpr(Ignorable ? on(expr(unless(*Ignorable)))
- : anything(),
- publicReceiverType(desugarsToOptionalType()),
- callee(cxxMethodDecl(matcher)));
+ return cxxMemberCallExpr(
+ Ignorable ? on(expr(unless(*Ignorable))) : anything(),
+ callee(cxxMethodDecl(matcher)),
+ hasOptionalReceiverType());
}
auto isOptionalOperatorCallWithName(
@@ -237,7 +231,7 @@ auto isOptionalOperatorCallWithName(
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
return cxxOperatorCallExpr(
hasOverloadedOperatorName(operator_name),
- callee(cxxMethodDecl(ofClass(optionalClass()))),
+ hasOptionalOperatorObjectType(),
Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
}
diff --git a/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp b/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp
index 7eee7c9bcef5c..2a85a08e819d5 100644
--- a/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp
@@ -607,10 +607,25 @@ struct __optional_destruct_base {
template <class _Tp>
struct __optional_storage_base : __optional_destruct_base<_Tp> {
constexpr bool has_value() const noexcept;
+
+ const _Tp& operator*() const&;
+ _Tp& operator*() &;
+ const _Tp&& operator*() const&&;
+ _Tp&& operator*() &&;
+
+ const _Tp* operator->() const;
+ _Tp* operator->();
+
+ const _Tp& value() const&;
+ _Tp& value() &;
+ const _Tp&& value() const&&;
+ _Tp&& value() &&;
};
+// Note: the inheritance may or may not be private:
+// https://github.com/llvm/llvm-project/issues/187788
template <typename _Tp>
-class optional : private __optional_storage_base<_Tp> {
+class optional : public __optional_storage_base<_Tp> {
using __base = __optional_storage_base<_Tp>;
public:
@@ -745,19 +760,6 @@ class optional : private __optional_storage_base<_Tp> {
int> = 0>
constexpr optional& operator=(optional<_Up>&& __v);
- const _Tp& operator*() const&;
- _Tp& operator*() &;
- const _Tp&& operator*() const&&;
- _Tp&& operator*() &&;
-
- const _Tp* operator->() const;
- _Tp* operator->();
-
- const _Tp& value() const&;
- _Tp& value() &;
- const _Tp&& value() const&&;
- _Tp&& value() &&;
-
template <typename U>
constexpr _Tp value_or(U&& v) const&;
template <typename U>
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 5485c91de8f82..6e99b2cc4c9e4 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -1599,6 +1599,36 @@ TEST_P(UncheckedOptionalAccessTest, WithAlias) {
void target(MyOptional<int> opt) {
opt.value(); // [[unsafe]]
+ *opt; // [[unsafe]]
+ if (opt.has_value()) {
+ opt.value();
+ *opt;
+ }
+ if (opt) {
+ opt.value();
+ *opt;
+ }
+ }
+ )");
+
+ ExpectDiagnosticsFor(
+ R"(
+ #include "unchecked_optional_access_test.h"
+
+ template <typename T>
+ using MyOptional = $ns::$optional<T>;
+
+ void target(const MyOptional<int>* opt) {
+ opt->value(); // [[unsafe]]
+ **opt; // [[unsafe]]
+ if (opt->has_value()) {
+ opt->value();
+ **opt;
+ }
+ if (*opt) {
+ opt->value();
+ **opt;
+ }
}
)");
}
>From 65c5e6c8853f5a1a846231c653bbb13eaf671feb Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Mar 2026 14:35:06 +0000
Subject: [PATCH 2/5] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 51038ff51b1c7..7ff57ac8b663b 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -222,16 +222,14 @@ auto isOptionalMemberCallWithNameMatcher(
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
return cxxMemberCallExpr(
Ignorable ? on(expr(unless(*Ignorable))) : anything(),
- callee(cxxMethodDecl(matcher)),
- hasOptionalReceiverType());
+ callee(cxxMethodDecl(matcher)), hasOptionalReceiverType());
}
auto isOptionalOperatorCallWithName(
llvm::StringRef operator_name,
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
return cxxOperatorCallExpr(
- hasOverloadedOperatorName(operator_name),
- hasOptionalOperatorObjectType(),
+ hasOverloadedOperatorName(operator_name), hasOptionalOperatorObjectType(),
Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
}
>From 94cccf4a8d586a624e82321d7fc5d25da3981d8b Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Mar 2026 14:36:12 +0000
Subject: [PATCH 3/5] Update comment
---
.../Models/UncheckedOptionalAccessModel.cpp | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 7ff57ac8b663b..b23fe583f96e3 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -203,9 +203,12 @@ bool hasReceiverTypeDesugaringToOptional(const Expr *E) {
return true;
}
- // We didn't find a optional in the cast path. There may be more casts in
- // `getSubExpr()`, so recurse. If there aren't any more casts, just check the
- // type of `getSubExpr()`.
+ // We didn't find a optional in the cast path. It may be that the
+ // subexpression itself is an optional type. For example, if we just have
+ // `std::optional<int>` instead of
+ // `struct Derived : public std::optional<int>`
+ // then the Cast path() won't include `optional` itself. However, the
+ // SubExpr type is the optional type.
return hasReceiverTypeDesugaringToOptional(Cast->getSubExpr());
}
>From 85b448223cb484bdb8e3523a670b65942785d740 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Mar 2026 15:03:30 +0000
Subject: [PATCH 4/5] simplify comment a bit
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index b23fe583f96e3..5cb9ac134e793 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -195,9 +195,8 @@ bool hasReceiverTypeDesugaringToOptional(const Expr *E) {
if (Cast == nullptr || Cast->getCastKind() != CK_UncheckedDerivedToBase)
return isDesugaredTypeOptionalOrPointerToOptional(E->getType());
- // See if we hit an optional type in the cast path, going from derived to base
- // (we could stop earlier, without going into the internal
- // `std::__optional_storage_base` type).
+ // See if we hit an optional type in the cast path, going from derived
+ // to base.
for (const CXXBaseSpecifier *Base : Cast->path()) {
if (isDesugaredTypeOptionalOrPointerToOptional(Base->getType()))
return true;
>From 090ef512ef3e482ee1c6ea5fef8a93d3fa73f07f Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 23 Mar 2026 15:41:26 +0000
Subject: [PATCH 5/5] Fix comment
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 5cb9ac134e793..ad7d9c237a493 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -188,8 +188,8 @@ bool isDesugaredTypeOptionalOrPointerToOptional(QualType Ty) {
// calling a method on `optional`.
//
// Instead, starting with the most derived type, we need to follow the chain of
-// casts, until we reach a class that matches `isDesugaredTypeOptional`
-// (if at all).
+// casts, until we reach a class that matches
+// `isDesugaredTypeOptionalOrPointerToOptional` (if at all).
bool hasReceiverTypeDesugaringToOptional(const Expr *E) {
auto *Cast = dyn_cast<ImplicitCastExpr>(E->IgnoreParens());
if (Cast == nullptr || Cast->getCastKind() != CK_UncheckedDerivedToBase)
More information about the cfe-commits
mailing list