[clangd] Improve action `RemoveUsingNamespace` for user-defined literals

[v1nh1shungry]

Summary

This patch aims at:

When triggering the action RemoveUsingNamespace for

namespace ns {
long double operator""_a(long double);
}
using namespace n^s;
int main() {
  1.5_a;
}

, the codes change to

// namespace ns above
int main() {
  using ns::operator""_a;
  1.5_a;
}

Inspired by the discussion in https://reviews.llvm.org/D137550 and below.

[tom-anders]

This looks really cool :)

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

133

nit: const auto

249

If you're sorting and removing duplicates in the end anyway, maybe llvm::StringSet would be a better fit instead of vector<string>?

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

286

Your code also handles cases where there are multiple inline namespaces involved, right? If so, it would be a good idea also test this here (i.e. add ns3 with another user-defined literal)

[v1nh1shungry]

Apply the review suggestions

[v1nh1shungry]

@tom-anders Thank you for reviewing and giving suggestions!

Hope I use the llvm::StringSet correctly.

[tom-anders]

LGTM, but I'll give @kadircet a chance to review as well

[kadircet]

thanks for pinging @tom-anders , i've added some concerns about the behaviour in general. sorry if these are discussed somewhere else but i've missed.

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

314

sorry i am having trouble understanding why we are:

• only handling user defined literals from inline namespaces and not others?
• producing using directives and not using declarations
• inserting these at top level rather than where the usage is

[v1nh1shungry]

@kadircet Thank you for reviewing and giving suggestions! Please check my reply to your concerns. Sorry if I misunderstood anything.

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

314

The first question:

Because others have already been handled. Say

namespace a { inline namespace b { void foobar(); } }
using namespace ^a;
int main() { foobar(); }

can become

namespace a { inline namespace b { void foobar(); } }

int main() { a::foobar(); }

But user-defined literals just can't add a qualifier, right?

---

The second question:

Yes, this is a good idea. I didn't realize we can use using-declarations instead of using-directives.

---

The last question:

Hmm, I think it is cleaner if there are multiple usages. Since we can only remove the using-directives at the top level, this doesn't hurt anything. And it is the easiest solution to implement as well :)

[kadircet]

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

314

Because others have already been handled. Say

i was emphasizing on the difference between user defined literals inside inline namespaces, and user defined literals from regular namespaces. not other types of decls inside an inline namespace, eg:

namespace ns { long double operator"" _o(long double); }

we should also introduce a using-decl for _o despite it not being inside an inline namespace.

314

Hmm, I think it is cleaner if there are multiple usages. Since we can only remove the using-directives at the top level, this doesn't hurt anything. And it is the easiest solution to implement as well :)

right, but introducing these at the top level will have unintended consequences (e.g. if this is a header, symbols will all of a sudden be visible in all the dependents).

[v1nh1shungry]

@kadircet Thank you for the excellent suggestions! Will dive into it to figure out the implementations.

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

314

You're right. Sorry for misunderstanding it.

314

Yes, I agree.

[v1nh1shungry]

apply review's suggestions

[v1nh1shungry]

Reimplemented.

Note:

Failed to find a way to use ReferenceLoc to insert the using-declarations.
Currently I have to visit the Decl again and look for CompoundStmt, if there
is any target user-defined literal in the CompoundStmt I mark it so that
I can add the using-declaration in it later.

In this way this implementation covers many cases, and does nothing for user-defined
literals used in the top-level. But it can cause redundancy and is too complex.

[v1nh1shungry]

Note:
Currently if there are user-defined literals, this patch will find them and get their
CompoundStmt parents. If there is one, it will record the parent so that it can add the
using-declaration in it later. If there is not, it means the user-defined literal is in
the top-level, and it is reasonable to do nothing for it. I choose CompoundStmt because
it covers many cases, like blocks, functions, lambdas.

Since I failed to find a way to use only ReferenceLoc to get the parents, I have to
traverse the whole Decl again to find out the DeclRefExpr and then use ASTContext
to get its parents. Also this implementation can cause redundance in some cases.

[v1nh1shungry]

I think this patch is ready for another review now.

[v1nh1shungry]

This comment was removed by v1nh1shungry.

[v1nh1shungry]

Insert the using-declarations in the outermost CompoundStmt instead of
the innermost one. Can reduce some potential rebundancy.

[v1nh1shungry]

Abandon because of the terrible implementation.

• The cost seems to be higher than the value.

