Page MenuHomePhabricator

[clang] Introduce new completion context types
ClosedPublic

Authored by kadircet on Oct 12 2018, 5:02 AM.

Details

Summary

New name suggestions were being used in places where existing names should have been used, this patch tries to fix some of those situations.

Diff Detail

Repository
rC Clang

Event Timeline

kadircet created this revision.Oct 12 2018, 5:02 AM
kadircet updated this revision to Diff 169374.Oct 12 2018, 5:09 AM
  • Use statement as context for using decls.
sammccall added inline comments.Oct 12 2018, 5:19 AM
lib/Sema/SemaCodeComplete.cpp
4847

I'm not sure statement is correct here.
First, CCC_Expression seems strictly better: this can't be ns::while() or such.

Secondly, ISTM there are other cases we might have a qualified ID in a non-expression context, like implementing a function void foo::bar() or so? Are we sure none of those hit this codepath?

No need to fix this now (this code is clearly an improvement) but maybe this is worth a comment like // Usually qualified-IDs occur in expression context or so.

4905–4908

Hmm, I don't think CCC_Namespace is right either. This is a using declaration, not a using directive.
So ultimately we're completing a namespace member, not a namespace itself. But there's no enum value for that: Expression would be strings like vector<T> or foo(), and we want vector or foo.

I'd suggest either adding a new CCC enum value for this (PotentiallyQualifiedName is actually the best name for this! we'd need to come up with a new one), or use Expression with a comment that this isn't really accurate and someone should fix it :-)

kadircet updated this revision to Diff 170628.Oct 23 2018, 6:28 AM
  • Use new types
  • Rebase
kadircet retitled this revision from [clang] Use Statement and Namespace instead of Name and PotentiallyQualifiedName to [clang] Introduce new completion context types.Oct 23 2018, 6:30 AM
kadircet edited the summary of this revision. (Show Details)

There are still some contexts where both a new name and an existing name is permissible, going to add a comment on those to leave some traces for the next person who cares

kadircet updated this revision to Diff 170631.Oct 23 2018, 6:38 AM
  • Add comments.
sammccall added inline comments.Oct 23 2018, 7:31 AM
include/clang/Sema/CodeCompleteConsumer.h
277

It's not obvious what "name" means here, e.g. what distinguishes this from "expression".

I think the answer is "we want the name of *something*, but we don't know what". I think it's ok to say that explicitly. Or to call this CCC_Symbol, etc.

281

what distinguishes potentiallyqualifiedexistingname from existingname?

The one use of ExistingName is the invalid scope case. This seems a) pretty marginal, and b) not to matter to clangd. Does it matter to libclang?

(The behavior is detectable through the API, but the current behavior doesn't seem that sensible or obviously important)

kadircet updated this revision to Diff 170677.Oct 23 2018, 9:25 AM
kadircet marked 2 inline comments as done.
  • Use Symol, SymbolOrNewName, NewName
  • Address comments.
kadircet marked 2 inline comments as done.Oct 24 2018, 1:08 AM
kadircet added inline comments.
include/clang/Sema/CodeCompleteConsumer.h
277

Renaming to CCC_Symbol

281

It kind of matters to libclang, but it is not that important as well. It is used when caching completion items, to decide whether they allow a name specifiers or not. But when I think about the use case, I believe nested name specifiers should be allowed in that context(when scope is invalid, but we still report for global code completion). So seems like it is redundant(not exactly, but currently we have no way of telling it apart due to mentioned fixme).

Logic looks good. Any way we can exercise it with a test via c-index-test?

include/clang/Sema/CodeCompleteConsumer.h
281

nit: try to describe what a symbol is in the comment, rather than just repeating the term?
e.g. "where an existing name is expected (such as a type, function, or variable)"

kadircet updated this revision to Diff 170829.Oct 24 2018, 2:03 AM
kadircet marked an inline comment as done.
  • Update comment.

Since it doesn't change any CXCompletionContext's in CIndex it doesn't seem possible. There are tests being added to clangd(D53192), since it uses context provided by sema to decide whether query the index or not, we can check the behavior.

sammccall accepted this revision.Oct 24 2018, 8:00 AM

(sorry, thought I had accepted this!)

This revision is now accepted and ready to land.Oct 24 2018, 8:00 AM
This revision was automatically updated to reflect the committed changes.
nik added a subscriber: nik.Jul 17 2019, 1:59 AM

I've bisected https://bugs.llvm.org/show_bug.cgi?id=42646 to this change. Reverting it fixes the issue.

Please look into it.

Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2019, 1:59 AM