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

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...

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.

Issue 146 has been merged into this issue.

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!

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.

I ran into this a while ago:
http://clang.llvm.org/extra/PassByValueTransform.html#note-about-delayed-template-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.

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.

Awesome Kim, I'll check it out tomorrow.

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.

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!

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.

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?

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...

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 :-)

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?

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

@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?

@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.

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.

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!

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

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.

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.

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!

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 :-)

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`.

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.

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

Thanks, I'll commit tonight!

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

Fixed in r566.

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!

Sorry, meant classB.cpp

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?

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!

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