[clangd] AddUsing: Used spelled text instead of type name.

[adamcz]

Summary

This improves the behavior related to type aliases, as well as cases of
typo correction.

[kadircet]

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

268

what about saving the full range here, and using it's start and length - name.size() as removal range in apply?

similarly we can also use the range.text as text to insert. that way we can inline getNNSLAsString as it won't be needed elsewhere anymore.

274

it is not that important, but I suppose NameRef is going to be alive during apply, so I suppose we can keep Name as a llvm::StringRef.

clang-tools-extra/clangd/unittests/TweakTests.cpp

2828

Tests don't seem to be covering something like:

namespace foo { namespace bar { struct X {}; } }
using namespace foo;
bar::X^ x;

[adamcz]

addressed review comments

[adamcz]

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

268

We only have a range in this branch of the code. The other one (for DeclRefExpr) is not using TokenBuffers, so no range right now.

We may change it in the future. Or would you prefer this changed now?

274

Right, it used to be NameRef, then in some random version of the code pre-review it had to be a string and I never reverted that. Good point, thanks.

clang-tools-extra/clangd/unittests/TweakTests.cpp

2828

So this has the unfortunate side-effect of triggering a variation of https://github.com/clangd/clangd/issues/585

That's something I'm fixing in a later change (although this case is still not handled). For now, I'm putting an extra namespace scope to make this test correct, while still testing your example.

[kadircet]

thanks, lgtm!

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

268

We only have a range in this branch of the code. The other one (for DeclRefExpr) is not using TokenBuffers, so no range right now.

Ah right, I've forgot that one. My main concern was the extra getSpellingLoc in apply.

We may change it in the future. Or would you prefer this changed now?

No hurries.

clang-tools-extra/clangd/unittests/TweakTests.cpp

2828

sorry for being confusing, I was actually suggesting putting:

namespace foo { namespace bar { struct X {}; } }
using namespace foo;

into the header (to prevent that bug from triggering), and only having the usage in the CC file. but this works too.

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

public:
   bool prepare(const Selection &Inputs) override;
   Expected<Effect> apply(const Selection &Inputs) override;
   std::string title() const override;
   llvm::StringLiteral kind() const override {
     return CodeAction::REFACTOR_KIND;
   }
 
 private:
-  // The qualifier to remove. Set by prepare().
+  // All of the following are set by prepare().
+  // The qualifier to remove.
   NestedNameSpecifierLoc QualifierToRemove;
-  // The name following QualifierToRemove. Set by prepare().
+  // The name following QualifierToRemove.
   llvm::StringRef Name;
 };
 REGISTER_TWEAK(AddUsing)
 
 std::string AddUsing::title() const {
   return std::string(llvm::formatv(
       "Add using-declaration for {0} and remove qualifier", Name));
 }
for (StringRef Banned : Config::current().Style.FullyQualifiedNamespaces) {
     StringRef PrefixMatch = NamespaceStr;
     if (PrefixMatch.consume_front(Banned) && PrefixMatch.consume_front("::"))
       return true;
   }
 
   return false;
 }
 
