iwyu does not detect a necessary include: Covariant return type not detected

[GoogleCodeExporter]

When using iwyu on code with a overloaded virtual function that has a return 
type different from the function in the base class, iwyu is satisfied with a 
forward declaration even though the compiler seems to need a full definition. I 
have attached a test for this.

I can work on this; I will, however, need to familiarize myself with the clang 
AST first. My current guess is that IwyuBaseAstVisitor::VisitFunctionDecl 
should be the right place for the change or is there a more appropriate place?

Original issue reported on code.google.com by lars.sch...@gmail.com on 27 Oct 2011 at 11:47

Attachments:

• cvr_test.diff

[GoogleCodeExporter]

I'm not sure I understand the problem -- what type is it satisfied with the 
forward declaration of?  It would be helpful if you could include the output of 
the iwyu run.

I can believe there's a bug here -- iwyu has lots of bugs around corner cases 
in the language around when a full type is needed and when it's not.  Honestly, 
I feel trying to get them all right is like playing whac-a-mole.  I'd like to 
rework iwyu (and clang) to have clang tell iwyu when it needs a full type for 
some reason, rather than tryint to rederive the information.  That should fix 
problems like this 'for free'.

That said, it's a major undertaking, and it's probably going to be quite a 
while before it's ready -- at least half a year.  So trying to make things 
better in the meantime is sensible if it's something you'd like to do!  But 
first step is to figure out what's going wrong.  We won't know what fix to make 
until we know what error iwyu is making.  You'll probably want to play around 
with --v=6 output (yes, it's quite messy).

Original comment by csilv...@gmail.com on 27 Oct 2011 at 11:47

[GoogleCodeExporter]

Ok, sorry, I was a bit too vague in my report. iwyu thinks that it only needs a 
forward declaration for Derived. If I then follow the suggestion and replace 
the include with the forward declaration, you get a compiler error (2nd 
attachment). I hope this makes the issue clearer.

I came to my guess why this happens from looking at the verbose output. I think 
iwyu is deducing that if the return type of a function is a pointer it only 
needs a forward declaration whereas in this case (where I override another 
function in the base class) I need to have a full definition to check that the 
return types are compatible.

I think you are right, your rework will solve this problem.

Original comment by lars.sch...@gmail.com on 28 Oct 2011 at 8:13

Attachments:

• cvr_iwyu_output.log
• clang_cvr_fwd.log

[GoogleCodeExporter]

Forgot to add the verbose iwyu log ...

Original comment by lars.sch...@gmail.com on 28 Oct 2011 at 8:17

Attachments:

• cvr_iwyu_output_verbose.log

[GoogleCodeExporter]

Yes, your explanation makes sense to me.  We definitely assume you don't need 
the full type if you're a pointer.

The place to fix this would probably be in CanForwardDeclareType in iwyu.cc.  
It has code already like:
    // If we're in a typedef, we don't want to forward-declare even if
    // we're a pointer.  ('typedef Foo* Bar; Bar x; x->a' needs full
    // type of Foo.)
    if (ast_node->ParentIsA<TypedefDecl>())
      return false;

so we could probably add code that looks at the node and figures out if it's 
the return type of a function that is overloaded with covariant return types, 
or something like that.  Seems much easier to wait for v2.0 that does this 
automagically (even if it is a long way away...)  In the meantime, you can use 
an iwyu pragma (at the top of iwyu_preprocessor.h) to make iwyu do the right 
thing there anyway.

Original comment by csilv...@gmail.com on 28 Oct 2011 at 10:14

[GoogleCodeExporter]

Yes, this is best taken care of in v2.0. Nevertheless, I might give fixing it a 
try though. The code I am using iwyu on uses this idiom at a number of places 
and adding iwyu pragmas in the master branch is discouraged (as I am the only 
one using iwyu).

Original comment by lars.sch...@gmail.com on 2 Nov 2011 at 3:47

[GoogleCodeExporter]

Sounds good!

Original comment by csilv...@gmail.com on 2 Nov 2011 at 11:55

[GoogleCodeExporter]

Here's a patch to fix this issue, based on the attached test patch (thanks for 
the clear repro case!)

lars.schewe, I'm not sure if you're still listening, but it would be nice if 
you could verify that this patch solves the actual problem you had with 
covariant return types.

Review comments very welcome!

Original comment by kim.gras...@gmail.com on 21 Jan 2013 at 9:34

• Changed state: Accepted

Attachments:

• covariance.patch

[GoogleCodeExporter]

First of all, have you tried Craig's suggestion to handle this case in 
CanForwardDeclareType?

VisitCXXMethodDecl shouldn't be in 'Visitors of types derived from 
clang::Type.' section.

I am not sure that usage of term 'covariant' is appropriate. We actually don't 
check covariance and don't require it. Also I think that const qualifier 
relation for returned type is contravariance (return type in base class is more 
specific). I don't really understand covariance and that's why prefer not to 
use it.

