[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

1390

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.

1441

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

1422

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) {
   // }
   //
   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 (!InnermostFileScopeDC)
+            InnermostFileScopeDC = Ctx;
+
           // 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;
+            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;
-          }
-
           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();
   }
 
+  for (DeclContext *Ctx = InnermostFileScopeDC, *LastCtx = S->getEntity();
+       Ctx && !Ctx->Equals(LastCtx); Ctx = Ctx->getLookupParent())
+    if (Ctx->isFileContext() &&
+        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.

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) {}
+};