<ext/type_traits.h> suggested for no apparent reason

[firewave]

#include <algorithm>
#include <string>
#include <vector>

class C {
public:
    const std::string &str() const;
};

void f()
{
    std::vector<const C*> f;
    std::vector<const C*> c;
    bool b = std::equal(f.begin(), f.end(), c.begin(), [](const C* t1, const C* t2) {
        return t1->str() == t2->str();
    });
}

test.cpp should add these lines:
#include <ext/type_traits.h>  // for __gnu_cxx::__enable_if

test.cpp should remove these lines:

The full include-list for test.cpp:
#include <ext/type_traits.h>  // for __gnu_cxx::__enable_if
#include <algorithm>          // for std::equal
#include <string>             // for std::operator==, std::basic_string, std::string
#include <vector>             // for std::vector
---

If I change str() to return bool instead it is no longer suggested.

Happens with 0.23dev and I can reproduce it with back to 0.20 (no earlier binaries to test with).

[bolshakov-a]

Reduced:

// A.h
struct A {
    typedef int Int;
};

// getInt.h
#include "A.h"

A::Int getInt();

// main.cpp
#include "getInt.h"

int main() {
    return []{ return getInt(); }();
}

IWYU wrongly reports A as needed in main.cpp. @kimgr, it is not a template issue at all. The problem is in that IWYU reports sugar in implicitly generated code (namely, in the lambda conversion function):

[bolshakov-a]

An obvious way to fix it is just ignoring lambda conversion operators during traversal, like this:

--- a/iwyu.cc
+++ b/iwyu.cc
@@ -1634,6 +1634,12 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
     return true;
   }
 
+  bool TraverseCXXConversionDecl(clang::CXXConversionDecl* decl) {
+    if (current_ast_node()->template GetAncestorAs<clang::LambdaExpr>(2))
+      return true;
+    return Base::TraverseCXXConversionDecl(decl);
+  }
+

But this looks like an ad-hoc. For example, it doesn't handle implicit declarations of capture variables, like in this case:

(void)[i = getInt()]{};

