Page MenuHomePhabricator

[clangd] Use identifiers in file as completion candidates when build is not ready.
ClosedPublic

Authored by ioeric on Apr 2 2019, 6:28 AM.

Details

Summary

o Lex the code to get the identifiers and put them into a "symbol" index.
o Adds a new completion mode without compilation/sema into code completion workflow.
o Make IncludeInserter work even when no compile command is present, by avoiding
inserting non-verbatim headers.

Diff Detail

Event Timeline

ioeric created this revision.Apr 2 2019, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 6:28 AM
sammccall added inline comments.Apr 4 2019, 5:28 AM
clangd/CodeComplete.cpp
1142

as discussed offline, I love the lexer approach but returning it as an index seems like the wrong level of abstraction.
These are just strings, and if we want to tweak e.g. the scoring or merging of these we'll be in a better position to do so if we don't pretend they have Symbol semantics (their own signals, ids, etc).
So I'd suggest this fn should return something like StringSet<> or StringMap<unsigned> for counts.

Can we move it to SourceCode.h too?

1142

I don't think we need FileName as a parameter - it shouldn't affect the return value, so we can just use a dummy value.

1189

I'm not sure these changes (mostly to comment formatting) improve clarity.
I think it would be enough to add a comment inside the top of runWithoutCompile saying // Fill in fields normally set by runWithSema() or so

1192

ReplacedRange?

1289

runWithoutSema maybe? - compile is a fine name but this file calls this concept sema for whatever reason

1291

hmm, this function should likely also move to SourceCode, use Offset instead of Position, and move to SourceCode.h. But probably a different patch.

1312

you could just erase the typed filter from the suggestion list.
(It may be a valid word spelled elsewhere, but there's no point suggesting it)

1328

I suspect you want to pass the filtered strings through to mergeResults and have it/CompletionCandidate do the scoring.

clangd/CodeComplete.h
248

It seems a little odd to have a different entrypoint here.
Could we simply indicate this with Preamble=nullptr?

It seems it might even make sense to use the same speculative fuzzyfind?
So I think PCHContainerOperations would be the only odd one out. I'm going to send a patch to remove that anyway :-)

ioeric updated this revision to Diff 194311.Apr 9 2019, 6:42 AM
ioeric marked 9 inline comments as done.
  • address review comments
ioeric updated this revision to Diff 194315.Apr 9 2019, 6:49 AM
  • minor cleanup
ioeric added a comment.Apr 9 2019, 6:49 AM

Thanks for the review!

clangd/CodeComplete.cpp
1291

ack

1312

This is following conventions of other sources. Both index and sema provide results for fully-typed names. Considering that users might be already used to this, and completion results tend to give reassurance for correctly typed names, I am inclined to keep the typed filter if it's seen somewhere else in the file.

Very nice! D60503 will conflict, feel free to stall that until this is landed.
On the other hand it will simplify some things, e.g. the prefix is already calculated, and typed scope is available if you want that (no enclosing namespaces though).

clangd/CodeComplete.cpp
185

This is a bit vague - most results are for identifiers in some sense.
Maybe RawIdentifier?

281

move this comment to the member?
The invariant is important to readers, not just callers of the constructor.

1178

rename NBoth -> NSemaAndIndex?

1306

it doesn't make sense to set this here, as long as we're not querying the index.

1312

OK, but we should now be able to just decrement the count for the right entry in the collected identifiers then? rather than carving up the string

1355

add this to the non-sema case too (though the if is always false for now), or add a fixme?
Worried about forgetting this.

1516

why are we not initializing fileproximity in the fallback case?
(To only a single source)

1558

can we have a SymbolOrigin bit for identifiers, and use that instead?

clangd/Headers.cpp
182

nit: i'd move this above the previous case, it seems both more fundamental and cheaper

196

Do we expect this code path to ever be called?
We may want to assert or elog here, depending on how this can be hit.

clangd/SourceCode.cpp
403

why isn't Add just ++Identifiers[Id]?

ioeric updated this revision to Diff 194647.Apr 11 2019, 1:32 AM
ioeric marked 14 inline comments as done.
  • address review comments
clangd/CodeComplete.cpp
1355

Added a FIXME.

clangd/Headers.cpp
196

This can be hit when we start serving index symbols in fallback mode completion. shouldInsertInclude will return false, but the calculation will be attempted regardless. We can probably elog this, but the number of log messages would be O(# of index symbols), which I worry might be too spammy.

sammccall accepted this revision.Apr 11 2019, 2:26 AM
sammccall added inline comments.
clangd/CodeComplete.cpp
345

Completion.Kind = CompletionItemKind::Text

1318

comment should explain why, rather than what :-)

This revision is now accepted and ready to land.Apr 11 2019, 2:26 AM
ioeric updated this revision to Diff 194652.Apr 11 2019, 2:31 AM
ioeric marked 2 inline comments as done.
  • address comments
This revision was automatically updated to reflect the committed changes.