Test case comments:
Return type is too specific (cvr_base.h). I don't think that virtual methods 
and pure virtual methods are needed.
It is unclear if we need #include "cvr_base.h" because of covariant_cv_qual or 
because of non_covariant.
Some characters in cvr.cc are unknown. For example, cv-quali?cation.
In Instructions for Developers we have test case naming rule "whatever.cc (not 
.cpp), and, if necessary, whatever.h, and whatever-<extension>.h"
In test case { shouldn't be on a separate line.

Test suggestions:
Add not virtual method with derived return type.
Add method with different typed return type - typedef or full type with 
namespace. Though I'm not sure about this one.

Original comment by vsap...@gmail.com on 29 Jan 2013 at 9:43

[GoogleCodeExporter]

Thanks for your comments!

> First of all, have you tried Craig's suggestion to handle
> this case in CanForwardDeclareType?

I looked at it briefly, but I didn't see any benefit over handling it where 
full context was available in the Visit method. In CanForwardDeclareType I 
would have had to back up to see if the type was a return value from a method 
before doing the covariance check.

> VisitCXXMethodDecl shouldn't be in 'Visitors of types derived from 
> clang::Type.' section.

Good catch, I'll see if I can find a more suitable place.

> I am not sure that usage of term 'covariant' is appropriate.
> We actually don't check covariance and don't require it.

Quite true. I'm assuming that the code would compile clean with Clang. I 
thought that was an assumption of IWYU at large? This change detects covariance 
by way of checking that return types differ. As far as I can tell, the only 
case where that's valid is a covariant return type.

> Also I think that const qualifier relation for returned type
> is contravariance (return type in base class is more specific).

I see your line of reasoning, and I would agree, but the standard and Clang 
both seem to argue the opposite;

  7 The return type of an overriding function shall be either identical
    to the return type of the overridden function or covariant with the
    classes of the functions. If a function D::f overrides a function
    B::f, the return types of the functions are covariant if they
    satisfy the following criteria:
    ...
    - both pointers or references have the same cv-qualification and
      the class type in the return type of D::f has the same
      cv-qualification as or less cv-qualification than the class type
      in the return type of B::f.

When I experimented with Clang it also said that return types were covariant in 
the case where there were more CV-qualifiers in the base (there are 
covariance-specific diagnostics that ask for the complete type.)

> Return type is too specific (cvr_base.h). I don't think that virtual methods
> and pure virtual methods are needed.

Too specific how? They need to be virtual because otherwise they can't be 
polymorphically overridden. The covariance legalese in the standard all relates 
to virtual methods. I need to check how this behaves with non-virtual methods 
-- they wouldn't be overridden in the formal sense, but rather hidden.

> It is unclear if we need #include "cvr_base.h" because of covariant_cv_qual
> or because of non_covariant.

Good point! I might need another header and separate base class to diagnose 
these separately.

> Some characters in cvr.cc are unknown. For example, cv-quali?cation.

Huh. Must've come in when I copied and pasted from the standard draft PDF.

> In Instructions for Developers we have test case naming rule "whatever.cc
> (not .cpp), and, if necessary, whatever.h, and whatever-<extension>.h"

Thanks, will rename.

> In test case { shouldn't be on a separate line.

Oops! Too many coding standards...

Original comment by kim.gras...@gmail.com on 30 Jan 2013 at 8:50

[GoogleCodeExporter]

> Return type is too specific (cvr_base.h). I don't think that virtual methods
> and pure virtual methods are needed.

Oh, you mean the Base and Derived classes? That's true, they probably don't 
need to be polymorphic as they are. I'll dumb them down and check, thanks!

Original comment by kim.gras...@gmail.com on 30 Jan 2013 at 8:51

[GoogleCodeExporter]

I haven't looked further into CanForwardDeclareType, because I don't see what 
value it would add to detect it there as opposed to in the visitor method. Can 
we reap any benefits by working backwards from the type instead of working 
forwards from the method declaration?

As for more correct detection of covariance, assuming the code compiles when 
IWYU sees it, I can't imagine a code fragment where return type differs and 
they're not covariant. I'd be happy to be proven wrong, though!

Revised patch:
- Move VisitCXXMethodDecl to the right section in iwyu.cc
- Use PrintableType instead of constructing a QualType for printing
- More explanatory comments, including one to describe the assumption that the 
code is valid
- Test files all use conventional style
- K&R-style braces everywhere
- Base and Derived are now empty classes, they didn't need any frills
- Added new class Class used for the case where covariance is triggered due to 
difference in cv-qualifiers only
- Inlined ReturnsBase into cvr.cc, only the return types need to be in their 
own headers

Let me know how this looks, thanks!

Original comment by kim.gras...@gmail.com on 31 Jan 2013 at 5:53

Attachments:

• covariance2.patch

[GoogleCodeExporter]

Patch looks good to me. The only thing I'd like to tune is indentation. Patch 
with Google-style indentation is attached.

Original comment by vsap...@gmail.com on 10 Feb 2013 at 5:18

Attachments:

• covariance_tune_indentation.patch

[GoogleCodeExporter]

Thank you, I hadn't got my head around the indentation rules.

Committed in r444.

Original comment by kim.gras...@gmail.com on 10 Feb 2013 at 9:44

• Changed state: Fixed