Pull request 1084 in include-what-you-use: Fix aliased template parameter reporting

[notifi...@include-what-you-use.org]

New pull request 1084 by bolshakov-a: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

This change fixes reporting of template argument type referred to with nested typedef of the template. Template-nested typedef can't be responsible for any parameter of the template, because an exact type is known only on template specialization.
`typedef_in_template` test improved: previously, it doesn't really check the need of including a file with a type referred to by typedef from specialized template `Container<Class>`, because that file was reported due to declaration of `Pair` typedef.

[notifi...@include-what-you-use.org]

Comment #1 on pull request 1084 by kimgr: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

Looks good overall.

There's an interesting TODO comment just below your change, that hints at additional complexity. I wonder if that's severely outdated, and your patch is enough, or if there are nested template dragons hiding here (e.g. can the `SubstTemplateTypeParmType` itself be a template, etc, etc).

It's hard to see which changes in the testcase comes from the new behavior, and which are just noise from the `Class` split into `Class1` and `Class2`. I wonder if it's possible to refactor the testcase first so it's more obvious what the new behavior is?

I'd probably try something like:

* Refactor commit:
* Split Pair out into its own header
* Keep `Class` in its own header
* Add a `Class2` in same header, and use it with `Pair`
* Then behavioral change commit:
* Make the behavior change
* Add `Class3` with the other `Class` types
* Use it as an alias, and add the new tests

I think that way the diff will be smaller for the behavioral change, which is to support `using` aliases in templates, right?

[notifi...@include-what-you-use.org]

Comment #2 on pull request 1084 by bolshakov-a: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

> There's an interesting TODO comment just below your change

Do you mean [that TODO](https://github.com/include-what-you-use/include-what-you-use/pull/1084/files#diff-7ca107c8b836c1765e8b735b74f06833bec48f308a76017886cee1d5736002d9R1437)? No, it is still actual. As far as I understand, it is about typedefs of template specializations:
```cpp
typedef Template<Struct1, Class2> Type;
```
Similar TODOs are present in fn-return- and autocast-handling pieces of code. I just work on them. You may take a look at my [draft branch](https://github.com/bolshakov-a/include-what-you-use/commits/type_components_providing).

> I wonder if it's possible to refactor the testcase first so it's more obvious what the new behavior is?

Yes, it seems reasonable. But note that this change is not about `using` aliases only, but about `typedef`s as well (`GetCallerResponsibleTypesForTypedef(const TypedefNameDecl*)` handles both). Maybe, I used misleading terminology... I just like the word "alias" more than "typedef".

[notifi...@include-what-you-use.org]

Comment #2 on pull request 1084 by bolshakov-a: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

> There's an interesting TODO comment just below your change

Do you mean [that TODO](https://github.com/include-what-you-use/include-what-you-use/pull/1084/files#diff-7ca107c8b836c1765e8b735b74f06833bec48f308a76017886cee1d5736002d9R1437)? No, it is still actual. As far as I understand, it is about typedefs of template specializations, not necessarily nested in a class or class template:

[notifi...@include-what-you-use.org]

Yes, it seems reasonable. But note that this change is not about `using` aliases only, but about `typedef`s as well (`GetCallerResponsibleTypesForTypedef(const TypedefNameDecl*)` handles both). Maybe, I used misleading terminology... I just like the word "alias" more than "typedef". So, adding `Class3` seems superfluous.

[notifi...@include-what-you-use.org]

Comment #3 on pull request 1084 by kimgr: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

Oh, I misread. I thought you had added test coverage for `using` alias, but that was already there. So what's going on in the tests, really? This looks suspicious, for example:
```
// IWYU: Class1 is...*typedef_in_template-i1.h
// IWYU: Class1 needs a declaration
// IWYU: Class2 is...*typedef_in_template-i2.h
// IWYU: Class2 needs a declaration
Container<Class1, Class2>::alias_type at;
```

`alias_type` is a typedef/using alias for `T1`, which is `Class1`. Why is `Class2` required there?

[notifi...@include-what-you-use.org]

Comment #4 on pull request 1084 by bolshakov-a: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

> Why is Class2 required there?

It isn't needed, it is another bug, mentioned in [TODO](https://github.com/include-what-you-use/include-what-you-use/pull/1084/files#diff-675163cd16bf7bae6e4b310268e8ba5b51d4ad0bd06b2f6e644b03a8edf3e5baL33) in that test file. I've fixed it [here](https://github.com/bolshakov-a/include-what-you-use/commit/73ffaec0f2ee0718062e27655df55d13c0f21a2f).

[notifi...@include-what-you-use.org]

Comment #5 on pull request 1084 by kimgr: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

Could you tweak the first commit subject to be imperative and a little more specific, e.g. `Improve template typedef test`. Otherwise this looks good, much easier to see the improvement with the test changes separate!

[notifi...@include-what-you-use.org]

Comment #6 on pull request 1084 by kimgr: Fix aliased template parameter reporting
https://github.com/include-what-you-use/include-what-you-use/pull/1084

Thanks!