[PATCH] D141226: [clangd] support expanding `decltype(expr)`

[Sam McCall via Phabricator via llvm-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:139
 
-  // if it's a lambda expression, return an error message
-  if (isa<RecordType>(*DeducedType) &&
-      cast<RecordType>(*DeducedType)->getDecl()->isLambda()) {
-    return error("Could not expand type of lambda expression");
+  // we shouldn't replace a dependent type, e.g.
+  //   template <class T>
----------------
v1nh1shungry wrote:
> sammccall wrote:
> > v1nh1shungry wrote:
> > > sammccall wrote:
> > > > why not? replacing this with `T` seems perfectly reasonable.
> > > > (The fact that we don't do this with `auto t = T{}` is just because we're hitting a limitation of clang's AST)
> > > I'd like it too. But the `printType()` would give out a `<dependent type>`. Though I haven't looked into this, would you mind giving some suggestions about it?
> > Ah, there are three subtly different concepts here:
> >  - is this type usefully printable? There are various reasons why it may not be, it can contain DependentTy, or a canonical template parameter (`<template-param-0-0>`), or a lambda.
> >  - is this type dependent in the language sense - an example is some type parameter `T`. This may or may not be usefully printable. (e.g. try `template<class X> std::vector<X> y;` and hover on y)
> >  - is this type specifically `DependentTy`, the placeholder dependent type which we have no information about. This is never usefully printable: prints as `<dependent type>`
> > 
> > As a heuristic, I'm happy with saying dependent types (in the language sense) are likely not to print usefully, so don't replace them. But the comment should say so (this is a heuristic for unprintable rather than an end in itself), and probably give examples.
> Hmm, do you mean it's okay to refuse to replace dependent types but I should leave a comment saying the reason is that they are likely not to print usefully? Sorry if I misunderstood anything! I'm really not a good reader :(
Yeah, exactly!


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandDeducedType.cpp:151
+  //   ^^^^^^^^^^^^^^ is `int[10]`
+  if ((*DeducedType)->isArrayType())
+    return error("Could not expand type of array");
----------------
v1nh1shungry wrote:
> sammccall wrote:
> > the commonality between these cases you're excluding (and the function types below) is that they use C-style declarator syntax that may have chunks following the declarator-id. Specifically:
> >  - array, function, and paren types always have chunks following the declarator
> >  - pointer and reference types compose inside-out so their pointee types may contain trailing chunks
> > 
> > If we want to make some attempt to detect more cases, we should pull out a function here and do it properly, something like: `bool hasTrailingChunks(QualType)` which calls recursively for pointertype etc.
> > 
> > But there's a cheaper way: when we call `printType()` below, we can optionally specify the declarator-id. Then we can simply check whether it's at the end of the string:
> > 
> > ```
> > std::string PrettyDeclarator = printType(..., "DECLARATOR_ID");
> > llvm::StringRef PrettyType = PrettyDeclarator;
> > if (!PrettyType.consume_back("DECLARATOR_ID"))
> >   return error("could not expand non-contiguous type {0}", PrettyType);
> > PrettyType = PrettyType.rtrim();
> > ```
> > 
> > This feels slightly hacky, but it's direct and simple and we shouldn't miss a case.
> TBH, I'm confused about `printType()` and its placeholder `DECLARATOR_ID`. Is the code your offered above can be used directly?
> 
> If so, I had a try on it and it did refuse to replace types like function and array. But it would give an error message saying "Could not expand non-contiguous type const char (&DECLARATOR_ID)[6]". Is this okay? I mean isn't the "DECLARATOR_ID" in the message a bit confusing?
That was the idea, but I think you're right the error message is confusing.

We need to strike a balance between being precise, being easy to understand, and being reasonable to maintain.

I think it's probably best to be a bit vague here rather than using precise but obscure language, maybe "could not expand type that isn't a simple string".
Whether to leave out the actual type, include it with DECLARATOR_ID in it, or call printType() again with an empty placeholder is up to you - I don't think any of those are too confusing in context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141226