[Sema] Search template parameter scopes before searching namespace/file scopes when performing unqualified name lookup

[ahatanak]

Summary

This fixes a bug where namespace scopes were being searched before template parameter scopes.

Delay searching namespace/file scopes in Sema::CppLookupName until all local scopes are searched.

[ahatanak]

• Defer collecting using directives until lookup into local scopes is done.
• Simplify the second loop.
• Add more comments.

[ahatanak]

clang/lib/Sema/SemaLookup.cpp

1392

I noticed that we aren't visiting the contexts from the inside out despite the comment here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaLookup.cpp#L133

// A given context is only every visited once, so it is important
// that contexts be visited from the inside out in order to get
// the effective DCs right.

But I think that's OK. I haven't been able to come up with an example where this is causing a problem.

1418

I think this line can be moved to the beginning of the function .I can try that after committing this patch.

[ahatanak]

clang/lib/Sema/SemaLookup.cpp

1397

This isn't needed as InnermostFileScope is set at the beginning of the loop.

[ahatanak]

Remove unneeded code.

[ahatanak]

ping

[ahatanak]

ping

clang/lib/Sema/SemaLookup.cpp

bool Sema::CppLookupName(LookupResult &R, Scope *S) {
   //   int i;
   //   {
   //     using namespace A;
   //     ++i; // finds local 'i', A::i appears at global scope
   //   }
   // }
   //
   UnqualUsingDirectiveSet UDirs(*this);
-  bool VisitedUsingDirectives = false;
   bool LeftStartingScope = false;
 
   // When performing a scope lookup, we want to find local extern decls.
   FindLocalExternScope FindLocals(R);
+  Scope *InnermostFileScope = nullptr;
+  DeclContext *InnermostFileScopeDC = nullptr;
+
+  for (; S; S = S->getParent()) {
+    if (isNamespaceOrTranslationUnitScope(S)) {
+      if (!InnermostFileScope)
+        InnermostFileScope = S;
+      continue;
+    }
 
-  for (; S && !isNamespaceOrTranslationUnitScope(S); S = S->getParent()) {
     bool SearchNamespaceScope = true;
     // Check whether the IdResolver has anything in this scope.
     for (; I != IEnd && S->isDeclScope(*I); ++I) {
       if (NamedDecl *ND = R.getAcceptableDecl(*I)) {
         if (NameKind == LookupRedeclarationWithLinkage &&
             !(*I)->isTemplateParameter()) {
           // If it's a template parameter, we still find it, so we can diagnose
           // the invalid redeclaration.
if (DeclContext *Ctx = S->getLookupEntity()) {
           }
 
           continue;
         }
 
         // If this is a file context, we need to perform unqualified name
         // lookup considering using directives.
         if (Ctx->isFileContext()) {
-          // If we haven't handled using directives yet, do so now.
-          if (!VisitedUsingDirectives) {
-            // Add using directives from this context up to the top level.
-            for (DeclContext *UCtx = Ctx; UCtx; UCtx = UCtx->getParent()) {
-              if (UCtx->isTransparentContext())
-                continue;
-
-              UDirs.visit(UCtx, UCtx);
-            }
-
-            // Find the innermost file scope, so we can add using directives
-            // from local scopes.
-            Scope *InnermostFileScope = S;
-            while (InnermostFileScope &&
-                   !isNamespaceOrTranslationUnitScope(InnermostFileScope))
-              InnermostFileScope = InnermostFileScope->getParent();
-            UDirs.visitScopeChain(Initial, InnermostFileScope);
-
-            UDirs.done();
-
-            VisitedUsingDirectives = true;
-          }
-
-          if (CppNamespaceLookup(*this, R, Context, Ctx, UDirs)) {
-            R.resolveKind();
-            return true;
-          }
-
+          if (!InnermostFileScopeDC)
+            InnermostFileScopeDC = Ctx;
           continue;
         }
 
         // Perform qualified name lookup into this context.
         // FIXME: In some cases, we know that every name that could be found by
         // this qualified name lookup will also be on the identifier chain. For
         // example, inside a class without any base classes, we never need to
         // perform qualified lookup because all of the members are on top of the
         // identifier chain.
         if (LookupQualifiedName(R, Ctx, /*InUnqualifiedLookup=*/true))
           return true;
       }
     }
   }
 
+  S = InnermostFileScope;
+
   // Stop if we ran out of scopes.
   // FIXME:  This really, really shouldn't be happening.
   if (!S) return false;
 
   // If we are looking for members, no need to look into global/namespace scope.
   if (NameKind == LookupMemberName)
     return false;
 
   // Collect UsingDirectiveDecls in all scopes, and recursively all
   // nominated namespaces by those using-directives.
   //
   // FIXME: Cache this sorted list in Scope structure, and DeclContext, so we
   // don't build it for each lookup!
-  if (!VisitedUsingDirectives) {
-    UDirs.visitScopeChain(Initial, S);
-    UDirs.done();
+
+  // Add using directives from the innermost file scope context up to the top
+  // level.
+  for (DeclContext *UCtx = InnermostFileScopeDC; UCtx;
+       UCtx = UCtx->getParent()) {
+    if (UCtx->isTransparentContext())
+      continue;
+
+    UDirs.visit(UCtx, UCtx);
+  }
+
+  // Add using directives from local scopes.
+
+  // Search the file scope DCs between the innermost file scope DC and the DC
+  // corresponding to the innermost file scope. This is needed, for example, in
+  // the following case where namespace 'N' cannot be found on the scope stack:
+  //
+  // namespace N {
+  // struct B {
+  //   B(int);
+  // };
+  // typedef B typedef_B;
+  // struct D : B {
+  //   D();
+  // };
+  // }
+  //
+  // N::D::D() : typedef_B(0) {}
+
+  for (DeclContext *Ctx = InnermostFileScopeDC, *LastCtx = S->getEntity();
+       Ctx && !Ctx->Equals(LastCtx); Ctx = Ctx->getLookupParent())
+    if (!Ctx->isTransparentContext() &&
+        CppNamespaceLookup(*this, R, Context, Ctx, UDirs)) {
+      R.resolveKind();
+      return true;
     }
 
   // If we're not performing redeclaration lookup, do not look for local
   // extern declarations outside of a function scope.
   if (!R.isForRedeclaration())
     FindLocals.restore();
 
   // Lookup namespace scope, and global scope.
   // Unqualified name lookup in C++ requires looking into scopes
   // that aren't strictly lexical, and therefore we walk through the
   // context as well as walking through the scopes.
   for (; S; S = S->getParent()) {
+    if (!isNamespaceOrTranslationUnitScope(S))
+      continue;
+
     // Check whether the IdResolver has anything in this scope.
     bool Found = false;
     for (; I != IEnd && S->isDeclScope(*I); ++I) {
       if (NamedDecl *ND = R.getAcceptableDecl(*I)) {
         // We found something.  Look for anything else in our scope
         // with this same name and in an acceptable identifier
         // namespace, so that we can construct an overload set if we
         // need to.
         Found = true;
         R.addDecl(ND);
       }
     }
 
-    if (Found && S->isTemplateParamScope()) {
-      R.resolveKind();
-      return true;
-    }
-
     DeclContext *Ctx = S->getLookupEntity();
     if (Ctx) {
       DeclContext *OuterCtx = findOuterContext(S);
       for (; Ctx && !Ctx->Equals(OuterCtx); Ctx = Ctx->getLookupParent()) {
         // We do not directly look into transparent contexts, since
         // those entities will be found in the nearest enclosing
         // non-transparent context.
         if (Ctx->isTransparentContext())
           continue;
 
         // If we have a context, and it's not a context stashed in the
         // template parameter scope for an out-of-line definition, also
         // look into that context.
-        if (!(Found && S->isTemplateParamScope())) {
-          assert(Ctx->isFileContext() &&
-              "We should have been looking only at file context here already.");
+        assert(
+            Ctx->isFileContext() &&
 
-          // Look into context considering using-directives.
-          if (CppNamespaceLookup(*this, R, Context, Ctx, UDirs))
-            Found = true;
-        }
 
         if (Found) {
           R.resolveKind();
           return true;
         }
 
         if (R.isForRedeclaration() && !Ctx->isTransparentContext())
           return false;

clang/test/SemaCXX/namespace.cpp

namespace numeric {
     }
 
     template<typename T, typename U>
     int f(T& l, U& r)
     { numeric::greater(l, r); }
 
   }
 }
+
+namespace N0 {
+  template <typename T> class S;
+  template <typename S2> class C;
+}
+
+template <typename S> class N0::C {
+  void foo(S c) {}
+};