[clang-tools-extra] r354330 - [clangd] Handle unresolved scope specifier when fixing includes.

Eric Liu via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 19 06:32:22 PST 2019


Author: ioeric
Date: Tue Feb 19 06:32:22 2019
New Revision: 354330

URL: http://llvm.org/viewvc/llvm-project?rev=354330&view=rev
Log:
[clangd] Handle unresolved scope specifier when fixing includes.

Summary:
In the following examples, "clangd" is unresolved, and the fixer will try to fix
include for `clang::clangd`; however, clang::clangd::X is usually intended. So
when handling a qualifier that is unresolved, we change the unresolved name and
scopes so that the fixer will fix "clang::clangd::X" in the following example.
```
  namespace clang {
  	clangd::X
	~~~~~~
  }
  // or
  clang::clangd::X
         ~~~~~~
```

Reviewers: sammccall

Reviewed By: sammccall

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, jdoerfert, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D58185

Modified:
    clang-tools-extra/trunk/clangd/IncludeFixer.cpp
    clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp

Modified: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/IncludeFixer.cpp?rev=354330&r1=354329&r2=354330&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp (original)
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp Tue Feb 19 06:32:22 2019
@@ -19,6 +19,10 @@
 #include "clang/AST/Type.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
 #include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Scope.h"
@@ -28,6 +32,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FormatVariadic.h"
@@ -172,6 +177,121 @@ std::vector<Fix> IncludeFixer::fixesForS
   }
   return Fixes;
 }
