Skip to content

[Clang] Fix __is_trivially_equality_comparable returning true with ineligebile defaulted overloads #93113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 27, 2024
Merged

[Clang] Fix __is_trivially_equality_comparable returning true with ineligebile defaulted overloads #93113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ ABI Changes in This Version
ifuncs. Its purpose was to preserve backwards compatibility when the ".ifunc"
suffix got removed from the name mangling. The alias interacts badly with
GlobalOpt (see the issue #96197).

- Fixed Microsoft name mangling for auto non-type template arguments of pointer
type for MSVC 1920+. This change resolves incompatibilities with code compiled
by MSVC 1920+ but will introduce incompatibilities with code compiled by
Expand Down Expand Up @@ -740,6 +740,9 @@ Bug Fixes in This Version
negatives where the analysis failed to detect unchecked access to guarded
data.

- ``__is_trivially_equality_comparable`` no longer returns true for types which
have a constrained defaulted comparison operator (#GH89293).

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down
3 changes: 0 additions & 3 deletions clang/include/clang/AST/Type.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,9 +1142,6 @@ class QualType {
/// Return true if this is a trivially relocatable type.
bool isTriviallyRelocatableType(const ASTContext &Context) const;

/// Return true if this is a trivially equality comparable type.
bool isTriviallyEqualityComparableType(const ASTContext &Context) const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unfortunate that the other traits are implemented directly on Type but this one is in SemaExprCXX.cpp and isn't possible to compute without access to Sema. ISTM we should be able to answer this question on the type rather than requiring semantic analysis (considering potential uses like in clang-tidy where there is no Sema available).

Perhaps another way to approach this is with the DefinitionData for classes -- as we add members to the class, we build up information about whether something is trivially copyable, etc and store that on the bits defined in CXXRecordDeclDefinitionBits.def. Maybe we could do the same for equality comparable types, then we can leave this interface in Type and do special handling if the type is a CXXRecordDecl. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem with this is that you don't have to have a member. Take this test case for example:

struct NotTriviallyEqualityComparableMoreConstrainedExternalOp {
  int i;
  bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) const = default;
};

bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&,
                const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) __attribute__((enable_if(true, ""))) {}

static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableMoreConstrainedExternalOp));

I'm sure this can also be written with some requires clause. Without the builtin telling the library there is no way to figure out that a free function instead of the member is called.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well shoot! That's... frustrating. What about splitting the logic between Type and Sema? e.g., Type tracks whether the type in isolation is trivially equality comparable, and Sema can use that to early return if a type is not trivially equality comparable in isolation and do additional semantic checking otherwise to provide the value for the trait.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not actually storing any infomation for __is_trivially_equaltiy_comparable, so I'm not sure what the benefit would be? If we ever do that we can of course split things up to say "this definitely isn't trivially equaility comparable".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not that we already have this information, it's a different approach to your current patch so that the functionality continues to live on Type (the benefit is that external consumers like downstreams or clang-tidy could continue to use the same interface they always have but get slightly more accurate results from it). If that is more effort than it's worth, I don't insist -- I'm not aware of any breakage from your current changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm not sure the is much use to "this is definitely not trivially equality comparable". Right now I think I'd rather just keep it in SemaExprCXX. If there comes up a use-case or we want to save some of the information we can still move some info into Type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'm sold. Thanks for entertaining the idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always happy to do that!


/// Returns true if it is a class and it might be dynamic.
bool mayBeDynamicClass() const;

Expand Down
60 changes: 0 additions & 60 deletions clang/lib/AST/Type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2815,66 +2815,6 @@ bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
}
}

