[clangd] A code action to qualify an unqualified name

[ilya-biryukov]

• Add the code to actually instantiate a code action

[ilya-biryukov]

• Add some forgotten helpers

[ilya-biryukov]

This is a somewhat simple action to illustrate the use of code action APIs.
Still missing tests and trying to figure out what information we want to expose in order to avoid walking over ASTs in each of the actions, so this is not final. Should be a good reference point for how the code of the action would look like, though.

[jkorous]

Hi Ilya, I got here from reading your other patch (https://reviews.llvm.org/D56611). I'm wondering if we could make those range utility functions more understandable.

clangd/SourceCode.h

64 ↗

(On Diff #181312)

It seems to me the range is actually closed on both sides.
Do I get it right that the result is?

[begin of first token : end of last token]

Wouldn't some name like toWholeTokenRange be easier to understand?

81 ↗

(On Diff #181312)

I'd find it helpful to mention that the range is actually interpreted as closed-open (knowing which half is which).

[ilya-biryukov]

• Update after changes to parent revision

[ilya-biryukov]

clangd/SourceCode.h

64 ↗

(On Diff #181312)

The range is supposed to be half-open to allow representing points as empty range starting at the specified position.
This literally represents two byte offsets inside the same file.

My plan is to introduce a class to capture this abstraction, but let me see how the review goes.

81 ↗

(On Diff #181312)

Good point, will do.

[ilya-biryukov]

• Rebase after parent change

[ilya-biryukov]

• Update to reflect changes in parent revision

[ilya-biryukov]

• Remove the header file

[ilya-biryukov]

• Move to the monorepo
• Move out the source code helpers (they're now in swap-if-branches)

[JonasToth]

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

[ilya-biryukov]

In D56610#1363408, @JonasToth wrote:

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

No, this action is not about adding a type qualifier (like const or volatile), it adds a namespace qualifier, e.g.

using namespace std;

vector<int> foo; // --> std::vector<int> foo;

Moreover, it's just an example to illustrate how one could write a simple action like that in clangd.

[JonasToth]

In D56610#1363461, @ilya-biryukov wrote:

In D56610#1363408, @JonasToth wrote:

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

No, this action is not about adding a type qualifier (like const or volatile), it adds a namespace qualifier, e.g.

using namespace std;

vector<int> foo; // --> std::vector<int> foo;

Moreover, it's just an example to illustrate how one could write a simple action like that in clangd.

I see, nvmd then :)

[ilya-biryukov]

• Rebase

[ilya-biryukov]

• Update license header

[sammccall]

(Grabbing this as discussed offline)

[sammccall]

Update matching to use Inputs.ASTSelection.
Cover some more cases of names (I think?)

Handle under-qualified names as well as unqualified ones.
The main benefit of this is it's a step closer to extracting all references to
qualified/unqualified names from the selection.

[hokein]

You'd need to rebase this patch, D57739 had some changes to the Tweak API.

clangd/Selection.h

struct Node {
     // The parent within the selection tree. nullptr for TranslationUnitDecl.
     Node *Parent;
     // Direct children within the selection tree.
     llvm::SmallVector<const Node *, 8> Children;
     // The corresponding node from the full AST.
     ast_type_traits::DynTypedNode ASTNode;
     // The extent to which this node is covered by the selection.
     Selection Selected;
+    // The DeclContext that is the lexical parent of this node.
+    const DeclContext *lexicalContext() const;
   };
 
   // The most specific common ancestor of all the selected nodes.
   // If there is no selection, this is nullptr.
   const Node *commonAncestor() const;
   // The selection node corresponding to TranslationUnitDecl.
   // If there is no selection, this is nullptr.
   const Node *root() const { return Root; }

clangd/Selection.cpp

public:
   bool TraverseDecl(Decl *X) {
     if (isa<TranslationUnitDecl>(X))
       return Base::TraverseDecl(X); // Already pushed by constructor.
     return traverseNode(X, [&] { return Base::TraverseDecl(X); });
   }
   bool TraverseTypeLoc(TypeLoc X) {
     return traverseNode(&X, [&] { return Base::TraverseTypeLoc(X); });
   }
-  bool TraverseTypeNestedNameSpecifierLoc(NestedNameSpecifierLoc X) {
+  bool TraverseNestedNameSpecifierLoc(NestedNameSpecifierLoc X) {
     return traverseNode(
         &X, [&] { return Base::TraverseNestedNameSpecifierLoc(X); });
   }
   bool TraverseConstructorInitializer(CXXCtorInitializer *X) {
     return traverseNode(
         X, [&] { return Base::TraverseConstructorInitializer(X); });
   }
   // Stmt is the same, but this form allows the data recursion optimization.
const Node *SelectionTree::commonAncestor() const {
   for (const Node *Ancestor = Root;; Ancestor = Ancestor->Children.front()) {
     if (Ancestor->Selected || Ancestor->Children.size() > 1)
       return Ancestor;
     // The tree only contains ancestors of the interesting nodes.
     assert(!Ancestor->Children.empty() && "bad node in selection tree");
   }
 }
 
+const DeclContext* SelectionTree::Node::lexicalContext() const {
+  if (!Parent) // Must be TUDecl.
+    return cast<DeclContext>(ASTNode.get<Decl>());
+  for (const Node *N = Parent; N; N = N->Parent)
+    if (auto *DC = dyn_cast_or_null<DeclContext>(N->ASTNode.get<Decl>()))
+      return DC;
+  llvm_unreachable("No DeclContext in parent chain!");
+}
+
 } // namespace clangd
 } // namespace clang

clangd/refactor/tweaks/CMakeLists.txt

 include_directories(${CMAKE_CURRENT_SOURCE_DIR}/../..)
 
 set(LLVM_LINK_COMPONENTS
   support
   )
 
 # A target containing all code tweaks (i.e. mini-refactorings) provided by
 # clangd.
 # Built as an object library to make sure linker does not remove global
 # constructors that register individual tweaks in a global registry.
 # To enable these tweaks in exectubales or shared libraries, add
 # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
   SwapIfBranches.cpp
+  QualifyName.cpp
 
   LINK_LIBS
   clangAST
   clangDaemon
   clangToolingCore
   )

clangd/refactor/tweaks/QualifyName.cpp

+//===--- QualifyName.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 "ClangdUnit.h"
+#include "Logger.h"
+#include "SourceCode.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/Support/Error.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+// TODO: this tweak extracts references to names from the selection in an
+// efficient way. This should be extracted somewhere common.
+
+// Describes the qualifiers implied by a reference to a name in the source code.
+// This check may attempt to make these explicit.
+struct ImpliedQualifier {
+  // The position immediately before the name and any written qualifiers.
+  SourceLocation Begin;
+  // The scope that was implied, that contains the first qualifier or the name.
+  const DeclContext *Implied;
+};
+
+// Returns the qualifier implied before an already-qualified name. e.g.
+//   namespace a::b::c { int D; }
+//   using namespace a;
+//   int X = b::c::D;
+//           ^^^^^^
+// Here a:: could be inserted. It's the enclosing context of namespace b,
+// which is named by the first section of the nested name specifier.
+llvm::Optional<ImpliedQualifier>
+impliedQualifier(NestedNameSpecifierLoc L) {
+  const auto* NNS = L.getNestedNameSpecifier();
+  // Navigate to the unqualified part. (e.g. b:: in b::c::).
+  while (auto Prefix = NNS->getPrefix())
+    NNS = Prefix;
+  // Get the enclosing context of whatever NNS refers to.
+  const DeclContext *DC = nullptr;
+  switch (NNS->getKind()) {
+  case clang::NestedNameSpecifier::Namespace:
+    DC = NNS->getAsNamespace()->getDeclContext();
+    break;
+  case clang::NestedNameSpecifier::NamespaceAlias:
+    DC = NNS->getAsNamespaceAlias()->getDeclContext();
+    break;
+  case clang::NestedNameSpecifier::TypeSpec:
+  case clang::NestedNameSpecifier::TypeSpecWithTemplate: {
+    auto *T = NNS->getAsType();
+    if (auto *TT = dyn_cast<TagType>(T))
+      DC = TT->getDecl()->getDeclContext();
+    break;
+  }
+  case clang::NestedNameSpecifier::Identifier:
+  case clang::NestedNameSpecifier::Super:
+  case clang::NestedNameSpecifier::Global:
+    break;
+  }
+  // The enclosing context is the qualifier to be inserted.
+  if (!DC)
+    return llvm::None;
+  return ImpliedQualifier{L.getBeginLoc(), DC};
+}
+
+// Returns the qualifier to be inserted before a name represented as N.
+// There are two cases:
+//   N is qualified: defer to requiredQualifier(NNSLoc above), it may need more.
+//   N is unqualified: return the DeclContext of the decl it refers to.
+llvm::Optional<ImpliedQualifier>
+impliedQualifier(ast_type_traits::DynTypedNode N,
+                 ast_type_traits::DynTypedNode *Parent) {
+  if (auto *S = N.get<Stmt>()) {
+    if (auto *DRE = dyn_cast<DeclRefExpr>(S)) {
+      if (auto Loc = DRE->getQualifierLoc())
+        return impliedQualifier(Loc);
+      return ImpliedQualifier{DRE->getLocation(),
+                               DRE->getDecl()->getDeclContext()};
+    }
+  }
+  if (auto *D = N.get<Decl>()) {
+    if (auto *U = dyn_cast<UsingDecl>(D))
+      return impliedQualifier(U->getQualifierLoc());
+    if (auto *U = dyn_cast<UsingShadowDecl>(D)) // FIXME: selection bug?
+      return impliedQualifier(U->getUsingDecl()->getQualifierLoc());
+  }
+  if (auto *TL = N.get<TypeLoc>()) {
+    // Qualifiers are stored in the ElaboratedType in the immediate parent.
+    if (Parent)
+      if (auto *PTL = Parent->get<TypeLoc>())
+        if (auto ET = PTL->getAs<ElaboratedTypeLoc>())
+          if (auto Qualifier = ET.getQualifierLoc())
+            return impliedQualifier(Qualifier);
+    // We hit this case if the cursor is on the qualifiers themselves.
+    if (auto ET = TL->getAs<ElaboratedTypeLoc>()) {
+      if (auto Qualifier = ET.getQualifierLoc())
+        return impliedQualifier(Qualifier);
+      return llvm::None;
+    }
+    // Remaining cases handle unqualified names.
+    if (auto TT = TL->getAs<TagTypeLoc>())
+      return ImpliedQualifier{TT.getNameLoc(), TT.getDecl()->getDeclContext()};
+    if (auto TT = TL->getAs<TemplateSpecializationTypeLoc>()) {
+      if (auto *TD = TT.getTypePtr()->getTemplateName().getAsTemplateDecl())
+        return ImpliedQualifier{TT.getTemplateNameLoc(), TD->getDeclContext()};
+      return llvm::None;
+    }
+    if (auto TT = TL->getAs<TypedefTypeLoc>())
+      return ImpliedQualifier{TT.getNameLoc(),
+                              TT.getTypedefNameDecl()->getDeclContext()};
+  }
+  return llvm::None;
+}
+
+/// Fully qualifies a name under a cursor.
+/// Before:
+///   using namespace std;
+///   ^vector<int> foo;
+/// After:
+///   std::vector<int> foo;
+class QualifyName : public Tweak {
+public:
+  const char *id() const override final;
+
+  bool prepare(const Selection &Inputs) override {
+    llvm::Optional<ImpliedQualifier> Qualifier;
+    const DeclContext *Enclosing = nullptr;
+    for (const auto *Node = Inputs.ASTSelection.commonAncestor();
+         Node != nullptr; Node = Node->Parent) {
+      if (auto Q = impliedQualifier(
+              Node->ASTNode, Node->Parent ? &Node->Parent->ASTNode : nullptr)) {
+        Qualifier = std::move(Q);
+        Enclosing = Node->lexicalContext();
+        break;
+      }
+    }
+    if (!Qualifier)
+      return false;
+    // Traverse upward, printing the context until reaching an enclosing one.
+    for (const DeclContext *DC = Qualifier->Implied; DC; DC = DC->getParent()) {
+      if (!DC->isTransparentContext() && !DC->isNamespace() &&
+          !DC->isTranslationUnit()) // Skip hard cases.
+        return false;
+    }
+    // Don't add prefixes of the current namespace.
+    if (Qualifier->Implied->Encloses(Enclosing))
+      return false;
+    this->InsertLoc = Qualifier->Begin;
+    this->Qualifier = printNamespaceScope(*Qualifier->Implied);
+    return !this->Qualifier.empty();
+  }
+
+  std::string title() const override {
+    return llvm::formatv("Add '{0}' qualifier", Qualifier);
+  }
+
+  Expected<tooling::Replacements> apply(const Selection &Inputs) override {
+    return tooling::Replacements(tooling::Replacement(
+        Inputs.AST.getASTContext().getSourceManager(), InsertLoc, 0,
+        Qualifier));
+  }
+
+private:
+  SourceLocation InsertLoc;
+  std::string Qualifier;
+};
+
+REGISTER_TWEAK(QualifyName);
+
+} // namespace
+} // namespace clangd
+} // namespace clang

