Aliases in templates #775

jbytheway · 2020-03-18T00:57:47Z

I've been working to fix the issues around uses of typedefs / aliases in templates. This PR replaces #707, fixes #431 and fixes #780.

However, it's not a complete solution yet. In particular, it's still not correctly handling libstdc++'s std::unordered_map, which was the issue that originally inspired me to work on this.

So, I hope to continue to improve the solution until it does handle that case correctly.

For now, I'm posting the PR to get feedback on the general approach taken, which is as follows:

The existing code detected uses of template arguments via VisitSubstTemplateTypeParmType which appears in the AST whenever a template argument is used in a template instantiation.

However, when an alias of a template argument is used, no such node appears. Instead, we get VisitTypedefType.

The approach of this PR is to track all aliases of template arguments in a map, and when VisitTypedefType is called on an expression using such an alias, treat it similarly to how VisitSubstTemplateTypeParmType used to work (the two now use a common helper function for the bulk of their functionality).

The test I've rewritten demonstrates a few cases where IWYU used to (incorrectly) suggest forward declarations were sufficient, and now treats as full-uses.

There are more cases which still fail, so (as mentioned above) I hope to understand them and improve this further. (However, this is nevertheless an improvement on the status quo, so if you're reading this a few months after I wrote it and I haven't made further progress then it might be worth merging anyway.)

iwyu.cc

jbytheway · 2020-04-01T12:02:51Z

I've pushed some further updates. This is now feature-complete in the sense that it is sufficient to allow for use of libstdc++'s std::unordered_map to be handled correctly, which was my goal with this PR.

The major behavioural change in these new commits is that IWYU now performs more recursive template visitation. Specifically, when an template being visited uses another template in a full-use context, passing down to it an argument that originates from the user's top-level template instantiation, it now recursively visits that other template specialization.

Currently this is unnecessarily slow, because it does so every time. I still need to hook this into the traversed_decls_ set so that it doesn't repeat traversals unnecessarily.

I've refactored and renamed TemplateTypeParmTypeHelper. It's now two functions: IsTemplateTypeParmUseFullUse and AnalyzeTemplateTypeParmUse.

The commit history still needs cleaning up, but in terms of the overall change, this is (I think) close to what I want, so your thoughts are welcome.

kimgr · 2020-04-01T18:15:43Z

Impressive! This is a lot to chew through, so it will take me a while. But just looking at the new succeeding test cases makes me really hopeful.

kimgr · 2020-04-01T18:16:23Z

I can recommend https://github.com/kimgr/git-format if you want to clean up your commit series wrt clang-format.

kimgr

Nicely done!

I haven't looked at the commit series or commit messages yet, but I guess there's some work there to turn this into a comprehensible story.

Left some notes inline, will spend some more time trying to understand the bigger picture.

iwyu.cc

tests/cxx/typedef_in_template.cc

run_iwyu_tests.py

iwyu_ast_util.cc

iwyu.cc

jbytheway · 2020-05-24T12:21:05Z

OK, I think I've actioned or responded to all your comments. I've also rebased the commit history into something which is hopefully somewhat logical and comprehensible.

Reinstating the old test cases actually revealed a case which is not currently handled correctly, but it's a non-full-use being treated as a full-use, which is less bad than the opposite, so I've left it as is, with a TODO in the test case for now.

Ready for you to take another look now.

tests/cxx/typedef_in_template.cc

kimgr

Very nice!
I went over the commits one by one in my local repo, and they tell a nice story, progressing in small steps. Some of the test TODO cleanups are truly impressive!

A few notes on commit messages:

Fix another alias-in-template issue

I wish this commit message had a small example with real code. The fix is so small (good thing) that it's hard to understand where and how it has effect (bad thing) :)

Commit-message typo:

The CanIgnoreType is suitable for determining whether a type was
reportable, but not whether it was expandansion-worthy.
expandansion -> expansion

kimgr · 2020-06-06T11:21:15Z

tests/cxx/typedef_in_template.cc

@@ -7,6 +7,7 @@
 //
 //===----------------------------------------------------------------------===//

+#include "direct.h"


Prefer tests/cxx/direct.h

iwyu.cc

