IncludeCleaner's filtering of UsingShadows is too aggressive with templates

[sam-mccall]

// foo.cpp
using std::vector;
vector<int> x;

The using-decl should be marked as a reference to either vector<T> or vector<int>, but is not.

The reason is that (in an implementation file) we iterate over the UsingShadowDecls and drop those whose targets are not Used or Referenced. In this case the UsingShadowDecl points at the primary template vector<T> but only the specialization vector<int> is marked as used.

Possible fixes:

• change the AST to also mark the primary template as referenced/used
• change the logic to query all template specializations when the target is a templatedecl
• drop this filtering approach to UsingDecls (we already don't use it for headers)

[hokein]

change the AST to also mark the primary template as referenced/used

It is unclear whether it will introduce side effects (diagnostic regression etc) in clang.

change the logic to query all template specializations when the target is a templatedecl

Probably works, but TemplateDecl is not the only case, EnumDecl is another case. I'm worried about we might run into a long tail to fix the missing cases.

drop this filtering approach to UsingDecls (we already don't use it for headers)

This seems the best to me (simple code), and it aligns with the current clangd include-cleaner behavior.

[sam-mccall]

EnumDecl is another case

Can you please provide an example?

drop this filtering approach
This seems the best to me (simple code)

SGTM, this was added in D135859, @kadircet may have an opinion

[kadircet]

change the AST to also mark the primary template as referenced/used

this definitely sounds like possibly a bigger change (due to rest of the places that might be depending on it, as @hokein mentioned). but I don't see anything conceptually wrong about an implicit instantiation marking the template it's using as used.
that being said, it's unclear if that'll actually solve the issue with (partial) specializations, as I think the using-decl will still only refer to the primary template (?) but we'll actually be using a different specialization. So it might require further changes to AST.

drop this filtering approach to UsingDecls (we already don't use it for headers)

I don't think that's gonna work without additional changes. even if this wasn't in place, we don't really get shadowdecls for (partial) specializations of the template, only the primary pattern is mentioned.
also there's a chance that this will limit our ability to trim includes in the future, just because you're still depending on an overload set but not using a particular one.

change the logic to query all template specializations when the target is a templatedecl

this feels like the right approach to me. it'd be better to have this properly handled in the AST one day, but until then I think it's fine to special case templates here and report a a bunch of extra used decls by our own search, rather than AST telling us.

[sam-mccall]

drop this filtering approach to UsingDecls (we already don't use it for headers)

I don't think that's gonna work without additional changes. even if this wasn't in place, we don't really get shadowdecls for (partial) specializations of the template, only the primary pattern is mentioned.

That seems like a relatively tiny/inconsequential limitation (how often this really result in a different set of includes in practice), and also one that might be addressed in other ways (on the use side, rather than at the UsingDecl).

also there's a chance that this will limit our ability to trim includes in the future, just because you're still depending on an overload set but not using a particular one.

Certainly, that's why it made sense to try this approach. But this is "only" a false-negative issue, and we don't have any evidence that there are many of these.

[kadircet]

But this is "only" a false-negative

not sure what you mean by false-negative exactly, but to me this feels like a false positive. just because a library you transitively depend on declares void ns::foo();, we'll say it's used by the main file when it has using ns::foo; (so i think we're providing a false-positive here).

But still i think you're right about us not having enough evidence in real world to see how much this matters. So we can strip off the filtering and report as ambiguous.

[hokein]

Can you please provide an example?

The UsingShadowDecl refers to the EnumDecl which is not marked as used (while the EnumConstantDecl is marked as used).

namespace ns {
enum A { Red };
}

using ns::A;
int k2 = A::Red;