[clang-include-cleaner] False positives when using forward declarations

[jpothier-figma]

My company has been experimenting with include-cleaner. Our codebase heavily uses forward declarations in headers, and this trips up include-cleaner in several ways.

Setup

On LLVM 62f2641d603db9aef99dd5c434a1dfe7d3f56346

// b.h
struct B{};

// ptr.h
template <typename T>
struct Ptr {
    T* p;

    ~Ptr() {
        if (p) {
            delete p;
        }
    }

    operator bool() const {
        return p != nullptr;
    }
};

Test is: clang-include-cleaner --print=changes -disable-insert --remove=true a.cpp. The tool outputs - "b.h" @Line:<X> in these failure cases. (Same problems happen in clang-tidy and clangd, of course).

Issue 1

Implicit destructor calls are not accounted for

// a.h
#include "ptr.h"
struct B;

struct A {
    ~A();
   Ptr<B> bMember;
};

// a.cpp

#include "a.h"
#include "b.h" // gets marked unused

// Bad: implicit destructor call, `B` is not considered used. But we call `Ptr<B>::~Ptr()` which calls `sizeof(B)`
A::~A() {};

Issue 2

Function return types are not accounted for in cases where the return type is un-named

// get_b.h
#include "ptr.h"
struct B;

Ptr<B> getB();

// a.h
struct B;

struct A {
   void callGetB();
};

// a.cpp

#include "a.h"
#include "get_b.h"
#include "b.h" // gets marked unused

void A::callGetB() {
    // Bad: Use of `auto` variable, `B` is not considered used. But we call `Ptr<B>::~Ptr()` which calls `sizeof(B)`
    auto b = getB();

    // Bad: Unnamed expression result, `B` is not considered used. But we call `Ptr<B>::~Ptr()` which calls `sizeof(B)`
    if (getB()) {  }

   // OK: type of b is spelled, `B` is considered used.
   // Ptr<B> bOK = getB();
}

These are just a few examples. The problem is not limited to implicit destructor calls, either--implicit calls to operator bool() and implicit casts have the same behavior.

These problems seem to stem from the assumption that all symbols are provided by full definitions. If we add #include "b.h" to a.h in these examples, include-cleaner's analysis is correct and we can remove the include from a.cpp. This is unfortunate, because we think this tool would be great for our codebase if we could stop it from reporting these false positives.

Maybe there could be an option, AllImplicitUses, which considers all implicit uses of a symbol as a use? In these examples, the implicit destructor calls would be considered full uses, and B would be reported as used.

[llvmbot]

@llvm/issue-subscribers-clang-include-cleaner

Author: John Pothier (jpothier-figma)

My company has been experimenting with include-cleaner. Our codebase heavily uses forward declarations in headers, and this trips up include-cleaner in several ways.

Setup

On LLVM 62f2641d603db9aef99dd5c434a1dfe7d3f56346

// b.h
struct B{};

// ptr.h
template <typename T>
struct Ptr {
    T* p;

    ~Ptr() {
        if (p) {
            delete p;
        }
    }

    operator bool() const {
        return p != nullptr;
    }
};

Test is: clang-include-cleaner --print=changes -disable-insert --remove=true a.cpp. The tool outputs - "b.h" @<!-- -->Line:&lt;X&gt; in these failure cases. (Same problems happen in clang-tidy and clangd, of course).

Issue 1

Implicit destructor calls are not accounted for

// a.h
#include "ptr.h"
struct B;

struct A {
    ~A();
   Ptr<B> bMember;
};

// a.cpp

#include "a.h"
#include "b.h" // gets marked unused

// Bad: implicit destructor call, `B` is not considered used. But we call `Ptr<B>::~Ptr()` which calls `sizeof(B)`
A::~A() {};

Issue 2

Function return types are not accounted for in cases where the return type is un-named

// get_b.h
#include "ptr.h"
struct B;

Ptr<B> getB();

// a.h
struct B;

struct A {
   void callGetB();
};

// a.cpp

#include "a.h"
#include "get_b.h"
#include "b.h" // gets marked unused

void A::callGetB() {
    // Bad: Use of `auto` variable, `B` is not considered used. But we call `Ptr<B>::~Ptr()` which calls `sizeof(B)`
    auto b = getB();

    // Bad: Unnamed expression result, `B` is not considered used. But we call `Ptr<B>::~Ptr()` which calls `sizeof(B)`
    if (getB()) {  }

   // OK: type of b is spelled, `B` is considered used.
   // Ptr<B> bOK = getB();
}

These are just a few examples. The problem is not limited to implicit destructor calls, either--implicit calls to operator bool() and implicit casts have the same behavior.

These problems seem to stem from the assumption that all symbols are provided by full definitions. If we add #include "b.h" to a.h in these examples, include-cleaner's analysis is correct and we can remove the include from a.cpp. This is unfortunate, because we think this tool would be great for our codebase if we could stop it from reporting these false positives.

Maybe there could be an option, AllImplicitUses, which considers all implicit uses of a symbol as a use? In these examples, the implicit destructor calls would be considered full uses, and B would be reported as used.

[asenzz]

I'd like to add another case for the include cleaner:

When a forward declaration of a class is present in one of the includes, another include that contains the actual class declaration will be marked as unused. Clangd should take into consideration that the marked include contains the full declaration of the class and unmark it.