[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

2715–2717

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

2718–2722

[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

2705–2711

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

822

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/docs/ReleaseNotes.rst

- Fix a number of recursively-instantiated constraint issues, which would possibly
   `Issue 50891 <https://github.com/llvm/llvm-project/issues/50891>`_
 - Clang 14 predeclared some builtin POSIX library functions in ``gnu2x`` mode,
   and Clang 15 accidentally stopped predeclaring those functions in that
   language mode. Clang 16 now predeclares those functions again. This fixes
   `Issue 56607 <https://github.com/llvm/llvm-project/issues/56607>`_.
 - GNU attributes being applied prior to standard attributes would be handled
   improperly, which was corrected to match the behaviour exhibited by GCC.
   `Issue 58229 <https://github.com/llvm/llvm-project/issues/58229>`_
+- Fix an issue about ``decltype`` in the members of class templates derived from
+  templates with related parameters.
+  `Issue 58674 <https://github.com/llvm/llvm-project/issues/58674>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.
- Introduced the new function ``clang_CXXMethod_isDeleted``, which queries
   whether the method is declared ``= delete``.
 - Introduced the new function ``clang_CXXMethod_isCopyAssignmentOperator``,
   which identifies whether a method cursor is a copy-assignment
   operator.
 - Introduced the new function ``clang_CXXMethod_isMoveAssignmentOperator``,
   which identifies whether a method cursor is a move-assignment
   operator.
 - ``clang_Cursor_getNumTemplateArguments``, ``clang_Cursor_getTemplateArgumentKind``, 
   ``clang_Cursor_getTemplateArgumentType``, ``clang_Cursor_getTemplateArgumentValue`` and 
   ``clang_Cursor_getTemplateArgumentUnsignedValue`` now work on struct, class,
   and partial template specialization cursors in addition to function cursors.
 
 Static Analyzer
 ---------------
 - Removed the deprecated ``-analyzer-store`` and
   ``-analyzer-opt-analyze-nested-blocks`` analyzer flags.
   ``scanbuild`` was also updated accordingly.

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.
+  //
+  // We also don't need to do this if R resolved to a member in another
+  // class, which can happen in an unevaluated operand:
+  //
+  // C++ [expr.prim.id]p3.3:
+  //   If that id-expression denotes a non-static data member and it
+  //   appears in an unevaluated operand.
   if (!R.empty() && (*R.begin())->isCXXClassMember()) {
-    bool MightBeImplicitMember;
-    if (!IsAddressOfOperand)
-      MightBeImplicitMember = true;
-    else if (!SS.isEmpty())
-      MightBeImplicitMember = false;
-    else if (R.isOverloadedResult())
+    bool MightBeImplicitMember = true, CheckField = true;
+    if (IsAddressOfOperand) {
+      MightBeImplicitMember = SS.isEmpty() && !R.isOverloadedResult();
+      CheckField = !R.isUnresolvableResult();
+    }
+    if (MightBeImplicitMember && CheckField) {
+      if (R.isSingleResult() &&
+          isa<FieldDecl, IndirectFieldDecl, MSPropertyDecl>(R.getFoundDecl())) {
+        auto Class = cast<CXXRecordDecl>((*R.begin())->getDeclContext());
+        for (auto Curr = S->getLookupEntity(); Curr && !Curr->isFileContext();
+             Curr = Curr->getParent()) {
+          if (auto ThisClass = dyn_cast_if_present<CXXRecordDecl>(Curr)) {
+            if ((MightBeImplicitMember = ThisClass->Equals(Class) ||
+                                         ThisClass->isDerivedFrom(Class)))
+              break;
+          }
+        }
+      } else if (IsAddressOfOperand)
         MightBeImplicitMember = false;
-    else if (R.isUnresolvableResult())
-      MightBeImplicitMember = true;
-    else
-      MightBeImplicitMember = isa<FieldDecl>(R.getFoundDecl()) ||
-                              isa<IndirectFieldDecl>(R.getFoundDecl()) ||
-                              isa<MSPropertyDecl>(R.getFoundDecl());
+    }
 
     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_;
+    struct nested {
+      float value_;
+    };
+  };
+
+  template <typename T>
+  struct TemplateFoo {
+    float value_;
+  };
+
+  float bar;
+
+  template <typename T>
+  struct Animal{};
+
+  template <typename T>
+  class Cat : Animal<T> {
+    using okay = decltype(Foo::value_);
+    using also_okay = decltype(bar);
+    using okay2 = decltype(Foo::nested::value_);
+    using okay3 = decltype(TemplateFoo<T>::value_);
+  public:
+    void meow() {
+      using okay = decltype(Foo::value_);
+      using also_okay = decltype(bar);
+      using okay2 = decltype(Foo::nested::value_);
+      using okay3 = decltype(TemplateFoo<T>::value_);
+    }
+  };
+
+  void baz() {
+      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 ')'}}