diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp --- a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp @@ -143,8 +143,53 @@ return true; } -// Rewrites body of FD to fully-qualify all of the decls inside. -llvm::Expected qualifyAllDecls(const FunctionDecl *FD) { +// Gets the nested name specifiers necessary for spelling ND in DestContext. +// It qualifies until first qualifier of ND that is either in +// VisibleNamespaceDecls or encloses DestContext. +// For example, if you want to qualify clang::clangd::bar::foo in +// clang::clangd::x, this function will return bar. And if you have +// clang::clangd::bar in VisibleNamespaceDecls, this function will return null. +NestedNameSpecifier *getQualification( + ASTContext &Context, const DeclContext *DestContext, const NamedDecl *ND, + const llvm::DenseSet &VisibleNamespaceDecls) { + // Goes over all parents of ND until we find a comman ancestor for DestContext + // and ND. Any qualifier including and above common ancestor is redundant, + // therefore we stop at lowest common ancestor. + std::vector SourceParents; + for (const DeclContext *Context = ND->getLexicalDeclContext(); Context; + Context = Context->getLookupParent()) { + // Stop once we reach a common ancestor. + if (Context->Encloses(DestContext)) + break; + // Inline namespace names are not spelled while qualifying a name, so skip + // those. + if (Context->isInlineNamespace()) + continue; + + auto *NSD = llvm::dyn_cast(Context); + assert(NSD && "Non-namespace decl context found."); + // Stop if this namespace is already visible at DestContext. + if (VisibleNamespaceDecls.count(NSD->getCanonicalDecl())) + break; + // Again, ananoymous namespaces are not spelled while qualifying a name. + if (NSD->isAnonymousNamespace()) + continue; + + SourceParents.push_back(NSD); + } + + // Go over name-specifiers in reverse order to create necessary qualification, + // since we stored inner-most parent first. + NestedNameSpecifier *Result = nullptr; + for (const auto *Parent : llvm::reverse(SourceParents)) + Result = NestedNameSpecifier::Create(Context, Result, Parent); + return Result; +} + +// Rewrites body of FD by re-spelling all of the names to make sure they are +// still valid in context of Target. +llvm::Expected qualifyAllDecls(const FunctionDecl *FD, + const FunctionDecl *Target) { // There are three types of spellings that needs to be qualified in a function // body: // - Types: Foo -> ns::Foo @@ -155,16 +200,29 @@ // using ns3 = ns2 -> using ns3 = ns1::ns2 // // Go over all references inside a function body to generate replacements that - // will fully qualify those. So that body can be moved into an arbitrary file. + // will qualify those. So that body can be moved into an arbitrary file. // We perform the qualification by qualyfying the first type/decl in a // (un)qualified name. e.g: // namespace a { namespace b { class Bar{}; void foo(); } } // b::Bar x; -> a::b::Bar x; // foo(); -> a::b::foo(); - // FIXME: Instead of fully qualyfying we should try deducing visible scopes at - // target location and generate minimal edits. + auto *TargetContext = Target->getLexicalDeclContext(); + SourceLocation TargetLoc = Target->getBeginLoc(); const SourceManager &SM = FD->getASTContext().getSourceManager(); + + llvm::DenseSet VisibleNamespacesInTargetContext; + for (const auto *DC = TargetContext; DC; DC = DC->getLookupParent()) { + for (const auto *D : DC->decls()) { + if (!SM.isWrittenInSameFile(D->getLocation(), TargetLoc) || + !SM.isBeforeInTranslationUnit(D->getLocation(), TargetLoc)) + continue; + if (auto *UDD = llvm::dyn_cast(D)) { + VisibleNamespacesInTargetContext.insert( + UDD->getNominatedNamespace()->getCanonicalDecl()); + } + } + } tooling::Replacements Replacements; bool HadErrors = false; findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) { @@ -195,10 +253,18 @@ // namespace a { class Bar { public: static int x; } } // void foo() { Bar::x; } // ~~~~~ -> we need to qualify Bar not x. - if (!ND->getDeclContext()->isNamespace()) + if (!ND->getLexicalDeclContext()->isNamespace()) return; - std::string Qualifier = printNamespaceScope(*ND->getDeclContext()); + std::string Qualifier; + // FIXME: Also take using directives and namespace aliases inside function + // body into account. + if (auto *NNS = getQualification(FD->getASTContext(), TargetContext, ND, + VisibleNamespacesInTargetContext)) { + llvm::raw_string_ostream OS(Qualifier); + NNS->print(OS, FD->getASTContext().getPrintingPolicy(), true); + } + if (auto Err = Replacements.add( tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) { HadErrors = true; @@ -527,7 +593,7 @@ if (!ParamReplacements) return ParamReplacements.takeError(); - auto QualifiedBody = qualifyAllDecls(Source); + auto QualifiedBody = qualifyAllDecls(Source, Target); if (!QualifiedBody) return QualifiedBody.takeError(); 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 @@ -1045,22 +1045,20 @@ namespace a { template class Bar {}; } - using namespace a; template void foo() /*Comment -_-*/ ; + using namespace a; template void f^oo() { Bar B; Bar> q; - } - )cpp"), + })cpp"), R"cpp( namespace a { template class Bar {}; } - using namespace a; template void foo() /*Comment -_-*/ { @@ -1068,7 +1066,7 @@ a::Bar> q; } - + using namespace a; )cpp"); } @@ -1169,18 +1167,17 @@ class Foo{}; }; } - using namespace a; - using namespace b; - using namespace c; void foo() /*Comment -_-*/ ; + using namespace a; + using namespace b; + using namespace c; void f^oo() { Bar B; b::Foo foo; a::Bar>::Baz> q; - } - )cpp"), + })cpp"), R"cpp( namespace a { template class Bar { @@ -1191,9 +1188,6 @@ class Foo{}; }; } - using namespace a; - using namespace b; - using namespace c; void foo() /*Comment -_-*/ { a::Bar B; @@ -1201,7 +1195,9 @@ a::Bar>::Baz> q; } - + using namespace a; + using namespace b; + using namespace c; )cpp"); } @@ -1223,12 +1219,12 @@ } } } - using namespace a; - using namespace b; - using namespace c; void foo() /*Comment -_-*/ ; + using namespace a; + using namespace b; + using namespace c; void f^oo() { a::Bar B; B.foo(); @@ -1239,8 +1235,7 @@ Bar::y = 3; bar(); c::test(); - } - )cpp"), + })cpp"), R"cpp( namespace a { template class Bar { @@ -1258,9 +1253,6 @@ } } } - using namespace a; - using namespace b; - using namespace c; void foo() /*Comment -_-*/ { a::Bar B; @@ -1274,7 +1266,9 @@ a::b::c::test(); } - + using namespace a; + using namespace b; + using namespace c; )cpp"); } @@ -1402,6 +1396,138 @@ template <> inline void foo(){})cpp"))); } +TEST_F(DefineInlineTest, DropCommonNamespecifiers) { + EXPECT_EQ(apply(R"cpp( + namespace a { namespace b { void aux(); } } + namespace ns1 { + void foo(); + namespace qq { void test(); } + namespace ns2 { + void bar(); + namespace ns3 { void baz(); } + } + } + + using namespace a; + using namespace a::b; + using namespace ns1::qq; + void ns1::ns2::ns3::b^az() { + foo(); + bar(); + baz(); + ns1::ns2::ns3::baz(); + aux(); + test(); + })cpp"), + R"cpp( + namespace a { namespace b { void aux(); } } + namespace ns1 { + void foo(); + namespace qq { void test(); } + namespace ns2 { + void bar(); + namespace ns3 { void baz(){ + foo(); + bar(); + baz(); + ns1::ns2::ns3::baz(); + a::b::aux(); + qq::test(); + } } + } + } + + using namespace a; + using namespace a::b; + using namespace ns1::qq; + )cpp"); + + EXPECT_EQ(apply(R"cpp( + namespace ns1 { + namespace qq { struct Foo { struct Bar {}; }; using B = Foo::Bar; } + namespace ns2 { void baz(); } + } + + using namespace ns1::qq; + void ns1::ns2::b^az() { Foo f; B b; })cpp"), + R"cpp( + namespace ns1 { + namespace qq { struct Foo { struct Bar {}; }; using B = Foo::Bar; } + namespace ns2 { void baz(){ qq::Foo f; qq::B b; } } + } + + using namespace ns1::qq; + )cpp"); + + EXPECT_EQ(apply(R"cpp( + namespace ns1 { + namespace qq { + template struct Foo { template struct Bar {}; }; + template + using B = typename Foo::template Bar; + } + namespace ns2 { void baz(); } + } + + using namespace ns1::qq; + void ns1::ns2::b^az() { B b; })cpp"), + R"cpp( + namespace ns1 { + namespace qq { + template struct Foo { template struct Bar {}; }; + template + using B = typename Foo::template Bar; + } + namespace ns2 { void baz(){ qq::B b; } } + } + + using namespace ns1::qq; + )cpp"); +} + +TEST_F(DefineInlineTest, QualifyWithUsingDirectives) { + // FIXME: The last reference to cux() in body of foo should not be qualified, + // since there is a using directive inside the function body. + EXPECT_EQ(apply(R"cpp( + namespace a { + void bar(); + namespace b { struct Foo{}; void aux(); } + namespace c { void cux(); } + } + using namespace a; + using X = b::Foo; + void foo(); + + using namespace b; + using namespace c; + void ^foo() { + cux(); + bar(); + X x; + aux(); + using namespace c; + cux(); + })cpp"), R"cpp( + namespace a { + void bar(); + namespace b { struct Foo{}; void aux(); } + namespace c { void cux(); } + } + using namespace a; + using X = b::Foo; + void foo(){ + c::cux(); + bar(); + X x; + b::aux(); + using namespace c; + c::cux(); + } + + using namespace b; + using namespace c; + )cpp"); +} } // namespace } // namespace clangd } // namespace clang