+std::string getNNSLAsString(NestedNameSpecifierLoc &NNSL,
+                            const PrintingPolicy &Policy) {
+  std::string Out;
+  llvm::raw_string_ostream OutStream(Out);
+  NNSL.getNestedNameSpecifier()->print(OutStream, Policy);
+  return OutStream.str();
+}
+
 bool AddUsing::prepare(const Selection &Inputs) {
   auto &SM = Inputs.AST->getSourceManager();
+  const auto &TB = Inputs.AST->getTokens();
 
   // Do not suggest "using" in header files. That way madness lies.
   if (isHeaderFile(SM.getFileEntryForID(SM.getMainFileID())->getName(),
                    Inputs.AST->getLangOpts()))
     return false;
 
   auto *Node = Inputs.ASTSelection.commonAncestor();
   if (Node == nullptr)
bool AddUsing::prepare(const Selection &Inputs) {
 
   if (auto *D = Node->ASTNode.get<DeclRefExpr>()) {
     if (auto *II = D->getDecl()->getIdentifier()) {
       QualifierToRemove = D->getQualifierLoc();
       Name = II->getName();
     }
   } else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
     if (auto E = T->getAs<ElaboratedTypeLoc>()) {
-      if (auto *BaseTypeIdentifier =
-              E.getType().getUnqualifiedType().getBaseTypeIdentifier()) {
-        Name = BaseTypeIdentifier->getName();
-        QualifierToRemove = E.getQualifierLoc();
-      }
+
+      auto SpelledTokens =
+          TB.spelledForExpanded(TB.expandedTokens(E.getSourceRange()));
+      if (!SpelledTokens)
+        return false;
+      auto SpelledRange = syntax::Token::range(SM, SpelledTokens->front(),
+                                               SpelledTokens->back());
+      Name = SpelledRange.text(SM);
+
+      std::string QualifierToRemoveStr = getNNSLAsString(
+          QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
+      if (!Name.consume_front(QualifierToRemoveStr))
+        return false; // What's spelled doesn't match the qualifier.
     }
   }
 
   // FIXME: This only supports removing qualifiers that are made up of just
   // namespace names. If qualifier contains a type, we could take the longest
   // namespace prefix and remove that.
   if (!QualifierToRemove.hasQualifier() ||
       !QualifierToRemove.getNestedNameSpecifier()->getAsNamespace() ||
       Name.empty()) {
if (SM.isMacroBodyExpansion(QualifierToRemove.getBeginLoc()) ||
     return false;
   }
 
   return true;
 }
 
 Expected<Tweak::Effect> AddUsing::apply(const Selection &Inputs) {
   auto &SM = Inputs.AST->getSourceManager();
-  auto &TB = Inputs.AST->getTokens();
 
-  // Determine the length of the qualifier under the cursor, then remove it.
-  auto SpelledTokens = TB.spelledForExpanded(
-      TB.expandedTokens(QualifierToRemove.getSourceRange()));
-  if (!SpelledTokens) {
-    return error("Could not determine length of the qualifier");
-  }
-  unsigned Length =
-      syntax::Token::range(SM, SpelledTokens->front(), SpelledTokens->back())
-          .length();
+  std::string QualifierToRemoveStr = getNNSLAsString(
+      QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
   tooling::Replacements R;
   if (auto Err = R.add(tooling::Replacement(
-          SM, SpelledTokens->front().location(), Length, ""))) {
+          SM, SM.getSpellingLoc(QualifierToRemove.getBeginLoc()),
+          QualifierToRemoveStr.length(), ""))) {
     return std::move(Err);
   }
 
   auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove, Name);
   if (!InsertionPoint) {
     return InsertionPoint.takeError();
   }
 
   if (InsertionPoint->Loc.isValid()) {
     // Add the using statement at appropriate location.
     std::string UsingText;
     llvm::raw_string_ostream UsingTextStream(UsingText);
     UsingTextStream << "using ";
     if (InsertionPoint->AlwaysFullyQualify &&
         !isFullyQualified(QualifierToRemove.getNestedNameSpecifier()))
       UsingTextStream << "::";
-    QualifierToRemove.getNestedNameSpecifier()->print(
-        UsingTextStream, Inputs.AST->getASTContext().getPrintingPolicy());
-    UsingTextStream << Name << ";" << InsertionPoint->Suffix;
+    UsingTextStream << QualifierToRemoveStr << Name << ";"
+                    << InsertionPoint->Suffix;
 
     assert(SM.getFileID(InsertionPoint->Loc) == SM.getMainFileID());
     if (auto Err = R.add(tooling::Replacement(SM, InsertionPoint->Loc, 0,
                                               UsingTextStream.str()))) {
       return std::move(Err);
     }
   }
 
   return Effect::mainFileEdit(Inputs.AST->getASTContext().getSourceManager(),
                               std::move(R));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/TweakTests.cpp

public:
   // Test that we don't crash or misbehave on unnamed DeclRefExpr.
   EXPECT_UNAVAILABLE(Header +
                      "void fun() { one::two::cc() ^| one::two::cc(); }");
   // Do not offer code action when operating on a banned namespace.
   EXPECT_UNAVAILABLE(Header + "void fun() { ban::fo^o(); }");
   EXPECT_UNAVAILABLE(Header + "void fun() { ::ban::fo^o(); }");
   EXPECT_AVAILABLE(Header + "void fun() { banana::fo^o(); }");
 
+  // Do not offer code action on typo-corrections.
+  EXPECT_UNAVAILABLE(Header + "/*error-ok*/c^c C;");
+
   // Check that we do not trigger in header files.
   FileName = "test.h";
   ExtraArgs.push_back("-xc++-header"); // .h file is treated a C by default.
   EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
   FileName = "test.hpp";
   EXPECT_UNAVAILABLE(Header + "void fun() { one::two::f^f(); }");
 }
 
one::two::f^f();
              R"cpp(
 #include "test.hpp"
 
 using ::one::two::cc;
 using one::two::ff;using one::two::ee;
 
 void fun() {
   ff();
+})cpp"},
+            // using alias; insert using for the spelled name.
+            {R"cpp(
+#include "test.hpp"
+
+void fun() {
+  one::u^u u;
+})cpp",
+             R"cpp(
+#include "test.hpp"
+
+using one::uu;
+
+void fun() {
+  uu u;
+})cpp"},
+            // using namespace.
+            {R"cpp(
+#include "test.hpp"
+using namespace one;
+namespace {
+two::c^c C;
+})cpp",
+             R"cpp(
+#include "test.hpp"
+using namespace one;
+namespace {using two::cc;
+
+cc C;
 })cpp"}};
   llvm::StringMap<std::string> EditedFiles;
   for (const auto &Case : Cases) {
     for (const auto &SubCase : expandCases(Case.TestSource)) {
       ExtraFiles["test.hpp"] = R"cpp(
 namespace one {
 void oo() {}
 namespace two {
 enum ee {};
 void ff() {}
 class cc {
 public:
   struct st { struct nested {}; };
   static void mm() {}
 };
 }
+using uu = two::cc;
 })cpp";
       EXPECT_EQ(apply(SubCase, &EditedFiles), Case.ExpectedSource);
     }
   }
 }
 
 TWEAK_TEST(PopulateSwitch);
 TEST_F(PopulateSwitchTest, Test) {