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

[Nathan Ridge via Phabricator via cfe-commits]

nridge 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), but I did look through the entire patch now, and it looks good to me modulo the mentioned, mostly minor, comments.



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4771
+//
+// 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.
----------------
nit: stray word "it"


================
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 {
----------------
Very interesting idea :)

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


================
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);
----------------
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.


================
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) {
----------------
"given that the expression E is valid"?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4860
+      return;
+    if (auto *CSE = llvm::dyn_cast<ConceptSpecializationExpr>(E)) {
+      // If the concept is
----------------
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.


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4875
+      for (const auto &Arg : CSE->getTemplateArguments()) {
+        if (Index > Params->size())
+          break; // Won't happen in valid code.
----------------
`>=` ?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4878
+        if (isApprox(Arg, T)) {
+          auto *TTPD = dyn_cast<TemplateTypeParmDecl>(Params->getParam(Index));
+          if (!TTPD)
----------------
We're pretty inconsistent about qualifying `dyn_cast` vs. not in this function.


================
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)) {
----------------
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?


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4972
+
+    // In T::foo::bar, `foo` must be a type.
+    bool VisitNestedNameSpecifier(NestedNameSpecifier *NNS) {
----------------
It would be nice if the test exercised this case.


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


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


================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5466
-  NestedNameSpecifier *NNS = SS.getScopeRep();
-  if (!Results.empty() && NNS->isDependent())
     Results.AddResult("template");
----------------
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?)


================
Comment at: clang/test/CodeCompletion/concepts.cpp:34
+  // RUN: | FileCheck %s -check-prefix=DOT -implicit-check-not=xxx_t
+  // DOT: Pattern : [#convertible_to<double>#]aaa()
+  // DOT: Pattern : bb() (requires fix-it: {{.*}} to "->")
----------------
sammccall wrote:
> nridge wrote:
> > 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?)
> 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.
Thanks. No change needed here, just wanted to clarify my own understanding.


================
Comment at: clang/test/CodeCompletion/concepts.cpp:35
+  // DOT: Pattern : [#convertible_to<double>#]aaa()
+  // DOT: Pattern : bb() (requires fix-it: {{.*}} to "->")
+  // DOT: Pattern : bbb()
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73649