ioeric created this revision.
ioeric added a reviewer: sammccall.
Herald added subscribers: cfe-commits, jdoerfert, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
ioeric updated this revision to Diff 186674.
ioeric added a comment.
ioeric edited the summary of this revision.
- Remove unintended change.
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
~~~~~~
Repository:
rCTE Clang Tools Extra
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,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
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<std::string> 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<std::string> 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<Sema::LookupNameKind>(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<std::string> Scopes;
if (SpecifiedScope) {
Scopes.push_back(*SpecifiedScope);
@@ -255,6 +298,10 @@
if (isa<NamespaceDecl>(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<std::string> 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<UnresolvedName> &LastUnresolvedName;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
