types used in functions marked "override" need not have the corresponding header included

[GoogleCodeExporter]

1. Run IWYU on a subclass that overrides a virtual function that uses a type, 
and has the c++11 override specifier.
2. IWYU suggests that the subclass should provide an include for the type in 
the overridden method.

> What is the expected output? What do you see instead?

The base class header file must somehow include the type, and so the subclass 
header is guaranteed to have a definition of the type since it must include the 
base header (otherwise "override" will cause an error).  Therefore it is 
redundant to include the type's header in the subclass.

> What version of the product are you using? On what operating system?

SVN linux build from 2013-08-27.

Original issue reported on code.google.com by most...@opera.com on 29 Aug 2013 at 8:05

Attachments:

• override-include.tar.gz

[GoogleCodeExporter]

Thanks, the example makes sense. It sounds like if we can prove that a method 
is an override (with or without C++11 override), we can ignore its arguments in 
IWYU analysis. Does that sound about right?

Original comment by kim.gras...@gmail.com on 29 Aug 2013 at 8:14

[GoogleCodeExporter]

I think so (arguments and return type, I guess).

Original comment by most...@opera.com on 29 Aug 2013 at 8:25

[GoogleCodeExporter]

I'm a bit worried about covariance for return types, I know we already handle 
that, so we just need to make sure we don't short-circuit it.

Anyway, as soon as I have time, I can start from your sample code and see how 
it plays out.

Original comment by kim.gras...@gmail.com on 29 Aug 2013 at 8:33

[GoogleCodeExporter]

Just to let you know that I'm working on this.

I think this is a variation on the examples listed under Author Intent:
https://code.google.com/p/include-what-you-use/wiki/WhatIsAUse#Author_Intent

I wonder if this is actually a bug: by the reasoning in Wiki article, I feel 
like base.h should already re-export all symbols to sub.h.

I'll keep digging.

Original comment by kim.gras...@gmail.com on 2 Sep 2013 at 6:28

[GoogleCodeExporter]

Still struggling with this.

Author intent and re-exports don't currently seem to be first-class concepts in 
IWYU, and I think they should be. I keep discovering new subtle aspects of this 
every time I look.

