Unused templates are ignored on Windows

[GoogleCodeExporter]

Steps to reproduce:
1. Run IWYU on Windows on a file

    #include "tests/cxx/direct.h"
    template <typename T> void foo() {
      IndirectClass ic;
    }

Actual result:
IWYU recommends to remove #include "tests/cxx/direct.h".

Expected result:
IWYU should recommend to remove #include "tests/cxx/direct.h" and to add 
#include "tests/cxx/indirect.h".

Original issue reported on code.google.com by vsap...@gmail.com on 24 Apr 2014 at 4:03

[GoogleCodeExporter]

This issue was discovered while fixing issue #125.  Seems like the culprit is 
-fdelayed-template-parsing used by default on Windows [1].  You can reproduce 
the problem on other systems by specifying -fdelayed-template-parsing option.

I'm not fixing the issue right now, just writing down what we know so far.

[1] http://clang.llvm.org/docs/UsersManual.html#microsoft-extensions

Original comment by vsap...@gmail.com on 5 May 2014 at 9:43

[GoogleCodeExporter]

You mentioned in e-mail earlier that it's probably not as simple as having IWYU 
force "-fno-delayed-template-parsing", and I just wanted to expand on one 
reason why that might be the case (I don't know what your motivation was):

When IWYU runs on Windows it defaults to MSVC compatibility, to be able to 
parse Microsoft's STL and Platform SDK headers. Turning off delayed template 
parsing will likely make it fail to parse these system headers, in turn making 
IWYU useless.

So it looks like we need to do something else...

Original comment by kim.gras...@gmail.com on 13 May 2014 at 8:03

[GoogleCodeExporter]

I didn't know about any specific problems with -fno-delayed-template-parsing.  
I've just assumed that Clang had good reason to add -fdelayed-template-parsing.

Original comment by vsap...@gmail.com on 16 May 2014 at 5:45

[GoogleCodeExporter]

Issue 146 has been merged into this issue.

Original comment by kim.gras...@gmail.com on 5 Jun 2014 at 9:06

[GoogleCodeExporter]

Hi!

I was wondering if you could give me any hint about how to resolve this issue, 
to see if I could fix it myself. This issue makes many template uses to fail 
with IWYU, it's quite a problem.

I tried looking at the parsing of the AST, because that's where the uses are 
logged. In my example I have a template function that makes use of a singleton, 
like this:

void B::DoTest()
{
    A test;
    sfunctor f;

    test.DoTest(f);
}

template <class F>
bool A::DoTest(F& functor) const
{
    return Singleton::s_this->DoTest();
}

(iwyu suggest to remove singleton.h)

and this is what the log says when it gets there:

Marked full-info use of decl 00000000029EFCC0 f (from 
F:/tests/templateTest/classB.cpp:19:11) at 
F:/tests/templateTest/classB.cpp:21:14
F:/tests/templateTest/classB.cpp:21:2: (3) [ FunctionCall ] 00000000029F0570 
bool DoTest(sfunctor &functor) const {
    return Singleton::s_this->DoTest();
}

Marked full-info use of decl 00000000029EDAF0 A::DoTest (from 
F:/tests/templateTest/classA.h:13:9) at F:/tests/templateTest/classB.cpp:21:2
Adding an implicit tpl-function type of interest: struct sfunctor
[Uninstantiated template AST-node] 0x29edaf0 [CXXMethodDecl] bool DoTest(F 
&functor) const {
    return Singleton::s_this->DoTest();
}

[Uninstantiated template AST-node] 0xa9eb30 [NestedNameSpecifier] class A::

[Uninstantiated template AST-node] 0xa9e940 [RecordTypeLoc] class A

[Uninstantiated template AST-node] class A
[Uninstantiated template AST-node] 0xa9eb60 [FunctionProtoTypeLoc] _Bool (F &) 
const

[Uninstantiated template AST-node] _Bool (F &) const
[Uninstantiated template AST-node] 0xa9e370 [BuiltinTypeLoc] _Bool

[Uninstantiated template AST-node] _Bool
[Uninstantiated template AST-node] 0x29eda30 [ParmVarDecl] F &functor

[Uninstantiated template AST-node] 0xa9e0c0 [LValueReferenceTypeLoc] F &

[Uninstantiated template AST-node] F &
[Uninstantiated template AST-node] 0xa9d990 [TemplateTypeParmTypeLoc] F

[Uninstantiated template AST-node] F
[Uninstantiated template AST-node] 0x29f0890 [CompoundStmt] CompoundStmt 
0x29f0890 <F:/tests/templateTest/classA.h:14:1, line:16:1>
`-ReturnStmt 0x29f0870 <line:15:2, col:35>
  `-CXXMemberCallExpr 0x29f0848 <col:9, col:35> '_Bool'
    `-MemberExpr 0x29f0818 <col:9, col:28> '<bound member function type>' ->DoTest 0x29edf50
      `-ImplicitCastExpr 0x29f0800 <col:9, col:20> 'class Singleton *' <LValueToRValue>
        `-DeclRefExpr 0x29f07c8 <col:9, col:20> 'class Singleton *' lvalue Var 0x29edea0 's_this' 'class Singleton *'

[Uninstantiated template AST-node] 0x29f0870 [ReturnStmt] ReturnStmt 0x29f0870 
<F:/tests/templateTest/classA.h:15:2, col:35>
`-CXXMemberCallExpr 0x29f0848 <col:9, col:35> '_Bool'
  `-MemberExpr 0x29f0818 <col:9, col:28> '<bound member function type>' ->DoTest 0x29edf50
    `-ImplicitCastExpr 0x29f0800 <col:9, col:20> 'class Singleton *' <LValueToRValue>
      `-DeclRefExpr 0x29f07c8 <col:9, col:20> 'class Singleton *' lvalue Var 0x29edea0 's_this' 'class Singleton *'

[Uninstantiated template AST-node] 0x29f0848 [CXXMemberCallExpr] 
CXXMemberCallExpr 0x29f0848 <F:/tests/templateTest/classA.h:15:9, col:35> 
'_Bool'
`-MemberExpr 0x29f0818 <col:9, col:28> '<bound member function type>' ->DoTest 
0x29edf50
  `-ImplicitCastExpr 0x29f0800 <col:9, col:20> 'class Singleton *' <LValueToRValue>
    `-DeclRefExpr 0x29f07c8 <col:9, col:20> 'class Singleton *' lvalue Var 0x29edea0 's_this' 'class Singleton *'

[Uninstantiated template AST-node] 0x29f0818 [MemberExpr] MemberExpr 0x29f0818 
<F:/tests/templateTest/classA.h:15:9, col:28> '<bound member function type>' 
->DoTest 0x29edf50
`-ImplicitCastExpr 0x29f0800 <col:9, col:20> 'class Singleton *' 
<LValueToRValue>
  `-DeclRefExpr 0x29f07c8 <col:9, col:20> 'class Singleton *' lvalue Var 0x29edea0 's_this' 'class Singleton *'

[Uninstantiated template AST-node] 0x29f0800 [ImplicitCastExpr] 
ImplicitCastExpr 0x29f0800 <F:/tests/templateTest/classA.h:15:9, col:20> 'class 
Singleton *' <LValueToRValue>
`-DeclRefExpr 0x29f07c8 <col:9, col:20> 'class Singleton *' lvalue Var 
0x29edea0 's_this' 'class Singleton *'

[Uninstantiated template AST-node] 0x29f07c8 [DeclRefExpr] DeclRefExpr 
0x29f07c8 <F:/tests/templateTest/classA.h:15:9, col:20> 'class Singleton *' 
lvalue Var 0x29edea0 's_this' 'class Singleton *'

[Uninstantiated template AST-node] 0xa9e000 [NestedNameSpecifier] class 
Singleton::

[Uninstantiated template AST-node] 0xa9de50 [RecordTypeLoc] class Singleton

I was wondering if by looking at the information displayed from the parsing of 
the AST we could report the use of the symbols used on the template function, 
since the AST appears to go through them?

Thank you!

Original comment by dpun...@gmail.com on 16 Jun 2014 at 10:32

[GoogleCodeExporter]

I don't have any actionable ideas how to fix -fdelayed-template-parsing.  My 
plan was to find a use case when it breaks LibTooling and bring this issue to 
cfe-dev mailing list.  More specifically, I was going to find a use case when 
clang-modernize doesn't modernize code in template.  Yeah, that's a crappy 
plan, but that's all I have right now.

Original comment by vsap...@gmail.com on 18 Jun 2014 at 1:39

[GoogleCodeExporter]

I ran into this a while ago:
http://clang.llvm.org/extra/PassByValueTransform.html#note-about-delayed-templat
e-parsing

so it seems it's already known by the tooling folks. Clang-modernize seems to 
have been abandoned lately.

It would be nice to discuss this issue on the cfe-dev list, it's quite a major 
drawback for Clang tooling on Windows.

Original comment by kim.gras...@gmail.com on 21 Jun 2014 at 5:30

[GoogleCodeExporter]

For reference, I brought this up on cfe-dev, and got a nice suggestion for a 
workaround: http://lists.cs.uiuc.edu/pipermail/cfe-dev/2014-August/038415.html

Attached patch shows a really sketchy implementation of this that seems to do 
the right thing for your reduced example above. And it also fixes two test 
failures in the main test suite (I forgot to check which ones), so the approach 
can't be all bad.

I wonder if we should pull the same trick for InstantiateImplicitMethods, i.e. 
just record which decls need implicit methods instantiated, then instantiate 
them in a separate pass after AST visitation, and finally revisit the specific 
decls again.

Original comment by kim.gras...@gmail.com on 3 Aug 2014 at 6:09

Attachments:

• late-parse.patch

[GoogleCodeExporter]

Awesome Kim, I'll check it out tomorrow.

Original comment by dpun...@gmail.com on 3 Aug 2014 at 10:53

[GoogleCodeExporter]

Here's a more polished version of this patch.

If this seems like a good approach in general, I can add a focused test case as 
well.

Original comment by kim.gras...@gmail.com on 4 Aug 2014 at 9:15

Attachments:

• 129.patch

[GoogleCodeExporter]

Hi Kim

I tried your latest patch, but I don't seem to get a good result with the test 
I posted. I attach the solution, can you try it? Here is the command line I use:

iwyu.exe F:\tests\templateTest\classB.cpp -IF:/tests/templateTest/ 
-IF:/external/mssdk/Include/ -DWIN32 -D_DEBUG -D_CONSOLE -w -g -O3 -ffast-math 
-funroll-loops -fno-rtti -DIWYU -Xiwyu --pch_in_code -Xiwyu 
--prefix_header_includes=keep

It is telling me to remove singleton.h, but it is needed to be able to call 
test.DoTest(f)

Thank you!

Original comment by dpun...@gmail.com on 4 Aug 2014 at 3:30

Attachments:

• templateTest.rar

[GoogleCodeExporter]

I think IWYU wants you to move the singleton.h include from classB.cpp to 
classA.h; that's where it's actually used.

If you add  -Xiwyu --check_also=classA.h you'll see this in action.

This works with or without my patch, presumably because A::DoTest is 
instantiated in B::DoTest, so the template body is already parsed.

Original comment by kim.gras...@gmail.com on 4 Aug 2014 at 3:57

[GoogleCodeExporter]

Hmm. But if I use IWYU on classA.cpp it won't say anything, it will say both 
the cpp and h are ok. It only works when done from B adding A. If that was the 
case, it should tell me to add singleton.h when runing the tool on classA, no?

Original comment by dpun...@gmail.com on 4 Aug 2014 at 4:10

[GoogleCodeExporter]

Yes, it should.

But this is a tricky situation -- when there is no definition of Singleton, the 
template body appears to come out half-baked. Clang's AST-dump (clang-check.exe 
-ast-dump classA.cpp -- -fno-delayed-template-parsing) shows a similarly 
fragmented AST.

I think this analogous to SFINAE -- it's not until a template is fully 
instantiated with types that it can be semantically checked, parsing just gets 
part of the way there. If parsing can't resolve types (because they're not 
defined), it just seems to muddle on through.

It would be nice if we could diagnose this, but I'm not sure how to do that...

Original comment by kim.gras...@gmail.com on 4 Aug 2014 at 5:12

[GoogleCodeExporter]

I've been away, but I ran this by the Clang list before I left.

Unless I'm completely misunderstanding something, the lack of diagnostic is a 
Clang bug. The parsing can't, in fact, complete if the code doesn't type-check, 
but they have another MSVCCompat-hack in place that postpones errors on 
dependent base class lookup. So because your project says "Singleton::", Clang 
assumes it's dependent base class lookup (which it can't be in a function 
template), and delays warnings until instantiation time.

I'll try and get a patch in place for this; then your classA.cpp should plain 
fail, because singleton.h is not included. And by including singleton.h in 
classA.h, you'd also satisfy IWYU -- win-win :-)

Original comment by kim.gras...@gmail.com on 10 Aug 2014 at 6:54

[GoogleCodeExporter]

Great job, Kim.  Glad you've got so far in fixing this issue.  Sorry for late 
response, moved to a new apartment, didn't have a table and a monitor.

Patch looks good to me, doesn't break anything on Mac OS X.  You are right, 
need some test for this case.  One comment regarding the code: it would be nice 
to use range-based for in InstantiateLateParsedFunctions.  Apart of that I have 
just a few clarifying questions.

You are fixing function templates, can you remind please what is the state of 
class templates?
What about referenced function templates, are they isLateTemplateParsed too?
Traversing parts of AST later seems a little bit inelegant.  Separate passes 
with special visitors to instantiate implicit methods and to instantiate late 
parsed functions feels like cleaner approach.  Thoughts?

Original comment by vsap...@gmail.com on 11 Aug 2014 at 12:59

[GoogleCodeExporter]

Awesomeness Kim. Can't wait to try the patch tomorrow.

Original comment by dpun...@gmail.com on 11 Aug 2014 at 1:50

[GoogleCodeExporter]

@vsapsai: Thanks for review. Funny you should mention range-for, I used it 
initially because it was so much easier to prototype with, but then fell back 
on Each for consistency :-) I'd be happy to start using range-for more 
consistently and slowly deprecate Each.

> You are fixing function templates, can you
> remind please what is the state of class templates?

As I understand it, only function templates are late-parsed, but I've found no 
concrete evidence one way or the other.

> What about referenced function templates, are they isLateTemplateParsed too?

Referenced how? An example would help me understand.

> Traversing parts of AST later seems a little bit inelegant.  Separate passes 
with
> special visitors to instantiate implicit methods and to instantiate late 
parsed
> functions feels like cleaner approach.  Thoughts?

I've never liked InstantiateImplicitMethods being called as part of AST 
traversal, it feels risky to modify the AST while it's being traversed. So a 
multi-pass approach seems hygienic. We'd have to collect all candidates 
(late-parsed function templates and records with uninstantiated implicit 
methods) first and then parse/instantiate them outside of AST traversal.

It seems like a good idea to do all this before we run the actual IWYU visitor, 
is that what you meant?

Original comment by kim.gras...@gmail.com on 11 Aug 2014 at 9:23

[GoogleCodeExporter]

@dpunset: there's no patch as of yet :-) This is a Clang change I'm hoping to 
get in place, I can attach it here if you want to experiment. Apply it in 
llvm/tools/clang.

As it turns out, it doesn't diagnose in classA because your DoTest template is 
a class method, not a free function, so the dependent base class-workaround in 
Clang still produces a partial AST. The Clang folks suggested that there should 
be a compatibility warning when this happens, so I'll try and create a separate 
patch for that as well.

Original comment by kim.gras...@gmail.com on 11 Aug 2014 at 10:17

Attachments:

• clang-diagnose-undeclared-in-template.patch

[GoogleCodeExporter]

I tried doing the parsing before running the IWYU AST visitor, and it turned 
out much nicer, IMHO. Please see attached patch.

I think we can do the same with InstantiateImplicitMethods to avoid modifying 
the AST while traversing.

Ideally I'd like to get some kind of API support in Clang Sema for these 
things, but it will probably be a hard sell.

Original comment by kim.gras...@gmail.com on 14 Aug 2014 at 8:07

Attachments:

• late-parse-before.patch

[GoogleCodeExporter]

The Clang changes were committed in r215683: 
http://llvm.org/viewvc/llvm-project?rev=215683&view=rev.

IWYU built against that version now throws an error on classA.h because 
`Singleton` is undeclared. Yay! If, on the other hand, Singleton is defined at 
the point of instantiation (e.g. in classA.cpp), no error occurs, but IWYU 
suggests you move `#include "singleton.h"` to classA.h.

That seems to wrap up the Clang side of things. Let me work on some test cases 
for the IWYU side!

Original comment by kim.gras...@gmail.com on 15 Aug 2014 at 12:51

[GoogleCodeExporter]

Please try the attached patch and let me know what you think!

Original comment by kim.gras...@gmail.com on 15 Aug 2014 at 1:22

Attachments:

• issue129.patch

[GoogleCodeExporter]

On second thought, I don't think the new approach does the right thing.

The new visitor collects _all_ nodes, unlike the IWYU visitor which filters out 
nodes it doesn't need to care about (outside the file in question.)

I think this means this solution is the exact equivalent of passing 
-fno-delayed-template-parsing, which runs the risk of breaking system headers 
:-/

I'll check again if there's a simple way to exclude all templates outside of 
IWYU's interest.

Original comment by kim.gras...@gmail.com on 15 Aug 2014 at 2:14

[GoogleCodeExporter]

I think the attached should do it.

I've now extracted a new CanIgnoreLocation function out of 
CanIgnoreCurrentASTNode, and then I just avoid parsing FunctionDecls that 
should be ignored/are not part of the main file + associated header + 
check_also.

Added a test to check that this strategy doesn't force-parse unused templates 
from outside of the files of interest.

I believe this solves dpunset's issue, and it fixes the two prefix-header tests 
in IWYU's suite that previously failed due to late-parsing.

Original comment by kim.gras...@gmail.com on 15 Aug 2014 at 7:35

Attachments:

• issue129.patch

[GoogleCodeExporter]

Hey Kim, one silly question. When you post a patch like this, what's best to 
do? Just apply the patch, or sync the latest trunk of iwyu, or sync the latest 
of all together (llvm, clang and iwyu)?

Thank you!

Original comment by dpun...@gmail.com on 16 Aug 2014 at 1:30

[GoogleCodeExporter]

It depends :-)

