Page MenuHomePhabricator

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

Authored by sammccall on Feb 11 2022, 3:11 AM.

Details

Summary

This makes hover/go-to-definition/expand-auto etc work for auto params in many
common cases.
This includes when a generic lambda is passed to a function accepting
std::function. (The tests don't use this case, it requires a lot of setup).

Note that this doesn't affect the AST of the function body itself, cause its
nodes not to be dependent, improve code completion etc.
(These sort of improvements seem possible, in a similar "if there's a single
instantiation, traverse it instead of the primary template" way).

Fixes https://github.com/clangd/clangd/issues/493
Fixes https://github.com/clangd/clangd/issues/1015

Diff Detail

Event Timeline

sammccall created this revision.Feb 11 2022, 3:11 AM
sammccall requested review of this revision.Feb 11 2022, 3:11 AM
avogelsgesang added inline comments.
clang-tools-extra/clangd/AST.cpp
508

out of curiosity: when would this happen?

sammccall added inline comments.Feb 11 2022, 8:30 AM
clang-tools-extra/clangd/AST.cpp
508

I think it probably can't happen.
In principle there may be several sets of implicit TTPs in scope (generic lambda nested in generic lambda) but auto would always refer to the immediately enclosing set.

Added an assert, but I lack the courage to make this llvm_unreachable() and crash horribly in production if I'm wrong :-)

sammccall updated this revision to Diff 407891.Feb 11 2022, 8:30 AM

Assert on presumed-impossible case.

code looks good to me, but I don't know the code base well enough for a "LGTM"

Thanks for implementing this!

Should this be added to the Release Notes?

Should this be added to the Release Notes?

We usually go back over the git log around the time of a release cut.
It started as laziness but I think it's a good system: less work writing notes for stuff that doesn't end up landing, and gives us a chance to group and summarize related changes.

And thanks for taking a look, and filing bugs - sorry we can't get to everything :-)

nridge accepted this revision.Feb 14 2022, 1:13 AM

Very neat! A few minor suggestions but generally looks good.

clang-tools-extra/clangd/AST.cpp
519

Should we break in this case? Otherwise there could be an explicit specialization with a second type

545

Maybe call out an example like vector<auto> as a future nice-to-handle?

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
88

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?

This revision is now accepted and ready to land.Feb 14 2022, 1:13 AM
Trass3r added a comment.EditedFeb 15 2022, 8:25 AM

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
sammccall updated this revision to Diff 409902.Feb 18 2022, 5:07 AM
sammccall marked 4 inline comments as done.

address review comments

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.

clang-tools-extra/clangd/AST.cpp
519

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.

545

Done. Note that clang currently rejects vector<auto>, AFAICT it's valid though.

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
88

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?

Trass3r added inline comments.Feb 18 2022, 6:47 AM
clang-tools-extra/clangd/AST.cpp
491

Since this is implemented generically in the AST, can this functionality easily be reused for a type inlay hint?

nridge added inline comments.Feb 18 2022, 3:13 PM
clang-tools-extra/clangd/AST.cpp
545

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.

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
88

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

sammccall marked an inline comment as done.Feb 21 2022, 4:23 AM
sammccall added inline comments.
clang-tools-extra/clangd/AST.cpp
491

In principle yes. Rather than expand the scope further, I'll send a followup patch.

clang-tools-extra/clangd/unittests/tweaks/ExpandAutoTypeTests.cpp
88

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)

sammccall marked an inline comment as done.Feb 21 2022, 4:25 AM

BTW, D120131 demonstrates the other idea of actually using the instantiated AST instead of the template.
It's pretty cool but would need some refinement to be production-ready I suppose.

D120258 is the followup with inlay hints.

It rearranges some of the code from this patch for reuse, and proposes dropping support for multiple instantiations yielding the same result, as this inevitably pushes complexity into each place this is used.