[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

include/clang/Driver/CC1Options.td

 def code_completion_at_EQ : Joined<["-"], "code-completion-at=">,
   Alias<code_completion_at>;
 def code_completion_macros : Flag<["-"], "code-completion-macros">,
   HelpText<"Include macros in code-completion results">;
 def code_completion_patterns : Flag<["-"], "code-completion-patterns">,
   HelpText<"Include code patterns in code-completion results">;
 def no_code_completion_globals : Flag<["-"], "no-code-completion-globals">,
   HelpText<"Do not include global declarations in code-completion results.">;
+def no_code_completion_ns_level_decls : Flag<["-"], "no-code-completion-ns-level-decls">,
+  HelpText<"Do not include declarations inside namespaces (incl. global namespace) in the code-completion results.">;
 def code_completion_brief_comments : Flag<["-"], "code-completion-brief-comments">,
   HelpText<"Include brief documentation comments in code-completion results.">;
 def disable_free : Flag<["-"], "disable-free">,
   HelpText<"Disable freeing of memory on exit">;
 def discard_value_names : Flag<["-"], "discard-value-names">,
   HelpText<"Discard value names in LLVM IR">;
 def load : Separate<["-"], "load">, MetaVarName<"<dsopath>">,
   HelpText<"Load the named plugin (dynamic shared object)">;

include/clang/Sema/CodeCompleteConsumer.h

public:
   }
 
   /// \brief Whether the code-completion consumer wants to see code patterns.
   bool includeCodePatterns() const {
     return CodeCompleteOpts.IncludeCodePatterns;
   }
 
   /// \brief Whether to include global (top-level) declaration results.
-  bool includeGlobals() const {
-    return CodeCompleteOpts.IncludeGlobals;
+  bool includeGlobals() const { return CodeCompleteOpts.IncludeGlobals; }
+
+  /// \brief Whether to include declarations in namespace contexts (including
+  /// the global namespace). If this is false, `includeGlobals()` will be
+  /// ignored.
+  bool includeNamespaceLevelDecls() const {
+    return CodeCompleteOpts.IncludeNamespaceLevelDecls;
   }
 
   /// \brief Whether to include brief documentation comments within the set of
   /// code completions returned.
   bool includeBriefComments() const {
     return CodeCompleteOpts.IncludeBriefComments;
   }
 

include/clang/Sema/CodeCompleteOptions.h

public:
   unsigned IncludeMacros : 1;
 
   /// Show code patterns in code completion results.
   unsigned IncludeCodePatterns : 1;
 
   /// Show top-level decls in code completion results.
   unsigned IncludeGlobals : 1;
 
+  /// Show decls in namespace (including the global namespace) in code
+  /// completion results. If this is 0, `IncludeGlobals` will be ignored.
+  ///
+  /// Currently, this only works when completing qualified IDs (i.e.
+  /// `Sema::CodeCompleteQualifiedId`).
+  /// FIXME: consider supporting more completion cases with this option.
+  unsigned IncludeNamespaceLevelDecls : 1;
+
   /// Show brief documentation comments in code completion results.
   unsigned IncludeBriefComments : 1;
 
-  CodeCompleteOptions() :
-      IncludeMacros(0),
-      IncludeCodePatterns(0),
-      IncludeGlobals(1),
-      IncludeBriefComments(0)
-  { }
+  CodeCompleteOptions()
+      : IncludeMacros(0), IncludeCodePatterns(0), IncludeGlobals(1),
+        IncludeNamespaceLevelDecls(1), IncludeBriefComments(0) {}
 };
 
 } // namespace clang
 
 #endif
 

lib/Frontend/CompilerInvocation.cpp

static InputKind ParseFrontendArgs(FrontendOptions &Opts, ArgList &Args,
   Opts.IncludeTimestamps = !Args.hasArg(OPT_fno_pch_timestamp);
 
   Opts.CodeCompleteOpts.IncludeMacros
     = Args.hasArg(OPT_code_completion_macros);
   Opts.CodeCompleteOpts.IncludeCodePatterns
     = Args.hasArg(OPT_code_completion_patterns);
   Opts.CodeCompleteOpts.IncludeGlobals
     = !Args.hasArg(OPT_no_code_completion_globals);
+  Opts.CodeCompleteOpts.IncludeNamespaceLevelDecls
+    = !Args.hasArg(OPT_no_code_completion_ns_level_decls);
   Opts.CodeCompleteOpts.IncludeBriefComments
     = Args.hasArg(OPT_code_completion_brief_comments);
 
   Opts.OverrideRecordLayoutsFile
     = Args.getLastArgValue(OPT_foverride_record_layout_EQ);
   Opts.AuxTriple =
       llvm::Triple::normalize(Args.getLastArgValue(OPT_aux_triple));
   Opts.FindPchSource = Args.getLastArgValue(OPT_find_pch_source_EQ);

lib/Sema/SemaCodeComplete.cpp

void Sema::CodeCompleteQualifiedId(Scope *S, CXXScopeSpec &SS,
   //
   // FIXME: This isn't wonderful, because we don't know whether we're actually
   // in a context that permits expressions. This is a general issue with
   // qualified-id completions.
   if (!EnteringContext)
     MaybeAddOverrideCalls(*this, Ctx, Results);
   Results.ExitScope();
 
+  if (CodeCompleter->includeNamespaceLevelDecls() ||
+      (!Ctx->isNamespace() && !Ctx->isTranslationUnit())) {
     CodeCompletionDeclConsumer Consumer(Results, CurContext);
     LookupVisibleDecls(Ctx, LookupOrdinaryName, Consumer,
                        /*IncludeGlobalScope=*/true,
                        /*IncludeDependentBases=*/true);
+  }
 
   auto CC = Results.getCompletionContext();
   CC.setCXXScopeSpecifier(SS);
 
   HandleCodeCompleteResults(this, CodeCompleter, CC, Results.data(),
                             Results.size());
 }
 

test/CodeCompletion/ignore-ns-level-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-ns-level-decls %s -o - | FileCheck %s --check-prefix=CHECK-EMPTY
+// CHECK-EMPTY-NOT: COMPLETION: bar : bar
+// CHECK-EMPTY: {{^}}{{$}}
+}