Most IWYU patches apply to HEAD of IWYU trunk when posted.

Sometimes, if IWYU trunk has been adjusted because of a breaking change in 
Clang trunk, you'll need to update Clang and LLVM to get back in sync between 
the trees.

Unfortunately, every time you update Clang+LLVM you run the risk of being the 
first to find a new breaking change, and then you'll have to spend a while 
getting to the bottom of it.

I tend to sync my LLVM/Clang/IWYU tree maybe once a week because the ensuing 
rebuild takes forever on my dev laptop. I think the trees are currently 
compatible, so now would be a good time to update everything :-)

Original comment by kim.gras...@gmail.com on 16 Aug 2014 at 1:41

[GoogleCodeExporter]

Overall patch looks good.  Just move GetLateParsedFunctionDecls out of "// --- 
Utilities for Type." section, please.

By referenced function templates I meant something like

    template <typename T> void foo() {
      IndirectClass ic;
    }
    foo<int>();

I've checked in debugger and foo<T> is not late template parsed.

Also I've checked if classes can be late parsed and it looks like with 
-fdelayed-template-parsing class templates are parsed immediately, but methods' 
bodies are late parsed.  And your patch fixes cases like

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

    class Bar {
      template <typename T> void bar() {
        IndirectClass ic;
      }
    };

