diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -42,6 +42,12 @@ /// 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/AST.cpp b/clang-tools-extra/clangd/AST.cpp --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -115,6 +115,18 @@ 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/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt --- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -19,6 +19,7 @@ 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 --- /dev/null +++ b/clang-tools-extra/clangd/refactor/tweaks/RemoveUsingNamespace.cpp @@ -0,0 +1,157 @@ +//===--- 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 "SourceCode.h" +#include "refactor/Tweak.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); +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()); +} +} // namespace + +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; + return true; +} + +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); + + // 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. + + // FIXME: if this is not coming from a macro argument, remove. + for (auto *T : Ref.Targets) { + // FIXME: handle inline namespaces, unscoped enumerators. + if (T->getDeclContext() != TargetDirective->getNominatedNamespace()) + return; + } + SourceLocation Loc = Ref.NameLoc; + if (Loc.isMacroID()) { + 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? + 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, add qualifiers instead"); +} +} // namespace clangd +} // namespace clang 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 @@ -69,7 +69,8 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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; @@ -521,7 +522,7 @@ 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(); } }"); @@ -554,7 +555,6 @@ EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail")); // Don't extract return EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail")); - } TEST_F(ExtractFunctionTest, FileTest) { @@ -648,6 +648,154 @@ 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"}, + {// FIXME: Doesn't work if not a top level namespace decl. + R"cpp( + namespace aa { + namespace bb { struct map {}; } + using namespace b^b; + } + int main() { aa::map m; } + )cpp", + R"cpp( + namespace aa { + namespace bb { struct map {}; } + + } + int main() { aa::map m; } + )cpp"}, + {// FIXME: Unable to qualify ident from inner ns. + R"cpp( + namespace aa { + namespace bb { struct map {}; } + using namespace bb; // Qualifies this. + } + using namespace a^a; + int main() { + map m; // Does not qualify this. + } + )cpp", + R"cpp( + namespace aa { + namespace bb { struct map {}; } + using namespace aa::bb; // Qualifies this. + } + + int main() { + map m; // Does not qualify this. + } + )cpp"}}; + for (auto C : Cases) + EXPECT_EQ(C.second, apply(C.first)) << C.first; +} + } // namespace } // namespace clangd } // namespace clang