This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add inlay hints for auto-typed parameters with one instantiation.
ClosedPublic

Authored by sammccall on Feb 21 2022, 9:16 AM.

Details

Summary

This takes a similar approach as b9b6938183e, and shares some code.
The code sharing is limited as inlay hints wants to deduce the type of the
variable rather than the type of the auto per-se.

It drops support (in both places) for multiple instantiations yielding the same
type, as this is pretty rare and hard to build a nice API around.

Diff Detail

Event Timeline

sammccall created this revision.Feb 21 2022, 9:16 AM
sammccall requested review of this revision.Feb 21 2022, 9:16 AM

Looks like this only covers a subset.
I see hints for some generic lambdas but not for others while hovering over auto reveals the type.

Thanks - any chance you can reduce an example where hover works and inlay hints do not?

No apparent differences. Is there a way to automatically reduce it?

I don't know a good way to automatically reduce examples, I usually do it by hand.

Thanks, this is a nice extension of D119537.

I don't have a good sense for how common the "multiple instantiations, same type" scenario is; perhaps @Trass3r has an opinion on that.

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

Is it possible to extract this into a helper function template?

I'm not a huge fan of using macros like this, as the macro definition is harder to read (e.g. its tokens don't get semantic highlighting) and edit (e.g. completion).

clang-tools-extra/clangd/AST.h
136

Why Optional if TypeLoc can represent a null type loc?

clang-tools-extra/clangd/unittests/ASTTests.cpp
303

leftover or deliberate?

nridge added inline comments.Feb 22 2022, 12:28 AM
clang-tools-extra/clangd/InlayHints.cpp
323

Here's a test case which slips past these checks:

int waldo(auto... args, auto last);
int x = waldo<int, float>(1, 2.0, 'x');

last incorrectly gets float (rather than char) as a hint

sammccall marked 4 inline comments as done.Feb 22 2022, 1:48 AM
sammccall added inline comments.
clang-tools-extra/clangd/AST.cpp
572

Oops, of course.

clang-tools-extra/clangd/AST.h
136

It was to match getContainedAutoParamType, which I guess wanted to be explicit about the fact that we may not find one. But this isn't common, so I've dropped it.

clang-tools-extra/clangd/InlayHints.cpp
323

Ah, good catch! For some reason I had it in my head that a pack had to be at the end. (It can't be deduced though? Weird...)

I made this give up hinting once we see a pack.

Added that test.

sammccall updated this revision to Diff 410482.Feb 22 2022, 1:50 AM
sammccall marked 3 inline comments as done.

address comments

I don't have a good sense for how common the "multiple instantiations, same type" scenario is; perhaps @Trass3r has an opinion on that.

Hmm you said the function needs to be defined in the cpp file, right? Then I guess lambdas are indeed the main use-case. I've seen them getting reused sometimes, though just for generic error callbacks.

Trass3r added a comment.EditedFeb 22 2022, 1:30 PM

For a callback like

[](auto) {...}

the inlay hint is displayed after the ).
Which also looks confusingly like a hint for the lambda return type (which isn't implemented yet afaik).

And I've even seen this:


With a lambda in a versioned method like

template <Version V> void Foo<V>::foo()

For a callback like

[](auto) {...}

the inlay hint is displayed after the ).

Good catch, thanks!
I'll just suppress the hint here, ParamVarDecl::getLocation() appears to incorrectly point at the rparen and it's not obvious what we're hinting without a name in any case.

Which also looks confusingly like a hint for the lambda return type (which isn't implemented yet afaik).

It's not, but great idea, and I think easy to implement (-> foo is already shown for auto-typed functions).

And I've even seen this:


With a lambda in a versioned method like

template <Version V> void Foo<V>::foo()

OK, this is scary... this is multiple hints at the same point right?

And I've even seen this:


With a lambda in a versioned method like

template <Version V> void Foo<V>::foo()

OK, this is scary... this is multiple hints at the same point right?

I haven't been able to reproduce this.

I tried:

template <int> class Foo { void foo(); };

template <int X>
void Foo<X>::foo() {
    auto m = [this](auto x) {};
    m(42);
}

Foo<0> a;
// Foo<1> b;

And I get no hints at all. (Presumably because we only traverse the primary template of foo, and Foo<X>::foo()::m::operator() is never instantiated, only Foo<0>::foo()::m::operator() is (with int).

sammccall updated this revision to Diff 410814.Feb 23 2022, 7:02 AM

suppress (wrong-location) type hint on unnamed auto-typed params

I'll just suppress the hint here, ParamVarDecl::getLocation() appears to incorrectly point at the rparen and it's not obvious what we're hinting without a name in any case.

I guess (auto : type) would be ok and maybe helpful in some cases.
Though one could also argue it's not so important when you ignore the parameter anyway.

I haven't been able to reproduce this.

I tried:

template <int> class Foo { void foo(); };

template <int X>
void Foo<X>::foo() {
    auto m = [this](auto x) {};
    m(42);
}

Foo<0> a;
// Foo<1> b;

And I get no hints at all. (Presumably because we only traverse the primary template of foo, and Foo<X>::foo()::m::operator() is never instantiated, only Foo<0>::foo()::m::operator() is (with int).

In my case the lambda gets instantiated with different types depending on the template parameter.
Funny enough they disappear for me too if I change auto to auto&&.

Ah ok explicit instantiation made the difference:

template <typename T>
class Foo
{
    void foo();
};

template <typename T>
void Foo<T>::foo()
{
    auto m = [](auto x) {}; // -> (auto x: short: int)
    m(T(42));
}

Foo<int> a;
Foo<short> b;

template class Foo<int>;
template class Foo<short>;

(I think this patch is good to go now, ready for a stamp if anyone agrees :-)).

Ah ok explicit instantiation made the difference:

Interesting! Looks like we're traversing the bodies of explicitly instantiated templates.
This violates our assumption we're only traversing explicitly written code (here the instantiation is explicit but the bodies are not!)
This feels like a RecursiveASTVisitor bug, but IDK if we'll be able to fix it or need to hack around it.

In any case it's not specific to this change:

template <typename T> void foo() {
  auto x = T{}; // x: short: int
}
template void foo<int>();
template void foo<short>();

(I think this patch is good to go now, ready for a stamp if anyone agrees :-)).

I agree. There's still D120258#3336114 but I don't have the time to reduce it atm.

nridge accepted this revision.Feb 23 2022, 11:28 AM

LGTM, thanks!

This revision is now accepted and ready to land.Feb 23 2022, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2022, 4:40 AM