I _think_ (and I'm not sure) that one significant part of the problem is that 
re-exports are only honored one level down. Your example calls for re-exporting 
<string> from base.h and base.h from sub.h.

For what it's worth, you can work around this by specifying an export pragma on 
the #include <string> in base.h:

  #include <string>  // IWYU pragma: export

This will force a re-export.

I'll try to get my head around this and come up with a more permanent fix.

Original comment by kim.gras...@gmail.com on 20 Sep 2013 at 8:22

[GoogleCodeExporter]

I think I have a solution for this problem (I'm testing with a user-defined 
struct instead of std::string, because there are other issues with the latter 
on Windows.)

However, I made the mistake of pulling in the latest LLVM/Clang, which broke 
everything, and I haven't had time to follow up.

I'll try and post a patch some time this week and we can see whether that 
solves your local issues.

Original comment by kim.gras...@gmail.com on 3 Oct 2013 at 4:10

[GoogleCodeExporter]

I'm back!

Attached is a raw patch that I *think* will solve this problem, but want to try 
the waters to make sure a test case covers all the bases. Mostyn, could you 
give this a try and see if it acts as you'd expect?

Volodymyr, I would appreciate your input on the solution as such.

Thanks!

Original comment by kim.gras...@gmail.com on 5 Oct 2013 at 3:50

Attachments:

• bug105-raw.patch

[GoogleCodeExporter]

A small question before I examine the patch more closely: should "override" 
affect IWYU behavior? Or the behavior without "override" should be the same as 
with it?

Original comment by vsap...@gmail.com on 6 Oct 2013 at 8:16

[GoogleCodeExporter]

No, I think the behavior should be the same.

I'm assuming that Clang's handling of overridden methods 
(CXXMethodDecl::size_overridden_methods()) works the same with or without 
override attribute, and it seems to do so.

Original comment by kim.gras...@gmail.com on 7 Oct 2013 at 4:38

[GoogleCodeExporter]

It might take me a couple of days to test the patch.

Original comment by most...@opera.com on 7 Oct 2013 at 6:47

[GoogleCodeExporter]

Kim, I think that bug105-raw.patch doesn't really resolve the issue because now 
#include <string> is recommended in sub.cc, not sub.h. It is recommended due to 
std::string constructor usage in return statement.

As far as I understand, you've implemented solution described in comment #1, 
right? But I think that the right approach (also described by you) is to treat 
#include <string> in base.h as re-export. In this case sub.h, sub.cc don't need 
to #include <string> themselves.

Original comment by vsap...@gmail.com on 14 Oct 2013 at 8:56

[GoogleCodeExporter]

Thanks for trying it out!

Yeah, that doesn't sound right. I haven't been able to test with std::string 
because I'm on Windows and there are problems with mapping.

I honestly couldn't find a way to re-export explicitly -- I was expecting this 
to be manifest in the source, but I wasn't able to isolate the behavior 
anywhere.

I wish we could make re-exporting more obvious, because we have several issues 
hinging on it at the moment.

Original comment by kim.gras...@gmail.com on 20 Oct 2013 at 7:17

[GoogleCodeExporter]

@Kim: as vsapsai mentioned, the patch doesn't seem to work.

Original comment by most...@opera.com on 20 Oct 2013 at 9:17

[GoogleCodeExporter]

I had an idea to use transitive desired includes instead of associated desired 
includes. Corresponding patch is attached. Please note that it's only proof of 
concept and the patch is quite messy. I need more time to think if there are 
cases where proposed approach doesn't work. Meanwhile I am open to suggestions 
and comments.

Original comment by vsap...@gmail.com on 27 Oct 2013 at 3:25

Attachments:

• transitive_desired_includes.patch

[GoogleCodeExporter]

Thanks for picking this up.

I think it's a healthy direction to move IWYU policy from the RAV to 
iwyu_output, that's where all the policy decisions should be, IMO.

I don't fully understand what's going on, though... Do you investigate the 
entire #include graph and collect all the headers for which there are uses? And 
then from that set do IWYU analysis? Won't that invalidate the analysis?

The change in iwyu.cc is a little suspect. It slows things down enormously and 
exposes an unrelated bug for MSVC STL (I'll commit a fix for that shortly.) I'm 
guessing it's necessary because you now need use data for the entire graph as 
opposed to only the files we report IWYU violations for?

I still don't see how this ties into the base re-export, but it does seem to 
solve the problem :-)

It'd be great to hear a little more about the overall idea.

Original comment by kim.gras...@gmail.com on 28 Oct 2013 at 8:54

[GoogleCodeExporter]

The main question is why shouldn't IWYU offer to #include <string> in sub.h, 
sub.cc. I understand that the reason is the following: sub.h won't compile 
without #include "base.h" and base.h requires <string>. I.e. sub.h don't need 
to #include <string> directly because it includes it transitively through 
"base.h". We already allow transitive includes through associated headers (for 
example, usage of std::string in base.cc doesn't trigger recommendation to add 
#include <string> in base.cc). So I've decided to apply this mechanism for all 
transitive includes.

I've attached a patch which makes "associated desired includes" mechanism more 
visible. transitive_desired_includes.patch is basically replacing 
AssociatedDesiredIncludes with TransitiveDesiredIncludes and additional changes 
to make it work.

> Do you investigate the entire #include graph and collect all the headers for 
which there are uses? And then from that set do IWYU analysis? Won't that 
invalidate the analysis?

I perform IWYU analysis for each file in #include graph (except of emitting 
suggested changes), so I can tell for each file which symbols are used and 
which headers are desired. Then in the main file I suppress suggested #includes 
if the same #include is already accessed via a chain of desired includes (for 
example, we don't need to #include "foo.h", if we need to #include "bar.h" and 
bar.h itself needs to #include "foo.h").

I think I've already answered the question "Won't that invalidate the 
analysis?" but it led to more thinking. Let's consider IWYU spectrum from 
"include everything you use directly" to "include what you use and minimize 
number of includes". Offered change moves IWYU closer to the latter point. It 
contradicts some of IWYU goals, especially for large codebases. So to some 
extent it invalidates analysis. That's why I think it is reasonable to turn 
on/off TransitiveDesiredIncludes with a command line argument. By default it is 
turned off and current behavior is used.

Original comment by vsap...@gmail.com on 1 Nov 2013 at 4:15

Attachments:

• transitive_desired_includes_intermediate_step.patch

[GoogleCodeExporter]

Thanks, now I understand better.

Unconditionally allowing transitive includes seems wrong to me. IWYU has had a 
pretty strong policy against that and with good reason, I believe. IWYU is not 
strictly a minimization problem, it's just as much a way to make source files 
stand on their own, which helps refactoring and reuse.

I'm trying to think of an example of where this is clearly wrong, but I'm 
having trouble coming up with one, which may indicate that it's actually a good 
idea :-) The discussion on issue 73 is the closest I can find, I think comment 
#1 was to the point.

