[clangd] Remove using-namespace present inside a namespace.

[usaxena95]

Summary

This patch extends the removing using-namespace code action to remove
using-namespace decl that are present inside a namespace.

Goes over all the references that are declared in TargetNS and used in
ContainingNS. Removes the current qualifier and qualifies it with only
the TargetNS. No other qualifier is needed since it was legal to use
using namespace TargetNS inside ContainingNS. Therefore all target
references inside the ContainingNS can be qualified with just
TargetNS.

Limitations:

• The using decl must be present in TUDecl or directly under a NSDecl.
• Only references inside the DeclContext of using-decl are removed. Any references outside this DeclContext are retained as is which is incorrect.

[merge_guards_bot]

Build result: fail - 33547 tests passed, 1 failed and 464 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[usaxena95]

Added additional tests.

[merge_guards_bot]

Build result: fail - 33547 tests passed, 1 failed and 464 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[ilya-biryukov]

The most important comment is in the tests. Is there a way to have the same effect with less changes?

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

47

NIT: could you add a short comment mentioning this is exactly the DeclContext of TargetDirective?

110

This check looks redundant now that we check the decl context is a namespace of a translation unit.
I think it was originally intended to check the whether the using namespace is inside a function or not.

173

NIT: maybe simplify to

QualifierReplacements.push_back(
  CharSourceRange::getTokenRange(
    Ref.Qualifier ? Ref.Qualifier.getBeginLoc : Loc, 
    Loc));

?

180

Do we also need to run in the redeclarations of the same namespace?

namespace clangd {
  using namespace clang;
}

namespace clangd {
  /*clang::*/Decl* D = nullptr;
}

182

NIT: could you add braces around the branches of the if statement?
for loop is complicated, so it probably qualifies as something that asks for the braces.

clang-tools-extra/clangd/unittests/TweakTests.cpp

898

Is there a way to avoid re-qualifying this?
We should aim for minimal changes and there is probably a simple way to achieve this by only qualifying when the qualifier itself references the ContainingNS and target is inside the target namespace of the directive being removed.

Or this too complicated for some reason?

[usaxena95]

Addressed comments.

[usaxena95]

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

180

Aah. Didn't know we have the redecls here itself. I had a FIXME as a negative test for this. Fixed that now.

[merge_guards_bot]

Build result: fail - 59446 tests passed, 1 failed and 764 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/lock.pass.cpp

Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

One important question about running on the whole TU in all cases. Other than that LG

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

111

NIT: use auto*
knowing it's a pointer is quite useful when reading and writing the code (e.g. suggests one need to use -> instead of . to access the variable)

179

Could you explain why don't we always just run across the whole TU?
What disadvantages would that have?

182

NIT: remove empty line

[usaxena95]

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

179

Working with a using decl inside the ContainingNS is quite similar to our initial version with GlobalNS.
For example we take advantage of that to replace the whole qualifier with just TargetNS. We use the fact that since using namespace TargetNS; was valid inside ContainingNS then qualifying the reference with just TargetNS would also be valid inside ContainingNS.

When we are outside ContainingNS, we would have to fully qualify the ref to be 100% correct. To reduce the qualification we might have to figure out which is enclosing NS for the reference.

namespace aa {
namespace bb { 
struct map {}; 
}
using namespace b^b;
}
int main() { 
 aa::map m1;  // We do not qualify this currently.
}

[usaxena95]

Addressed comments.

[merge_guards_bot]

Build result: fail - 33521 tests passed, 1 failed and 463 were skipped.

failed: LLVM.tools/llvm-objdump/X86/disassemble-functions.test

Log files: console-log.txt, CMakeCache.txt

[ilya-biryukov]

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

111

NIT: you could use DC here, clangd uses Go-style short identifiers for locals and parameters extensively

146

NIT: Maybe call it QualifyRef? Was a bit confused by Select

179

If we detect references outside ContainingNS, which are definitely broken, fully-qualifying seems better than silently ignoring those.

I think they're quite easy to detect:

• Their target should be inside TargetNS,
• Their QualifierLoc should reference ContainingNS.

We know that those won't work anymore, so we should qualify them.

Leaving broken code is definitely a bad idea, if we can easily detect and fix it.

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

 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
 #include "llvm/ADT/ScopeExit.h"