Another solution is to skip any sugar (not interesting for IWYU in the sense that it doesn't "provide" types) in types not explicitly written in the source (which are handled by TraverseType as opposed to TraverseTypeLoc):

--- a/iwyu.cc
+++ b/iwyu.cc
@@ -4289,6 +4289,13 @@ class IwyuAstConsumer
 
   // --- Visitors of types derived from Type.
 
+  bool TraverseType(QualType type) {
+    if (type.isNull())
+      return true;
+    const Type* desugared = Desugar(type.getTypePtr());
+    return Base::TraverseType(QualType{desugared, 0});
+  }
+

This seems to be more correct. In this form, it causes a bug with exception specifications which is easy to fix, however. Note that this handler should be placed into IwyuAstConsumer and not into InstantiatedTemplateVisitor because at least some TypeLocs are lost after transforming templates into its instantiations, AFAIU. This should not be currently a big problem because InstantiatedTemplateVisitor doesn't report any type except template arguments, but it may lead to issues if it stops to be the case.

[kimgr]

In this form, it causes a bug with exception specifications which is easy to fix, however.

I was playing around with this, and see that with the desugaring type traversal, we get a forward-decl use reported for exception specs, and on mainline we get a full use reported. The only difference I can see in the AST is the presence of an ElaboratedType node on mainline. That makes sense as a difference, because I suppose Desugar would remove ElaboratedTypes. But why does the presence of ElaboratedType trigger a full use?

[bolshakov-a]

IwyuBaseAstVisitor::VisitType turns off forward-declare context for types coinciding with some type in the exception specification. But the exact type (ElaboratedType) is now skipped. The fix is simple:

--- a/iwyu.cc
+++ b/iwyu.cc
@@ -2435,7 +2435,7 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
       for (FunctionProtoType::exception_iterator it =
                fn_type->exception_begin();
            it != fn_type->exception_end(); ++it)
-        if (it->getTypePtr() == type) {  // *we're* an exception decl
+        if (GetCanonicalType(it->getTypePtr()) ==  GetCanonicalType(type)) {  // *we're* an exception decl

[kimgr]

I was about to argue that the naked RecordType should hit the same path, but I guess the way we step up using GetParentAs<> to find the enclosing function won't make it if there are nodes in between. OK, nice! Thanks for walking me through that.

Btw, I wonder if my old idea about doing this as part of traversal would make this more obvious;

def traverse_function_prototype_loc(type):
    current_context.set_fwd_decl(is_provided(type.getReturnType())
    traverse_type(type.getReturnType())
    for param_type in type.getParamTypes():
        current_context.set_fwd_decl(is_provided(param_type)
        traverse_type(param_type)

    current_context.set_fwd_decl(false)
    traverse_exception_spec()

(very pseudo, but you get the idea)

[bolshakov-a]

Btw, I wonder if my old idea about doing this as part of traversal would make this more obvious;

I like it too. It looks more straightforward.

[kimgr]

diff --git a/iwyu.cc b/iwyu.cc
index 6741a2d..4e00465 100644
--- a/iwyu.cc
+++ b/iwyu.cc
@@ -2435,7 +2435,8 @@ class IwyuBaseAstVisitor : public BaseAstVisitor<Derived> {
       for (FunctionProtoType::exception_iterator it =
                fn_type->exception_begin();
            it != fn_type->exception_end(); ++it)
-        if (it->getTypePtr() == type) {  // *we're* an exception decl
+        if (GetCanonicalType(it->getTypePtr()) ==
+            GetCanonicalType(type)) {  // *we're* an exception decl
           current_ast_node()->set_in_forward_declare_context(false);
           break;
         }
@@ -4289,6 +4290,13 @@ class IwyuAstConsumer
 
   // --- Visitors of types derived from Type.
 
+  bool TraverseType(QualType type) {
+    if (type.isNull())
+      return true;
+    const Type* desugared = Desugar(type.getTypePtr());
+    return Base::TraverseType(QualType{desugared, 0});
+  }
+
   bool VisitTypedefType(TypedefType* type) {
     if (CanIgnoreCurrentASTNode())
       return true;

Unfortunately this still recommends we include A.h in main.cpp. Looks like it's still spinning through the conversion decl the same way (and twice ???):

tests/1613/reduced.cc:5:12: (10) [ CXXConversionDecl ] 0x56341634f510 inline operator auto (*)() -> A::Int() const noexcept
tests/1613/reduced.cc:5:12: (11, fwd decl) [ PointerTypeLoc ] auto (*)(void) -> struct A::Int
tests/1613/reduced.cc:5:12: (12, fwd decl) [ FunctionProtoTypeLoc ] auto (void) -> struct A::Int
tests/1613/reduced.cc:5:12: (13, fwd decl) [ ElaboratedTypeLoc ] struct A::Int
tests/1613/reduced.cc:5:12: (14, fwd decl) [ NestedNameSpecifier ] 0x56341634ec28 struct A::
tests/1613/reduced.cc:5:12: (15) [ RecordTypeLoc ] struct A           
Marked full-info use of decl 0x56341634e9e8 A (from tests/1613/A.h:2:8) at tests/1613/reduced.cc:5:12
tests/1613/reduced.cc:5:12: (14, fwd decl) [ TypedefTypeLoc ] A::Int
Marked full-info use of decl 0x56341634ebb0 A::Int (from tests/1613/A.h:3:17) at tests/1613/reduced.cc:5:12
tests/1613/reduced.cc:5:12: (11, fwd decl) [ FunctionProtoTypeLoc ] auto (*(void) const noexcept)(void) -> struct A::Int
tests/1613/reduced.cc:5:12: (12, fwd decl) [ PointerTypeLoc ] auto (*)(void) -> struct A::Int
tests/1613/reduced.cc:5:12: (13, fwd decl) [ FunctionProtoTypeLoc ] auto (void) -> struct A::Int
tests/1613/reduced.cc:5:12: (14, fwd decl) [ ElaboratedTypeLoc ] struct A::Int
tests/1613/reduced.cc:5:12: (15, fwd decl) [ NestedNameSpecifier ] 0x56341634ec28 struct A::
tests/1613/reduced.cc:5:12: (16) [ RecordTypeLoc ] struct A
Marked full-info use of decl 0x56341634e9e8 A (from tests/1613/A.h:2:8) at tests/1613/reduced.cc:5:12
tests/1613/reduced.cc:5:12: (15, fwd decl) [ TypedefTypeLoc ] A::Int
Marked full-info use of decl 0x56341634ebb0 A::Int (from tests/1613/A.h:3:17) at tests/1613/reduced.cc:5:12

The original testcase no longer complains, though.

[bolshakov-a]

For standards later than -std=c++11 my solution works...

[bolshakov-a]

I think that some correct handling of TypeLocs with invalid or empty source locations is needed. This fixes the C++11-mode problem you've found:

--- a/iwyu.cc
+++ b/iwyu.cc
@@ -4296,6 +4296,16 @@ class IwyuAstConsumer
     return Base::TraverseType(QualType{desugared, 0});
   }
 
+  bool TraverseTypeLoc(TypeLoc typeloc) {
+    if (typeloc.isNull())
+      return true;
+    if (typeloc.getBeginLoc() == typeloc.getEndLoc()) {
+      const Type* desugared = Desugar(typeloc.getTypePtr());
+      return Base::TraverseTypeLoc(TypeLoc{desugared, typeloc.getOpaqueData()});
+    }
+    return Base::TraverseTypeLoc(typeloc);
+  }
+

but causes crashes in other cases.