[clang] Add the check of membership in decltype for the issue #58674

[lime]

Summary

Originally, the code would take a lookup result as a member in the current scope
and build a member expression accordingly, if the lookup result was not an
operand of the address operator, or it was a field declaration. However, a field
declaration may come from another class, and cause the issue #58674.

Thus, this patch fixes the issue via checking where does the field declaration
comes from, and if it comes from another class, then marks it as not member in
the current scope. The parent scopes of the current scope are also checked, as
the current scope may be associated to a lambda or friend declaration.

[lime]

Remove unnecessary changes in test cases.

[aaron.ballman]

FWIW, it'd help reviewers out if you would use a more descriptive title for the patch than just the GitHub issue number (it's easier for us to keep reviews straight when they have more concrete titles). I made a quick pass over this review and spotted some tiny nits, but I need to give it a more thorough review when I have some spare cycles.

clang/lib/Sema/SemaExpr.cpp

2708–2710

We can simplify this since we're in the area anyway.

2711–2715

[cor3ntin]

Thanks for working on this.

I agree with Aaron, can you make the commit message clearer as to what is fixed,
and mention in clang/docs/ReleaseNotes that the issue is fixed.

clang/lib/Sema/SemaExpr.cpp

2698–2704

clang/test/SemaCXX/decltype.cpp

104

I think we are reproducing the code in the issue, we should try to reproduce all the example in that issue, including the cases that already work.

116

Maybe we should add tests for

decltype(TemplateFoo<T>::value_);
decltype(Foo::nested::value);

[shafik]

clang/docs/ReleaseNotes.rst

787 ↗

(On Diff #473943)

Looks like an unrelated formatting change.

[lime]

Remove an if-statement.

[erichkeane]

I dont really get the logic here, I'm in need of a much better commit message/description here before I can review this.

[erichkeane]

T

clang/lib/Sema/SemaExpr.cpp

2671

So what I was asking above: You're making a ton of changes to this section, without properly updating the comment that is trying to explain what is happening. Please update this to match, particularly the bit around the % operands.

[lime]

Ping!

[lime]

Rebase.

[lime]

Undo unrelated format changes.

[lime]

Rebase.

[lime]

Rebase and ping!

[cor3ntin]

In D137531#4003723, @lime wrote:

Rebase and ping!

I think looks good, but I want to make sure @erichkeane finds his comments resolved, and he is in vacation until January.

[lime]

Rebased. Could you please help to commit this patch? My signature is Liming Liu <[e-mail]>.

[lime]

Is there a place to see the log of the powerpc64le self-built buildbot? @erichkeane

[erichkeane]

In D137531#4028986, @lime wrote:

Is there a place to see the log of the powerpc64le self-built buildbot? @erichkeane

It was sent via email to us: https://lab.llvm.org/buildbot#builders/121/builds/26738

clang/lib/Sema/SemaExpr.cpp

if (ObjCIvarDecl *Ivar = R.getAsSingle<ObjCIvarDecl>()) {
         return ExprError();
       return E;
     }
   }
 
   // This is guaranteed from this point on.
   assert(!R.empty() || ADL);
 
   // Check whether this might be a C++ implicit instance member access.
   // C++ [class.mfct.non-static]p3:
   //   When an id-expression that is not part of a class member access
   //   syntax and not used to form a pointer to member is used in the
   //   body of a non-static member function of class X, if name lookup
   //   resolves the name in the id-expression to a non-static non-type
   //   member of some class C, the id-expression is transformed into a
   //   class member access expression using (*this) as the
   //   postfix-expression to the left of the . operator.
   //
   // But we don't actually need to do this for '&' operands if R
   // resolved to a function or overloaded function set, because the
   // expression is ill-formed if it actually works out to be a
   // non-static member function:
   //
   // C++ [expr.ref]p4:
   //   Otherwise, if E1.E2 refers to a non-static member function. . .
   //   [t]he expression can be used only as the left-hand operand of a
   //   member function call.
   //
   // There are other safeguards against such uses, but it's important
   // to get this right here so that we don't end up making a
   // spuriously dependent expression if we're inside a dependent
   // instance method.
   if (!R.empty() && (*R.begin())->isCXXClassMember()) {
-    bool MightBeImplicitMember;
-    if (!IsAddressOfOperand)
-      MightBeImplicitMember = true;
-    else if (!SS.isEmpty())
+    bool MightBeImplicitMember = true, CheckField = true;
+    if (IsAddressOfOperand) {
+      if (!SS.isEmpty())
         MightBeImplicitMember = false;
       else if (R.isOverloadedResult())
         MightBeImplicitMember = false;
-    else if (R.isUnresolvableResult())
+      else if (R.isUnresolvableResult()) {
         MightBeImplicitMember = true;
-    else
-      MightBeImplicitMember = isa<FieldDecl>(R.getFoundDecl()) ||
+        CheckField = false;
+      }
+    }
+    if (MightBeImplicitMember && CheckField) {
+      if (R.isSingleResult() && (isa<FieldDecl>(R.getFoundDecl()) ||
                                  isa<IndirectFieldDecl>(R.getFoundDecl()) ||
-                              isa<MSPropertyDecl>(R.getFoundDecl());
+                                 isa<MSPropertyDecl>(R.getFoundDecl()))) {
+        if (auto Class = dyn_cast_or_null<CXXRecordDecl>(
+                (*R.begin())->getDeclContext())) {
+          for (auto Curr = S->getLookupEntity(); Curr && !Curr->isFileContext();
+               Curr = Curr->getParent()) {
+            if (auto ThisClass = dyn_cast_or_null<CXXRecordDecl>(Curr)) {
+              if ((MightBeImplicitMember = ThisClass->Equals(Class) ||
+                                           ThisClass->isDerivedFrom(Class)))
+                break;
+            }
+          }
+        }
+      } else if (IsAddressOfOperand)
+        MightBeImplicitMember = false;
+    }
 
     if (MightBeImplicitMember)
       return BuildPossibleImplicitMemberExpr(SS, TemplateKWLoc,
                                              R, TemplateArgs, S);
   }
 
   if (TemplateArgs || TemplateKWLoc.isValid()) {
 

clang/test/SemaCXX/decltype.cpp

namespace D5789 {
   void foo(decltype(T(LP1{ .p1 = { "foo" }, .p1.x[1] = 'x'}))) {} // okay
 
   // expected-warning@+3 {{initializer partially overrides}}
   // expected-note@+2 {{previous initialization}}
   // expected-error@+1 {{redefinition of 'foo'}}
   template<class T> void foo(decltype(T(LP1{ .p1 = g1, .p1.x[1] = 'x' }))) {}
 }
 
+namespace GH58674 {
+  struct Foo {
+    float value_;
+  };
+
+  template <typename T>
+  struct Animal{};
+
+  template <typename T>
+  class Cat : Animal<T> {
+  public:
+    void meow() {
+      using okay = decltype(Foo::value_);
+    }
+  };
+
+  void bar() {
+      Cat<void>{}.meow();
+  }
+}
+
 template<typename>
 class conditional {
 };
 
 // FIXME: The diagnostics here are produced twice.
 void foo(conditional<decltype((1),int>) {  // expected-note 2 {{to match this '('}} expected-error {{expected ')'}} expected-note 2{{to match this '<'}}
 } // expected-error {{expected function body after function declarator}} expected-error 2 {{expected '>'}} expected-error {{expected ')'}}