[clangd] Fix action `RemoveUsingNamespace` for inline namespace

[v1nh1shungry]

Summary

Existing version ignores symbols declared in an inline namespace ns
when removing using namespace ns

[v1nh1shungry]

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

212

We can replace printUsingNamespaceName with printNamespaceScope here so that we can get a::foobar() in the test.

However, it can sometimes cause redundancy such as in the 10th test.

And I don't know whether it is worth it. WDYT?

[tom-anders]

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

212

Just making sure I understood this correctly:

If you replace printUsingNamespaceName with printNamespaceScope, then...

• ...in the test you added it would result in a::foobar() instead of a::b::foobar() (which is better)
• ... but in this test (which is the 10th test if I counted correctly):

namespace a::b { struct Foo {}; }
 using namespace a;
 using namespace a::[[b]];
 using namespace b;
 int main() { Foo F;}

what would be the result..? would you get a::Foo instead of a::b::Foo?

[v1nh1shungry]

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

212

Sorry, I mean the next test. I read 10 from the inlay hint but I forgot the index starts from 0 :(

The test I want to mention:

namespace a::b { struct Foo {}; }
using namespace a;
using namespace a::b;
using namespace [[b]];
int main() { Foo F;}

We will get a::b::Foo in both the 10th and 11th tests. So in the 10th test, we don't get any benefits and don't sacrifice anything. In the 11th test, we get more redundancy than the existing version.

Apologize again for my mistake.

[v1nh1shungry]

A gentle ping? Sorry if it bothers you.

[tom-anders]

In D138028#3957139, @v1nh1shungry wrote:

A gentle ping? Sorry if it bothers you.

Sorry! I thought you’d probably first wanted to address the remaining comments in https://reviews.llvm.org/D137817 because they might also affect this patch here..?

[v1nh1shungry]

Oops! Thank you for your thoughtful consideration.

You're right, but I currently get stuck there and need more time. And I prefer to clear the existing patches simultaneously if you don't mind.

Or do you think I should merge the modification of this patch into there and give up this?

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

212

FYI, we have a discussion left here.

[tom-anders]

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

93

212

Ok I’d say let’s just go with printUsingNamespaceName here for now. When your other patch is merged as well, maybe we could have a look at this again.

[v1nh1shungry]

address the comment's suggestion

[v1nh1shungry]

Sorry for the delay! And many thanks for reviewing and giving suggestions!

[tom-anders]

LGTM!

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

return tooling::Replacement(
       "", 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())
+// Return true if `LHS` is declared in `RHS`
+bool isDeclaredIn(const NamedDecl *LHS, const DeclContext *RHS) {
+  const auto *D = LHS->getDeclContext();
+  while (D->isInlineNamespace() || D->isTransparentContext()) {
+    if (D->Equals(RHS))
+      return true;
     D = D->getParent();
-  return D;
+  }
+  return D->Equals(RHS);
 }
 
 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>();
Expected<Tweak::Effect> RemoveUsingNamespace::apply(const Selection &Inputs) {
   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()))
+            if (!isDeclaredIn(T, TargetDirective->getNominatedNamespace()))
               return;
             auto Kind = T->getDeclName().getNameKind();
             // Avoid adding qualifiers before operators, e.g.
             //   using namespace std;
             //   cout << "foo"; // Must not changed to std::cout std:: << "foo"
             if (Kind == DeclarationName::CXXOperatorName)
               return;
             // Avoid adding qualifiers before user-defined literals, e.g.
Expected<Tweak::Effect> RemoveUsingNamespace::apply(const Selection &Inputs) {
   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));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/tweaks/RemoveUsingNamespaceTests.cpp

)cpp"},
       int main() { 1.5_w; }
     )cpp",
        R"cpp(
       namespace ns {
       long double operator "" _w(long double);
       }
       
       int main() { 1.5_w; }
+    )cpp"},
+      {
+          R"cpp(
+      namespace a { inline namespace b { void foobar(); } }
+      using namespace a::[[b]];
+      int main() { foobar(); }
+    )cpp",
+          R"cpp(
+      namespace a { inline namespace b { void foobar(); } }
+      
+      int main() { a::b::foobar(); }
     )cpp"}};
   for (auto C : Cases)
     EXPECT_EQ(C.second, apply(C.first)) << C.first;
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang