[PATCH] D107696: [CodeComplete] Basic code completion for attribute names.

Mon Aug 9 10:01:05 PDT 2021

sammccall planned changes to this revision.
sammccall added a comment.

Thanks, I know what to do next!

While I have you here, any thoughts on future patches:

> Only the bare name is completed, with no args.
> For args to be useful we need arg names. These *are* in the tablegen but
> not currently emitted in usable form, so left this as future work.

How do you feel about this plan:

- add an `ArrayRef<const char*>` to `ParsedAttrInfo` for this purpose? (Or a null-terminated `char**` if we're worried about sizeof).
- It'd be populated by the names of the tablegen `Args`.
- If empty, completions render as now. If nonempty they render as `foo(bar, baz)` where `bar` and `baz` are placeholders - just like function args but without types.

> There's also no filtering by langopts: this is because currently the
> only way of checking is to try to produce diagnostics, which requires a
> valid ParsedAttr which is hard to get.

And for this, WDYT about making `diagLangOpts` non-virtual and moving the attr-specific test into a `virtual bool supportsLangOpts()` function with no side-effects?

================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4357
+        continue;
+      llvm::StringRef Name = S.NormalizedFullName;
+      if (Completion == AttributeCompletion::Scope) {
----------------
aaron.ballman wrote:
> sammccall wrote:
> > aaron.ballman wrote:
> > > Should we also add some special handling for attributes optionally starting with double underscores? e.g., `__attribute__((const))` and `__attribute__((__const__))`` are both equally useful to complete.
> > > 
> > > Also, we should add some explicit testing for `omp::sequence` and `omp::directive` as those are handled very specially and won't appear in the parsed attribute map. I think the OpenMP code completion would be super useful, but can be handled in a follow-up if you want.
> > > Should we also add some special handling for attributes optionally starting with double underscores?
> > 
> > I think so. Two questions:
> >  - Do I understand right that this is "just" a matter of adding leading/trailing `__` as a second option, for AS_GNU?
> >  - are there similar rules for other syntaxes I've missed?
> > 
> > Offering both seems potentially confusing for users who don't care (especially in the case of const!). But I guess enough users will care about macros. At least in clangd the underscore versions will get ranked lower for having "internal" names though.
> > 
> > FWIW The no-underscores version appears to be ~3x more common (87k vs 27k hits in third-party code in google's repo). Among headers, no-underscores is "only" 2x more common (40k vs 21k). 
> > 
> > ---
> > 
> > > Also, we should add some explicit testing for omp::sequence and omp::directive as those are handled very specially and won't appear in the parsed attribute map. 
> > 
> > Yeah, I punted on these because it seems they will need special case logic, I'll add some tests that they don't do anything.
> > Do I understand right that this is "just" a matter of adding leading/trailing __ as a second option, for AS_GNU?
> > are there similar rules for other syntaxes I've missed?
> 
> Clang supports GNU attributes in either `__attribute__((foo))` or `__attribute__((__foo__))` forms. So I'd say that autocompleting after the second `(` should either suggest attributes (preferred) or `__` (for the poor folks writing libraries). If the user wants to autocomplete after `__attribute__((__`, I think it should suggest `foo__` as the rest of the attribute name. (Basically, if the user looks like they want underscores, give them all the underscores.)
> 
> Clang also supports `[[]]` attributes but with somewhat different rules. We support `[[gnu::attr]]`, `[[__gnu__::attr]]`, `[[gnu::__attr__]]`, and `[[__gnu__::__attr__]]` for GCC attributes. We support `[[clang::attr]]`, `[[_Clang::attr]]`, `[[clang::__attr__]]`, and `[[_Clang::__attr__]]` for Clang attributes. For vendors other than Clang and GCC, we don't support any additional underscores for either the scope or the attribute name. I would say that if the user asked for underscores for the vendor scope, they likely want the underscores for the attribute as well.
> 
> I suppose there's a third case. That horrible `using` syntax that I've never really seen used in the wild. e.g., ``[[using clang: attr]``. We do support the underscore behavior there as well.
> 
> > Offering both seems potentially confusing for users who don't care (especially in the case of const!). But I guess enough users will care about macros.
> 
> Yeah, users who are writing portable libraries are far more likely to care than users writing typical application code.
> So I'd say that autocompleting after the second ( should either suggest attributes (preferred) or __ (for the poor folks writing libraries).

This is clever, unfortunately it doesn't really work for other reasons:
 - LSP allows clients to "narrow down" results as an identifier is typed with client-side filtering, and `_` is treated as an identifier character.
 - clang's code completion similarly returns all matches and leaves partial-identifier-filtering them to the CodeCompleteConsumer

I can't see a better plan than including both the underscore and non-underscore versions, with the latter downranked. Doesn't seem *too* bad.

> Clang also supports [[]] attributes...

Yeah, I think what we should do here is pretend the scoped namespace contains only the scoped attribute and vice versa:
 - When we want qualified attributes, emit `gnu::attr` and `__gnu__::__attr__` but no mixing
 - When we want scopes (after `using`), emit `gnu` and `__gnu__`
 - When we want unqualified attributes (after `ns::` or `using ns:`), emit either `attr` or `__attr__` depending on the scope

> That horrible using syntax

I really don't understand why this is worthwhile. Maybe feature parity with XMLNS? :-)
In any case, it happens to be easy to handle.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107696/new/

https://reviews.llvm.org/D107696