[clangd] Fix AddUsing in the face of typo-correction

[kadircet]

Summary

Fixes https://github.com/clangd/clangd/issues/559

[hokein]

Thanks!

[thakis]

This breaks tests on windows: http://45.33.8.238/win/76177/step_9.txt

Please take a look and revert for now if it takes a while to fix.

[kadircet]

Disable msvc compatibility to enable typo-correction on windows.

[kadircet]

relanded in 35c2aac6e3957c2e82bf92269039fa02bab0e1d9

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

 //===--- AddUsing.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 "Config.h"
+#include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "clang/Tooling/Syntax/Tokens.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/raw_ostream.h"
+#include <string>
+#include <tuple>
+#include <utility>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Tweak for removing full namespace qualifier under cursor on DeclRefExpr and
 // types and adding "using" statement instead.
 //
public:
   llvm::StringLiteral kind() const override {
     return CodeAction::REFACTOR_KIND;
   }
 
 private:
   // All of the following are set by prepare().
   // The qualifier to remove.
   NestedNameSpecifierLoc QualifierToRemove;
-  // The name following QualifierToRemove.
-  llvm::StringRef Name;
+  // Qualified name to use when spelling the using declaration. This might be
+  // different than SpelledQualifier in presence of error correction.
+  std::string QualifierToSpell;
+  // The name and qualifier as spelled in the code.
+  llvm::StringRef SpelledQualifier;
+  llvm::StringRef SpelledName;
   // If valid, the insertion point for "using" statement must come after this.
   // This is relevant when the type is defined in the main file, to make sure
   // the type/function is already defined at the point where "using" is added.
   SourceLocation MustInsertAfterLoc;
 };
 REGISTER_TWEAK(AddUsing)
 
 std::string AddUsing::title() const {
   return std::string(llvm::formatv(
-      "Add using-declaration for {0} and remove qualifier", Name));
+      "Add using-declaration for {0} and remove qualifier", SpelledName));
 }
 
 // Locates all "using" statements relevant to SelectionDeclContext.
 class UsingFinder : public RecursiveASTVisitor<UsingFinder> {
 public:
   UsingFinder(std::vector<const UsingDecl *> &Results,
               const DeclContext *SelectionDeclContext, const SourceManager &SM)
       : Results(Results), SelectionDeclContext(SelectionDeclContext), SM(SM) {}
if (auto *T = Node->ASTNode.get<TypeLoc>()) {
         continue;
       }
     }
     break;
   }
   if (Node == nullptr)
     return false;
 
+  SourceRange SpelledNameRange;
   if (auto *D = Node->ASTNode.get<DeclRefExpr>()) {
     if (auto *II = D->getDecl()->getIdentifier()) {
       QualifierToRemove = D->getQualifierLoc();
-      Name = II->getName();
+      SpelledNameRange = D->getSourceRange();
       MustInsertAfterLoc = D->getDecl()->getBeginLoc();
     }
   } else if (auto *T = Node->ASTNode.get<TypeLoc>()) {
     if (auto E = T->getAs<ElaboratedTypeLoc>()) {
       QualifierToRemove = E.getQualifierLoc();
-      if (!QualifierToRemove)
-        return false;
 
-      auto NameRange = E.getSourceRange();
+      SpelledNameRange = E.getSourceRange();
       if (auto T = E.getNamedTypeLoc().getAs<TemplateSpecializationTypeLoc>()) {
         // Remove the template arguments from the name.
-        NameRange.setEnd(T.getLAngleLoc().getLocWithOffset(-1));
+        SpelledNameRange.setEnd(T.getLAngleLoc().getLocWithOffset(-1));
       }
 
-      auto SpelledTokens = TB.spelledForExpanded(TB.expandedTokens(NameRange));
-      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.
-
       if (const auto *ET = E.getTypePtr()) {
         if (const auto *TDT =
                 dyn_cast<TypedefType>(ET->getNamedType().getTypePtr())) {
           MustInsertAfterLoc = TDT->getDecl()->getBeginLoc();
         } else if (auto *TD = ET->getAsTagDecl()) {
           MustInsertAfterLoc = TD->getBeginLoc();
         }
       }
     }
   }
-
+  if (!QualifierToRemove ||
       // 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() ||
+      // namespace names. If qualifier contains a type, we could take the
+      // longest namespace prefix and remove that.
       !QualifierToRemove.getNestedNameSpecifier()->getAsNamespace() ||
-      Name.empty()) {
+      // Respect user config.
+      isNamespaceForbidden(Inputs, *QualifierToRemove.getNestedNameSpecifier()))
     return false;
