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

Authored by ioeric on Tue, Nov 28, 8:28 AM.

Diff Detail

Repository
rL LLVM
ioeric created this revision.Tue, Nov 28, 8:28 AM

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 updated this revision to Diff 124740.Wed, Nov 29, 6:33 AM
  • Clarify comment for includeGlobals()

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 requested changes to this revision.Wed, Nov 29, 4:49 PM
This revision now requires changes to proceed.Wed, Nov 29, 4:49 PM
arphaman added a comment.EditedWed, Nov 29, 4:52 PM

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:

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.

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 added a comment.Fri, Dec 1, 1:58 AM

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.

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).

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'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).

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 updated this revision to Diff 125777.Wed, Dec 6, 11:45 AM
  • Add a new code-completion option IncludeNamespaceLevelDecls. For now, I only restrict this option work for qualified id completion to reduce the impact.

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 added a comment.Thu, Dec 7, 8:46 AM

Ping.

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

ilya-biryukov accepted this revision.Wed, Dec 13, 1:57 AM

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 updated this revision to Diff 126694.Wed, Dec 13, 2:14 AM
  • Merged with origin/master
This revision was not accepted when it landed; it landed in state Needs Review.Wed, Dec 13, 2:27 AM
This revision was automatically updated to reflect the committed changes.