[clangd]Fix addUsing tweak doesn't traverse same-level anon namespaces

[chouzz]

Summary

This patch provide fixes about addUsing tweak doesn't traverse same-level anon namespaces.

Fixes: https://github.com/clangd/clangd/issues/1572

[chouzz]

Hi, @kadircet Could you please take a look this patch?

[sammccall]

This would be a great fix to have! However I don't think the specific changes to the RecursiveASTVisitor are correct.

I can see a couple of approaches:

1. prevent the RecursiveASTVisitor from traversing into uninteresting contexts, and drop the Encloses check in VisitUsingDecl
2. explicitly gather the list of interesting contexts

option 1 is most similar to this patch, I believe TraverseNamespaceDecl needs to call base::TraverseNamespaceDecl only if the NS either encloses the selection DC or is anonymous.

option 2 would mean walking up the declcontext chain and grabbing each entry and also getAnonymousNamespace() from TranslationUnitDecl and NamespaceDecl, and changing our Encloses() tests to check membership in that set.

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp

97

This doesn't look like it can be right:

• there's no distinction on whether the namespace is anonymous or not
• you're calling VisitUsingDecl, but also Base::TraverseNamespaceDecl which will visit the contained using decls again

this also suggests we need more testcases here: some negative ones verifying that we don't look at UsingDecls that are inside named namespaces.

clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp

UsingFinder(std::vector<const UsingDecl *> &Results,
               const DeclContext *SelectionDeclContext, const SourceManager &SM)
       : Results(Results), SelectionDeclContext(SelectionDeclContext), SM(SM) {}
 
   bool VisitUsingDecl(UsingDecl *D) {
     auto Loc = D->getUsingLoc();
     if (SM.getFileID(Loc) != SM.getMainFileID()) {
       return true;
     }
-    if (D->getDeclContext()->Encloses(SelectionDeclContext)) {
-      Results.push_back(D);
-    }
     return true;
   }
 
+  bool TraverseNamespaceDecl(NamespaceDecl *D) {
+    for (auto *Decl : D->decls()) {
+      if (auto *Node = dyn_cast<UsingDecl>(Decl)) {
+        VisitUsingDecl(Node);
+      }
+    }
+    return RecursiveASTVisitor<UsingFinder>::TraverseNamespaceDecl(D);
+  }
+
   bool TraverseDecl(Decl *Node) {
     if (!Node)
       return true;
     // There is no need to go deeper into nodes that do not enclose selection,
     // since "using" there will not affect selection, nor would it make a good
     // insertion point.
     if (!Node->getDeclContext() ||
         Node->getDeclContext()->Encloses(SelectionDeclContext)) {

clang-tools-extra/clangd/unittests/tweaks/AddUsingTests.cpp

 })cpp",
        R"cpp(
 #include "test.hpp"
 using namespace one;
 namespace {using two::cc;
 
 cc C;
 })cpp"},
+      // Should drop the qualifier in anon namespaces
+      {R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { ns::^Foo *f; ns::Bar *b;
+})cpp",
+       R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { Foo *f; ns::Bar *b;
+})cpp"},
+      // Should insert the using decl into the anon namespace
+      {R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Foo; }
+void foo() { ns::Foo *f; ns::^Bar *b; }
+)cpp",
+       R"cpp(
+namespace ns { struct Foo; struct Bar; }
+namespace { using ns::Bar;using ns::Foo; }
+void foo() { ns::Foo *f; Bar *b; }
+)cpp"},
       // Type defined in main file, make sure using is after that.
       {R"cpp(
 namespace xx {
   struct yy {};
 }
 
 x^x::yy X;
 )cpp",