[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL354330: [clangd] Handle unresolved scope specifier when
fixing includes. (authored by ioeric, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
Files:
clang-tools-extra/trunk/clangd/IncludeFixer.cpp
clang-tools-extra/trunk/unittests/clangd/DiagnosticsTests.cpp
Index: clang-tools-extra/trunk/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/trunk/clangd/IncludeFixer.cpp
+++ clang-tools-extra/trunk/clangd/IncludeFixer.cpp
@@ -19,6 +19,10 @@
#include "clang/AST/Type.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
#include "clang/Sema/DeclSpec.h"
#include "clang/Sema/Lookup.h"
#include "clang/Sema/Scope.h"
@@ -28,6 +32,7 @@
#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"
@@ -172,6 +177,121 @@
}
return Fixes;
}
+
+// Returns the identifiers qualified by an unresolved name. \p Loc is the
+// start location of the unresolved name. For the example below, this returns
+// "::X::Y" that is qualified by unresolved name "clangd":
+// clang::clangd::X::Y
+//~
+llvm::Optional qualifiedByUnresolved(const SourceManager &SM,
+ SourceLocation Loc,
+ const LangOptions &LangOpts) {
+ std::string Result;
+
+ SourceLocation NextLoc = Loc;
+ while (auto CCTok = Lexer::findNextToken(NextLoc, SM, LangOpts)) {
+if (!CCTok->is(tok::coloncolon))
+ break;
+auto IDTok = Lexer::findNextToken(CCTok->getLocation(), SM, LangOpts);
+if (!IDTok || !IDTok->is(tok::raw_identifier))
+ break;
+Result.append(("::" + IDTok->getRawIdentifier()).str());
+NextLoc = IDTok->getLocation();
+ }
+ if (Result.empty())
+return llvm::None;
+ return Result;
+}
+
+// An unresolved name and its scope information that can be extracted cheaply.
+struct CheapUnresolvedName {
+ std::string Name;
+ // 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::`).
+ llvm::Optional ResolvedScope;
+
+ // Unresolved part of the scope. When the unresolved name is a specifier, we
+ // use the name that comes after it as the alternative name to resolve and use
+ // the specifier as the extra scope in the accessible scopes.
+ llvm::Optional UnresolvedScope;
+};
+
+// Extracts unresolved name and scope information around \p Unresolved.
+// FIXME: try to merge this with the scope-wrangling code in CodeComplete.
+llvm::Optional extractUnresolvedNameCheaply(
+const SourceManager &SM, const DeclarationNameInfo &Unresolved,
+CXXScopeSpec *SS, const LangOptions &LangOpts, bool UnresolvedIsSpecifier) {
+ bool Invalid = false;
+ llvm::StringRef Code = SM.getBufferData(
+ SM.getDecomposedLoc(Unresolved.getBeginLoc()).first, &Invalid);
+ if (Invalid)
+return llvm::None;
+ CheapUnresolvedName Result;
+ Result.Name = Unresolved.getAsString();
+ if (SS && SS->isNotEmpty()) { // "::" or "ns::"
+if (auto *Nested = SS->getScopeRep()) {
+ if (Nested->getKind() == NestedNameSpecifier::Global)
+Result.ResolvedScope = "";
+ else if (const auto *NS = Nested->getAsNamespace()) {
+auto SpecifiedNS = printNamespaceScope(*NS);
+
+// Check the specifier 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" specifier 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))
+ Result.ResolvedScope = SpecifiedNS;
+else
+ Result.UnresolvedScope = Spelling;
+ } else if (const auto *ANS = Nested->getAsNamespaceAlias()) {
+Result.ResolvedScope = printNamespaceScope(*ANS->getNamespace());
+ } else {
+// We don't fix symbols in scopes that are not top-level
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
ioeric updated this revision to Diff 187359.
ioeric added a comment.
- minor cleanup
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 specifier. (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",
@@ -487,6 +487,88 @@
}
}
+TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
+ 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, UnresolvedSpecifierWithSemaCorrection) {
+ 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 clan
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
ioeric updated this revision to Diff 187343.
ioeric marked 16 inline comments as done.
ioeric added a comment.
- address review comment
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 specifier. (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",
@@ -487,6 +487,88 @@
}
}
+TEST(IncludeFixerTest, UnresolvedNameAsSpecifier) {
+ 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, UnresolvedSpecifierWithSemaCorrection) {
+ 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");
+}
+
} // na
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
ioeric added inline comments.
Comment at: clangd/IncludeFixer.cpp:191
+//~~
+llvm::Optional qualifiedByUnresolved(llvm::StringRef Code,
+ size_t Offset) {
sammccall wrote:
> this isn't wrong per se (conservative, bails out e.g. on unicode)
> But is it much harder to call Lexer::findNextToken twice and expect a raw
> identifier and ::?
Good idea. Lexer is a better option.
Comment at: clangd/IncludeFixer.cpp:251
+// namespace clang { clangd::X; }
+// In this case, we use the "typo" qualifier as extra scope instead
+// of using the scope assumed by sema.
jkorous wrote:
> Does this work with anonymous namespaces?
I'm not sure... how do you use an anonymous namespace as a scope specifier?
Comment at: clangd/IncludeFixer.cpp:256
+std::string Spelling = (Code.substr(B, E - B) + "::").str();
+vlog("Spelling scope: {0}, SpecifiedNS: {1}", Spelling, SpecifiedNS);
+if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
sammccall wrote:
> I don't think this vlog is useful as-is (quite far down the stack with no
> context)
> Did you intend to remove it?
Indeed. Thanks for the catch!
Comment at: clangd/IncludeFixer.cpp:271
+ if (IsUnrsolvedSpecifier) {
+// If the unresolved name is a qualifier e.g.
+// clang::clangd::X
jkorous wrote:
> Is the term `qualifier` applicable here? (Honest question.)
>
> It seems like C++ grammar uses `specifier` (same as you in
> `IsUnrsolvedSpecifier `)
> http://www.nongnu.org/hcb/#nested-name-specifier
You've raised a good point. We've been using `specifier` and `qualifier`
interchangeably in the project. But specifier does seem to be a more
appropriate name to use. I've fixed uses in this file. Uses in other files
probably need a separate cleanup.
Comment at: clangd/IncludeFixer.cpp:322
UnresolvedName Unresolved;
Unresolved.Name = Typo.getAsString();
Unresolved.Loc = Typo.getBeginLoc();
sammccall wrote:
> Following up on our offline discussion :-)
> I think that since `extractSpecifiedScopes` can want to modify the name, we
> should just expand that function's signature/responsibility to always
> determine the name.
>
> So we'd pass the `const DeclarationNameInfo&` to `extractSpecifiedScopes`,
> and it would return a struct `{optional ResolvedScope;
> optional UnresolvedScope; string Name}`.
> Maybe need to call them `CheapUnresolvedName`/`extractUnresolvedNameCheaply`
> or similar.
> But I think the code change is small.
Sounds good. Thanks!
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
jkorous added a comment.
Hi Eric, I have just couple details.
Comment at: clangd/IncludeFixer.cpp:188
+// 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
qualfied -> qualified
Comment at: clangd/IncludeFixer.cpp:193
+ size_t Offset) {
+ auto IsValidIdentifierChar = [](char C) {
+return ((C >= 'a' && C <= 'z') || (C >= 'A' && C <= 'Z') ||
It seems to me that calling Lexer would be better indeed.
Comment at: clangd/IncludeFixer.cpp:216
+ // 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.
Maybe move the comment about work done eagerly to the function?
Comment at: clangd/IncludeFixer.cpp:251
+// namespace clang { clangd::X; }
+// In this case, we use the "typo" qualifier as extra scope instead
+// of using the scope assumed by sema.
Does this work with anonymous namespaces?
Comment at: clangd/IncludeFixer.cpp:263
+Result.Resolved = printNamespaceScope(*ANS->getNamespace());
+ else
+// We don't fix symbols in scopes that are not top-level e.g. class
I'd personally prefer to add parentheses here to have the if/else if/else
consistent. Up to you though.
Comment at: clangd/IncludeFixer.cpp:271
+ if (IsUnrsolvedSpecifier) {
+// If the unresolved name is a qualifier e.g.
+// clang::clangd::X
Is the term `qualifier` applicable here? (Honest question.)
It seems like C++ grammar uses `specifier` (same as you in
`IsUnrsolvedSpecifier `)
http://www.nongnu.org/hcb/#nested-name-specifier
Comment at: unittests/clangd/DiagnosticsTests.cpp:452
+TEST(IncludeFixerTest, UnresolvedNameAsQualifier) {
+ Annotations Test(R"cpp(
If (see above) we decide `qualifier` should be replaced by `specifier` or smth
else please replace here as well, otherwise ignore this.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks! The layering is *much* clearer now.
I can still suggest a couple of tweaks, but they're pretty much cosmetic.
Comment at: clangd/IncludeFixer.cpp:190
+// clang::clangd::X::Y
+//~~
+llvm::Optional qualifiedByUnresolved(llvm::StringRef Code,
you're showing a range here, it should be a point though?
Comment at: clangd/IncludeFixer.cpp:191
+//~~
+llvm::Optional qualifiedByUnresolved(llvm::StringRef Code,
+ size_t Offset) {
this isn't wrong per se (conservative, bails out e.g. on unicode)
But is it much harder to call Lexer::findNextToken twice and expect a raw
identifier and ::?
Comment at: clangd/IncludeFixer.cpp:231
+const SourceManager &SM, CXXScopeSpec *SS, llvm::StringRef UnresolvedName,
+SourceLocation UnresolvedLoc, bool IsUnrsolvedSpecifier) {
+ bool Invalid = false;
Unrsolved -> Unresolved
emphasis might be clearer as `UnresolvedIsSpecifier` - there's always an
unresolved entity, the boolean is indicating that it's a specifier
Comment at: clangd/IncludeFixer.cpp:256
+std::string Spelling = (Code.substr(B, E - B) + "::").str();
+vlog("Spelling scope: {0}, SpecifiedNS: {1}", Spelling, SpecifiedNS);
+if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
I don't think this vlog is useful as-is (quite far down the stack with no
context)
Did you intend to remove it?
Comment at: clangd/IncludeFixer.cpp:322
UnresolvedName Unresolved;
Unresolved.Name = Typo.getAsString();
Unresolved.Loc = Typo.getBeginLoc();
Following up on our offline discussion :-)
I think that since `extractSpecifiedScopes` can want to modify the name, we
should just expand that function's signature/responsibility to always determine
the name.
So we'd pass the `const DeclarationNameInfo&` to `extractSpecifiedScopes`, and
it would return a struct `{optional ResolvedScope; optional
UnresolvedScope; string Name}`.
Maybe need to call them `CheapUnresolvedName`/`extractUnresolvedNameCheaply` or
similar.
But I think the code change is small.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
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",
+
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
ioeric added inline comments.
Comment at: clangd/IncludeFixer.cpp:235
+ std::string Spelling = (Code.substr(B, E - B) + "::").str();
+ if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
+SpecifiedScope = SpecifiedNS;
sammccall wrote:
> hmm, won't this heuristic have false positives?
> ```
> // indexed-header.h
> namespace a { int X; }
>
> // main-file.cc
> namespace b = a;
> namespace c { int Y = b::x; }
> ```
> I worry spelling is going to be "b::" here, while SpecifiedNS is going to be
> "a::".
Thanks for pointing this out! I completely missed the namespace alias case.
Fixed and added a test.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
sammccall added inline comments.
Comment at: clangd/IncludeFixer.cpp:208
+SM.getBufferData(SM.getDecomposedLoc(Typo.getLoc()).first, &Invalid);
+assert(!Invalid);
// Extract the typed scope. This is not done lazily because `SS` can get
No idea when this can be invalid, but we could just return here?
Comment at: clangd/IncludeFixer.cpp:211
// out of scope and it's relatively cheap.
llvm::Optional SpecifiedScope;
+// Extra scope to append to the query scopes. This is useful, for example,
We can be more precise about this now I think: IIUC this is the **resolved**
specified scope.
in two senses: it's 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::`)
Comment at: clangd/IncludeFixer.cpp:212
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
nit: I don't think this is an example, it's the only case right?
Consider calling this UnresolvedSpecifiedScope
Comment at: clangd/IncludeFixer.cpp:213
+// 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
nit: qualier -> qualifier
accissible -> accessible
Comment at: clangd/IncludeFixer.cpp:216
+// scope in the accissible scopes.
+llvm::Optional ExtraScope;
if (SS && SS->isNotEmpty()) { // "::" or "ns::"
I'm a bit concerned about how this mixes extraction of scopes from the source
code/AST (which is now *really* complicated) with building the query and
assembling the correction.
Can we reasonably extract the former to a helper function? Not sure exactly
what the signature would be, but it would be helpful to find out.
The other thing is I think this has a lot in common with the scope-wrangling
code in CodeComplete, and could *maybe* be unified a bit once isolated.
Comment at: clangd/IncludeFixer.cpp:235
+ std::string Spelling = (Code.substr(B, E - B) + "::").str();
+ if (llvm::StringRef(SpecifiedNS).endswith(Spelling))
+SpecifiedScope = SpecifiedNS;
hmm, won't this heuristic have false positives?
```
// indexed-header.h
namespace a { int X; }
// main-file.cc
namespace b = a;
namespace c { int Y = b::x; }
```
I worry spelling is going to be "b::" here, while SpecifiedNS is going to be
"a::".
Comment at: clangd/IncludeFixer.cpp:315
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
identifiers
Comment at: clangd/IncludeFixer.cpp:320
+ //~~
+ llvm::Optional qualifiedByUnresolved(llvm::StringRef Code,
+size_t Offset) const {
this only depends on the params right?
This could be static or even moved out of the class.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58185/new/
https://reviews.llvm.org/D58185
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
ioeric updated this revision to Diff 186674.
ioeric added a comment.
- Remove unintended change.
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,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/StringR
[PATCH] D58185: [clangd] Handle unresolved scope specifier when fixing includes.
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 clang
