[clang] [clang][dataflow] Fix bugprone-unchecked-optional-access for recent changes to libcxx (PR #188044)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 23 08:40:28 PDT 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-analysis
Author: Jan Voung (jvoung)
<details>
<summary>Changes</summary>
Fix for https://github.com/llvm/llvm-project/issues/187788
- 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. This covers the case when the class derives from optional.
- or check if the SubExpr of the cast is already the public optional type (when not deriving from optional -- since you won't see this first layer in the cast chain).
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/188044.diff
3 Files Affected:
- (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+43-49)
- (modified) clang/unittests/Analysis/FlowSensitive/MockHeaders.cpp (+16-14)
- (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+30)
``````````diff
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index b6eb8f0228932..5cb9ac134e793 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,31 +188,50 @@ 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.
+ for (const CXXBaseSpecifier *Base : Cast->path()) {
+ if (isDesugaredTypeOptionalOrPointerToOptional(Base->getType()))
+ return true;
+ }
+
+ // 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());
+}
+
+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(
llvm::StringRef operator_name,
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
return cxxOperatorCallExpr(
- hasOverloadedOperatorName(operator_name),
- callee(cxxMethodDecl(ofClass(optionalClass()))),
+ hasOverloadedOperatorName(operator_name), 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;
+ }
}
)");
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/188044
More information about the cfe-commits
mailing list