+
+// Returns the identifiers qualified by an unresolved name. \p Loc is the
+// start location of the unresolved name. For the example below, this returns
+// "::X::Y" that is qualified by unresolved name "clangd":
+//     clang::clangd::X::Y
+//            ~
+llvm::Optional<std::string> qualifiedByUnresolved(const SourceManager &SM,
+                                                  SourceLocation Loc,
+                                                  const LangOptions &LangOpts) {
+  std::string Result;
+
+  SourceLocation NextLoc = Loc;
+  while (auto CCTok = Lexer::findNextToken(NextLoc, SM, LangOpts)) {
+    if (!CCTok->is(tok::coloncolon))
+      break;
+    auto IDTok = Lexer::findNextToken(CCTok->getLocation(), SM, LangOpts);
+    if (!IDTok || !IDTok->is(tok::raw_identifier))
+      break;
+    Result.append(("::" + IDTok->getRawIdentifier()).str());
+    NextLoc = IDTok->getLocation();
+  }
+  if (Result.empty())
+    return llvm::None;
+  return Result;
+}
+
+// An unresolved name and its scope information that can be extracted cheaply.
+struct CheapUnresolvedName {
+  std::string Name;
+  // This is the part of what was typed that was resolved, and it's in its
+  // resolved form not its typed form (think `namespace clang { clangd::x }` -->
+  // `clang::clangd::`).
+  llvm::Optional<std::string> ResolvedScope;
+
+  // Unresolved part of the scope. When the unresolved name is a specifier, we
+  // use the name that comes after it as the alternative name to resolve and use
+  // the specifier as the extra scope in the accessible scopes.
+  llvm::Optional<std::string> UnresolvedScope;
+};
+
+// Extracts unresolved name and scope information around \p Unresolved.
+// FIXME: try to merge this with the scope-wrangling code in CodeComplete.
+llvm::Optional<CheapUnresolvedName> extractUnresolvedNameCheaply(
+    const SourceManager &SM, const DeclarationNameInfo &Unresolved,
+    CXXScopeSpec *SS, const LangOptions &LangOpts, bool UnresolvedIsSpecifier) {
+  bool Invalid = false;
+  llvm::StringRef Code = SM.getBufferData(
+      SM.getDecomposedLoc(Unresolved.getBeginLoc()).first, &Invalid);
+  if (Invalid)
+    return llvm::None;
+  CheapUnresolvedName Result;
+  Result.Name = Unresolved.getAsString();
+  if (SS && SS->isNotEmpty()) { // "::" or "ns::"
+    if (auto *Nested = SS->getScopeRep()) {
+      if (Nested->getKind() == NestedNameSpecifier::Global)
+        Result.ResolvedScope = "";
+      else if (const auto *NS = Nested->getAsNamespace()) {
+        auto SpecifiedNS = printNamespaceScope(*NS);
+
+        // Check the specifier spelled in the source.
+        // If the resolved scope doesn't end with the spelled scope. The
+        // resolved scope can come from a sema typo correction. For example,
+        // sema assumes that "clangd::" is a typo of "clang::" and uses
+        // "clang::" as the specified scope in:
+        //     namespace clang { clangd::X; }
+        // In this case, we use the "typo" specifier as extra scope instead
+        // of using the scope assumed by sema.
+        auto B = SM.getFileOffset(SS->getBeginLoc());
+        auto E = SM.getFileOffset(SS->getEndLoc());
+        std::string Spelling = (Code.substr(B, E - B) + "::").str();
+        if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
+          Result.ResolvedScope = SpecifiedNS;
+        else
+          Result.UnresolvedScope = Spelling;
+      } else if (const auto *ANS = Nested->getAsNamespaceAlias()) {
+        Result.ResolvedScope = printNamespaceScope(*ANS->getNamespace());
+      } else {
+        // We don't fix symbols in scopes that are not top-level e.g. class
+        // members, as we don't collect includes for them.
+        return llvm::None;
+      }
+    }
+  }
+
+  if (UnresolvedIsSpecifier) {
+    // If the unresolved name is a specifier e.g.
+    //      clang::clangd::X
+    //             ~~~~~~
+    // We try to resolve clang::clangd::X instead of clang::clangd.
+    // FIXME: We won't be able to fix include if the specifier is what we
+    // should resolve (e.g. it's a class scope specifier). Collecting include
+    // headers for nested types could make this work.
+
+    // Not using the end location as it doesn't always point to the end of
+    // identifier.
+    if (auto QualifiedByUnresolved =
+            qualifiedByUnresolved(SM, Unresolved.getBeginLoc(), LangOpts)) {
+      auto Split = splitQualifiedName(*QualifiedByUnresolved);
+      if (!Result.UnresolvedScope)
+        Result.UnresolvedScope.emplace();
+      // If UnresolvedSpecifiedScope is already set, we simply append the
+      // extra scope. Suppose the unresolved name is "index" in the following
+      // example:
+      //   namespace clang {  clangd::index::X; }
+      //                      ~~~~~~  ~~~~~
+      // "clangd::" is assumed to be clang:: by Sema, and we would have used
+      // it as extra scope. With "index" being a specifier, we append "index::"
+      // to the extra scope.
+      Result.UnresolvedScope->append((Result.Name + Split.first).str());
+      Result.Name = Split.second;
+    }
+  }
+  return Result;
+}
+
 class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource {
 public:
   UnresolvedNameRecorder(llvm::Optional<UnresolvedName> &LastUnresolvedName)
@@ -192,51 +312,30 @@ public:
     if (!SemaPtr->SourceMgr.isWrittenInMainFile(Typo.getLoc()))
       return clang::TypoCorrection();
 
-    // FIXME: support invalid scope before a type name. In the following
-    // example, namespace "clang::tidy::" hasn't been declared/imported.
-    //    namespace clang {
-    //    void f() {
-    //      tidy::Check c;
-    //      ~~~~
-    //      // or
-    //      clang::tidy::Check c;
-    //             ~~~~
-    //    }
-    //    }
-    // For both cases, the typo and the diagnostic are both on "tidy", and no
-    // diagnostic is generated for "Check". However, what we want to fix is
-    // "clang::tidy::Check".
-
-    // Extract the typed scope. This is not done lazily because `SS` can get
-    // out of scope and it's relatively cheap.
-    llvm::Optional<std::string> SpecifiedScope;
-    if (SS && SS->isNotEmpty()) { // "::" or "ns::"
-      if (auto *Nested = SS->getScopeRep()) {
-        if (Nested->getKind() == NestedNameSpecifier::Global)
-          SpecifiedScope = "";
-        else if (const auto *NS = Nested->getAsNamespace())
-          SpecifiedScope = printNamespaceScope(*NS);
-        else
-          // We don't fix symbols in scopes that are not top-level e.g. class
-          // members, as we don't collect includes for them.
-          return TypoCorrection();
-      }
-    }
-    if (!SpecifiedScope && !S) // Give up if no scope available.
+    // This is not done lazily because `SS` can get out of scope and it's
+    // relatively cheap.
+    auto Extracted = extractUnresolvedNameCheaply(
+        SemaPtr->SourceMgr, Typo, SS, SemaPtr->LangOpts,
+        static_cast<Sema::LookupNameKind>(LookupKind) ==
+            Sema::LookupNameKind::LookupNestedNameSpecifierName);
+    if (!Extracted)
       return TypoCorrection();
-
+    auto CheapUnresolved = std::move(*Extracted);
     UnresolvedName Unresolved;
-    Unresolved.Name = Typo.getAsString();
+    Unresolved.Name = CheapUnresolved.Name;
     Unresolved.Loc = Typo.getBeginLoc();
 
+    if (!CheapUnresolved.ResolvedScope && !S) // Give up if no scope available.
+      return TypoCorrection();
+
     auto *Sem = SemaPtr; // Avoid capturing `this`.
-    Unresolved.GetScopes = [Sem, SpecifiedScope, S, LookupKind]() {
+    Unresolved.GetScopes = [Sem, CheapUnresolved, S, LookupKind]() {
       std::vector<std::string> Scopes;
-      if (SpecifiedScope) {
-        Scopes.push_back(*SpecifiedScope);
+      if (CheapUnresolved.ResolvedScope) {
+        Scopes.push_back(*CheapUnresolved.ResolvedScope);
       } else {
         assert(S);
-        // No scope qualifier is specified. Collect all accessible scopes in the
+        // No scope specifier is specified. Collect all accessible scopes in the
         // context.
         VisitedContextCollector Collector;
         Sem->LookupVisibleDecls(
@@ -249,6 +348,10 @@ public:
           if (isa<NamespaceDecl>(Ctx))
             Scopes.push_back(printNamespaceScope(*Ctx));
       }
+
+      if (CheapUnresolved.UnresolvedScope)
+        for (auto &Scope : Scopes)
+          Scope.append(*CheapUnresolved.UnresolvedScope);
       return Scopes;
     };
     LastUnresolvedName = std::move(Unresolved);

Modified: clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp?rev=354330&r1=354329&r2=354330&view=diff
==============================================================================
--- clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp Tue Feb 19 06:32:22 2019
@@ -20,6 +20,7 @@ namespace clang {
 namespace clangd {
 namespace {
 
+using testing::_;
 using testing::ElementsAre;
 using testing::Field;
 using testing::IsEmpty;
@@ -369,6 +370,8 @@ TEST(IncludeFixerTest, Typo) {
 $insert[[]]namespace ns {
 void foo() {
   $unqualified1[[X]] x;
+  // No fix if the unresolved type is used as specifier. (ns::)X::Nested will be
+  // considered the unresolved type.
   $unqualified2[[X]]::Nested n;
 }
 }
@@ -391,10 +394,7 @@ void bar() {
           AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"),
                 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
                             "Add include \"x.h\" for symbol ns::X"))),
-          AllOf(Diag(Test.range("unqualified2"),
-                     "use of undeclared identifier 'X'"),
-                WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-                            "Add include \"x.h\" for symbol ns::X"))),
+          Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"),
           AllOf(Diag(Test.range("qualified1"),
                      "no type named 'X' in namespace 'ns'"),
                 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
@@ -487,6 +487,88 @@ void bar(X *x) {
   }
 }
 
+TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace ns {
+}
+void g() {  ns::$[[scope]]::X_Y();  }
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+      SymbolWithHeader{"ns::scope::X_Y", "unittest:///x.h", "\"x.h\""});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      UnorderedElementsAre(AllOf(
+          Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
+          WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+                      "Add include \"x.h\" for symbol ns::scope::X_Y")))));
+}
+
+TEST(IncludeFixerTest, UnresolvedSpecifierWithSemaCorrection) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace clang {
+void f() {
+  // "clangd::" will be corrected to "clang::" by Sema.
+  $q1[[clangd]]::$x[[X]] x;
+  $q2[[clangd]]::$ns[[ns]]::Y y;
+}
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+      {SymbolWithHeader{"clang::clangd::X", "unittest:///x.h", "\"x.h\""},
+       SymbolWithHeader{"clang::clangd::ns::Y", "unittest:///y.h", "\"y.h\""}});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      UnorderedElementsAre(
+          AllOf(
+              Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; "
+                                     "did you mean 'clang'?"),
+              WithFix(_, // change clangd to clang
+                      Fix(Test.range("insert"), "#include \"x.h\"\n",
+                          "Add include \"x.h\" for symbol clang::clangd::X"))),
+          AllOf(
+              Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"),
+              WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+                          "Add include \"x.h\" for symbol clang::clangd::X"))),
+          AllOf(
+              Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; "
+                                     "did you mean 'clang'?"),
+              WithFix(
+                  _, // change clangd to clangd
+                  Fix(Test.range("insert"), "#include \"y.h\"\n",
+                      "Add include \"y.h\" for symbol clang::clangd::ns::Y"))),
+          AllOf(Diag(Test.range("ns"),
+                     "no member named 'ns' in namespace 'clang'"),
+                WithFix(Fix(
+                    Test.range("insert"), "#include \"y.h\"\n",
+                    "Add include \"y.h\" for symbol clang::clangd::ns::Y")))));
+}
+
+TEST(IncludeFixerTest, SpecifiedScopeIsNamespaceAlias) {
+  Annotations Test(R"cpp(
+$insert[[]]namespace a {}
+namespace b = a;
+namespace c {
+  b::$[[X]] x;
+}
+  )cpp");
+  auto TU = TestTU::withCode(Test.code());
+  auto Index = buildIndexWithSymbol(
+      SymbolWithHeader{"a::X", "unittest:///x.h", "\"x.h\""});
+  TU.ExternalIndex = Index.get();
+
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(AllOf(
+                  Diag(Test.range(), "no type named 'X' in namespace 'a'"),
+                  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+                              "Add include \"x.h\" for symbol a::X")))));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang




More information about the cfe-commits mailing list