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

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
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) {
----------------
sammccall wrote:
> 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.
Huh, on second look, it appears that `vector<auto>` is a feature that was dropped when merging the Concepts TS into C++20 (at least, I don't see anything in http://eel.is/c++draft/dcl.spec.auto that would allow it). GCC appears to support it as an extension.


================
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);");
----------------
sammccall wrote:
> 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?
To clarify, I meant excepting only cases like this:

```
// header.hpp
void foo(std::function<void(ConcreteType)> callback);
void bar() {
  // We know this will only be instantiated with ConceteType,
  // regardless of what an including source file does
  foo([](auto x){});
}
```

but I'm sympathetic to the argument that neither false-positives (we offer a refactoring that breaks code) nor false-negative (we fail to offer a refactoring the user wants to make) are super impactful here and so we should do the easier thing (i.e. not try to restrict at all).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119537