[PATCH] D119537: [clangd] Treat 'auto' params as deduced if there's a single instantiation.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D119537#3323096 <https://reviews.llvm.org/D119537#3323096>, @Trass3r wrote:

> Is it intentional that the resolved type is not shown in the variable hover (I guess, looking at the code):
>
>   param payload
>   Type: const auto &
>   
>   // In subscribe::(anonymous class)::operator()
>   const auto& payload
>
> Only when hovering over `auto`?
>
>   type-alias auto
>   struct Foo

Not intended per se but a limitation of the implementation approach: we do the analysis, but it's not feasible to track how the results would filter through the AST in general.
Maybe it's possible to do the easy+important cases, but this patch doesn't attempt it.

An alternative here is to have the selection walk the single instantiation, if there is one, instead of the template.
This is a more general/powerful approach.
I will try it out before landing this patch, but I suspect it'll cause some stuff to break in which case I'll move forward with this one.



================
Comment at: clang-tools-extra/clangd/AST.cpp:519
+      if (Spec->getTemplateSpecializationKind() != TSK_ImplicitInstantiation)
+        continue;
+      // Find the type for this specialization.
----------------
nridge wrote:
> Should we `break` in this case? Otherwise there could be an explicit specialization with a second type
I guess we're talking about this case:

```
void foo(auto X) {}
template <> void foo(char Y) {}
foo(0);
```

Now the meaning of the `auto` is tricky... if we're reading the body of the function, it's `int`. If we're reading the signature of foo it could be int or char. And obviously expand-auto-ing this into `int` would break the program.

I think it makes sense to be conservative and bail out if there are any explicit specializations for now. I expect this case to be rare.

On the other hand explicit instantiation is actually fine, so changed the condition here to fail on explicit specialization only.


================
Comment at: clang-tools-extra/clangd/AST.cpp:545
+  // TemplateTypeParmTypes for implicit TTPs, instead of AutoTypes.
+  // Also we don't look very hard, just stripping const, references, pointers.
+  static TemplateTypeParmTypeLoc findContainedAutoTTPLoc(TypeLoc TL) {
----------------
nridge wrote:
> Maybe call out an example like `vector<auto>` as a future nice-to-handle?
Done. Note that clang currently rejects `vector<auto>`, AFAICT it's valid though.


================
Comment at: clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp:88
+              StartsWith("fail: Could not deduce"));
+  EXPECT_EQ(apply("auto X = [](^auto){return 0;}; int Y = X(42);"),
+            "auto X = [](int){return 0;}; int Y = X(42);");
----------------
nridge wrote:
> Maybe add a conflicting-deduction case? (I know the GetDeducedType test already has one, but here it would be especially wrong.)
> 
> Also, should we perhaps disable this in header files (maybe excepting function-local symbols), since an including source file could have additional instantiations?
> Maybe add a conflicting-deduction case? (I know the GetDeducedType test already has one, but here it would be especially wrong.)
Done.

> Also, should we perhaps disable this in header files (maybe excepting function-local symbols), since an including source file could have additional instantiations?

Disabling features in headers seems sad. Detecting when the function is local doesn't seem like a great solution to this:
 - it's going to be hard to detect the common ways functions are hidden within headers (`detail` namespaces, inaccessible members, ...)
 - it's still going to be wrong when some instantiations come via (local) templates that are themselves instantiated from the including file

I suspect in most cases where multiple instantiations are intended we're going to see either no instantiations or many. Can we try it and see if there are false positive complaints?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119537