unittests/clangd/SelectionTests.cpp

for (const SelectionTree::Node *N : allNodes(T))
         Complete.push_back(nodeRange(N, AST));
       else if (N->Selected == SelectionTree::Partial)
         Partial.push_back(nodeRange(N, AST));
     EXPECT_THAT(Complete, UnorderedElementsAreArray(Test.ranges("C"))) << C;
     EXPECT_THAT(Partial, UnorderedElementsAreArray(Test.ranges())) << C;
   }
 }
 
+TEST(SelectionTest, Context) {
+  const char *C = "namespace a { int ^x; }";
+  Annotations Test(C);
+  auto AST = TestTU::withCode(Test.code()).build();
+  auto T = makeSelectionTree(C, AST);
+
+  ASSERT_TRUE(T.commonAncestor());
+  EXPECT_EQ(T.commonAncestor()->lexicalContext(),
+            findDecl(AST, "a::x").getLexicalDeclContext());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

unittests/clangd/TweakTests.cpp

checkNotAvailable(ID, R"cpp(
     }
   )cpp");
   // Only one if statement is supported!
   checkNotAvailable(ID, R"cpp(
     [[if(1){}else{}if(2){}else{}]]
   )cpp");
 }
 
+TEST(TweakTest, QualifyName) {
+  llvm::StringLiteral ID = "QualifyName";
+
+  const char *Input = R"cpp(
+    namespace a { namespace b { namespace c { int D; } } }
+    using namespace a;
+    int X = ^b::c::D;
+  )cpp";
+  const char *Output = R"cpp(
+    namespace a { namespace b { namespace c { int D; } } }
+    using namespace a;
+    int X = a::b::c::D;
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  Input = R"cpp(
+    namespace std { template <typename> class vector; }
+    using namespace std;
+    vector<^vector<int>>* v;
+  )cpp";
+  // FIXME: Outer vector is qualified rather than inner, because Selection
+  // doesn't include located template parameters.
+  Output = R"cpp(
+    namespace std { template <typename> class vector; }
+    using namespace std;
+    std::vector<vector<int>>* v;
+  )cpp";
+  checkTransform(ID, Input, Output);
+
+  checkAvailable(ID, R"cpp(
+    namespace a { namespace b { namespace c { int D; class E{}; } } }
+    using namespace a;
+    ^using ^b::^c::^D;
+    int X = ^b::^c::^D;
+    ^b::^c::^E instance;
+  )cpp");
+  checkNotAvailable(ID, R"cpp(
+    namespace a { namespace b { namespace c { int D; class E{}; } } }
+    using namespace a;
+    using b::c::D^;
+    int ^X = b::c::D;
+    b::c::E ^instance;
+  )cpp");
+  checkNotAvailable(ID, R"cpp(
+    namespace a { namespace b { namespace c { int D; class E{}; } } }
+    namespace { int x; }
+    using namespace a;
+    // Don't qualify fully-qualified things.
+    using ^a::^b::c::^D;
+    int y = ^x;
+
+    // Don't add a:: while in namespace a.
+    namespace a { namespace z { int X = ^b::c::^D; } }
+  )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang