This is an archive of the discontinued LLVM Phabricator instance.

[CodeComplete] Member completion for concept-constrained types.
ClosedPublic

Authored by sammccall on Jan 29 2020, 10:40 AM.

Details

Summary

The basic idea is to walk through the concept definition, looking for
t.foo() where t has the constrained type.

In this patch:

  • nested types are recognized and offered after ::
  • variable/function members are recognized and offered after the correct dot/arrow/colon trigger
  • member functions are recognized (anything directly called). parameter types are presumed to be the argument types. parameters are unnamed.
  • result types are available when a requirement has a type constraint. These are printed as constraints, except same_as<T> which prints as T.

Not in this patch:

  • support for merging/overloading when two locations describe the same member. The last one wins, for any given name. This is probably important...
  • support for nested template members (T::x<int>)
  • support for completing members of (instantiations of) template template parameters

Diff Detail

Event Timeline

sammccall created this revision.Jan 29 2020, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2020, 10:40 AM

Unit tests: pass. 62282 tests passed, 0 failed and 827 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 7 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Thanks for looking at this! Sorry for the late response, I was travelling for a few weeks.

So far I've only had a chance to look at the tests.

clang/test/CodeCompletion/concepts.cpp
35

Doesn't the presence of the x mean we should only get results that start with x?

(Or, if "column 5" actually means we're completing right after the dot, why is the x present in the testcase at all -- just so that the line is syntactically well formed?)

36

Should we be taking completions from just one branch of a logical-or in a requirement?

To simplify the scenario a bit:

template <typename T>
  requires (requires(T t) { t.foo(); } || requires(T t) { t.bar(); })
void f(T t) {
  t.^
}

Do we want to be offering both foo() and bar() as completions here? Logically, it seems we only ought to offer completions from expressions that appear in _both_ branches of the logical-or (so, if t.foo() appeared as a requirement in both branches, we could offer foo()).

sammccall marked 3 inline comments as done.Feb 18 2020, 4:52 PM
sammccall added inline comments.
clang/lib/Sema/CodeCompleteConsumer.cpp
592

(This is just a fix to the -code-complete-at testing facility: it wasn't printing fixits for RK_Pattern completions)

clang/test/CodeCompletion/concepts.cpp
35

Yeah we're completing before the x.
And in fact it doesn't matter, since clang's internal completion engine doesn't filter the results using the incomplete identifier (which is what lets clangd apply its own fuzzy-matching rules).

I think this is well-enough understood around the CodeCompletion tests and I tend to worry about incomplete lines confusing the parser (I don't want to repeat this function decl 3 times!), but I can try to drop these if you think it makes a big difference.

36

Strictly speaking yes, a function is only guaranteed available if it's in both branches.

In practice I think this behavior would be no more useful (probably less useful), and obviously more complicated to implement (as we have to track result sets for subexpressions to intersect them).

AIUI the language has no way to force you to only use properties guaranteed by the constraints. So it's perfectly plausible to write something like (forgive suspicious syntax)

template <typename T> requires Dumpable<T> || Printable<T>
void output(const T &val) {
  if constexpr (Dumpable<T>)
    val.dump();
  else
    val.print();
}

Or even cases where the same expression is valid in all cases but we lose track of that somehow (e.g. T is either a dumb pointer or a smart pointer to a constrained type, and our analysis fails on the smart pointer case).

Basically I think we're desperate enough for information in these scenarios that we'll accept a little inaccuracy :-)

I'm not sure that I'm qualified to approve this patch (this is my first time looking at clang's completion code), but I did look through the entire patch now, and it looks good to me modulo the mentioned, mostly minor, comments.

clang/lib/Sema/SemaCodeComplete.cpp
4771

nit: stray word "it"

4772

Very interesting idea :)

A hypothetical "infer constraints based on usage" code action could share the same infrastructure as well.

4839

Minor observation: based on the current usage of this class, we know at construction time which AccessOperator we want. We could potentially pass that in and use it to do less work in the constructor (filtering members earlier). However, it's not clear that this is worth it, we'd only be avoiding a small amount of work.

4856

"given that the expression E is valid"?

4860

clang-tidy gives me an 'auto *CSE' can be declared as 'const auto *CSE' here, and several similar diagnostics below.

Not sure if that's something we want to obey, or alter our configuration to silence it.

4875

>= ?

4878

We're pretty inconsistent about qualifying dyn_cast vs. not in this function.

4901

Are we sure a dependent requirement can't tell us anything about T?

For example, a constraint with a dependent return type requirement might still give us a useful member name and arguments. Even a call expression with dependent arguments could give us a useful member name.

Or am I misunderstanding what Requirement::isDependent() signifies?

4972

It would be nice if the test exercised this case.

5004

What if there is already a result for this name?

5044

Do we need to null-check Inner here?

5466

Is the removal of the !Results.empty() condition fixing a pre-existing bug? (i.e. it looks like it's not possible for Results to be nonempty at this stage?)

clang/test/CodeCompletion/concepts.cpp
35

Thanks. No change needed here, just wanted to clarify my own understanding.

36

I think in the mentioned example, the ideal behaviour would be to offer dump() as a completion only in the then-branch of the if constexpr, and offer print() only in the else-branch.

But that's quite a bit more involved to implement, and I agree that for now, it's more useful to offer both completions in the entire function than to offer neither.

Maybe just add a "TODO" comment (or even just a "to potentially explore in the future" comment) about this? (Could be in the code rather than the test.)

njames93 added inline comments.
clang/lib/Sema/SemaCodeComplete.cpp
4860

That warning will go away next time the bot updates the version of clang tidy it uses. It was decided to reduce the constraints on diagnosing that check inside llvm but that hasn't reached the pre merge bot.

sammccall updated this revision to Diff 247751.Mar 2 2020, 4:15 PM
sammccall marked 27 inline comments as done.

Address review comments

I'm not sure that I'm qualified to approve this patch (this is my first time looking at clang's completion code)

I feel the same way about writing the code :-)
Thanks for the comments! I'll get another review from someone who's stared into this abyss before, too.

clang/lib/Sema/SemaCodeComplete.cpp
4772

Yup. There'd need to be some differences - this code uses Scope which is available during parsing but not after, a code action would need to use AST walking, which is available after parsing but not during :-)

This is pretty hypothetical/far out/needs prototyping, so I wouldn't try to solve any of the layering/API problems now.

4839

Yeah, I don't think this is worth the complexity, but added a comment to the class.

4856

E comes from concept constraints, we actually know that E is not only valid but its value is exactly true.

In particular, knowing that E is *valid* doesn't tell us anything at all about T if E is a ConceptSpecializationExpr like Integral<T>, as we'd get from a requires Integral<T> clause or an Integral auto foo parameter. (Note that Integral<std::string> is a valid expression with the value false)

4860

Indeed, I'm no longer seeing these locally. Thanks!

4901

I think you're reading this backwards, a *non-dependent* requirement can't tell us anything about T, because it doesn't depend on T!

This isn't terribly important except that it takes care of a few cases at once (e.g. all requirements below must have an expr rather than an error, because constraints with an error aren't dependent)

4972

Oops, this was actually broken because VisitNestedNameSpecifier doesn't seem to be a thing :-(
Fixed to use TraverseNestedNameSpecifierLoc and added a test.

5004

Right, it's doing some arbitrary clobber/merge thing :-(

Fixed: now if we get a conflict we pick either the new or the old one, where more info is better.

5044

Done. It's that or assert that the initial S is never the declaring scope of D, which it never seems to be.
But these are just passed through from elsewhere in sema and I'm really leery of relying on any undocumented invariants.

5466

Oops, yeah. Not the right time to fix it. Added a FIXME instead.

clang/test/CodeCompletion/concepts.cpp
36

Added this as a comment on ConceptInfo.

nridge marked 3 inline comments as done.Mar 3 2020, 12:28 PM
nridge added inline comments.
clang/lib/Sema/SemaCodeComplete.cpp
4856

You're totally right on this one, my bad! (When I wrote this, I imagined believe() being called on the expression inside an ExprRequirement, but that's not the case.)

4901

Indeed, I was reading this backwards. Makes sense now!

4972

Oops, this was actually broken because VisitNestedNameSpecifier doesn't seem to be a thing :-(

(I suspected this, but the heavy macro usage in RecursiveASTVisitor.h made me second-guess myself and think I was just overlooking a place that defines VisitNestedNameSpecifier. I figured adding a test wouldn't hurt even if I'm mistaken and the code works. :-))

