[PATCH] D46751: [clangd] Filter out private proto symbols in SymbolCollector.

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

> I think this is good if that's true that the comment is always there. I think it would be OK for this to be enabled by default, with a general option to turn heuristics off. Not sure what to call it... -use-symbol-filtering-heuristics :)

@malaperle Having an option for filtering heuristics seems a bit confusing. We have other filters in the symbol collector that we think could improve user experience, and we don't provide options for those. Similarly, for proto headers, I think we could also get away without such an option if we strike for low/no false positive (e.g. correctly identify proto headers). If folks run into problems with the filter, we would like to understand the use cases and improve the filters. In general, we think the proto filter, when it works, would improve user experience.

>> I'm not a big fan of parsing file comment in proto. It seems a bit cumbersome and we might not be able (or too expensive) to do so for completion results from sema (if we do want to filter at completion time).
> 
> Why do you feel it's cubersome? Getting the first line of the file from SourceManager and looking at it seems easy.
>  I certainly don't see why this should be expensive if we do it at the right time (perhaps it means doing that when building the preamble and stashing the results alongside it, but that's also easy in our current setup).

@ilya-biryukov You are right, getting a working solution seems easy enough. I was more concerned about finding a design that doesn't intrude completion workflow with library specific logic. But this makes more sense now that we are not doing the filtering on code completion workflow.



================
Comment at: clangd/index/SymbolCollector.cpp:156
+  if (isPrivateProtoSymbol(ND, ASTCtx->getSourceManager()))
+    return true;
+
----------------
ilya-biryukov wrote:
> ioeric wrote:
> > ilya-biryukov wrote:
> > > We should also run the same code to filter our those decls coming from sema completions.
> > > Otherwise, they might still pop up in completion results if internal proto symbols were deserialized from preamble by clang before running the code completion.
> > Just want to clarify before going further. IIUC, in *index-based* completion, the preamble could still contain symbols from headers such that sema completion could still give you symbols from headers. 
> > 
> > If we do need to build the filter into code completion, we would need more careful design as code completion code path is more latency sensitive.
> For reference. As discussed outside this thread, we might have decls from headers in the sema completion items.
> It does not seem too hard to add filtering for those as well: essentially, we just need to call the same function at code completion time.
After further discussion, we agreed that filtering on code completion results may not be the right approach. For example, it would break assumptions in code completion workflow e.g. limit of completion results from index. Alternatively, we could push filtering to the symbol source level. Currently, we have two sources of symbols: sema and index, and both of them can gather symbols from headers, which are then merged in code completion. With this setup, we would need to apply filters on both sources if we want to do any filtering on header symbols. One solution (for index-based completion) is to make sema only collect symbols in main file (just for code completion) and make indexer index headers (current behavior), where we would only need to filter on index. This doesn't address problem for sema-only code completion, but we think it's not a priority to strike for feature parity between sema-based completion and index-based completion, which we don't really have at this point.

So to proceed:
1) I'll go ahead with the current approach (filter index only) with a stricter check for proto headers.
2) Make sema only collect symbols in main files.
3)  Potentially also apply the filter on sema completion results. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46751