I think `Foo` is worth including in the test case, but not `Bar`.

Original comment by vsap...@gmail.com on 17 Aug 2014 at 5:49

[GoogleCodeExporter]

Thanks, I completely overlooked the grouping of functions in iwyu_ast_util. 
Moved in attached patch.

I did a test with class templates and didn't see any late-parsing, but I never 
tried with an actual method, so thanks for finding that. I added variations on 
both Foo and Bar to the test, because I think both are relevant.

Right, when a function template is instantiated, it is parsed and marked as no 
longer late-parsed. This all happens before we search the AST for FunctionDecls 
and before we run the IWYU visitor, so only uninstantiated templates will be 
handled by ParseFunctionTemplates.

Please take another look.

Original comment by kim.gras...@gmail.com on 17 Aug 2014 at 9:04

Attachments:

• 129.patch

[GoogleCodeExporter]

Everything looks fine, good job. By the way, nice trick in 
lateparsed_template-notchecked.h to ensure function stays unparsed.

Original comment by vsap...@gmail.com on 20 Aug 2014 at 5:53

[GoogleCodeExporter]

Thanks, I'll commit tonight!

Yeah, SFINAE in MSVC-land is a little more spectacular :-)

Original comment by kim.gras...@gmail.com on 20 Aug 2014 at 6:53

[GoogleCodeExporter]

Fixed in r566.

Original comment by kim.gras...@gmail.com on 20 Aug 2014 at 7:49

• Changed state: Fixed

[GoogleCodeExporter]

Hi Kim, I finally got some time to go back to this.

I synced latest llvm, clang and iwyu and built it. I tried again the 
TemplateTest thing I sent, but I haven't noticed any difference. If I run IWYU 
on classA.cpp it fails compiling saying that Singleton is undeclared in 
classA.h, but IWYU says it has correct includes. If I run it on classB.h it 
tells me to remove the include of Singleton.h.

Am I missing a patch? I thought everything was submitted.

Thank you!

Original comment by dpun...@gmail.com on 26 Aug 2014 at 7:39

[GoogleCodeExporter]

Sorry, meant classB.cpp

Original comment by dpun...@gmail.com on 26 Aug 2014 at 7:40

[GoogleCodeExporter]

Hi David,

Thanks for the feedback. Everything is submitted, and I think the current 
behavior is right, let me try and explain:

- In Clang (and GCC), the default behavior is to handle member lookup in two 
phases, one when a template is parsed and one when a template is instantiated 
(i.e. called, for a function template)
- With MSVC-compatibility in Clang, parsing is delayed until instantiation, so 
the phases essentially become one. This is what my IWYU patch "fixes", by 
explicitly parsing all un-instantiated templates.
- Another MSVC quirk is that it allows unqualified dependent base member 
lookup, so you can say:

  template<class T>
  struct Foo : T {
    void bar() {
      xyzzy(); // assumed to be defined in T
    }
  }

This is not accepted by Clang/GCC by default, rather you need to say 
`this->xyzzy()` to _qualify_ the dependent lookup.

- But! Clang has another MSVC-compat hook where, if a member lookup fails at 
parse-time, it postpones the diagnostic until instantiation time, where it 
knows the type of T.
- However, this latter compatibility hack postponed errors for a lot of cases 
that could never be dependent base name lookups, e.g. 

  template<class T>
  void bar() {
    Xyz::zy();  // Function templates cannot have dependent bases
  }

I managed to convince the Clang crowd that earlier diagnostics were better, so 
I got a patch in that fails at parse-time rather than instantiation-time where 
dependent base lookup is impossible:
http://reviews.llvm.org/rL215683

So with all this background...

- include-what-you-use classA.cpp will now fail with an error because Singleton 
is not a known identifier.
  It's not possible for IWYU to reason about code that doesn't compile -- fix the error by including "singleton.h"

- include-what-you-use classB.cpp will suggest that you remove singleton.h 
because classB.cpp has no direct use of Singleton. Rather, it uses classA.h, 
which now includes singleton.h

A takeaway from this is, I guess, that you can't run IWYU in isolation; you 
need to make sure all your dependencies are IWYU-clean and working first (first 
classA.*, then classB.*)

Does this help at all? I'm worried that IWYU on your production code still 
doesn't work the way you'd expect, right?

Original comment by kim.gras...@gmail.com on 27 Aug 2014 at 6:39

[GoogleCodeExporter]

Ok forget what I said, I think it's fine now. If I do what IWYU tells me on 
both files, then everything compiles properly, and that's good for me. I'm 
stupid, I forgot that you can't trust the results if the compilation fails.

Thanks a lot!

Original comment by dpun...@gmail.com on 27 Aug 2014 at 7:08

[GoogleCodeExporter]

Phew, I'm glad to hear it! :-)

Original comment by kim.gras...@gmail.com on 27 Aug 2014 at 7:21