release/19.x: [Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458) #107886
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
… 'operator->' in the current instantiation (llvm#104458) Currently, clang erroneously rejects the following: ``` struct A { template<typename T> void f(); }; template<typename T> struct B { void g() { (*this)->template f<int>(); // error: no member named 'f' in 'B<T>' } A* operator->(); }; ``` This happens because `Sema::ActOnStartCXXMemberReference` does not adjust the `ObjectType` parameter when `ObjectType` is a dependent type (except when the type is a `PointerType` and the class member access is the `->` form). Since the (possibly adjusted) `ObjectType` parameter (`B<T>` in the above example) is passed to `Parser::ParseOptionalCXXScopeSpecifier`, we end up looking up `f` in `B` rather than `A`. This patch fixes the issue by identifying cases where the type of the object expression `T` is a dependent, non-pointer type and: - `T` is the current instantiation and lookup for `operator->` finds a member of the current instantiation, or - `T` has at least one dependent base case, and `operator->` is not found in the current instantiation and using `ASTContext::DependentTy` as the type of the object expression when the optional _nested-name-specifier_ is parsed. Fixes llvm#104268. (cherry picked from commit 3cdb30e)
|
@cor3ntin What do you think about merging this PR to the release branch? |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (llvmbot) ChangesBackport 3cdb30e Requested by: @cor3ntin Full diff: https://github.com/llvm/llvm-project/pull/107886.diff 5 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 7bfdaaae45a93e..f1e31f52de0ef5 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10615,9 +10615,9 @@ class Sema final : public SemaBase {
/// BuildOverloadedArrowExpr - Build a call to an overloaded @c operator->
/// (if one exists), where @c Base is an expression of class type and
/// @c Member is the name of the member we're trying to find.
- ExprResult BuildOverloadedArrowExpr(Scope *S, Expr *Base,
- SourceLocation OpLoc,
- bool *NoArrowOperatorFound = nullptr);
+ ExprResult BuildOverloadedArrowExpr(Expr *Base, SourceLocation OpLoc,
+ bool *NoArrowOperatorFound,
+ bool &IsDependent);
ExprResult BuildCXXMemberCallExpr(Expr *Exp, NamedDecl *FoundDecl,
CXXConversionDecl *Method,
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 14d1f395af90e3..9061cb5a957f2e 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -7850,18 +7850,6 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
QualType BaseType = Base->getType();
MayBePseudoDestructor = false;
- if (BaseType->isDependentType()) {
- // If we have a pointer to a dependent type and are using the -> operator,
- // the object type is the type that the pointer points to. We might still
- // have enough information about that type to do something useful.
- if (OpKind == tok::arrow)
- if (const PointerType *Ptr = BaseType->getAs<PointerType>())
- BaseType = Ptr->getPointeeType();
-
- ObjectType = ParsedType::make(BaseType);
- MayBePseudoDestructor = true;
- return Base;
- }
// C++ [over.match.oper]p8:
// [...] When operator->returns, the operator-> is applied to the value
@@ -7876,7 +7864,8 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
SmallVector<FunctionDecl*, 8> OperatorArrows;
CTypes.insert(Context.getCanonicalType(BaseType));
- while (BaseType->isRecordType()) {
+ while (
+ isa<InjectedClassNameType, RecordType>(BaseType.getCanonicalType())) {
if (OperatorArrows.size() >= getLangOpts().ArrowDepth) {
Diag(OpLoc, diag::err_operator_arrow_depth_exceeded)
<< StartingType << getLangOpts().ArrowDepth << Base->getSourceRange();
@@ -7886,15 +7875,26 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
return ExprError();
}
+ bool IsDependent;
Result = BuildOverloadedArrowExpr(
- S, Base, OpLoc,
+ Base, OpLoc,
// When in a template specialization and on the first loop iteration,
// potentially give the default diagnostic (with the fixit in a
// separate note) instead of having the error reported back to here
// and giving a diagnostic with a fixit attached to the error itself.
(FirstIteration && CurFD && CurFD->isFunctionTemplateSpecialization())
? nullptr
- : &NoArrowOperatorFound);
+ : &NoArrowOperatorFound,
+ IsDependent);
+
+ if (IsDependent) {
+ // BuildOverloadedArrowExpr sets IsDependent to indicate that we need
+ // to build a dependent overloaded arrow expression.
+ assert(BaseType->isDependentType());
+ BaseType = Context.DependentTy;
+ break;
+ }
+
if (Result.isInvalid()) {
if (NoArrowOperatorFound) {
if (FirstIteration) {
@@ -7914,6 +7914,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
}
return ExprError();
}
+
Base = Result.get();
if (CXXOperatorCallExpr *OpCall = dyn_cast<CXXOperatorCallExpr>(Base))
OperatorArrows.push_back(OpCall->getDirectCallee());
@@ -7951,7 +7952,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
// it's legal for the type to be incomplete if this is a pseudo-destructor
// call. We'll do more incomplete-type checks later in the lookup process,
// so just skip this check for ObjC types.
- if (!BaseType->isRecordType()) {
+ if (!isa<InjectedClassNameType, RecordType>(BaseType.getCanonicalType())) {
ObjectType = ParsedType::make(BaseType);
MayBePseudoDestructor = true;
return Base;
@@ -7969,6 +7970,10 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
return CreateRecoveryExpr(Base->getBeginLoc(), Base->getEndLoc(), {Base});
}
+ // We can't implicitly declare the destructor for a templated class.
+ if (BaseType->isDependentType())
+ MayBePseudoDestructor = true;
+
// C++ [basic.lookup.classref]p2:
// If the id-expression in a class member access (5.2.5) is an
// unqualified-id, and the type of the object expression is of a class
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 28fd3b06156b9b..6fd01064f1012b 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -15805,12 +15805,14 @@ Sema::BuildCallToObjectOfClassType(Scope *S, Expr *Obj,
return CheckForImmediateInvocation(MaybeBindToTemporary(TheCall), Method);
}
-ExprResult
-Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
- bool *NoArrowOperatorFound) {
- assert(Base->getType()->isRecordType() &&
+ExprResult Sema::BuildOverloadedArrowExpr(Expr *Base, SourceLocation OpLoc,
+ bool *NoArrowOperatorFound,
+ bool &IsDependent) {
+ assert(Base->getType()->getAsRecordDecl() &&
"left-hand side must have class type");
+ IsDependent = false;
+
if (checkPlaceholderForOverload(*this, Base))
return ExprError();
@@ -15831,7 +15833,19 @@ Sema::BuildOverloadedArrowExpr(Scope *S, Expr *Base, SourceLocation OpLoc,
return ExprError();
LookupResult R(*this, OpName, OpLoc, LookupOrdinaryName);
- LookupQualifiedName(R, Base->getType()->castAs<RecordType>()->getDecl());
+ LookupParsedName(R, /*S=*/nullptr, /*SS=*/nullptr, Base->getType());
+
+ // If the expression is dependent and we either:
+ // - found a member of the current instantiation named 'operator->', or
+ // - found nothing, and the lookup context has no dependent base classes
+ //
+ // then we should build a dependent class member access expression.
+ if (R.wasNotFoundInCurrentInstantiation() ||
+ (Base->isTypeDependent() && !R.empty())) {
+ IsDependent = true;
+ return Base;
+ }
+
R.suppressAccessDiagnostics();
for (LookupResult::iterator Oper = R.begin(), OperEnd = R.end();
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 51e6a4845bf6fd..690e3835e8cac1 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -16376,10 +16376,14 @@ ExprResult TreeTransform<Derived>::RebuildCXXOperatorCallExpr(
} else if (Op == OO_Arrow) {
// It is possible that the type refers to a RecoveryExpr created earlier
// in the tree transformation.
- if (First->getType()->isDependentType())
+ if (First->containsErrors())
return ExprError();
+ bool IsDependent;
// -> is never a builtin operation.
- return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
+ ExprResult Result = SemaRef.BuildOverloadedArrowExpr(
+ First, OpLoc, /*NoArrowOperatorFound=*/nullptr, IsDependent);
+ assert(!IsDependent);
+ return Result;
} else if (Second == nullptr || isPostIncDec) {
if (!First->getType()->isOverloadableType() ||
(Op == OO_Amp && getSema().isQualifiedMemberAccess(First))) {
diff --git a/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp b/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
index f32f49ef4539a5..89c22a0bc137d9 100644
--- a/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
+++ b/clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp
@@ -484,16 +484,19 @@ namespace N4 {
template<typename T>
struct A {
void not_instantiated(A a, A<T> b, T c) {
- a->x;
- b->x;
+ a->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
+ b->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
c->x;
}
void instantiated(A a, A<T> b, T c) {
- a->x; // expected-error {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
- // expected-error@-1 {{no member named 'x' in 'N4::A<int>'}}
- b->x; // expected-error {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
- // expected-error@-1 {{no member named 'x' in 'N4::A<int>'}}
+ // FIXME: We should only emit a single diagnostic suggesting to use '.'!
+ a->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
+ // expected-error@-1 {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
+ // expected-error@-2 {{no member named 'x' in 'N4::A<int>'}}
+ b->x; // expected-error {{member reference type 'A<T>' is not a pointer; did you mean to use '.'?}}
+ // expected-error@-1 {{member reference type 'A<int>' is not a pointer; did you mean to use '.'?}}
+ // expected-error@-2 {{no member named 'x' in 'N4::A<int>'}}
c->x; // expected-error {{member reference type 'int' is not a pointer}}
}
};
@@ -540,11 +543,10 @@ namespace N4 {
a->T::f();
a->T::g();
- // FIXME: 'U' should be a dependent name, and its lookup context should be 'a.operator->()'!
- a->U::x; // expected-error {{use of undeclared identifier 'U'}}
- a->U::y; // expected-error {{use of undeclared identifier 'U'}}
- a->U::f(); // expected-error {{use of undeclared identifier 'U'}}
- a->U::g(); // expected-error {{use of undeclared identifier 'U'}}
+ a->U::x;
+ a->U::y;
+ a->U::f();
+ a->U::g();
}
void instantiated(D a) {
|
|
Hi, since we are wrapping up LLVM 19.1.0 we are very strict with the fixes we pick at this point. Can you please respond to the following questions to help me understand if this has to be included in the final release or not. Is this PR a fix for a regression or a critical issue? What is the risk of accepting this into the release branch? What is the risk of NOT accepting this into the release branch? |
|
Yes, this is a 19 regression #104268 I think it's fairly low risk to accept - but if we don't clang will reject fairly common c++ code (lookup of operator-> in template function) |
AaronBallman
approved these changes
Sep 10, 2024
nikic
requested changes
Sep 10, 2024
There was a problem hiding this comment.
This change has already been reverted on main because it breaks clang bootstrap and causes assertion failures. This was already known at the time it was accepted as "low risk" here. Please exercise at least a minimum amount of due diligence before approving backports.
Apologies, I missed the new comments because I happened to have the tab for the original review already open and didn't hit Refresh on it. I hadn't realized this was causing problems -- mea culpa! Thank you for catching this!! |
|
Should we drop this then? Or will the fix be fixed? |
|
we should (unfortunately) drop this. |
|
Alright - reopen a new PR if/when a fix is ready. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport 3cdb30e
Requested by: @cor3ntin