5006

So Colons is more info than Arrow which is more info than Dot? Is there some intuition behind that?

nridge added inline comments.Mar 3 2020, 12:47 PM
clang/lib/Sema/SemaCodeComplete.cpp
4972

(Tangentially related, but a C++ editor feature I sometimes wish existed was to show a (semantically-colored and navigation-feature-enabled) post-preprocessing view of a source file. (But not edit that view! That would be madness.) Something to add to the backlog, perhaps.)

sammccall updated this revision to Diff 247993.Mar 3 2020, 12:47 PM

comment and tweak order of AccessOperator

sammccall marked an inline comment as done.Mar 3 2020, 12:49 PM
sammccall added inline comments.
clang/lib/Sema/SemaCodeComplete.cpp
5006

no, it's basically arbitrary.
I don't think collisions between these are likely and worth dealing with carefully, but making the choice consistently seems worthwhile to me, in case someone ever has to debug it.

nridge added a comment.Mar 3 2020, 1:00 PM

Thanks! Consider the patch "accepted" from my POV.

One last nit: please link to https://github.com/clangd/clangd/issues/261 in the commit message

@kadircet I realized this change was gathering dust, and you've touched SemaCodeComplete in recent memory... any interest?

kadircet added inline comments.Mar 26 2020, 4:22 AM
clang/include/clang/Sema/Scope.h
323

also mark the member as const ?

clang/lib/Sema/SemaCodeComplete.cpp
4905

nit s/concepts::Req../auto

4910

TR->getType might fail if there was a substitution failure. check for it before ?

if(TR->isSubstitutionFailure()) continue;

4984

as T is dependent i suppose NNS should always be an identifier, but would be nice to spell it out explicitly with a comment.

5007

so we'll give up result in case of (syntax might be wrong):

... concept C requires { T::foo; { t.foo() }-> bar }

assuming we first traversed t.foo and then T::foo ? I would rather move operator to the end.

5077

s/::SmallVector/llvm::SmallVector

5094

maybe rename this to deduceType?

Also some comments explaining that this is a beautify heuristic might be nice.

5168

nit:

// ObjcPointers(properties) and TTPT(concepts) are handled below, bail out for the resst.
else if (!objcPointer && ! TTPT) return false;
5830

SS.isNotEmpty is same as NNS!=nullptr, maybe replace with that to make it more clear ?

sammccall marked 13 inline comments as done.

address review comments

Thanks!

clang/lib/Sema/SemaCodeComplete.cpp
4905

I actually don't think the type is obvious here... before I got closely familiar with the concepts AST, I assumed these were expressions.

4910

substitution failures aren't dependent, so we already bailed out in that case. Added a comment.

4984

It doesn't need to be an identifier - it returns null and addType handles null.

5007

Done. This won't ever happen, though.

5077

What the heck?

(preferred spelling seems to be SmallVector here)

5168

I actually find the 3 cases much harder to spot here, and there's no nice place to put the comment.
Added more braces and extended the comment, WDYT?

kadircet accepted this revision.Mar 26 2020, 2:05 PM

LGTM. thanks!

clang/lib/Sema/SemaCodeComplete.cpp
4984

i thought NNS->getAsIdentifier had an assertion instead of returning null.

out of curiosity, when can this be non identifier though ?

This revision is now accepted and ready to land.Mar 26 2020, 2:05 PM
sammccall marked an inline comment as done.Mar 26 2020, 3:02 PM
sammccall added inline comments.
clang/lib/Sema/SemaCodeComplete.cpp
4984

In general (non-dependent code) NNS is normally a decl rather than an identifier.

But here in particular it can certainly be a typespec: that's the T::foo<X>::bar case mentioned in the comment, I think the NNS stores a TemplateSpecializationType for foo<X>.

This revision was automatically updated to reflect the committed changes.