iwyu_ast_util.h

iwyu.cc

kimgr · 2020-06-06T12:55:14Z

iwyu_ast_util.h

@@ -694,6 +698,11 @@ bool IsClassType(const clang::Type* type);
 // However, vector<T> is *not* converted to vector<int>.
 const clang::Type* RemoveSubstTemplateTypeParm(const clang::Type* type);

+// Returns true if any type involved (recursively examinging template


typo: examinging

tests/cxx/alias_template.cc

This tracks aliases of template parameters and treats uses thereof similarly to uses of the original parameters.

The previous code to catch aliases of template parameters would not catch the case where the template parameter was elaborated. Walk past the relevant ASTNodes to catch such cases.

It wasn't correct to ignore all uses of a template argument somewhere below a TypedefNameDecl ASTNode. When a template specialization happens between the two, the typedef is irrelevant. Account for that, and add a suitable test case. This makes cases like this count as a full use of the argument T: template<typename T> struct Container { using value_type = pair<T>; }; which may not always be correct, but is sometimes necessary. Currently this is erring on the side of too many full-uses.

jbytheway · 2020-07-03T21:09:47Z

Fix another alias-in-template issue

I wish this commit message had a small example with real code. The fix is so small (good thing) that it's hard to understand where and how it has effect (bad thing) :)

You have good instincts. This is actually the most dubious change, and isn't always correct. If you're concerned, we can drop this commit and the remainder still works. But I think it's a step in the right direction. In any case, I expanded the commit message to clarify.

I think I've addressed your other comments. Over to you again.

kimgr

Looks good! Just a few small nits remaining. Thank you.

kimgr · 2020-07-04T09:11:44Z

tests/cxx/sizeof_in_template_arg.cc

+//
+//===----------------------------------------------------------------------===//
+
+#include "direct.h"


tests/cxx/direct.h

kimgr · 2020-07-04T09:14:14Z

iwyu.cc

@@ -2897,10 +2903,22 @@ class InstantiatedTemplateVisitor

  string GetSymbolAnnotation() const override { return " in tpl"; }

