[Clang][Sema] Fix lookup of dependent operator= outside of complete-class contexts

[sdkrystian]

Fixes this crash caused by #90152.

Will add tests shortly.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Krystian Stasiowski (sdkrystian)

Changes

Fixes this crash caused by #90152.

Will add tests shortly.

---

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

2 Files Affected:

• (modified) clang/lib/Sema/SemaLookup.cpp (+13-15)
• (modified) clang/lib/Sema/SemaTemplate.cpp (+2-5)

diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index e63da5875d2c9..6bd5932212b92 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -1269,19 +1269,19 @@ struct FindLocalExternScope {
 };
 } // end anonymous namespace
 
+static bool isDependentAssignmentOperator(DeclarationName Name,
+                                          DeclContext *LookupContext) {
+  auto *LookupRecord = dyn_cast_if_present<CXXRecordDecl>(LookupContext);
+  return Name.getCXXOverloadedOperator() == OO_Equal && LookupRecord &&
+         !LookupRecord->isBeingDefined() && LookupRecord->isDependentContext();
+}
+
 bool Sema::CppLookupName(LookupResult &R, Scope *S) {
   assert(getLangOpts().CPlusPlus && "Can perform only C++ lookup");
 
   DeclarationName Name = R.getLookupName();
   Sema::LookupNameKind NameKind = R.getLookupKind();
 
-  // If this is the name of an implicitly-declared special member function,
-  // go through the scope stack to implicitly declare
-  if (isImplicitlyDeclaredMemberFunctionName(Name)) {
-    for (Scope *PreS = S; PreS; PreS = PreS->getParent())
-      if (DeclContext *DC = PreS->getEntity())
-        DeclareImplicitMemberFunctionsWithName(*this, Name, R.getNameLoc(), DC);
-  }
   // C++23 [temp.dep.general]p2:
   //   The component name of an unqualified-id is dependent if
   //   - it is a conversion-function-id whose conversion-type-id
@@ -1299,9 +1299,8 @@ bool Sema::CppLookupName(LookupResult &R, Scope *S) {
   if (isImplicitlyDeclaredMemberFunctionName(Name)) {
     for (Scope *PreS = S; PreS; PreS = PreS->getParent())
       if (DeclContext *DC = PreS->getEntity()) {
-        if (DC->isDependentContext() && isa<CXXRecordDecl>(DC) &&
-            Name.getCXXOverloadedOperator() == OO_Equal &&
-            !R.isTemplateNameLookup()) {
+        if (!R.isTemplateNameLookup() &&
+            isDependentAssignmentOperator(Name, DC)) {
           R.setNotFoundInCurrentInstantiation();
           return false;
         }
@@ -2472,8 +2471,6 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
     }
   } QL(LookupCtx);
 
-  bool TemplateNameLookup = R.isTemplateNameLookup();
-  CXXRecordDecl *LookupRec = dyn_cast<CXXRecordDecl>(LookupCtx);
   if (!InUnqualifiedLookup && !R.isForRedeclaration()) {
     // C++23 [temp.dep.type]p5:
     //   A qualified name is dependent if
@@ -2486,13 +2483,14 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
     if (DeclarationName Name = R.getLookupName();
         (Name.getNameKind() == DeclarationName::CXXConversionFunctionName &&
          Name.getCXXNameType()->isDependentType()) ||
-        (Name.getCXXOverloadedOperator() == OO_Equal && LookupRec &&
-         LookupRec->isDependentContext() && !TemplateNameLookup)) {
+        (!R.isTemplateNameLookup() &&
+         isDependentAssignmentOperator(Name, LookupCtx))) {
       R.setNotFoundInCurrentInstantiation();
       return false;
     }
   }
 
+  CXXRecordDecl *LookupRec = dyn_cast<CXXRecordDecl>(LookupCtx);
   if (LookupDirect(*this, R, LookupCtx)) {
     R.resolveKind();
     if (LookupRec)
@@ -2604,7 +2602,7 @@ bool Sema::LookupQualifiedName(LookupResult &R, DeclContext *LookupCtx,
         //   template, and if the name is used as a template-name, the
         //   reference refers to the class template itself and not a
         //   specialization thereof, and is not ambiguous.
-        if (TemplateNameLookup)
+        if (R.isTemplateNameLookup())
           if (auto *TD = getAsTemplateNameDecl(ND))
             ND = TD;
 
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 7e57fa0696725..480bc74c2001a 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -726,7 +726,7 @@ Sema::ActOnDependentIdExpression(const CXXScopeSpec &SS,
                                  const DeclarationNameInfo &NameInfo,
                                  bool isAddressOfOperand,
                            const TemplateArgumentListInfo *TemplateArgs) {
-  DeclContext *DC = getFunctionLevelDeclContext();
+  QualType ThisType = getCurrentThisType();
 
   // C++11 [expr.prim.general]p12:
   //   An id-expression that denotes a non-static data member or non-static
@@ -748,10 +748,7 @@ Sema::ActOnDependentIdExpression(const CXXScopeSpec &SS,
     IsEnum = isa_and_nonnull<EnumType>(NNS->getAsType());
 
   if (!MightBeCxx11UnevalField && !isAddressOfOperand && !IsEnum &&
-      isa<CXXMethodDecl>(DC) &&
-      cast<CXXMethodDecl>(DC)->isImplicitObjectMemberFunction()) {
-    QualType ThisType = cast<CXXMethodDecl>(DC)->getThisType().getNonReferenceType();
-
+      !ThisType.isNull()) {
     // Since the 'this' expression is synthesized, we don't need to
     // perform the double-lookup check.
     NamedDecl *FirstQualifierInScope = nullptr;

[erichkeane]

few little things? Mostly looks ok.

[erichkeane]

Why the !isBeingDefined? That seems weird given the name of this function.

Also, instead of making it static, can you just toss it in the anonymous NS above?

[sdkrystian]

The reason operator= is dependent when the current class is a templated entity is because each specialization declares its own set of special member functions, and according to [special] p1, they are "declared at the closing } of the class-specifier". When instantiating a templated class, they are declared after all other member declarations are instantiated.

If the lookup context is the current instantiation and the current instantiation is incomplete (e.g. because operator= is named outside a complete-class context), we will never find the implicitly declared copy/move assignment operators because they are always declared last (neither in the template definition context, nor in the template instantiation context). So, we just treat it like any other unqualified name during lookup.

[sdkrystian]

(I'll add a test which calls operator= with a non-dependent argument -- in that case it will be diagnosed before the template is instantiated)

[glandium]

This caused some breakage in something completely unrelated to operator= O_o

This is from webrtc code in Firefox:

/tmp/gecko/third_party/libwebrtc/rtc_base/containers/flat_map.h:331:49: error: out-of-line definition of 'try_emplace' does not match any declaration in 'flat_map<Key, Mapped, Compare, Container>'
  331 | auto flat_map<Key, Mapped, Compare, Container>::try_emplace(K&& key,
      |                                                 ^~~~~~~~~~~
/tmp/gecko/third_party/libwebrtc/rtc_base/containers/flat_map.h:343:49: error: out-of-line definition of 'try_emplace' does not match any declaration in 'flat_map<Key, Mapped, Compare, Container>'
  343 | auto flat_map<Key, Mapped, Compare, Container>::try_emplace(const_iterator hint,
      |                                                 ^~~~~~~~~~~
2 errors generated.

The lines with error are: https://searchfox.org/mozilla-central/rev/c34cf367c29601ed56ae4ea51e20b28cd8331f9c/third_party/libwebrtc/rtc_base/containers/flat_map.h#331,343

The corresponding declarations are:
https://searchfox.org/mozilla-central/rev/c34cf367c29601ed56ae4ea51e20b28cd8331f9c/third_party/libwebrtc/rtc_base/containers/flat_map.h#243,248

I don't see how they differ.

[glandium]

Reverting just the SemaTemplate.cpp change fixes it.

[sdkrystian]

@glandium I've reduced it to the following:

template<typename T>
struct A
{
    static constexpr bool B = true;
};

template<bool V>
struct C { };

template<typename T>
struct D
{
    C<A<T>::B> f();
};

template<typename T>
auto D<T>::f() -> C<A<T>::B> { }

The problem is that we build a DependentScopeDeclRefExpr for A<T>::B in the first declaration of f, and a CXXDependentScopeMemberExpr for the second declaration of f.

Since we have no idea whether A<T>::B will be an implicit member access, I think that the correct behavior is that ActOnDependentIdExpression should never build a CXXDependentScopeMemberExpr.

[aeubanks]

Chromium is also seeing similar breakages. @sdkrystian is this breaking valid code? I can't tell from your latest comment. (if it is breaking valid code we should revert)

[sdkrystian]

@aeubanks I think I'm going to revert this & maybe partially revert the changes in #90152 which cause operator= to be treated as a dependent name when the current class is templated. There are lots of edge cases that need to be accounted for. Thoughts @erichkeane ?

[erichkeane]

@aeubanks I think I'm going to revert this & maybe partially revert the changes in #90152 which cause operator= to be treated as a dependent name when the current class is templated. There are lots of edge cases that need to be accounted for. Thoughts @erichkeane ?

Its sad to do so, but I think it makes the most sense. We've got a good amount of feedback of the breakages, and I think figuring them out and reapplying these changes in a followup patch is a perfectly reasonable way forward.