Index: clangd/IncludeFixer.cpp =================================================================== --- clangd/IncludeFixer.cpp +++ clangd/IncludeFixer.cpp @@ -19,6 +19,7 @@ #include "clang/AST/Type.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticSema.h" +#include "clang/Basic/SourceLocation.h" #include "clang/Sema/DeclSpec.h" #include "clang/Sema/Lookup.h" #include "clang/Sema/Scope.h" @@ -27,6 +28,8 @@ #include "llvm/ADT/ArrayRef.h" #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" @@ -198,31 +201,42 @@ 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". - + auto &SM = SemaPtr->SourceMgr; + bool Invalid = false; + llvm::StringRef Code = + SM.getBufferData(SM.getDecomposedLoc(Typo.getLoc()).first, &Invalid); + assert(!Invalid); // Extract the typed scope. This is not done lazily because `SS` can get // out of scope and it's relatively cheap. llvm::Optional SpecifiedScope; + // Extra scope to append to the query scopes. This is useful, for example, + // when the unresolved name is a qualier, in which case we use the name that + // comes after it as the name to resolve and use the qualifier as the extra + // scope in the accissible scopes. + llvm::Optional ExtraScope; 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 + else if (const auto *NS = Nested->getAsNamespace()) { + auto SpecifiedNS = printNamespaceScope(*NS); + + // Check the qualifier 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" qualifier 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)) + SpecifiedScope = SpecifiedNS; + else + ExtraScope = Spelling; + } 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(); @@ -235,8 +249,37 @@ Unresolved.Name = Typo.getAsString(); Unresolved.Loc = Typo.getBeginLoc(); + if (static_cast(LookupKind) == + Sema::LookupNameKind::LookupNestedNameSpecifierName) { + // If the unresolved name is a qualifier 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 qualifier 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(Code, SM.getFileOffset(Unresolved.Loc) + + Unresolved.Name.size())) { + auto Split = splitQualifiedName(*QualifiedByUnresolved); + if (!ExtraScope) + ExtraScope.emplace(); + // If ExtraScope 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 qualifier, we append + // "index::" to the extra scope. + ExtraScope->append((Unresolved.Name + Split.first).str()); + Unresolved.Name = Split.second; + } + } auto *Sem = SemaPtr; // Avoid capturing `this`. - Unresolved.GetScopes = [Sem, SpecifiedScope, S, LookupKind]() { + Unresolved.GetScopes = [Sem, SpecifiedScope, S, LookupKind, ExtraScope]() { std::vector Scopes; if (SpecifiedScope) { Scopes.push_back(*SpecifiedScope); @@ -255,6 +298,10 @@ if (isa(Ctx)) Scopes.push_back(printNamespaceScope(*Ctx)); } + + if (ExtraScope) + for (auto &Scope : Scopes) + Scope.append(*ExtraScope); return Scopes; }; LastUnresolvedName = std::move(Unresolved); @@ -265,6 +312,32 @@ } private: + // Returns the idenfiers qualified by an unresolved name. \p Offset is the + // first character after the unresolved name in \p Code. For the example + // below, this returns "::X::Y" that is qualfied by unresolved name "clangd": + // clang::clangd::X::Y + // ~~~~~~ + llvm::Optional qualifiedByUnresolved(llvm::StringRef Code, + size_t Offset) const { + auto IsValidIdentifierChar = [](char C) { + return ((C >= 'a' && C <= 'z') || (C >= 'A' && C <= 'Z') || + (C >= '0' && C <= '9') || (C == '_')); + }; + auto Truncated = Code.substr(Offset); + size_t ResultLen = 0; + while (Truncated.consume_front("::")) { + size_t P = 0; + for (; P < Truncated.size() && IsValidIdentifierChar(Truncated[P]); ++P) { + } + if (P == 0) // Stop if there's nothing after :: + break; + ResultLen += P + 2; // include "::". + Truncated = Truncated.drop_front(P); + } + if (ResultLen == 0) + return llvm::None; + return Code.substr(Offset, ResultLen).str(); + } Sema *SemaPtr = nullptr; llvm::Optional &LastUnresolvedName; @@ -279,8 +352,9 @@ assert(LastUnresolvedName.hasValue()); auto &Unresolved = *LastUnresolvedName; std::vector Scopes = Unresolved.GetScopes(); - vlog("Trying to fix unresolved name \"{0}\" in scopes: [{1}]", - Unresolved.Name, llvm::join(Scopes.begin(), Scopes.end(), ", ")); + vlog("Trying to fix unresolved name \"{0}\" in scopes: [{1}] (loc={2})", + Unresolved.Name, llvm::join(Scopes.begin(), Scopes.end(), ", "), + Unresolved.Loc.getRawEncoding()); FuzzyFindRequest Req; Req.AnyScope = false; Index: unittests/clangd/DiagnosticsTests.cpp =================================================================== --- unittests/clangd/DiagnosticsTests.cpp +++ unittests/clangd/DiagnosticsTests.cpp @@ -20,6 +20,7 @@ namespace clangd { namespace { +using testing::_; using testing::ElementsAre; using testing::Field; using testing::IsEmpty; @@ -369,6 +370,8 @@ $insert[[]]namespace ns { void foo() { $unqualified1[[X]] x; + // No fix if the unresolved type is used as qualifier. (ns::)X::Nested will be + // considered the unresolved type. $unqualified2[[X]]::Nested n; } } @@ -391,10 +394,7 @@ 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", @@ -449,6 +449,68 @@ UnorderedElementsAre(Diag(Test.range(), "no member named 'xy' in 'X'"))); } +TEST(IncludeFixerTest, UnresolvedNameAsQualifier) { + 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, UnresolvedQualifierWithSemaCorrection) { + 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"))))); +} + } // namespace } // namespace clangd } // namespace clang