-  bool CanIgnoreType(const Type* type) const override {
-    if (!IsTypeInteresting(type) || !IsKnownTemplateParam(type))
+  bool CanIgnoreType(const Type* type, IgnoreKind ignore_type =


ignore_type -> ignore_kind

iwyu.cc

When using sizeof(T) in a forward-declare context, the argument (T) should *not* be considered to be in a forward-declare context. Fix and add test case.

Type argument needs to be the pre-resugaring type in order to correctly detect whether it's a template argument. Factor out the question of whether the use is a full use to new function IsTemplateTypeParmUseFullUse.

When an expanded template param is itself a template specialization, and is being full-used, and involves a user-provided template argument, then we need to recursively expand that template to find more uses.

In walking the AST there were various places where types were only expanded if !CanIgnoreType. But the criteria were wrong The CanIgnoreType is suitable for determining whether a type was reportable, but not whether it was expansion-worthy. Add a new parameter to CanIgnoreType to distinguish the two use cases, and pass it in a couple of cases. Also, tweak the resugar map construction code. Now it is capable of handling template aliases as well as template classes and functions. Add a new test that now passes with these changes. This also resolves a couple of TODOs in an existing test, so update that accordingly.

jbytheway · 2020-07-04T19:55:58Z

OK, nits fixed.

kimgr · 2020-07-05T10:29:03Z

Thank you!

It was introduced in include-what-you-use#742 to report underlying type aliases but it is seemingly not needed anymore, probably after include-what-you-use#775 where `TypedefType` visitation has been added. No functional change.

It was introduced in #742 to report underlying type aliases but it is seemingly not needed anymore, probably after #775 where `TypedefType` visitation has been added. No functional change.

kimgr reviewed Mar 22, 2020

View reviewed changes

iwyu.cc Show resolved Hide resolved

kimgr reviewed Mar 22, 2020

View reviewed changes

iwyu.cc Show resolved Hide resolved

jbytheway force-pushed the aliases_in_templates branch 2 times, most recently from 0a6ed21 to d3c6300 Compare April 1, 2020 11:57

kimgr reviewed Apr 10, 2020

View reviewed changes

kimgr mentioned this pull request Apr 10, 2020

iwyu disagrees that std::optional needs a full definition #780

Closed

This was referenced May 16, 2020

Template arguments are incorrectly treated as forward declarable #796

Open

Add missing handling of ElaboratedTypes in VisitMemberExpr #707

Closed

jbytheway mentioned this pull request May 24, 2020

Remove trailing whitespace #800

Merged

jbytheway force-pushed the aliases_in_templates branch 2 times, most recently from 6a153a1 to 6acbfe2 Compare May 24, 2020 12:11

kimgr reviewed Jun 6, 2020

View reviewed changes

tests/cxx/typedef_in_template.cc Show resolved Hide resolved

kimgr reviewed Jun 6, 2020

View reviewed changes

jbytheway added 3 commits July 3, 2020 15:52

Catch uses of aliases of template parameters

0ac0f2d

This tracks aliases of template parameters and treats uses thereof similarly to uses of the original parameters.

Fix use of aliases of elaborated types.

cd249ad

The previous code to catch aliases of template parameters would not catch the case where the template parameter was elaborated. Walk past the relevant ASTNodes to catch such cases.

jbytheway force-pushed the aliases_in_templates branch from 6acbfe2 to 8c8c67e Compare July 3, 2020 21:07

kimgr reviewed Jul 4, 2020

View reviewed changes

jbytheway added 5 commits July 4, 2020 15:50

sizeof() should disable forward-declare contexts

defe7f6

When using sizeof(T) in a forward-declare context, the argument (T) should *not* be considered to be in a forward-declare context. Fix and add test case.

Refactor AnalyzeTemplateTypeParmUse

6f20b79

Type argument needs to be the pre-resugaring type in order to correctly detect whether it's a template argument. Factor out the question of whether the use is a full use to new function IsTemplateTypeParmUseFullUse.

Go deeper on template instantiation

8ee3763

When an expanded template param is itself a template specialization, and is being full-used, and involves a user-provided template argument, then we need to recursively expand that template to find more uses.

Add another test case to alias_template

90fe9c5

jbytheway force-pushed the aliases_in_templates branch from 8c8c67e to 90fe9c5 Compare July 4, 2020 19:54

kimgr merged commit cbdd0f6 into include-what-you-use:master Jul 5, 2020

jbytheway deleted the aliases_in_templates branch July 20, 2020 15:15

kimgr added this to the iwyu 0.15 milestone Aug 2, 2020

bolshakov-a mentioned this pull request Oct 27, 2023

Remove EnumDecl visitation #1317

Merged

kimgr pushed a commit that referenced this pull request Oct 27, 2023

Remove EnumDecl visitation

7217bba

It was introduced in #742 to report underlying type aliases but it is seemingly not needed anymore, probably after #775 where `TypedefType` visitation has been added. No functional change.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aliases in templates #775

Aliases in templates #775

jbytheway commented Mar 18, 2020 •

edited

jbytheway commented Apr 1, 2020

kimgr commented Apr 1, 2020

kimgr commented Apr 1, 2020

kimgr left a comment

jbytheway commented May 24, 2020

kimgr left a comment

kimgr Jun 6, 2020

jbytheway Jul 3, 2020

kimgr Jun 6, 2020

jbytheway Jul 3, 2020

jbytheway commented Jul 3, 2020

kimgr left a comment

kimgr Jul 4, 2020

jbytheway Jul 4, 2020

kimgr Jul 4, 2020

jbytheway Jul 4, 2020

jbytheway commented Jul 4, 2020

kimgr commented Jul 5, 2020

Aliases in templates #775

Aliases in templates #775

Conversation

jbytheway commented Mar 18, 2020 • edited

jbytheway commented Apr 1, 2020

kimgr commented Apr 1, 2020

kimgr commented Apr 1, 2020

kimgr left a comment

Choose a reason for hiding this comment

jbytheway commented May 24, 2020

kimgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway commented Jul 3, 2020

kimgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbytheway commented Jul 4, 2020

kimgr commented Jul 5, 2020

jbytheway commented Mar 18, 2020 •

edited