[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation #104458

sdkrystian · 2024-08-15T15:31:04Z

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 #104268.

llvmbot · 2024-08-15T15:31:46Z

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Fixes #104268. Still need tests.

Full diff: https://github.com/llvm/llvm-project/pull/104458.diff

5 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+1-2)
(modified) clang/lib/Sema/SemaExprCXX.cpp (+10-15)
(modified) clang/lib/Sema/SemaOverload.cpp (+9-5)
(modified) clang/lib/Sema/TreeTransform.h (+2-2)
(modified) clang/test/CXX/temp/temp.res/temp.dep/temp.dep.type/p4.cpp (+13-11)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 25cb6c8fbf6104..b9c5c5e4b92ed9 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -10616,8 +10616,7 @@ 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,
+  ExprResult BuildOverloadedArrowExpr(Expr *Base, SourceLocation OpLoc,
                                       bool *NoArrowOperatorFound = nullptr);
 
   ExprResult BuildCXXMemberCallExpr(Expr *Exp, NamedDecl *FoundDecl,
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 124435330ca104..d33d635ff76c5b 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -7876,18 +7876,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
@@ -7902,7 +7890,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
     SmallVector<FunctionDecl*, 8> OperatorArrows;
     CTypes.insert(Context.getCanonicalType(BaseType));
 
-    while (BaseType->isRecordType()) {
+    while (BaseType->getAsRecordDecl()) {
       if (OperatorArrows.size() >= getLangOpts().ArrowDepth) {
         Diag(OpLoc, diag::err_operator_arrow_depth_exceeded)
           << StartingType << getLangOpts().ArrowDepth << Base->getSourceRange();
@@ -7913,7 +7901,7 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
       }
 
       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
@@ -7939,7 +7927,14 @@ ExprResult Sema::ActOnStartCXXMemberReference(Scope *S, Expr *Base,
           }
         }
         return ExprError();
+      } else if (Result.isUnset()) {
+        // BuildOverloadedArrowExpr returns an empty expression to indicate
+        // that we need to build a dependent overloaded arrow expression.
+        assert(BaseType->isDependentType());
+        BaseType = Context.DependentTy;
+        break;
       }
+
       Base = Result.get();
       if (CXXOperatorCallExpr *OpCall = dyn_cast<CXXOperatorCallExpr>(Base))
         OperatorArrows.push_back(OpCall->getDirectCallee());
@@ -7977,7 +7972,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 (!BaseType->getAsRecordDecl()) {
     ObjectType = ParsedType::make(BaseType);
     MayBePseudoDestructor = true;
     return Base;
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 52f640eb96b73b..4340181f316560 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -15864,10 +15864,9 @@ 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) {
+  assert(Base->getType()->getAsRecordDecl() &&
          "left-hand side must have class type");
 
   if (checkPlaceholderForOverload(*this, Base))
@@ -15890,7 +15889,12 @@ 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 (R.wasNotFoundInCurrentInstantiation() ||
+      (Base->isTypeDependent() && !R.empty()))
+    return ExprEmpty();
+
   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 78ec964037dfe9..6a849662015531 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -16542,10 +16542,10 @@ 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();
     // -> is never a builtin operation.
-    return SemaRef.BuildOverloadedArrowExpr(nullptr, First, OpLoc);
+    return SemaRef.BuildOverloadedArrowExpr(First, OpLoc);
   } 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) {

shafik · 2024-08-19T21:26:03Z

Can you please add more details in your summary about the problem and how the PR will fix the problem.

Having detailed summaries for got log is important. Also for the code reviewer as well, I should get a good snapshot of the problem and fix from the summary.

cor3ntin · 2024-08-26T08:53:17Z

@sdkrystian do you plan to add more tests?

sdkrystian · 2024-08-26T13:22:32Z

Can you please add more details in your summary about the problem and how the PR will fix the problem.

@shafik I will once I get back to this PR.

@sdkrystian do you plan to add more tests?

@cor3ntin I do, I just haven't gotten around to it yet :)

cor3ntin · 2024-08-26T13:24:36Z

thanks!
the window for a back port to 19 is closing fast fyi!

sdkrystian · 2024-08-26T18:17:47Z

@cor3ntin What's the deadline?

cor3ntin · 2024-08-26T20:37:59Z

I think there won't be a rc4 so it's actually probably too late (@tobias) - but presumably there will be a 19.0.2 version a couple of weeks after release. So ~2 or 3 weeks?

h-vetinari · 2024-09-01T09:10:57Z

I think there won't be a rc4 so it's actually probably too late

You have the wrong handle, on github it's @tru. 😉 Also, RC4 is being considered.

cor3ntin · 2024-09-01T09:54:39Z

I think there won't be a rc4 so it's actually probably too late

You have the wrong handle, on github it's @tru. 😉 Also, RC4 is being considered.

Thanks! However we can't consider a backport until we have something to backport!

sdkrystian · 2024-09-03T13:42:25Z

@cor3ntin After giving it another look, I think the existing tests are sufficient (the tests updated by this PR were written by me to test this exact scenario). WDYT?

cor3ntin · 2024-09-03T15:15:37Z

clang/lib/Sema/SemaExprCXX.cpp

+        // BuildOverloadedArrowExpr returns an empty expression to indicate
+        // that we need to build a dependent overloaded arrow expression.


I have 2 concerns with that
1/ It's a bit too clever of an interface
2/ We should make sure this can only happens in Sema, right? I wonder if we need an ActOnOverloadedArrowExpr to wrap that logic such that (it would duplicate the lookup code but i think that's a better outcome)

WDYT?

I have 2 concerns with that
1/ It's a bit too clever of an interface

Although I agree it's an imperfect interface, this is more of a "transitional" fix. Ideally we will build the full AST for calls to members of the current instantiation in the future. For now, the usage of ExprEmpty in this patch isn't unfounded so I think it's "good enough". Alternatively I can add a bool& IsDependent out parameter to implement this functionality.

2/ We should make sure this can only happens in Sema, right? I wonder if we need an ActOnOverloadedArrowExpr to wrap that logic such that (it would duplicate the lookup code but i think that's a better outcome)

I'm not entirely sure what this means. Are you saying that we should expect the result to not be ExprEmpty during instantiation?

I'm not sure RebuildCXXOperatorCallExpr and its caller would deal with an ExprEmpty when calling
ActOnOverloadedArrowExpr

So yeah, maybe output parameter + assert in treetransform would be better?

@cor3ntin Pushed changes that do what you described

cor3ntin · 2024-09-03T15:16:14Z

@cor3ntin After giving it another look, I think the existing tests are sufficient (the tests updated by this PR were written by me to test this exact scenario). WDYT?

I think the tests you have are fine

sdkrystian · 2024-09-03T15:27:31Z

@cor3ntin Excellent, then I think this is good to go

cor3ntin · 2024-09-03T16:02:49Z

Can you update the description too?
No changelog needed, I assume we want to backport that @AaronBallman

cor3ntin

LGTM, thanks (Please update the description before merging)

AaronBallman · 2024-09-03T16:35:37Z

Can you update the description too? No changelog needed, I assume we want to backport that @AaronBallman

If we can backport it, it seems like a reasonable one to backport; the biggest question I have is whether we have any concerns about the changes being risky or not.

clang/lib/Sema/SemaExprCXX.cpp

cor3ntin · 2024-09-08T13:40:45Z

@sdkrystian can you merge soon so that we can cherry pick in 19? thanks!

… 'operator->' in the current instantiation

cor3ntin · 2024-09-09T16:08:30Z

/cherry-pick 3cdb30e

… '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)

llvmbot · 2024-09-09T16:13:41Z

/pull-request #107886

nikic · 2024-09-09T17:06:27Z

It looks like this breaks the clang stage 2 build: https://llvm-compile-time-tracker.com/show_error.php?commit=3cdb30ebbc18fa894d3bd67aebcff76ce7c741ac

fhahn · 2024-09-09T18:27:33Z

I am also seeing assertions triggered when building LLVM Assertion failed: (BaseType->isDependentType()), function ActOnStartCXXMemberReference, file SemaExprCXX.cpp, line 8009.

…erloaded 'operator->' in the current instantiation (#104458)" This reverts commit 3cdb30e. Breaks clang bootstrap.

sdkrystian · 2024-09-13T16:46:41Z

I have a fix, so I'll reapply this sometime today.

porglezomp · 2024-09-17T20:32:46Z

@sdkrystian sorry to bother, do you have that change to re-land it?

sdkrystian · 2024-09-17T21:05:00Z

@porglezomp Sorry, I'm currently at cppcon but I'll try reland it when I have a moment.

…verloaded 'operator->' in the current instantiation (llvm#104458)"

…verloaded 'operator->' in the current instantiation (#104458)" (#109422) Reapplies #104458, fixing a bug that occurs when a class member access expression calls an `operator->` operator function that returns a non-dependent class type.

…ilding overloaded 'operator->' in the current instantiation (#104458)"" (#123982) Reverts #109422

sdkrystian requested a review from Endilll as a code owner August 15, 2024 15:31

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Aug 15, 2024

sdkrystian requested review from mizvekov and zyn0217 August 16, 2024 19:01

cor3ntin reviewed Sep 3, 2024

View reviewed changes

sdkrystian force-pushed the fix-overloaded-arrow branch from e5f3b06 to 4cdb2b4 Compare September 3, 2024 16:18

cor3ntin approved these changes Sep 3, 2024

View reviewed changes

AaronBallman reviewed Sep 3, 2024

View reviewed changes

clang/lib/Sema/SemaExprCXX.cpp Outdated Show resolved Hide resolved

sdkrystian added 4 commits September 9, 2024 11:32

[Clang][Sema] Use the correct lookup context when building overloaded…

11f67c7

… 'operator->' in the current instantiation

[FOLD] fix destructor lookup when the object expression is dependent

0211427

[FOLD] use IsDependent output parameter

29282d2

[FOLD] don't use getAsRecordDecl

0b4a403

sdkrystian force-pushed the fix-overloaded-arrow branch from a6fedba to 0b4a403 Compare September 9, 2024 15:33

[FOLD] fix capitalization

dd73430

sdkrystian merged commit 3cdb30e into llvm:main Sep 9, 2024
6 of 8 checks passed

cor3ntin added this to the LLVM 19.X Release milestone Sep 9, 2024

nikic added a commit that referenced this pull request Sep 9, 2024

Revert "[Clang][Sema] Use the correct lookup context when building ov…

3681d85

…erloaded 'operator->' in the current instantiation (#104458)" This reverts commit 3cdb30e. Breaks clang bootstrap.

h-vetinari mentioned this pull request Sep 13, 2024

clang fails to find operator -> #104268

Open

sdkrystian added a commit to sdkrystian/llvm-project that referenced this pull request Sep 20, 2024

Reapply "[Clang][Sema] Use the correct lookup context when building o…

d1b0cad

…verloaded 'operator->' in the current instantiation (llvm#104458)"

sdkrystian mentioned this pull request Sep 20, 2024

Reapply "[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation (#104458)" #109422

Merged

sdkrystian added a commit that referenced this pull request Jan 22, 2025

Revert "Reapply "[Clang][Sema] Use the correct lookup context when bu…

5ede7b6

…ilding overloaded 'operator->' in the current instantiation (#104458)"" (#123982) Reverts #109422

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation #104458

[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation #104458

sdkrystian commented Aug 15, 2024 •

edited

Loading

llvmbot commented Aug 15, 2024

shafik commented Aug 19, 2024

cor3ntin commented Aug 26, 2024

sdkrystian commented Aug 26, 2024 •

edited

Loading

cor3ntin commented Aug 26, 2024

sdkrystian commented Aug 26, 2024

cor3ntin commented Aug 26, 2024

h-vetinari commented Sep 1, 2024

cor3ntin commented Sep 1, 2024

sdkrystian commented Sep 3, 2024

cor3ntin Sep 3, 2024

sdkrystian Sep 3, 2024

cor3ntin Sep 3, 2024

sdkrystian Sep 3, 2024

cor3ntin commented Sep 3, 2024

sdkrystian commented Sep 3, 2024

cor3ntin commented Sep 3, 2024

cor3ntin left a comment

AaronBallman commented Sep 3, 2024

cor3ntin commented Sep 8, 2024

cor3ntin commented Sep 9, 2024

llvmbot commented Sep 9, 2024

nikic commented Sep 9, 2024

fhahn commented Sep 9, 2024

sdkrystian commented Sep 13, 2024

porglezomp commented Sep 17, 2024

sdkrystian commented Sep 17, 2024

		// BuildOverloadedArrowExpr returns an empty expression to indicate
		// that we need to build a dependent overloaded arrow expression.

[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation #104458

[Clang][Sema] Use the correct lookup context when building overloaded 'operator->' in the current instantiation #104458

Conversation

sdkrystian commented Aug 15, 2024 • edited Loading

llvmbot commented Aug 15, 2024

shafik commented Aug 19, 2024

cor3ntin commented Aug 26, 2024

sdkrystian commented Aug 26, 2024 • edited Loading

cor3ntin commented Aug 26, 2024

sdkrystian commented Aug 26, 2024

cor3ntin commented Aug 26, 2024

h-vetinari commented Sep 1, 2024

cor3ntin commented Sep 1, 2024

sdkrystian commented Sep 3, 2024

cor3ntin Sep 3, 2024

Choose a reason for hiding this comment

sdkrystian Sep 3, 2024

Choose a reason for hiding this comment

cor3ntin Sep 3, 2024

Choose a reason for hiding this comment

sdkrystian Sep 3, 2024

Choose a reason for hiding this comment

cor3ntin commented Sep 3, 2024

sdkrystian commented Sep 3, 2024

cor3ntin commented Sep 3, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

AaronBallman commented Sep 3, 2024

cor3ntin commented Sep 8, 2024

cor3ntin commented Sep 9, 2024

llvmbot commented Sep 9, 2024

nikic commented Sep 9, 2024

fhahn commented Sep 9, 2024

sdkrystian commented Sep 13, 2024

porglezomp commented Sep 17, 2024

sdkrystian commented Sep 17, 2024

sdkrystian commented Aug 15, 2024 •

edited

Loading

sdkrystian commented Aug 26, 2024 •

edited

Loading