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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/AST.cpp:491
+  // We'll examine visible specializations and see if they yield a unique type.
+  bool VisitParmVarDecl(ParmVarDecl *PVD) {
+    if (!PVD->getType()->isDependentType())
----------------
Trass3r wrote:
> Since this is implemented generically in the AST, can this functionality easily be reused for a type inlay hint?
In principle yes. Rather than expand the scope further, I'll send a followup patch.


================
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:
> 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).
OK, will leave the checks out and see how it goes.

(I agree that's probably the most important case, but I haven't worked in a c++20 codebase so I don't know how abbreviated function templates will play out)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119537