From b62b4541216887fad9613f5b944d7dce120757b7 Mon Sep 17 00:00:00 2001 From: Utkarsh Saxena Date: Wed, 16 Oct 2019 09:53:59 +0000 Subject: [PATCH] [clangd] Add RemoveUsingNamespace tweak. Summary: Removes the 'using namespace' under the cursor and qualifies all accesses in the current file. E.g.: using namespace std; vector foo(std::map); Would become: std::vector foo(std::map); Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, mgrang, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D68562 llvm-svn: 374982 --- clang-tools-extra/clangd/AST.cpp | 12 + clang-tools-extra/clangd/AST.h | 6 + .../clangd/refactor/tweaks/CMakeLists.txt | 1 + .../refactor/tweaks/RemoveUsingNamespace.cpp | 206 +++++++++++++++++ clang-tools-extra/clangd/unittests/TweakTests.cpp | 251 +++++++++++++++++++-- 5 files changed, 459 insertions(+), 17 deletions(-) create mode 100644 clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp index a4ea7b0..4242721 100644 --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -115,6 +115,18 @@ static NestedNameSpecifier *getQualifier(const NamedDecl &ND) { return nullptr; } +std::string printUsingNamespaceName(const ASTContext &Ctx, + const UsingDirectiveDecl &D) { + PrintingPolicy PP(Ctx.getLangOpts()); + std::string Name; + llvm::raw_string_ostream Out(Name); + + if (auto *Qual = D.getQualifier()) + Qual->print(Out, PP); + D.getNominatedNamespaceAsWritten()->printName(Out); + return Out.str(); +} + std::string printName(const ASTContext &Ctx, const NamedDecl &ND) { std::string Name; llvm::raw_string_ostream Out(Name); diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h index e7a614c..eddabda 100644 --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -42,6 +42,12 @@ std::string printQualifiedName(const NamedDecl &ND); /// Returns the first enclosing namespace scope starting from \p DC. std::string printNamespaceScope(const DeclContext &DC); +/// Returns the name of the namespace inside the 'using namespace' directive, as +/// written in the code. E.g., passing 'using namespace ::std' will result in +/// '::std'. +std::string printUsingNamespaceName(const ASTContext &Ctx, + const UsingDirectiveDecl &D); + /// Prints unqualified name of the decl for the purpose of displaying it to the /// user. Anonymous decls return names of the form "(anonymous {kind})", e.g. /// "(anonymous struct)" or "(anonymous namespace)". diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt index 051b6c4..afb591a 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -19,6 +19,7 @@ add_clang_library(clangDaemonTweaks OBJECT ExtractFunction.cpp ExtractVariable.cpp RawStringLiteral.cpp + RemoveUsingNamespace.cpp SwapIfBranches.cpp LINK_LIBS diff --git a/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp b/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp new file mode 100644 index 0000000..6511125 --- /dev/null +++ b/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp @@ -0,0 +1,206 @@ +//===--- RemoveUsingNamespace.cpp --------------------------------*- C++-*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +#include "AST.h" +#include "FindTarget.h" +#include "Selection.h" +#include "SourceCode.h" +#include "refactor/Tweak.h" +#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" + +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 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. +class RemoveUsingNamespace : public Tweak { +public: + const char *id() const override; + + bool prepare(const Selection &Inputs) override; + Expected apply(const Selection &Inputs) override; + std::string title() const override; + Intent intent() const override { return Refactor; } + +private: + const UsingDirectiveDecl *TargetDirective = nullptr; +}; +REGISTER_TWEAK(RemoveUsingNamespace) + +class FindSameUsings : public RecursiveASTVisitor { +public: + FindSameUsings(const UsingDirectiveDecl &Target, + std::vector &Results) + : TargetNS(Target.getNominatedNamespace()), + TargetCtx(Target.getDeclContext()), Results(Results) {} + + bool VisitUsingDirectiveDecl(UsingDirectiveDecl *D) { + if (D->getNominatedNamespace() != TargetNS || + D->getDeclContext() != TargetCtx) + return true; + Results.push_back(D); + return true; + } + +private: + const NamespaceDecl *TargetNS; + const DeclContext *TargetCtx; + std::vector &Results; +}; + +/// Produce edit removing 'using namespace xxx::yyy' and the trailing semicolon. +llvm::Expected +removeUsingDirective(ASTContext &Ctx, const UsingDirectiveDecl *D) { + auto &SM = Ctx.getSourceManager(); + llvm::Optional NextTok = + Lexer::findNextToken(D->getEndLoc(), SM, Ctx.getLangOpts()); + if (!NextTok || NextTok->isNot(tok::semi)) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "no semicolon after using-directive"); + // 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(); +} + +// 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(); + if (!TargetDirective) + return false; + if (!dyn_cast(TargetDirective->getDeclContext())) + 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); +} + +Expected 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 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 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()); + + // 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))) + 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 diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp index 36efbef..c5caa4b 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -69,7 +69,8 @@ TEST_F(SwapIfBranchesTest, Test) { EXPECT_EQ(apply("^if (true) {return 100;} else {continue;}"), "if (true) {continue;} else {return 100;}"); EXPECT_EQ(apply("^if () {return 100;} else {continue;}"), - "if () {continue;} else {return 100;}") << "broken condition"; + "if () {continue;} else {return 100;}") + << "broken condition"; EXPECT_AVAILABLE("^i^f^^(^t^r^u^e^) { return 100; } ^e^l^s^e^ { continue; }"); EXPECT_UNAVAILABLE("if (true) {^return ^100;^ } else { ^continue^;^ }"); // Available in subexpressions of the condition; @@ -100,7 +101,7 @@ TEST_F(RawStringLiteralTest, Test) { EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro - EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char + EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char const char *Input = R"cpp(R"(multi token)" "\nst^ring\n" "literal")cpp"; @@ -286,11 +287,11 @@ TEST_F(ExtractVariableTest, Test) { void f(int a) { int y = PLUS([[1+a]]); })cpp", - /*FIXME: It should be extracted like this. - R"cpp(#define PLUS(x) x++ - void f(int a) { - auto dummy = 1+a; int y = PLUS(dummy); - })cpp"},*/ + /*FIXME: It should be extracted like this. + R"cpp(#define PLUS(x) x++ + void f(int a) { + auto dummy = 1+a; int y = PLUS(dummy); + })cpp"},*/ R"cpp(#define PLUS(x) x++ void f(int a) { auto dummy = PLUS(1+a); int y = dummy; @@ -301,13 +302,13 @@ TEST_F(ExtractVariableTest, Test) { if(1) LOOP(5 + [[3]]) })cpp", - /*FIXME: It should be extracted like this. SelectionTree needs to be - * fixed for macros. - R"cpp(#define LOOP(x) while (1) {a = x;} - void f(int a) { - auto dummy = 3; if(1) - LOOP(5 + dummy) - })cpp"},*/ + /*FIXME: It should be extracted like this. SelectionTree needs to be + * fixed for macros. + R"cpp(#define LOOP(x) while (1) {a = x;} + void f(int a) { + auto dummy = 3; if(1) + LOOP(5 + dummy) + })cpp"},*/ R"cpp(#define LOOP(x) while (1) {a = x;} void f(int a) { auto dummy = LOOP(5 + 3); if(1) @@ -403,8 +404,8 @@ TEST_F(ExtractVariableTest, Test) { void f() { auto dummy = S(2) + S(3) + S(4); S x = S(1) + dummy + S(5); })cpp"}, - // Don't try to analyze across macro boundaries - // FIXME: it'd be nice to do this someday (in a safe way) + // Don't try to analyze across macro boundaries + // FIXME: it'd be nice to do this someday (in a safe way) {R"cpp(#define ECHO(X) X void f() { int x = 1 + [[ECHO(2 + 3) + 4]] + 5; @@ -519,7 +520,7 @@ TEST_F(ExpandAutoTypeTest, Test) { StartsWith("fail: Could not expand type of lambda expression")); // inline namespaces EXPECT_EQ(apply("au^to x = inl_ns::Visible();"), - "Visible x = inl_ns::Visible();"); + "Visible x = inl_ns::Visible();"); // local class EXPECT_EQ(apply("namespace x { void y() { struct S{}; ^auto z = S(); } }"), "namespace x { void y() { struct S{}; S z = S(); } }"); @@ -663,6 +664,222 @@ TEST_F(ExtractFunctionTest, ControlFlow) { EXPECT_THAT(apply(" for(;;) { [[while(1) break; break;]] }"), StartsWith("fail")); } + +TWEAK_TEST(RemoveUsingNamespace); +TEST_F(RemoveUsingNamespaceTest, All) { + std::pair Cases[] = { + {// Remove all occurrences of ns. Qualify only unqualified. + R"cpp( + namespace ns1 { struct vector {}; } + namespace ns2 { struct map {}; } + using namespace n^s1; + using namespace ns2; + using namespace ns1; + int main() { + ns1::vector v1; + vector v2; + map m1; + } + )cpp", + R"cpp( + namespace ns1 { struct vector {}; } + namespace ns2 { struct map {}; } + + using namespace ns2; + + int main() { + ns1::vector v1; + ns1::vector v2; + map m1; + } + )cpp"}, + {// Ident to be qualified is a macro arg. + R"cpp( + #define DECLARE(x, y) x y + namespace ns { struct vector {}; } + using namespace n^s; + int main() { + DECLARE(ns::vector, v1); + DECLARE(vector, v2); + } + )cpp", + R"cpp( + #define DECLARE(x, y) x y + namespace ns { struct vector {}; } + + int main() { + DECLARE(ns::vector, v1); + DECLARE(ns::vector, v2); + } + )cpp"}, + {// Nested namespace: Fully qualify ident from inner ns. + R"cpp( + namespace aa { namespace bb { struct map {}; }} + using namespace aa::b^b; + int main() { + map m; + } + )cpp", + R"cpp( + namespace aa { namespace bb { struct map {}; }} + + int main() { + aa::bb::map m; + } + )cpp"}, + {// Nested namespace: Fully qualify ident from inner ns. + R"cpp( + namespace aa { namespace bb { struct map {}; }} + using namespace a^a; + int main() { + bb::map m; + } + )cpp", + R"cpp( + namespace aa { namespace bb { struct map {}; }} + + int main() { + aa::bb::map m; + } + )cpp"}, + {// Typedef. + R"cpp( + namespace aa { namespace bb { struct map {}; }} + using namespace a^a; + typedef bb::map map; + int main() { map M; } + )cpp", + R"cpp( + namespace aa { namespace bb { struct map {}; }} + + typedef aa::bb::map map; + int main() { map M; } + )cpp"}, + {// FIXME: Nested namespaces: Not aware of using ns decl of outer ns. + R"cpp( + namespace aa { namespace bb { struct map {}; }} + using name[[space aa::b]]b; + using namespace aa; + int main() { + map m; + } + )cpp", + R"cpp( + namespace aa { namespace bb { struct map {}; }} + + using namespace aa; + int main() { + aa::bb::map m; + } + )cpp"}, + {// Does not qualify ident from inner namespace. + R"cpp( + namespace aa { namespace bb { struct map {}; }} + using namespace aa::bb; + using namespace a^a; + int main() { + map m; + } + )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. + R"cpp( + namespace aa { + namespace bb { struct map {}; } + using namespace b^b; + } + int main() { aa::map m; } + )cpp", + "unavailable"}, + {// 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() { + map m; + } + )cpp", + "unavailable"}, + {R"cpp( + namespace a::b { struct Foo {}; } + using namespace a; + using namespace a::[[b]]; + using namespace b; + int main() { Foo F;} + )cpp", + R"cpp( + namespace a::b { struct Foo {}; } + using namespace a; + + + int main() { a::b::Foo F;} + )cpp"}, + {R"cpp( + namespace a::b { struct Foo {}; } + using namespace a; + using namespace a::b; + using namespace [[b]]; + int main() { Foo F;} + )cpp", + R"cpp( + namespace a::b { struct Foo {}; } + using namespace a; + + + int main() { b::Foo F;} + )cpp"}, + {// Enumerators. + R"cpp( + namespace tokens { + enum Token { + comma, identifier, numeric + }; + } + using namespace tok^ens; + int main() { + auto x = comma; + } + )cpp", + R"cpp( + namespace tokens { + enum Token { + comma, identifier, numeric + }; + } + + int main() { + auto x = tokens::comma; + } + )cpp"}, + {// inline namespaces. + R"cpp( + namespace std { inline namespace ns1 { inline namespace ns2 { struct vector {}; }}} + using namespace st^d; + int main() { + vector V; + } + )cpp", + R"cpp( + namespace std { inline namespace ns1 { inline namespace ns2 { struct vector {}; }}} + + int main() { + std::vector V; + } + )cpp"}}; + for (auto C : Cases) + EXPECT_EQ(C.second, apply(C.first)) << C.first; +} + } // namespace } // namespace clangd } // namespace clang -- 2.7.4