[PATCH] D120616: [clangd] IncludeFixer: resolve Class in ns::Class::method();

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: usaxena95.
Herald added subscribers: kadircet, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

We distinguish by case, assuming a::b:: is a namespace and a::B:: is a class.
Crude, but be a significant improvement over assuming it's always a namespace.

(Motivating case from clangd: clangd::CommandMangler::detect()).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D120616

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


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1178,25 +1178,34 @@
 
 TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
   Annotations Test(R"cpp(// error-ok
-$insert[[]]namespace ns {
+$insert[[]]namespace ns {}
+void g() {
+  ns::$lower[[scope]]::X_Y();
+  ns::$upper[[Scope]]::X_Y();
 }
-void g() {  ns::$[[scope]]::X_Y();  }
   )cpp");
   TestTU TU;
   TU.Code = std::string(Test.code());
   // FIXME: Figure out why this is needed and remove it, PR43662.
   TU.ExtraArgs.push_back("-fno-ms-compatibility");
   auto Index = buildIndexWithSymbol(
-      SymbolWithHeader{"ns::scope::X_Y", "unittest:///x.h", "\"x.h\""});
+      {SymbolWithHeader{"ns::scope::X_Y", "unittest:///ns.h", "\"ns.h\""},
+       SymbolWithHeader{"ns::Scope", "unittest:///class.h", "\"class.h\""}});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(
       *TU.build().getDiagnostics(),
       UnorderedElementsAre(
-          AllOf(Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
+          AllOf(Diag(Test.range("lower"),
+                     "no member named 'scope' in namespace 'ns'"),
                 diagName("no_member"),
-                withFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-                            "Include \"x.h\" for symbol ns::scope::X_Y")))));
+                withFix(Fix(Test.range("insert"), "#include \"ns.h\"\n",
+                            "Include \"ns.h\" for symbol ns::scope::X_Y"))),
+          AllOf(Diag(Test.range("upper"),
+                     "no member named 'Scope' in namespace 'ns'"),
+                diagName("no_member"),
+                withFix(Fix(Test.range("insert"), "#include \"class.h\"\n",
+                            "Include \"class.h\" for symbol ns::Scope")))));
 }
 
 TEST(IncludeFixerTest, UnresolvedSpecifierWithSemaCorrection) {
Index: clang-tools-extra/clangd/IncludeFixer.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeFixer.cpp
+++ clang-tools-extra/clangd/IncludeFixer.cpp
@@ -417,15 +417,20 @@
     }
   }
 
-  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.
-
+  // If the unresolved name is a namespace qualifier e.g.
+  //      clang::clangd::X
+  //             ~~~~~~
+  // we want to resolve clang::clangd::X instead of clang::clangd.
+  //
+  // On the other hand if it's a class name qualifier e.g.
+  //      clang::PrecompiledPreamble::Build()
+  //             ~~~~~~~~~~~~~~~~~~~
+  // then we should resolve the class clang::PrecompiledPreamble.
+  // 
+  // We simply guess based on the case of the identifier. Namespaces are almost
+  // universally lowercase, while class names are often uppercase.
+  assert(!Result.Name.empty());
+  if (UnresolvedIsSpecifier && !isUppercase(Result.Name.front())) {
     // Not using the end location as it doesn't always point to the end of
     // identifier.
     if (auto QualifiedByUnresolved =


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D120616.411613.patch
Type: text/x-patch
Size: 3476 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20220226/2c8c988c/attachment.bin>