ioeric updated this revision to Diff 187222.
ioeric marked 9 inline comments as done.
ioeric added a comment.
- address review comments
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
Files:
clangd/IncludeFixer.cpp
unittests/clangd/DiagnosticsTests.cpp
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
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<std::string> 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<std::string> 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<std::string> Unresolved;
+ llvm::Optional<std::string> AlternativeUnresolvedName;
+};
+
+// Extracts the specified scopes around an unresolved name.
+// FIXME: try to merge this with the scope-wrangling code in CodeComplete.
+llvm::Optional<SpecifiedScopes> 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<UnresolvedName> &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<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.
- return TypoCorrection();
-
UnresolvedName Unresolved;
Unresolved.Name = Typo.getAsString();
Unresolved.Loc = Typo.getBeginLoc();
+ auto ExtractedScopes = extractSpecifiedScopes(
+ SemaPtr->SourceMgr, SS, Unresolved.Name, Unresolved.Loc,
+ static_cast<Sema::LookupNameKind>(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<std::string> 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<NamespaceDecl>(Ctx))
Scopes.push_back(printNamespaceScope(*Ctx));
}
+
+ if (SpecifiedScopes.Unresolved)
+ for (auto &Scope : Scopes)
+ Scope.append(*SpecifiedScopes.Unresolved);
return Scopes;
};
LastUnresolvedName = std::move(Unresolved);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