-  }
-
-  if (isNamespaceForbidden(Inputs, *QualifierToRemove.getNestedNameSpecifier()))
-    return false;
-
   // Macros are difficult. We only want to offer code action when what's spelled
   // under the cursor is a namespace qualifier. If it's a macro that expands to
   // a qualifier, user would not know what code action will actually change.
   // On the other hand, if the qualifier is part of the macro argument, we
   // should still support that.
   if (SM.isMacroBodyExpansion(QualifierToRemove.getBeginLoc()) ||
       !SM.isWrittenInSameFile(QualifierToRemove.getBeginLoc(),
                               QualifierToRemove.getEndLoc())) {
     return false;
   }
 
+  auto SpelledTokens =
+      TB.spelledForExpanded(TB.expandedTokens(SpelledNameRange));
+  if (!SpelledTokens)
+    return false;
+  auto SpelledRange =
+      syntax::Token::range(SM, SpelledTokens->front(), SpelledTokens->back());
+  // We only drop qualifiers that're namespaces, so this is safe.
+  std::tie(SpelledQualifier, SpelledName) =
+      splitQualifiedName(SpelledRange.text(SM));
+  QualifierToSpell = getNNSLAsString(
+      QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
+  if (!llvm::StringRef(QualifierToSpell).endswith(SpelledQualifier) ||
+      SpelledName.empty())
+    return false; // What's spelled doesn't match the qualifier.
   return true;
 }
 
 Expected<Tweak::Effect> AddUsing::apply(const Selection &Inputs) {
   auto &SM = Inputs.AST->getSourceManager();
 
-  std::string QualifierToRemoveStr = getNNSLAsString(
-      QualifierToRemove, Inputs.AST->getASTContext().getPrintingPolicy());
   tooling::Replacements R;
   if (auto Err = R.add(tooling::Replacement(
           SM, SM.getSpellingLoc(QualifierToRemove.getBeginLoc()),
-          QualifierToRemoveStr.length(), ""))) {
+          SpelledQualifier.size(), ""))) {
     return std::move(Err);
   }
 
-  auto InsertionPoint =
-      findInsertionPoint(Inputs, QualifierToRemove, Name, MustInsertAfterLoc);
+  auto InsertionPoint = findInsertionPoint(Inputs, QualifierToRemove,
+                                           SpelledName, MustInsertAfterLoc);
   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 << "::";
-    UsingTextStream << QualifierToRemoveStr << Name << ";"
+    UsingTextStream << QualifierToSpell << SpelledName << ";"
                     << 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/tweaks/AddUsingTests.cpp

 //===-- AddUsingTests.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 "Config.h"
 #include "TweakTesting.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 TWEAK_TEST(AddUsing);
 
 TEST_F(AddUsingTest, Prepare) {
   Config Cfg;
   Cfg.Style.FullyQualifiedNamespaces.push_back("ban");
   WithContextValue WithConfig(Config::Key, std::move(Cfg));
 
   const std::string Header = R"cpp(
 #define NS(name) one::two::name
 namespace ban { void foo() {} }
 namespace banana { void foo() {} }
 namespace one {
 void oo() {}
 template<typename TT> class tt {};
 namespace two {
-enum ee {};
+enum ee { ee_enum_value };
 void ff() {}
 class cc {
 public:
   struct st {};
   static void mm() {}
   cc operator|(const cc& x) const { return x; }
 };
 }
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;");
-
   // NestedNameSpecifier, but no namespace.
   EXPECT_UNAVAILABLE(Header + "class Foo {}; class F^oo foo;");
 
   // 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";

 #include "test.hpp"
 one::v^ec<int> foo;
 )cpp",
              R"cpp(
 #include "test.hpp"
 using one::vec;
 
 vec<int> foo;
-)cpp"}};
+)cpp"},
+      // Typo correction.
+      {R"cpp(
+// error-ok
+#include "test.hpp"
+c^c C;
+)cpp",
+       R"cpp(
+// error-ok
+#include "test.hpp"
+using one::two::cc;
+
+cc C;
+)cpp"},
+      {R"cpp(
+// error-ok
+#include "test.hpp"
+void foo() {
+  switch(one::two::ee{}) { case two::ee_^one:break; }
+}
+)cpp",
+       R"cpp(
+// error-ok
+#include "test.hpp"
+using one::two::ee_one;
+
+void foo() {
+  switch(one::two::ee{}) { case ee_one:break; }
+}
+)cpp"},
+  };
   llvm::StringMap<std::string> EditedFiles;
   for (const auto &Case : Cases) {
     ExtraFiles["test.hpp"] = R"cpp(
 namespace one {
 void oo() {}
 namespace two {
 enum ee {ee_one};
 void ff() {}
 class cc {
 public:
   struct st { struct nested {}; };
   static void mm() {}
 };
 }
 using uu = two::cc;
 template<typename T> struct vec {};
 })cpp";
+    // Typo correction is disabled in msvc-compatibility mode.
+    ExtraArgs.push_back("-fno-ms-compatibility");
     EXPECT_EQ(apply(Case.TestSource, &EditedFiles), Case.ExpectedSource);
   }
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang