[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

130

nit: const auto

276

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

275–279

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

303

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

303

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

303

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.

303

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

303

You're right. Sorry for misunderstanding it.

303

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

 //===--- 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 "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
+#include "clang/AST/ParentMapContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Tooling/Core/Replacement.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 /// Removes the 'using namespace' under the cursor and qualifies all accesses in

 // enum E { a, b, c, d };
 // }
 const DeclContext *visibleContext(const DeclContext *D) {
   while (D->isInlineNamespace() || D->isTransparentContext())
     D = D->getParent();
   return D;
 }
 
+using UserDefinedLiteralSet = llvm::SmallPtrSet<const NamedDecl *, 2>;
+
+void handleUserDefinedLiterals(
+    Decl *D, const UserDefinedLiteralSet &Literals,
+    std::map<SourceLocation, UserDefinedLiteralSet> &Result, ASTContext &Ctx) {
+  struct Visitor : RecursiveASTVisitor<Visitor> {
+    Visitor(const UserDefinedLiteralSet &Literals,
+            std::map<SourceLocation, UserDefinedLiteralSet> &Result,
+            ASTContext &Ctx)
+        : Literals{Literals}, Result{Result}, Ctx{Ctx} {}
+
+    const UserDefinedLiteralSet &Literals;
+    std::map<SourceLocation, UserDefinedLiteralSet> &Result;
+    ASTContext &Ctx;
+
+    bool VisitDeclRefExpr(DeclRefExpr *DRE) {
+      const NamedDecl *ND = DRE->getFoundDecl();
+      // If DRE references to a user-defined literal
+      if (ND->getDeclName().getNameKind() ==
+          DeclarationName::CXXLiteralOperatorName) {
+        for (const NamedDecl *Literal : Literals) {
+          if (ND == Literal) {
+            // If the user-defined literal locates in a CompoundStmt,
+            // we mark the CompoundStmt so that we can add the using-declaration
+            // in it later. If not, the user-defined literal is used in the
+            // top-level, it is reasonable to do nothing for it.
+            if (const CompoundStmt *CS = getCompoundParent(DRE))
+              Result[CS->getLBracLoc()].insert(ND);
+            return true;
+          }
+        }
+      }
+      return true;
+    }
+
+    // Get the closet CompoundStmt parent of DRE
+    const CompoundStmt *getCompoundParent(DeclRefExpr *DRE) {
+      DynTypedNodeList Parents = Ctx.getParents(*DRE);
+      llvm::SmallVector<DynTypedNode, 1> NodesToProcess{Parents.begin(),
+                                                        Parents.end()};
+      while (!NodesToProcess.empty()) {
+        DynTypedNode Node = NodesToProcess.back();
+        NodesToProcess.pop_back();
+        if (const auto *CS = Node.get<CompoundStmt>())
+          return CS;
+        Parents = Ctx.getParents(Node);
+        NodesToProcess.append(Parents.begin(), Parents.end());
+      }
+      return nullptr;
+    }
+  };
+
+  Visitor V{Literals, Result, Ctx};
+  V.TraverseDecl(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<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 user-defined literals from the namespace for which we're
+  // removing the directive
+  std::map<SourceLocation, UserDefinedLiteralSet> LiteralsToUsing;
+
   for (auto &D : Inputs.AST->getLocalTopLevelDecls()) {
+    if (SM.isBeforeInTranslationUnit(D->getBeginLoc(), FirstUsingDirectiveLoc))
+      continue; // Directive was not visible before this point.
+
+    UserDefinedLiteralSet Literals;
     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
-            // declared in an inline namespace, e.g.
-            //   using namespace s^td;
-            //   int main() { cout << "foo"s; }
-            // change to
-            //   using namespace std::literals;
-            //   int main() { std::cout << "foo"s; }
-            if (Kind == DeclarationName::NameKind::CXXLiteralOperatorName)
+            // Also add a using-declaration to keep codes well-formed, e.g.
+            //   using namespace std;
+            //   int main() {
+            //     auto s = "foo"s;
+            //   }
+            // will change to
+            //   int main() {
+            //     using std::operator""s;
+            //     auto s = "foo"s;
+            //   }
+            if (Kind == DeclarationName::NameKind::CXXLiteralOperatorName) {
+              // Can't get the AST node from the ReferenceLoc, we have to
+              // traverse D later to handle these user-defined literals
+              Literals.insert(T);
               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);
         },
         Inputs.AST->getHeuristicResolver());
+    if (!Literals.empty())
+      handleUserDefinedLiterals(D, Literals, LiteralsToUsing, Ctx);
   }
   // 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,
+    if (auto Err = R.add(tooling::Replacement(SM, Loc,
                                               /*Length=*/0, Qualifier)))
       return std::move(Err);
   }
+  // Produce replacements to add the using-declarations for user-defined
+  // literals
+  for (auto &[Loc, Literals] : LiteralsToUsing) {
+    std::string UsingDeclarations;
+    for (const auto *L : Literals)
+      UsingDeclarations +=
+          llvm::formatv("using {0};\n", L->getQualifiedNameAsString());
+    UsingDeclarations.pop_back();
+    if (auto Err = R.add(tooling::Replacement(SM, Loc.getLocWithOffset(1), 0,
+                                              UsingDeclarations)))
+      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",
       void operator+(const Foo &, int) {}
       }
       
       int main() {
         ns::Foo foo;
         foo + 10;
       }
     )cpp"},
-      {// Does not qualify user-defined literals
+      {
           R"cpp(
-      namespace ns {
-      long double operator "" _w(long double);
+      namespace a {
+      long double operator""_a(long double);
+      inline namespace b {
+      long double operator""_b(long double);
+      } // namespace b
+      } // namespace a
+      using namespace ^a;
+      int main() {
+        1.0_a;
+        1.0_b;
       }
-      using namespace n^s;
-      int main() { 1.5_w; }
     )cpp",
           R"cpp(
-      namespace ns {
-      long double operator "" _w(long double);
+      namespace a {
+      long double operator""_a(long double);
+      inline namespace b {
+      long double operator""_b(long double);
+      } // namespace b
+      } // namespace a
+      
+      int main() {using a::operator""_a;
+using a::operator""_b;
+        1.0_a;
+        1.0_b;
       }
-      
-      int main() { 1.5_w; }
     )cpp"}};
   for (auto C : Cases)
     EXPECT_EQ(C.second, apply(C.first)) << C.first;
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang