[PATCH] D73649: [CodeComplete] Member completion for concept-constrained types.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

> 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.



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4772
+// FIXME: it some of this machinery could be used for non-concept tparms too,
+// enabling completion for type parameters based on other uses of that param.
+class ConceptInfo {
----------------
nridge wrote:
> Very interesting idea :)
> 
> A hypothetical "infer constraints based on usage" code action could share the same infrastructure as well.
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.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4839
+  // that T is attached to in order to gather the relevant constraints.
+  ConceptInfo(const TemplateTypeParmType &BaseType, Scope *S) {
+    auto *TemplatedEntity = getTemplatedEntity(BaseType.getDecl(), S);
----------------
nridge wrote:
> 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.
Yeah, I don't think this is worth the complexity, but added a comment to the class.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4856
+private:
+  // Infer members of T, given that the expression E (dependent on T) is true.
+  void believe(const Expr *E, const TemplateTypeParmType *T) {
----------------
nridge wrote:
> "given that the expression E is valid"?
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`)


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4860
+      return;
+    if (auto *CSE = llvm::dyn_cast<ConceptSpecializationExpr>(E)) {
+      // If the concept is
----------------
njames93 wrote:
> nridge wrote:
> > 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.
> 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. 
Indeed, I'm no longer seeing these locally. Thanks!


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4901
+        if (!Req->isDependent())
+          continue; // Can't tell us anything about T.
+        if (auto *TR = llvm::dyn_cast<concepts::TypeRequirement>(Req)) {
----------------
nridge wrote:
> 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?
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)


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4972
+
+    // In T::foo::bar, `foo` must be a type.
+    bool VisitNestedNameSpecifier(NestedNameSpecifier *NNS) {
----------------
nridge wrote:
> It would be nice if the test exercised this case.
Oops, this was actually broken because VisitNestedNameSpecifier doesn't seem to be a thing :-(
Fixed to use TraverseNestedNameSpecifierLoc and added a test.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5004
+        return;
+      auto &Result = Outer->Results[Name.getAsIdentifierInfo()];
+      Result.Name = Name.getAsIdentifierInfo();
----------------
nridge wrote:
> What if there is already a result for this name?
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.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5044
+      if (S->isTemplateParamScope() && S->isDeclScope(D))
+        return Inner->getEntity();
+      Inner = S;
----------------
nridge wrote:
> Do we need to null-check `Inner` here?
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.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5466
-  NestedNameSpecifier *NNS = SS.getScopeRep();
-  if (!Results.empty() && NNS->isDependent())
     Results.AddResult("template");
----------------
nridge wrote:
> 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?)
Oops, yeah. Not the right time to fix it. Added a FIXME instead.


================
Comment at: clang/test/CodeCompletion/concepts.cpp:35
+  // DOT: Pattern : [#convertible_to<double>#]aaa()
+  // DOT: Pattern : bb() (requires fix-it: {{.*}} to "->")
+  // DOT: Pattern : bbb()
----------------
nridge wrote:
> sammccall wrote:
> > nridge wrote:
> > > 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()`).
> > 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 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.)
Added this as a comment on ConceptInfo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73649