The new patch in comment #16 looks obviously good since it eliminates 
duplication. I'm still on the fence regarding the one in comment #14. I'm also 
not sure about command-line arguments to alter the behavior -- I feel like 
"include what you use and minimize number of includes" is not really what we 
want to do, but it's a pragmatic way to work around short-comings.

Maybe we should move this discussion to the mailing list, as it's drifting away 
from the issue at hand? I'll send out a related idea there.

Original comment by kim.gras...@gmail.com on 2 Nov 2013 at 4:27

[GoogleCodeExporter]

Here's a refinement on my earlier patch. This works for me as-is in a Linux/GCC 
environment, and in an MSVC environment if I change the #include from <string> 
to <xstring> (mappings are not in place for MSVC.)

Idealistically, I think the logic is in the wrong place, but practically it's 
now solved in the same place as the typedef author-intent special rule. Until 
we can find a way to push that to a later phase, I'm fine with keeping it in 
RAV.

All tests pass. What do you think?

Original comment by kim.gras...@gmail.com on 15 Jan 2014 at 6:02

Attachments:

• bug105.patch

[GoogleCodeExporter]

Hi, Kim. GetAncestorOfType<CXXMethodDecl> is too broad approach. It gets 
triggered in method body too and as the result IWYU recommends to remove 
#include "override-include2/bar.h" in

{{{
#include "override-include2/sub.h"
#include "override-include2/bar.h"

IndirectClass sub::foo() {
    Bar bar;
    IndirectClass ic; 
    return ic; 
}
}}}

So, I think that size_overridden_methods() and HasCovariantReturnType() checks 
should be put closer to CXXMethodDecl.

Original comment by vsap...@gmail.com on 14 Feb 2014 at 5:49

[GoogleCodeExporter]

Ugh. Thanks, I completely overlooked that.

I did an experiment locally and only ignored type uses if the node was a 
descendant of FunctionProtoType. The intent was to only check return type and 
argument types, but treat the body as usual.

The problem is the mention of std::string in the return statement will count as 
a use and force inclusion of <string>, and we essentially want to black-list 
std::string (or in your example, IndirectClass) from being IWYU-analyzed in 
this entire file (though there may be exceptions).

Putting this close to CXXMethodDecl won't help (though I think I understand 
this well enough to move the check there now), because the body will still be 
visited and cause trouble.

Ultimately, we want to detect the override, and then say that for this entire 
compilation unit, the return type is exported by base.h as well as sub.h 
(indirectly but just as validly.) Unfortunately this seems really hard to do 
with IWYU's current design...

Original comment by kim.gras...@gmail.com on 15 Feb 2014 at 3:39

[GoogleCodeExporter]

Good point. I haven't realized you need to handle the return type in method 
body too.

Original comment by vsap...@gmail.com on 15 Feb 2014 at 10:21