[clang] Correct calculation of MemberExpr's dependence

[Fznamznon]

Summary

Due to incorrect calculation false positive diagnostics were emitted.

Fixes https://github.com/llvm/llvm-project/issues/48731

[Fznamznon]

clang/lib/AST/ComputeDependence.cpp

625

While implementing this patch I was following Richard's guide from https://github.com/llvm/llvm-project/issues/48731#issuecomment-1529150797 . The problem is, I didn't manage to write a test that would fail due to clang not taking into account NameInfo dependence. Turns out in most cases where it can be dependent either Base itself is dependent or CXXPseudoDestructorExpr/CXXDependentScopeMemberExpr is created inside the template function.

[cor3ntin]

clang/lib/AST/Expr.cpp

1760

Maybe we should do that now, by passing TemplateArgs to computeDependence?

Otherwise there is a good chance that fixme never gets fixed !

[Fznamznon]

clang/lib/AST/Expr.cpp

1760

computeDependence is called by the constructor of MemberExpr which doesn't accept template augments. In order to do that, I would need to modify the constructor of MemberExpr.
Is it still reasonable to do as a part of this fix?

[cor3ntin]

clang/lib/AST/Expr.cpp

1760

Oh, i missed that the fixme is pre-existing, NVM then, i think you can leave it as-is. Sorry!

[cor3ntin]

I think this makes sense and it implements richard's suggestion.
However, it's missing a release note, can you add that before landing?
Thanks

[Fznamznon]

In D154689#4480249, @cor3ntin wrote:

I think this makes sense and it implements richard's suggestion.
However, it's missing a release note, can you add that before landing?
Thanks

Thank you for the review.
I think I added a release note here - https://reviews.llvm.org/D154689#change-RNUd6wICb9iD . Is something else needed?

[cor3ntin]

In D154689#4480282, @Fznamznon wrote:

In D154689#4480249, @cor3ntin wrote:

I think this makes sense and it implements richard's suggestion.
However, it's missing a release note, can you add that before landing?
Thanks

Thank you for the review.
I think I added a release note here - https://reviews.llvm.org/D154689#change-RNUd6wICb9iD . Is something else needed?

Gosh, it's really not my day... nah, you're good! Sorry

[shafik]

LGTM, thank you for the quick fix!

[Fznamznon]

Rebase, add tests for dependent NameInfo

clang/docs/ReleaseNotes.rst

- Fix missing destructor calls and therefore memory leaks in generated code
   (`#60709 <https://github.com/llvm/llvm-project/issues/60709>`_).
 - Fixed a missed integer overflow warning with temporary values.
   (`#63629 <https://github.com/llvm/llvm-project/issues/63629>`_)
 - Fixed parsing of elaborated type specifier inside of a new expression.
   (`#34341 <https://github.com/llvm/llvm-project/issues/34341>`_)
 - Clang now correctly evaluates ``__has_extension (cxx_defaulted_functions)``
   and ``__has_extension (cxx_default_function_template_args)`` to 1.
   (`#61758 <https://github.com/llvm/llvm-project/issues/61758>`_)
+- Fixed false positive error diagnostic when pack expansion appears in template
+  parameters of a member expression.
+  (`#48731 <https://github.com/llvm/llvm-project/issues/48731>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Bug Fixes to Attribute Support
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug where attribute annotations on type specifiers (enums, classes,
   structs, unions, and scoped enums) were not properly ignored, resulting in

clang/lib/AST/ComputeDependence.cpp

 ExprDependence clang::computeDependence(OffsetOfExpr *E) {
   auto D = turnTypeToValueDependence(toExprDependenceAsWritten(
       E->getTypeSourceInfo()->getType()->getDependence()));
   for (unsigned I = 0, N = E->getNumExpressions(); I < N; ++I)
     D |= turnTypeToValueDependence(E->getIndexExpr(I)->getDependence());
   return D;
 }
 
+static inline ExprDependence getDependenceInExpr(DeclarationNameInfo Name) {
+  auto D = ExprDependence::None;
+  if (Name.isInstantiationDependent())
+    D |= ExprDependence::Instantiation;
+  if (Name.containsUnexpandedParameterPack())
+    D |= ExprDependence::UnexpandedPack;
+  return D;
+}
+
 ExprDependence clang::computeDependence(MemberExpr *E) {
-  auto *MemberDecl = E->getMemberDecl();
   auto D = E->getBase()->getDependence();
+  D |= getDependenceInExpr(E->getMemberNameInfo());
+
+  if (auto *NNS = E->getQualifier())
+    D |= toExprDependence(NNS->getDependence() &
+                          ~NestedNameSpecifierDependence::Dependent);
+
+  auto *MemberDecl = E->getMemberDecl();
   if (FieldDecl *FD = dyn_cast<FieldDecl>(MemberDecl)) {
     DeclContext *DC = MemberDecl->getDeclContext();
     // dyn_cast_or_null is used to handle objC variables which do not
     // have a declaration context.
     CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(DC);
     if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC)) {
       if (!E->getType()->isDependentType())
         D &= ~ExprDependence::Type;
if (auto *ST = E->getScopeTypeInfo())
     D |= turnTypeToValueDependence(
         toExprDependenceAsWritten(ST->getType()->getDependence()));
   if (auto *Q = E->getQualifier())
     D |= toExprDependence(Q->getDependence() &
                           ~NestedNameSpecifierDependence::Dependent);
   return D;
 }
 
-static inline ExprDependence getDependenceInExpr(DeclarationNameInfo Name) {
-  auto D = ExprDependence::None;
-  if (Name.isInstantiationDependent())
-    D |= ExprDependence::Instantiation;
-  if (Name.containsUnexpandedParameterPack())
-    D |= ExprDependence::UnexpandedPack;
-  return D;
-}
-
 ExprDependence
 clang::computeDependence(OverloadExpr *E, bool KnownDependent,
                          bool KnownInstantiationDependent,
                          bool KnownContainsUnexpandedParameterPack) {
   auto Deps = ExprDependence::None;
   if (KnownDependent)
     Deps |= ExprDependence::TypeValue;
   if (KnownInstantiationDependent)

clang/lib/AST/Expr.cpp

std::size_t Size =
                        TemplateArgumentLoc>(
           HasQualOrFound ? 1 : 0, HasTemplateKWAndArgsInfo ? 1 : 0,
           TemplateArgs ? TemplateArgs->size() : 0);
 
   void *Mem = C.Allocate(Size, alignof(MemberExpr));
   MemberExpr *E = new (Mem) MemberExpr(Base, IsArrow, OperatorLoc, MemberDecl,
                                        NameInfo, T, VK, OK, NOUR);
 
-  // FIXME: remove remaining dependence computation to computeDependence().
-  auto Deps = E->getDependence();
   if (HasQualOrFound) {
-    // FIXME: Wrong. We should be looking at the member declaration we found.
-    if (QualifierLoc && QualifierLoc.getNestedNameSpecifier()->isDependent())
-      Deps |= ExprDependence::TypeValueInstantiation;
-    else if (QualifierLoc &&
-             QualifierLoc.getNestedNameSpecifier()->isInstantiationDependent())
-      Deps |= ExprDependence::Instantiation;
-
     E->MemberExprBits.HasQualifierOrFoundDecl = true;
 
     MemberExprNameQualifier *NQ =
         E->getTrailingObjects<MemberExprNameQualifier>();
     NQ->QualifierLoc = QualifierLoc;
     NQ->FoundDecl = FoundDecl;
   }
 
   E->MemberExprBits.HasTemplateKWAndArgsInfo =
       TemplateArgs || TemplateKWLoc.isValid();
 
+  // FIXME: remove remaining dependence computation to computeDependence().
+  auto Deps = E->getDependence();
   if (TemplateArgs) {
     auto TemplateArgDeps = TemplateArgumentDependence::None;
     E->getTrailingObjects<ASTTemplateKWAndArgsInfo>()->initializeFrom(
         TemplateKWLoc, *TemplateArgs,
         E->getTrailingObjects<TemplateArgumentLoc>(), TemplateArgDeps);
-    if (TemplateArgDeps & TemplateArgumentDependence::Instantiation)
-      Deps |= ExprDependence::Instantiation;
+    for (const TemplateArgumentLoc &ArgLoc : TemplateArgs->arguments()) {
+      Deps |= toExprDependence(ArgLoc.getArgument().getDependence());
+    }
   } else if (TemplateKWLoc.isValid()) {
     E->getTrailingObjects<ASTTemplateKWAndArgsInfo>()->initializeFrom(
         TemplateKWLoc);
   }
   E->setDependence(Deps);
 
   return E;
 }

clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp

namespace DependentCTAD {
 
   void g() {
     // PR50039: Note that we could in principle deduce T here, but the language
     // deduction rules don't support that.
     f(A<B(0)>()); // expected-error {{no matching function}}
     f<B>(A<B(0)>()); // OK
   }
 }
+
+namespace GH48731 {
+template <int> using N = int;
+struct X { template<typename T> void f(); };
+template<int ...Is> decltype((X().f<N<Is>>(), ...)) x;
+template<int ...Is> decltype(((new X())->f<N<Is>>(), ...)) y;
+}