[PATCH] D60503: [clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[Sam McCall via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL358091: [clangd] Dont insert extra namespace 
qualifiers when Sema gets lost. (authored by sammccall, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60503?vs=194486=194520#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60503/new/

https://reviews.llvm.org/D60503

Files:
  clang-tools-extra/trunk/clangd/CodeComplete.cpp
  clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp

Index: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
===
--- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
+++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
@@ -2384,19 +2384,19 @@
   UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
 }
 
-// Regression test: clang parser gets confused here and doesn't report the ns::
-// prefix - naive behavior is to insert it again.
-// However we can recognize this from the source code.
-// Test disabled until we can make it pass.
-TEST(CompletionTest, DISABLED_NamespaceDoubleInsertion) {
+// Clang parser gets confused here and doesn't report the ns:: prefix.
+// Naive behavior is to insert it again. We examine the source and recover.
+TEST(CompletionTest, NamespaceDoubleInsertion) {
   clangd::CodeCompleteOptions Opts = {};
 
   auto Results = completions(R"cpp(
+namespace foo {
 namespace ns {}
 #define M(X) < X
 M(ns::ABC^
+}
   )cpp",
- {cls("ns::ABCDE")}, Opts);
+ {cls("foo::ns::ABCDE")}, Opts);
   EXPECT_THAT(Results.Completions,
   UnorderedElementsAre(AllOf(Qualifier(""), Named("ABCDE";
 }
Index: clang-tools-extra/trunk/clangd/CodeComplete.cpp
===
--- clang-tools-extra/trunk/clangd/CodeComplete.cpp
+++ clang-tools-extra/trunk/clangd/CodeComplete.cpp
@@ -45,6 +45,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
@@ -543,54 +544,59 @@
 // (e.g. enclosing namespace).
 std::pair, bool>
 getQueryScopes(CodeCompletionContext , const Sema ,
+   const CompletionPrefix ,
const CodeCompleteOptions ) {
-  auto GetAllAccessibleScopes = [](CodeCompletionContext ) {
-SpecifiedScope Info;
-for (auto *Context : CCContext.getVisitedContexts()) {
-  if (isa(Context))
-Info.AccessibleScopes.push_back(""); // global namespace
-  else if (isa(Context))
-Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
+  SpecifiedScope Scopes;
+  for (auto *Context : CCContext.getVisitedContexts()) {
+if (isa(Context))
+  Scopes.AccessibleScopes.push_back(""); // global namespace
+else if (isa(Context))
+  Scopes.AccessibleScopes.push_back(printNamespaceScope(*Context));
+  }
+
+  const CXXScopeSpec *SemaSpecifier =
+  CCContext.getCXXScopeSpecifier().getValueOr(nullptr);
+  // Case 1: unqualified completion.
+  if (!SemaSpecifier) {
+// Case 2 (exception): sema saw no qualifier, but there appears to be one!
+// This can happen e.g. in incomplete macro expansions. Use heuristics.
+if (!HeuristicPrefix.Qualifier.empty()) {
+  vlog("Sema said no scope specifier, but we saw {0} in the source code",
+   HeuristicPrefix.Qualifier);
+  StringRef SpelledSpecifier = HeuristicPrefix.Qualifier;
+  if (SpelledSpecifier.consume_front("::"))
+Scopes.AccessibleScopes = {""};
+  Scopes.UnresolvedQualifier = SpelledSpecifier;
+  return {Scopes.scopesForIndexQuery(), false};
 }
-return Info;
-  };
-
-  auto SS = CCContext.getCXXScopeSpecifier();
-
-  // Unqualified completion (e.g. "vec^").
-  if (!SS) {
-std::vector Scopes;
+// The enclosing namespace must be first, it gets a quality boost.
+std::vector EnclosingAtFront;
 std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
-Scopes.push_back(EnclosingScope);
-for (auto  : GetAllAccessibleScopes(CCContext).scopesForIndexQuery()) {
+EnclosingAtFront.push_back(EnclosingScope);
+for (auto  : Scopes.scopesForIndexQuery()) {
   if (EnclosingScope != S)
-Scopes.push_back(std::move(S));
+EnclosingAtFront.push_back(std::move(S));
 }
-// Allow AllScopes completion only for there is no explicit scope qualifier.
-return {Scopes, Opts.AllScopes};
-  }
-
-  // Qualified completion ("std::vec^"), we have two cases depending on whether
-  // the qualifier can be resolved by Sema.
-  if ((*SS)->isValid()) { // Resolved qualifier.
-return 

[PATCH] D60503: [clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[Eric Liu via Phabricator via cfe-commits]

ioeric accepted this revision.
ioeric added a comment.
This revision is now accepted and ready to land.

Feel free to land this. I'll rebase D60126  on 
this when it's in.




Comment at: clangd/CodeComplete.cpp:580
+  // Case 3: sema saw and resolved a scope qualifier.
+  if (SemaSpecifier && SemaSpecifier->isValid())
+return {Scopes.scopesForIndexQuery(), false};

nit: Sema Specifier can't be nullptr at this point.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60503/new/

https://reviews.llvm.org/D60503



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60503: [clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.



Comment at: clangd/CodeComplete.cpp:545
const CodeCompleteOptions ) {
-  auto GetAllAccessibleScopes = [](CodeCompletionContext ) {
-SpecifiedScope Info;
-for (auto *Context : CCContext.getVisitedContexts()) {
-  if (isa(Context))
-Info.AccessibleScopes.push_back(""); // global namespace
-  else if (isa(Context))
-Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
-}
-return Info;
-  };
-
-  auto SS = CCContext.getCXXScopeSpecifier();
+  SpecifiedScope Scopes;
+  for (auto *Context : CCContext.getVisitedContexts()) {

(AFAICT this lambda was executed the same way on every code path, so I just 
inlined it)


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60503/new/

https://reviews.llvm.org/D60503



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60503: [clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 194486.
sammccall added a comment.

Update comment


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60503/new/

https://reviews.llvm.org/D60503

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2374,19 +2374,19 @@
   UnorderedElementsAre(AllOf(Qualifier(""), Named("XYZ";
 }
 
-// Regression test: clang parser gets confused here and doesn't report the ns::
-// prefix - naive behavior is to insert it again.
-// However we can recognize this from the source code.
-// Test disabled until we can make it pass.
-TEST(CompletionTest, DISABLED_NamespaceDoubleInsertion) {
+// Clang parser gets confused here and doesn't report the ns:: prefix.
+// Naive behavior is to insert it again. We examine the source and recover.
+TEST(CompletionTest, NamespaceDoubleInsertion) {
   clangd::CodeCompleteOptions Opts = {};
 
   auto Results = completions(R"cpp(
+namespace foo {
 namespace ns {}
 #define M(X) < X
 M(ns::ABC^
+}
   )cpp",
- {cls("ns::ABCDE")}, Opts);
+ {cls("foo::ns::ABCDE")}, Opts);
   EXPECT_THAT(Results.Completions,
   UnorderedElementsAre(AllOf(Qualifier(""), Named("ABCDE";
 }
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -45,6 +45,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
@@ -539,54 +540,59 @@
 // (e.g. enclosing namespace).
 std::pair, bool>
 getQueryScopes(CodeCompletionContext , const Sema ,
+   const CompletionPrefix ,
const CodeCompleteOptions ) {
-  auto GetAllAccessibleScopes = [](CodeCompletionContext ) {
-SpecifiedScope Info;
-for (auto *Context : CCContext.getVisitedContexts()) {
-  if (isa(Context))
-Info.AccessibleScopes.push_back(""); // global namespace
-  else if (isa(Context))
-Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
-}
-return Info;
-  };
-
-  auto SS = CCContext.getCXXScopeSpecifier();
+  SpecifiedScope Scopes;
+  for (auto *Context : CCContext.getVisitedContexts()) {
+if (isa(Context))
+  Scopes.AccessibleScopes.push_back(""); // global namespace
+else if (isa(Context))
+  Scopes.AccessibleScopes.push_back(printNamespaceScope(*Context));
+  }
 
-  // Unqualified completion (e.g. "vec^").
-  if (!SS) {
-std::vector Scopes;
+  const CXXScopeSpec *SemaSpecifier =
+  CCContext.getCXXScopeSpecifier().getValueOr(nullptr);
+  // Case 1: unqualified completion.
+  if (!SemaSpecifier) {
+// Case 2 (exception): sema saw no qualifier, but there appears to be one!
+// This can happen e.g. in incomplete macro expansions. Use heuristics.
+if (!HeuristicPrefix.Qualifier.empty()) {
+  vlog("Sema said no scope specifier, but we saw {0} in the source code",
+   HeuristicPrefix.Qualifier);
+  StringRef SpelledSpecifier = HeuristicPrefix.Qualifier;
+  if (SpelledSpecifier.consume_front("::"))
+Scopes.AccessibleScopes = {""};
+  Scopes.UnresolvedQualifier = SpelledSpecifier;
+  return {Scopes.scopesForIndexQuery(), false};
+}
+// The enclosing namespace must be first, it gets a quality boost.
+std::vector EnclosingAtFront;
 std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
-Scopes.push_back(EnclosingScope);
-for (auto  : GetAllAccessibleScopes(CCContext).scopesForIndexQuery()) {
+EnclosingAtFront.push_back(EnclosingScope);
+for (auto  : Scopes.scopesForIndexQuery()) {
   if (EnclosingScope != S)
-Scopes.push_back(std::move(S));
+EnclosingAtFront.push_back(std::move(S));
 }
-// Allow AllScopes completion only for there is no explicit scope qualifier.
-return {Scopes, Opts.AllScopes};
+// Allow AllScopes completion as there is no explicit scope qualifier.
+return {EnclosingAtFront, Opts.AllScopes};
   }
-
-  // Qualified completion ("std::vec^"), we have two cases depending on whether
-  // the qualifier can be resolved by Sema.
-  if ((*SS)->isValid()) { // Resolved qualifier.
-return {GetAllAccessibleScopes(CCContext).scopesForIndexQuery(), false};
-  }
-
-  // Unresolved qualifier.
-  SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
-  Info.AccessibleScopes.push_back(""); // Make sure global scope is included.
-
-  llvm::StringRef SpelledSpecifier =
-  

[PATCH] D60503: [clangd] Don't insert extra namespace qualifiers when Sema gets lost.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ioeric.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ilya-biryukov.
Herald added a project: clang.

There are cases where Sema can't tell that "foo" in foo::Bar is a
namespace qualifier, like in incomplete macro expansions.

After this patch, if sema reports no specifier but it looks like there's one in
the source code, then we take it into account.

Reworked structure and comments in getQueryScopes to try to fight
creeping complexity - unsure if I succeeded.

I made the test harder (the original version also passes).


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D60503

Files:
  clangd/CodeComplete.cpp
  unittests/clangd/CodeCompleteTests.cpp

Index: unittests/clangd/CodeCompleteTests.cpp
===
--- unittests/clangd/CodeCompleteTests.cpp
+++ unittests/clangd/CodeCompleteTests.cpp
@@ -2378,15 +2378,17 @@
 // prefix - naive behavior is to insert it again.
 // However we can recognize this from the source code.
 // Test disabled until we can make it pass.
-TEST(CompletionTest, DISABLED_NamespaceDoubleInsertion) {
+TEST(CompletionTest, NamespaceDoubleInsertion) {
   clangd::CodeCompleteOptions Opts = {};
 
   auto Results = completions(R"cpp(
+namespace foo {
 namespace ns {}
 #define M(X) < X
 M(ns::ABC^
+}
   )cpp",
- {cls("ns::ABCDE")}, Opts);
+ {cls("foo::ns::ABCDE")}, Opts);
   EXPECT_THAT(Results.Completions,
   UnorderedElementsAre(AllOf(Qualifier(""), Named("ABCDE";
 }
Index: clangd/CodeComplete.cpp
===
--- clangd/CodeComplete.cpp
+++ clangd/CodeComplete.cpp
@@ -45,6 +45,7 @@
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/CodeCompleteConsumer.h"
+#include "clang/Sema/DeclSpec.h"
 #include "clang/Sema/Sema.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/None.h"
@@ -539,54 +540,59 @@
 // (e.g. enclosing namespace).
 std::pair, bool>
 getQueryScopes(CodeCompletionContext , const Sema ,
+   const CompletionPrefix ,
const CodeCompleteOptions ) {
-  auto GetAllAccessibleScopes = [](CodeCompletionContext ) {
-SpecifiedScope Info;
-for (auto *Context : CCContext.getVisitedContexts()) {
-  if (isa(Context))
-Info.AccessibleScopes.push_back(""); // global namespace
-  else if (isa(Context))
-Info.AccessibleScopes.push_back(printNamespaceScope(*Context));
-}
-return Info;
-  };
-
-  auto SS = CCContext.getCXXScopeSpecifier();
+  SpecifiedScope Scopes;
+  for (auto *Context : CCContext.getVisitedContexts()) {
+if (isa(Context))
+  Scopes.AccessibleScopes.push_back(""); // global namespace
+else if (isa(Context))
+  Scopes.AccessibleScopes.push_back(printNamespaceScope(*Context));
+  }
 
-  // Unqualified completion (e.g. "vec^").
-  if (!SS) {
-std::vector Scopes;
+  const CXXScopeSpec *SemaSpecifier =
+  CCContext.getCXXScopeSpecifier().getValueOr(nullptr);
+  // Case 1: unqualified completion.
+  if (!SemaSpecifier) {
+// Case 2 (exception): sema saw no qualifier, but there appears to be one!
+// This can happen e.g. in incomplete macro expansions. Use heuristics.
+if (!HeuristicPrefix.Qualifier.empty()) {
+  vlog("Sema said no scope specifier, but we saw {0} in the source code",
+   HeuristicPrefix.Qualifier);
+  StringRef SpelledSpecifier = HeuristicPrefix.Qualifier;
+  if (SpelledSpecifier.consume_front("::"))
+Scopes.AccessibleScopes = {""};
+  Scopes.UnresolvedQualifier = SpelledSpecifier;
+  return {Scopes.scopesForIndexQuery(), false};
+}
+// The enclosing namespace must be first, it gets a quality boost.
+std::vector EnclosingAtFront;
 std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
-Scopes.push_back(EnclosingScope);
-for (auto  : GetAllAccessibleScopes(CCContext).scopesForIndexQuery()) {
+EnclosingAtFront.push_back(EnclosingScope);
+for (auto  : Scopes.scopesForIndexQuery()) {
   if (EnclosingScope != S)
-Scopes.push_back(std::move(S));
+EnclosingAtFront.push_back(std::move(S));
 }
-// Allow AllScopes completion only for there is no explicit scope qualifier.
-return {Scopes, Opts.AllScopes};
+// Allow AllScopes completion as there is no explicit scope qualifier.
+return {EnclosingAtFront, Opts.AllScopes};
   }
-
-  // Qualified completion ("std::vec^"), we have two cases depending on whether
-  // the qualifier can be resolved by Sema.
-  if ((*SS)->isValid()) { // Resolved qualifier.
-return {GetAllAccessibleScopes(CCContext).scopesForIndexQuery(), false};
-  }
-
-  // Unresolved qualifier.
-  SpecifiedScope Info = GetAllAccessibleScopes(CCContext);
-