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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