• The place to insert isn't good enough.

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

 
 class FindSameUsings : public RecursiveASTVisitor<FindSameUsings> {
 public:
   FindSameUsings(const UsingDirectiveDecl &Target,
                  std::vector<const UsingDirectiveDecl *> &Results)
       : TargetNS(Target.getNominatedNamespace()),
         TargetCtx(Target.getDeclContext()), Results(Results) {}
 
+  FindSameUsings(const NamespaceDecl *TargetNS, const DeclContext *TargetCtx,
+                 std::vector<const UsingDirectiveDecl *> &Results)
+      : TargetNS(TargetNS), TargetCtx(TargetCtx), Results(Results) {}
+
   bool VisitUsingDirectiveDecl(UsingDirectiveDecl *D) {
     if (D->getNominatedNamespace() != TargetNS ||
         D->getDeclContext() != TargetCtx)
       return true;
     Results.push_back(D);
     return true;
   }
 

 // enum E { a, b, c, d };
 // }
 const DeclContext *visibleContext(const DeclContext *D) {
   while (D->isInlineNamespace() || D->isTransparentContext())
     D = D->getParent();
   return D;
 }
 
+// If the user-defined literal is declared in an inline namespace,
+// we add a using-directive for it
+void handleUserDefinedLiteral(const DeclContext *D, const DeclContext *Ctx,
+                              ASTContext &AST,
+                              std::vector<std::string> &UsingToAdd) {
+  if (D->isInlineNamespace()) {
+    const auto *ND = cast<NamespaceDecl>(D);
+    // Find if there is already a wanted using-directive,
+    // if there is, we are done. Otherwise we collect it
+    std::vector<const UsingDirectiveDecl *> AllDirectives;
+    FindSameUsings(ND, Ctx, AllDirectives).TraverseAST(AST);
+    if (AllDirectives.empty()) {
+      UsingToAdd.push_back(ND->getQualifiedNameAsString());
+    }
+  }
+}
+
+// Produce edit adding 'using namespace xxx::yyy;'
+// for user-defined literals declared in an inline namespace
+llvm::Optional<tooling::Replacement>
+addUsingDirectives(const std::vector<std::string> &UsingToAdd,
+                   SourceManager &SM, SourceLocation Loc) {
+  if (UsingToAdd.empty())
+    return llvm::None;
+  std::string Directives;
+  for (auto &Using : UsingToAdd)
+    Directives += llvm::formatv("using namespace {0};\n", Using);
+  // Remove the trailing newline
+  Directives.pop_back();
+  // For now we insert the using-directives to where the first using-directive
+  // to remove stands
+  return tooling::Replacement(SM, Loc, 0, Directives);
+}
+
 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>();
   if (!TargetDirective)
     return false;
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<SourceLocation> IdentsToQualify;
+  // Collect all namespaces' name to add for user-defined literals declared
+  // in an inline namespace
+  std::vector<std::string> UsingToAdd;
   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;
             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.
             //   using namespace std;
             //   auto s = "foo"s; // Must not changed to auto s = "foo" std::s;
-            // FIXME: Add a using-directive for user-defined literals
+            // And add a using-directive for user-defined literals
             // declared in an inline namespace, e.g.
             //   using namespace s^td;
             //   int main() { cout << "foo"s; }
             // change to
-            //   using namespace std::literals;
+            //   using namespace std::string_literals;
             //   int main() { std::cout << "foo"s; }
-            if (Kind == DeclarationName::NameKind::CXXLiteralOperatorName)
+            if (Kind == DeclarationName::NameKind::CXXLiteralOperatorName) {
+              handleUserDefinedLiteral(T->getDeclContext(),
+                                       TargetDirective->getDeclContext(), Ctx,
+                                       UsingToAdd);
               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
findExplicitReferences(
         Inputs.AST->getHeuristicResolver());
   }
   // Remove duplicates.
   llvm::sort(IdentsToQualify);
   IdentsToQualify.erase(
       std::unique(IdentsToQualify.begin(), IdentsToQualify.end()),
       IdentsToQualify.end());
 
+  llvm::sort(UsingToAdd);
+  UsingToAdd.erase(std::unique(UsingToAdd.begin(), UsingToAdd.end()),
+                   UsingToAdd.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);
   }
+  // Produce replacements to add the using directives for user-defined literals
+  if (auto AddUsing =
+          addUsingDirectives(UsingToAdd, SM, FirstUsingDirectiveLoc))
+    if (auto Err = R.add(AddUsing.value()))
+      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"},
+      {// Add using-directives for user-defined literals
+       // declared in an inline namespace
+       R"cpp(
+      namespace ns1 {
+      inline namespace ns2 {
+      long double operator"" _w(long double);
+      }
+      }
+      using namespace n^s1;
+      int main() { 1.5_w; }
+    )cpp",
+       R"cpp(
+      namespace ns1 {
+      inline namespace ns2 {
+      long double operator"" _w(long double);
+      }
+      }
+      using namespace ns1::ns2;
+      int main() { 1.5_w; }
     )cpp"}};
   for (auto C : Cases)
     EXPECT_EQ(C.second, apply(C.first)) << C.first;
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang