[Sema] Ignore decls in namespaces when global decls are not wanted.

[ioeric]

Summary

... in qualified code completion and decl lookup.

[ilya-biryukov]

This is how I always perceived this option in the first place, so LGTM.
But maybe its intention is different, so we should wait for @arphaman's comments.

Could you also update comments of CodeCompleteConsumer::includeGlobals and CodeCompleteOptions::IncludeGlobals to indicate we ignore all namespace-level decls now (it currently only mentions top-level decls, which may be confusing).

[ioeric]

• Clarify comment for includeGlobals()

[arphaman]

This change breaks cached completions for declarations in namespaces in libclang. What exactly are you trying to achieve here? We could introduce another flag maybe.

[arphaman]

Here's a simple test that breaks:

$ cat test.cpp
namespace ns {
  void func();
}
ns::
// complete
$ c-index-test -code-completion-at=test.cpp:5:1 test.cpp
FunctionDecl:{ResultType void}{TypedText func}{LeftParen (}{RightParen )} (50)
Completion contexts:
$ CINDEXTEST_COMPLETION_CACHING=1 c-index-test -code-completion-at=test.cpp:5:1 test.cpp
Completion contexts:

[ilya-biryukov]

In D40562#939950, @arphaman wrote:

This change breaks cached completions for declarations in namespaces in libclang. What exactly are you trying to achieve here? We could introduce another flag maybe.

Am I right to assume this cache is there to reduce the amount of Decls we need to deserialize from Preambles? Maybe we could fix the cache to also include namespace-level Decls? It should improve performance of the cached completions.

But for a quick workaround we could introduce a separate flag.

[arphaman]

In D40562#940201, @ilya-biryukov wrote:

In D40562#939950, @arphaman wrote:

This change breaks cached completions for declarations in namespaces in libclang. What exactly are you trying to achieve here? We could introduce another flag maybe.

Am I right to assume this cache is there to reduce the amount of Decls we need to deserialize from Preambles? Maybe we could fix the cache to also include namespace-level Decls? It should improve performance of the cached completions.

I'm not actually 100% sure, but I would imagine that this one of the reasons, yes. It would be nice to improve the cache to have things like namespace-level Decl, although how will lookup work in that case? Btw, do you think the cache can be reused in clangd as well?

But for a quick workaround we could introduce a separate flag.

[ioeric]

In D40562#941570, @arphaman wrote:

In D40562#940201, @ilya-biryukov wrote:

In D40562#939950, @arphaman wrote:

This change breaks cached completions for declarations in namespaces in libclang. What exactly are you trying to achieve here? We could introduce another flag maybe.

Am I right to assume this cache is there to reduce the amount of Decls we need to deserialize from Preambles? Maybe we could fix the cache to also include namespace-level Decls? It should improve performance of the cached completions.

I'm not actually 100% sure, but I would imagine that this one of the reasons, yes. It would be nice to improve the cache to have things like namespace-level Decl, although how will lookup work in that case? Btw, do you think the cache can be reused in clangd as well?

I took a quick look at the completion cache and lookup code. I think the completion cache also assumes that top-level decls are only TU-level decls, and this assumption seems to be also built into the lookup code. At this point, I am inclined to add a separate completion option for what I want (IgnoreDeclsInTUOrNamespaces?). Regarding cache in clangd, I think it might be useful short-term, when we still use Sema's global code completion, but long term, we would use symbols from clangd's indexes, so the cache would not be useful anymore.

[ilya-biryukov]

In D40562#941570, @arphaman wrote:

I'm not actually 100% sure, but I would imagine that this one of the reasons, yes. It would be nice to improve the cache to have things like namespace-level Decl, although how will lookup work in that case? Btw, do you think the cache can be reused in clangd as well?

As Eric mentioned, we are planning to have project-global completion for namespace-level Decls (to have completion items not #included in the current file and add the #include directive properly). So the cache is probably not that useful to clangd long-term.
For proper lookup in the cache that include all namespace-level Decls I'd go with tweaking LookupVisibleDecls so that it does not deserialize everything from the preamble, but rather provides a list of scopes that we need to get completion items from. Though sounds simple, it may be a non-trivial change and we shouldn't probably pursue it as part of this change.
(We'll probably need it for clangd too).

In D40562#941735, @ioeric wrote:

I took a quick look at the completion cache and lookup code. I think the completion cache also assumes that top-level decls are only TU-level decls, and this assumption seems to be also built into the lookup code. At this point, I am inclined to add a separate completion option for what I want (IgnoreDeclsInTUOrNamespaces?). Regarding cache in clangd, I think it might be useful short-term, when we still use Sema's global code completion, but long term, we would use symbols from clangd's indexes, so the cache would not be useful anymore.

+1 for having a separate flag. Maybe call it IncludeNamespaceLevelDecls instead? (I'd say TU is also a (global) namespace from completion's point of view).

[arphaman]

In D40562#941753, @ilya-biryukov wrote:

In D40562#941570, @arphaman wrote:

I'm not actually 100% sure, but I would imagine that this one of the reasons, yes. It would be nice to improve the cache to have things like namespace-level Decl, although how will lookup work in that case? Btw, do you think the cache can be reused in clangd as well?

As Eric mentioned, we are planning to have project-global completion for namespace-level Decls (to have completion items not #included in the current file and add the #include directive properly). So the cache is probably not that useful to clangd long-term.

Interesting, thanks! Will this be something that clients of clangd can opt-out from? Or at least configure certain aspects of the behaviour?

For proper lookup in the cache that include all namespace-level Decls I'd go with tweaking LookupVisibleDecls so that it does not deserialize everything from the preamble, but rather provides a list of scopes that we need to get completion items from. Though sounds simple, it may be a non-trivial change and we shouldn't probably pursue it as part of this change.
(We'll probably need it for clangd too).

In D40562#941735, @ioeric wrote:

I took a quick look at the completion cache and lookup code. I think the completion cache also assumes that top-level decls are only TU-level decls, and this assumption seems to be also built into the lookup code. At this point, I am inclined to add a separate completion option for what I want (IgnoreDeclsInTUOrNamespaces?). Regarding cache in clangd, I think it might be useful short-term, when we still use Sema's global code completion, but long term, we would use symbols from clangd's indexes, so the cache would not be useful anymore.

+1 for having a separate flag. Maybe call it IncludeNamespaceLevelDecls instead? (I'd say TU is also a (global) namespace from completion's point of view).

I agree with the new flag as well. I would also like to see a followup patch where this change is used before this is committed.

[ioeric]

• Add a new code-completion option IncludeNamespaceLevelDecls. For now, I only restrict this option work for qualified id completion to reduce the impact.

[ioeric]

In D40562#942521, @arphaman wrote:

In D40562#941753, @ilya-biryukov wrote:

In D40562#941570, @arphaman wrote:

I'm not actually 100% sure, but I would imagine that this one of the reasons, yes. It would be nice to improve the cache to have things like namespace-level Decl, although how will lookup work in that case? Btw, do you think the cache can be reused in clangd as well?

As Eric mentioned, we are planning to have project-global completion for namespace-level Decls (to have completion items not #included in the current file and add the #include directive properly). So the cache is probably not that useful to clangd long-term.

Interesting, thanks! Will this be something that clients of clangd can opt-out from? Or at least configure certain aspects of the behaviour?

Absolutely!

[ioeric]

Ping.

(fyi, you could find use of the new option in D40548)

[ioeric]

Ping?

[ilya-biryukov]

I'll LGTM this to unblock @ioeric . We'll also get a test using the flag in clangd soon. @arphaman, let us know if you feel there's something wrong with the patch.

[ioeric]

• Merged with origin/master

lib/Sema/SemaCodeComplete.cpp

void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
   // in a context that permits expressions. This is a general issue with
   // qualified-id completions.
   if (!EnteringContext)
     MaybeAddOverrideCalls(*this, Ctx, Results);
   Results.ExitScope();  
   
   CodeCompletionDeclConsumer Consumer(Results, CurContext);
   LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer,
-                     /*IncludeGlobalScope=*/true,
+                     CodeCompleter->includeGlobals(),
                      /*IncludeDependentBases=*/true);
 
   HandleCodeCompleteResults(this, CodeCompleter, 
                             Results.getCompletionContext(),
                             Results.data(),Results.size());
 }
 
 void Sema::CodeCompleteUsing(Scope *S) {

lib/Sema/SemaLookup.cpp

 
 void Sema::LookupVisibleDecls(DeclContext *Ctx, LookupNameKind Kind,
                               VisibleDeclConsumer &Consumer,
                               bool IncludeGlobalScope,
                               bool IncludeDependentBases) {
   LookupResult Result(*this, DeclarationName(), SourceLocation(), Kind);
   Result.setAllowHidden(Consumer.includeHiddenDecls());
   VisibleDeclsRecord Visited;
-  if (!IncludeGlobalScope)
+  if (!IncludeGlobalScope) {
     Visited.visitedContext(Context.getTranslationUnitDecl());
+    // Declarations in namespace are also considered global.
+    if (Ctx->isNamespace() || Ctx->isTranslationUnit())
+      Visited.visitedContext(Ctx);
+  }
   ShadowContextRAII Shadow(Visited);
   ::LookupVisibleDecls(Ctx, Result, /*QualifiedNameLookup=*/true,
                        /*InBaseClass=*/false, Consumer, Visited,
                        IncludeDependentBases);
 }
 
 /// LookupOrCreateLabel - Do a name lookup of a label with the specified name.
 /// If GnuLabelLoc is a valid source location, then this is a definition

test/CodeCompletion/ignore-global-decls.cpp

+namespace ns {
+  struct bar {
+  };
+
+  struct baz {
+  };
+
+  int func(int a, bar b, baz c);
+}
+
+void test() {
+  ns::
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:12:7 %s -o - | FileCheck %s --check-prefix=CHECK-1
+// CHECK-1-DAG: COMPLETION: bar : bar
+// CHECK-1-DAG: COMPLETION: baz : baz
+// CHECK-1-DAG: COMPLETION: func : [#int#]func(<#int a#>, <#bar b#>, <#baz c#>)
+
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:12:7 -no-code-completion-globals %s -o - | FileCheck %s -allow-empty --check-prefix=CHECK-EMPTY
+// CHECK-EMPTY: {{^}}{{$}}
+}