diff --git a/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp b/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp @@ -18,6 +18,7 @@ #include "clang/Tooling/Core/Replacement.h" #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h" #include "llvm/ADT/ScopeExit.h" +#include namespace clang { namespace clangd { @@ -28,9 +29,10 @@ /// vector foo(std::map); /// Would become: /// std::vector foo(std::map); -/// 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; @@ -42,6 +44,8 @@ private: const UsingDirectiveDecl *TargetDirective = nullptr; + // DeclContext of TargetDirective. + const NamespaceDecl *ContainingNS = nullptr; }; REGISTER_TWEAK(RemoveUsingNamespace) @@ -83,11 +87,6 @@ "", 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(); -} - // Returns the first visible context that contains this DeclContext. // For example: Returns ns1 for S1 and a. // namespace ns1 { @@ -108,8 +107,7 @@ TargetDirective = CA->ASTNode.get(); if (!TargetDirective) return false; - if (!dyn_cast(TargetDirective->getDeclContext())) - return false; + auto *DeclCtx = TargetDirective->getDeclContext(); // 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. @@ -122,7 +120,9 @@ // 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(DeclCtx); + return ContainingNS || DeclCtx->isTranslationUnit(); } Expected RemoveUsingNamespace::apply(const Selection &Inputs) { @@ -141,43 +141,48 @@ } // Collect all references to symbols from the namespace for which we're - // removing the directive. - std::vector 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); - }); + // removing the directive. Replace their current qualifier with the TargetNS. + std::vector QualifierReplacements; + auto SelectRefToQualify = [&](ReferenceLoc Ref) { + if (Ref.Targets.empty()) + return; + 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. + if (Ref.Qualifier && + Ref.Qualifier.getNestedNameSpecifier()->getAsNamespace() == + TargetDirective->getNominatedNamespace()) + return; // Ref is already qualified with the TargetNS. + QualifierReplacements.push_back(CharSourceRange::getCharRange( + Ref.Qualifier ? Ref.Qualifier.getBeginLoc() : Loc, Loc)); + }; + + if (ContainingNS) { + for (auto ReDeclNS : ContainingNS->redecls()) + findExplicitReferences(ReDeclNS, SelectRefToQualify); + } else { + for (auto &D : Inputs.AST.getLocalTopLevelDecls()) + findExplicitReferences(D, SelectRefToQualify); } - // Remove duplicates. - llvm::sort(IdentsToQualify); - IdentsToQualify.erase( - std::unique(IdentsToQualify.begin(), IdentsToQualify.end()), - IdentsToQualify.end()); // Produce replacements to remove the using directives. tooling::Replacements R; @@ -190,9 +195,9 @@ } // 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)); diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -789,15 +789,21 @@ 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; } + 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 { @@ -875,7 +881,116 @@ 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; // Already qualified with TargetNS. + ::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; + A::B::std::vector V4; // Already qualified with TargetNS. + ::A::B::std::vector V5; + ::A::B::ns1::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"}, + {// Redecls of a 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() { std::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; }