+#include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 /// Removes the 'using namespace' under the cursor and qualifies all accesses in
 /// the current file. E.g.,
 ///   using namespace std;
 ///   vector<int> foo(std::map<int, int>);
 /// Would become:
 ///   std::vector<int> foo(std::map<int, int>);
-/// Currently limited to using namespace directives inside global namespace to
-/// simplify implementation. Also the namespace must not contain using
-/// directives.
+///
+/// Currently limited to using namespace directive that is global or inside a
+/// namespace. Also the namespace must not contain any using directives.
+/// Qualifies all the references that are inside the enclosing namespace.
 class RemoveUsingNamespace : public Tweak {
 public:
   const char *id() const override;
 
   bool prepare(const Selection &Inputs) override;
   Expected<Effect> apply(const Selection &Inputs) override;
   std::string title() const override;
   Intent intent() const override { return Refactor; }
 
 private:
   const UsingDirectiveDecl *TargetDirective = nullptr;
+  const NamespaceDecl *ContainingNS = nullptr;
 };
 REGISTER_TWEAK(RemoveUsingNamespace)
 
 class FindSameUsings : public RecursiveASTVisitor<FindSameUsings> {
 public:
   FindSameUsings(const UsingDirectiveDecl &Target,
                  std::vector<const UsingDirectiveDecl *> &Results)
       : TargetNS(Target.getNominatedNamespace()),
removeUsingDirective(ASTContext &Ctx, const UsingDirectiveDecl *D) {
   // FIXME: removing the semicolon may be invalid in some obscure cases, e.g.
   //        if (x) using namespace std; else using namespace bar;
   return tooling::Replacement(
       SM,
       CharSourceRange::getTokenRange(D->getBeginLoc(), NextTok->getLocation()),
       "", Ctx.getLangOpts());
 }
 
-// Returns true iff the parent of the Node is a TUDecl.
-bool isTopLevelDecl(const SelectionTree::Node *Node) {
-  return Node->Parent && Node->Parent->ASTNode.get<TranslationUnitDecl>();
-}
-
 // Returns the first visible context that contains this DeclContext.
 // For example: Returns ns1 for S1 and a.
 // namespace ns1 {
 // inline namespace ns2 { struct S1 {}; }
 // enum E { a, b, c, d };
 // }
 const DeclContext *visibleContext(const DeclContext *D) {
   while (D->isInlineNamespace() || D->isTransparentContext())
     D = D->getParent();
   return D;
 }
 
 bool RemoveUsingNamespace::prepare(const Selection &Inputs) {
   // Find the 'using namespace' directive under the cursor.
   auto *CA = Inputs.ASTSelection.commonAncestor();
   if (!CA)
     return false;
   TargetDirective = CA->ASTNode.get<UsingDirectiveDecl>();
   if (!TargetDirective)
     return false;
-  if (!dyn_cast<Decl>(TargetDirective->getDeclContext()))
+  auto DeclCtx = TargetDirective->getDeclContext();
+  if (!dyn_cast<Decl>(DeclCtx))
     return false;
   // FIXME: Unavailable for namespaces containing using-namespace decl.
   // It is non-trivial to deal with cases where identifiers come from the inner
   // namespace. For example map has to be changed to aa::map.
   // namespace aa {
   //   namespace bb { struct map {}; }
   //   using namespace bb;
   // }
   // using namespace a^a;
   // int main() { map m; }
   // We need to make this aware of the transitive using-namespace decls.
   if (!TargetDirective->getNominatedNamespace()->using_directives().empty())
     return false;
-  return isTopLevelDecl(CA);
+  // The containing context must be a namespace or a TU decl.
+  ContainingNS = dyn_cast<NamespaceDecl>(DeclCtx);
+  return ContainingNS || DeclCtx->isTranslationUnit();;
 }
 
 Expected<Tweak::Effect> RemoveUsingNamespace::apply(const Selection &Inputs) {
   auto &Ctx = Inputs.AST.getASTContext();
   auto &SM = Ctx.getSourceManager();
   // First, collect *all* using namespace directives that redeclare the same
   // namespace.
   std::vector<const UsingDirectiveDecl *> AllDirectives;
   FindSameUsings(*TargetDirective, AllDirectives).TraverseAST(Ctx);
 
   SourceLocation FirstUsingDirectiveLoc;
   for (auto *D : AllDirectives) {
     if (FirstUsingDirectiveLoc.isInvalid() ||
         SM.isBeforeInTranslationUnit(D->getBeginLoc(), FirstUsingDirectiveLoc))
       FirstUsingDirectiveLoc = D->getBeginLoc();
   }
 
   // Collect all references to symbols from the namespace for which we're
-  // removing the directive.
-  std::vector<SourceLocation> IdentsToQualify;
-  for (auto &D : Inputs.AST.getLocalTopLevelDecls()) {
-    findExplicitReferences(D, [&](ReferenceLoc Ref) {
-      if (Ref.Qualifier)
-        return; // This reference is already qualified.
-
-      for (auto *T : Ref.Targets) {
-        if (!visibleContext(T->getDeclContext())
-                 ->Equals(TargetDirective->getNominatedNamespace()))
-          return;
-      }
-      SourceLocation Loc = Ref.NameLoc;
-      if (Loc.isMacroID()) {
-        // Avoid adding qualifiers before macro expansions, it's probably
-        // incorrect, e.g.
-        //   namespace std { int foo(); }
-        //   #define FOO 1 + foo()
-        //   using namespace foo; // provides matrix
-        //   auto x = FOO; // Must not changed to auto x = std::FOO
-        if (!SM.isMacroArgExpansion(Loc))
-          return; // FIXME: report a warning to the users.
-        Loc = SM.getFileLoc(Ref.NameLoc);
-      }
-      assert(Loc.isFileID());
-      if (SM.getFileID(Loc) != SM.getMainFileID())
-        return; // FIXME: report these to the user as warnings?
-      if (SM.isBeforeInTranslationUnit(Loc, FirstUsingDirectiveLoc))
-        return; // Directive was not visible before this point.
-      IdentsToQualify.push_back(Loc);
-    });
-  }
-  // Remove duplicates.
-  llvm::sort(IdentsToQualify);
-  IdentsToQualify.erase(
-      std::unique(IdentsToQualify.begin(), IdentsToQualify.end()),
-      IdentsToQualify.end());
+  // removing the directive. Replace their current qualifier with the TargetNS.
+  std::vector<CharSourceRange> QualifierReplacements;
+  auto SelectRefToQualify = [&](ReferenceLoc Ref) {
+    if (Ref.Targets.empty())
+      return;
+
+    CharSourceRange RemoveQualifier;
+    RemoveQualifier.setBegin(Ref.Qualifier ? Ref.Qualifier.getBeginLoc() : Loc);
+    RemoveQualifier.setEnd(Loc);
+    QualifierReplacements.push_back(RemoveQualifier);
+  };
+
+  if (ContainingNS)
+    findExplicitReferences(ContainingNS, SelectRefToQualify);
+  else
+    for (auto &D : Inputs.AST.getLocalTopLevelDecls())
+      findExplicitReferences(D, SelectRefToQualify);
 
   // Produce replacements to remove the using directives.
   tooling::Replacements R;
   for (auto *D : AllDirectives) {
     auto RemoveUsing = removeUsingDirective(Ctx, D);
     if (!RemoveUsing)
       return RemoveUsing.takeError();
     if (auto Err = R.add(*RemoveUsing))
       return std::move(Err);
   }
   // Produce replacements to add the qualifiers.
   std::string Qualifier = printUsingNamespaceName(Ctx, *TargetDirective) + "::";
-  for (auto Loc : IdentsToQualify) {
-    if (auto Err = R.add(tooling::Replacement(Ctx.getSourceManager(), Loc,
-                                              /*Length=*/0, Qualifier)))
+  for (auto RemoveRange : QualifierReplacements) {
+    if (auto Err = R.add(tooling::Replacement(Ctx.getSourceManager(),
+                                              RemoveRange, Qualifier)))
       return std::move(Err);
   }
   return Effect::mainFileEdit(SM, std::move(R));
 }
 
 std::string RemoveUsingNamespace::title() const {
   return llvm::formatv("Remove using namespace, re-qualify names instead.");
 }
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/TweakTests.cpp

)cpp",
        R"cpp(
       namespace aa { namespace bb { struct map {}; }}
       using namespace aa::bb;
       
       int main() {
         map m;
       }
     )cpp"},
