Index: clangd/IncludeFixer.cpp =================================================================== --- clangd/IncludeFixer.cpp +++ clangd/IncludeFixer.cpp @@ -19,6 +19,8 @@ #include "clang/AST/Type.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticSema.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/Basic/SourceManager.h" #include "clang/Sema/DeclSpec.h" #include "clang/Sema/Lookup.h" #include "clang/Sema/Scope.h" @@ -27,6 +29,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" @@ -178,6 +182,122 @@ } return Fixes; } + +// Returns the identifiers 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) { + 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(); +} + +struct SpecifiedScopes { + // 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::`). This is not done lazily because `SS` can get out of + // scope and it's relatively cheap. + llvm::Optional Resolved; + + // Unresolved part of the scope. When the unresolved name is a qualifier, we + // use the name that comes after it as the alternative name to resolve and use + // the qualifier as the extra scope in the accessible scopes. + llvm::Optional Unresolved; + llvm::Optional AlternativeUnresolvedName; +}; + +// Extracts the specified scopes around an unresolved name. +// FIXME: try to merge this with the scope-wrangling code in CodeComplete. +llvm::Optional extractSpecifiedScopes( + const SourceManager &SM, CXXScopeSpec *SS, llvm::StringRef UnresolvedName, + SourceLocation UnresolvedLoc, bool IsUnrsolvedSpecifier) { + bool Invalid = false; + llvm::StringRef Code = + SM.getBufferData(SM.getDecomposedLoc(UnresolvedLoc).first, &Invalid); + if (Invalid) + return llvm::None; + SpecifiedScopes Result; + if (SS && SS->isNotEmpty()) { // "::" or "ns::" + if (auto *Nested = SS->getScopeRep()) { + if (Nested->getKind() == NestedNameSpecifier::Global) + Result.Resolved = ""; + 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(); + vlog("Spelling scope: {0}, SpecifiedNS: {1}", Spelling, SpecifiedNS); + if (llvm::StringRef(SpecifiedNS).endswith(Spelling)) + Result.Resolved = SpecifiedNS; + else + Result.Unresolved = Spelling; + } else if (const auto *ANS = Nested->getAsNamespaceAlias()) + Result.Resolved = 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 (IsUnrsolvedSpecifier) { + // 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(UnresolvedLoc) + UnresolvedName.size())) { + auto Split = splitQualifiedName(*QualifiedByUnresolved); + if (!Result.Unresolved) + Result.Unresolved.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 qualifier, we append + // "index::" to the extra scope. + Result.Unresolved->append((UnresolvedName + Split.first).str()); + Result.AlternativeUnresolvedName = Split.second; + } + } + return Result; +} + class IncludeFixer::UnresolvedNameRecorder : public ExternalSemaSource { public: UnresolvedNameRecorder(llvm::Optional &LastUnresolvedName) @@ -198,48 +318,28 @@ 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 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. - return TypoCorrection(); - UnresolvedName Unresolved; Unresolved.Name = Typo.getAsString(); Unresolved.Loc = Typo.getBeginLoc(); + auto ExtractedScopes = extractSpecifiedScopes( + SemaPtr->SourceMgr, SS, Unresolved.Name, Unresolved.Loc, + static_cast(LookupKind) == + Sema::LookupNameKind::LookupNestedNameSpecifierName); + if (!ExtractedScopes) + return TypoCorrection(); + auto SpecifiedScopes = std::move(*ExtractedScopes); + if (SpecifiedScopes.AlternativeUnresolvedName) + Unresolved.Name = *SpecifiedScopes.AlternativeUnresolvedName; + + if (!SpecifiedScopes.Resolved && !S) // Give up if no scope available. + return TypoCorrection(); + auto *Sem = SemaPtr; // Avoid capturing `this`. - Unresolved.GetScopes = [Sem, SpecifiedScope, S, LookupKind]() { + Unresolved.GetScopes = [Sem, SpecifiedScopes, S, LookupKind]() { std::vector Scopes; - if (SpecifiedScope) { - Scopes.push_back(*SpecifiedScope); + if (SpecifiedScopes.Resolved) { + Scopes.push_back(*SpecifiedScopes.Resolved); } else { assert(S); // No scope qualifier is specified. Collect all accessible scopes in the @@ -255,6 +355,10 @@ if (isa(Ctx)) Scopes.push_back(printNamespaceScope(*Ctx)); } + + if (SpecifiedScopes.Unresolved) + for (auto &Scope : Scopes) + Scope.append(*SpecifiedScopes.Unresolved); return Scopes; }; LastUnresolvedName = std::move(Unresolved); 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,87 @@ 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"))))); +} +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