static bool
HasNonDeletedDefaultedEqualityComparison(const CXXRecordDecl *Decl) {
if (Decl->isUnion())
return false;
if (Decl->isLambda())
return Decl->isCapturelessLambda();

auto IsDefaultedOperatorEqualEqual = [&](const FunctionDecl *Function) {
return Function->getOverloadedOperator() ==
OverloadedOperatorKind::OO_EqualEqual &&
Function->isDefaulted() && Function->getNumParams() > 0 &&
(Function->getParamDecl(0)->getType()->isReferenceType() ||
Decl->isTriviallyCopyable());
};

if (llvm::none_of(Decl->methods(), IsDefaultedOperatorEqualEqual) &&
llvm::none_of(Decl->friends(), [&](const FriendDecl *Friend) {
if (NamedDecl *ND = Friend->getFriendDecl()) {
return ND->isFunctionOrFunctionTemplate() &&
IsDefaultedOperatorEqualEqual(ND->getAsFunction());
}
return false;
}))
return false;

return llvm::all_of(Decl->bases(),
[](const CXXBaseSpecifier &BS) {
if (const auto *RD = BS.getType()->getAsCXXRecordDecl())
return HasNonDeletedDefaultedEqualityComparison(RD);
return true;
}) &&
llvm::all_of(Decl->fields(), [](const FieldDecl *FD) {
auto Type = FD->getType();
if (Type->isArrayType())
Type = Type->getBaseElementTypeUnsafe()->getCanonicalTypeUnqualified();

if (Type->isReferenceType() || Type->isEnumeralType())
return false;
if (const auto *RD = Type->getAsCXXRecordDecl())
return HasNonDeletedDefaultedEqualityComparison(RD);
return true;
});
}

bool QualType::isTriviallyEqualityComparableType(
const ASTContext &Context) const {
QualType CanonicalType = getCanonicalType();
if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType() ||
CanonicalType->isEnumeralType() || CanonicalType->isArrayType())
return false;

if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
if (!HasNonDeletedDefaultedEqualityComparison(RD))
return false;
}

return Context.hasUniqueObjectRepresentations(
CanonicalType, /*CheckIfTriviallyCopyable=*/false);
}

bool QualType::isNonWeakInMRRWithObjCWeak(const ASTContext &Context) const {
return !Context.getLangOpts().ObjCAutoRefCount &&
Context.getLangOpts().ObjCWeak &&
Expand Down
78 changes: 77 additions & 1 deletion clang/lib/Sema/SemaExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5201,6 +5201,82 @@ static bool HasNoThrowOperator(const RecordType *RT, OverloadedOperatorKind Op,
return false;
}

static bool
HasNonDeletedDefaultedEqualityComparison(Sema &S, const CXXRecordDecl *Decl) {
if (Decl->isUnion())
return false;
if (Decl->isLambda())
return Decl->isCapturelessLambda();

{
EnterExpressionEvaluationContext UnevaluatedContext(
S, Sema::ExpressionEvaluationContext::Unevaluated);
Sema::SFINAETrap SFINAE(S, /*AccessCheckingSFINAE=*/true);
Sema::ContextRAII TUContext(S, S.Context.getTranslationUnitDecl());

// const ClassT& obj;
OpaqueValueExpr Operand(
{}, Decl->getTypeForDecl()->getCanonicalTypeUnqualified().withConst(),
ExprValueKind::VK_LValue);
UnresolvedSet<16> Functions;
// obj == obj;
S.LookupBinOp(S.TUScope, {}, BinaryOperatorKind::BO_EQ, Functions);

auto Result = S.CreateOverloadedBinOp({}, BinaryOperatorKind::BO_EQ,
Functions, &Operand, &Operand);
if (Result.isInvalid() || SFINAE.hasErrorOccurred())
return false;

const auto *CallExpr = dyn_cast<CXXOperatorCallExpr>(Result.get());
if (!CallExpr)
return false;
const auto *Callee = CallExpr->getDirectCallee();
auto ParamT = Callee->getParamDecl(0)->getType();
if (!Callee->isDefaulted())
return false;
if (!ParamT->isReferenceType() && !Decl->isTriviallyCopyable())
return false;
if (ParamT.getNonReferenceType()->getUnqualifiedDesugaredType() !=
Decl->getTypeForDecl())
return false;
}

return llvm::all_of(Decl->bases(),
[&](const CXXBaseSpecifier &BS) {
if (const auto *RD = BS.getType()->getAsCXXRecordDecl())
return HasNonDeletedDefaultedEqualityComparison(S,
RD);
return true;
}) &&
llvm::all_of(Decl->fields(), [&](const FieldDecl *FD) {
auto Type = FD->getType();
if (Type->isArrayType())
Type = Type->getBaseElementTypeUnsafe()
->getCanonicalTypeUnqualified();

if (Type->isReferenceType() || Type->isEnumeralType())
return false;
if (const auto *RD = Type->getAsCXXRecordDecl())
return HasNonDeletedDefaultedEqualityComparison(S, RD);
return true;
});
}

static bool isTriviallyEqualityComparableType(Sema &S, QualType Type) {
QualType CanonicalType = Type.getCanonicalType();
if (CanonicalType->isIncompleteType() || CanonicalType->isDependentType() ||
CanonicalType->isEnumeralType() || CanonicalType->isArrayType())
return false;

if (const auto *RD = CanonicalType->getAsCXXRecordDecl()) {
if (!HasNonDeletedDefaultedEqualityComparison(S, RD))
return false;
}

return S.getASTContext().hasUniqueObjectRepresentations(
CanonicalType, /*CheckIfTriviallyCopyable=*/false);
}

static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
SourceLocation KeyLoc,
TypeSourceInfo *TInfo) {
Expand Down Expand Up @@ -5633,7 +5709,7 @@ static bool EvaluateUnaryTypeTrait(Sema &Self, TypeTrait UTT,
Self.Diag(KeyLoc, diag::err_builtin_pass_in_regs_non_class) << T;
return false;
case UTT_IsTriviallyEqualityComparable:
return T.isTriviallyEqualityComparableType(C);
return isTriviallyEqualityComparableType(Self, T);
}
}

Expand Down
43 changes: 43 additions & 0 deletions clang/test/SemaCXX/type-traits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3677,6 +3677,12 @@ struct NonTriviallyEqualityComparableNoComparator {
};
static_assert(!__is_trivially_equality_comparable(NonTriviallyEqualityComparableNoComparator));

struct NonTriviallyEqualityComparableConvertibleToBuiltin {
int i;
operator unsigned() const;
};
static_assert(!__is_trivially_equality_comparable(NonTriviallyEqualityComparableConvertibleToBuiltin));

struct NonTriviallyEqualityComparableNonDefaultedComparator {
int i;
int j;
Expand Down Expand Up @@ -3885,8 +3891,45 @@ struct NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2 {

bool operator==(const NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2&) const = default;
};

static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableNonTriviallyEqualityComparableArrs2));

template<bool B>
struct MaybeTriviallyEqualityComparable {
int i;
bool operator==(const MaybeTriviallyEqualityComparable&) const requires B = default;
bool operator==(const MaybeTriviallyEqualityComparable& rhs) const { return (i % 3) == (rhs.i % 3); }
};
static_assert(__is_trivially_equality_comparable(MaybeTriviallyEqualityComparable<true>));
static_assert(!__is_trivially_equality_comparable(MaybeTriviallyEqualityComparable<false>));

struct NotTriviallyEqualityComparableMoreConstrainedExternalOp {
int i;
bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) const = default;
};

bool operator==(const NotTriviallyEqualityComparableMoreConstrainedExternalOp&,
const NotTriviallyEqualityComparableMoreConstrainedExternalOp&) __attribute__((enable_if(true, ""))) {}

static_assert(!__is_trivially_equality_comparable(NotTriviallyEqualityComparableMoreConstrainedExternalOp));

struct TriviallyEqualityComparableExternalDefaultedOp {
int i;
friend bool operator==(TriviallyEqualityComparableExternalDefaultedOp, TriviallyEqualityComparableExternalDefaultedOp);
};
bool operator==(TriviallyEqualityComparableExternalDefaultedOp, TriviallyEqualityComparableExternalDefaultedOp) = default;

static_assert(__is_trivially_equality_comparable(TriviallyEqualityComparableExternalDefaultedOp));

struct EqualityComparableBase {
bool operator==(const EqualityComparableBase&) const = default;
};

struct ComparingBaseOnly : EqualityComparableBase {
int j_ = 0;
};
static_assert(!__is_trivially_equality_comparable(ComparingBaseOnly));

namespace hidden_friend {

struct TriviallyEqualityComparable {
Expand Down
Loading