-      {// Available only for top level namespace decl.
+      {
+          // FIXME: Does not remove usages outside the enclosing NS.
           R"cpp(
-        namespace aa {
-          namespace bb { struct map {}; }
-          using namespace b^b;
-        }
-        int main() { aa::map m; }
     )cpp",
-       "unavailable"},
+          R"cpp(
+      namespace aa {
+      namespace bb { struct map {}; }
+      
+      }
+      int main() { aa::map m; }
+    )cpp",
+      },
       {// FIXME: Unavailable for namespaces containing using-namespace decl.
        R"cpp(
       namespace aa {
         namespace bb { struct map {}; }
         using namespace bb;
       }
       using namespace a^a;
       int main() {
)cpp"},
       }
     )cpp",
        R"cpp(
       namespace std { inline namespace ns1 { inline namespace ns2 { struct vector {}; }}}
       
       int main() {
         std::vector V;
       }
-    )cpp"}};
+    )cpp"},
+      {// using inner namespaces.
+       R"cpp(
+      namespace A { namespace B { 
+      inline namespace ns1 { namespace std { struct vector {}; } }
+      using namespace st^d;
+      using namespace A::B::std;
+      using namespace B::std;
+      void func() { 
+        vector V1;
+        B::vector V2;
+        A::B::vector V3;
+        A::B::std::vector V4;
+        ::A::B::std::vector V5;
+        ::A::B::ns1::std::vector V6;
+      }
+      }}
+    )cpp",
+       R"cpp(
+      namespace A { namespace B { 
+      inline namespace ns1 { namespace std { struct vector {}; } }
+      
+      
+      
+      void func() { 
+        std::vector V1;
+        std::vector V2;
+        std::vector V3;
+        std::vector V4;
+        std::vector V5;
+        std::vector V6;
+      }
+      }}
+    )cpp"},
+      {// using outer namespaces.
+       R"cpp(
+      namespace std { struct vector {}; }
+      namespace A { namespace B { 
+      using namespace st^d;
+      void func() { 
+        vector V1;
+        B::vector V2;
+        A::B::vector V3;
+        ::A::B::vector V4;
+      }
+      }}
+    )cpp",
+       R"cpp(
+      namespace std { struct vector {}; }
+      namespace A { namespace B { 
+      
+      void func() { 
+        std::vector V1;
+        std::vector V2;
+        std::vector V3;
+        std::vector V4;
+      }
+      }}
+    )cpp"},
+      {// FIXME: No replacements produced for extended namespace.
+       R"cpp(
+      namespace std { struct vector {}; }
+      namespace A { namespace B { using namespace st^d; }}
+      namespace A { namespace B { int func() { vector V1;}}}
+    )cpp",
+       R"cpp(
+      namespace std { struct vector {}; }
+      namespace A { namespace B {  }}
+      namespace A { namespace B { int func() { vector V1;}}}
+    )cpp"},
+      {// using namespace for outer and inner namespaces: remove inner.
+       R"cpp(
+      namespace out { namespace std { struct vector{}; }}
+      namespace A {
+      using namespace out;
+      using namespace st^d;
+      void func() { vector V;}
+      }
+    )cpp",
+       R"cpp(
+      namespace out { namespace std { struct vector{}; }}
+      namespace A {
+      using namespace out;
+      
+      void func() { std::vector V;}
+      }
+    )cpp"},
+      {// using namespace for outer and inner namespaces: remove outer.
+       R"cpp(
+      namespace out { namespace std { struct vector{}; }}
+      namespace A {
+      using namespace ou^t;
+      using namespace std;
+      void func() { vector V;}
+      }
+    )cpp",
+       R"cpp(
+      namespace out { namespace std { struct vector{}; }}
+      namespace A {
+      
+      using namespace out::std;
+      void func() { vector V;}
+      }
+    )cpp"},
+      {// Not available for contexts that are not global or namespaces.
+       R"cpp(
+      namespace std { struct vector {}; }
+      int main() { if(true) { using namespace st^d;}}
+    )cpp",
+       "unavailable"}};
   for (auto C : Cases)
     EXPECT_EQ(C.second, apply(C.first)) << C.first;
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang