Constructor call of a sugared type should not consider the underly type as a use

[hokein]

For the Bar(); usage in main.cpp, include-cleaner considers both UsingType (Bar) and the underlying type (Foo) Foo constructor as uses (thus we insert both foo.h, and bar.h). In this case, we should not consider the underlying type Foo as a use.

// foo.h
namespace ns {
class Foo {};
}

// bar.h
namespace ns2 {
using Bar = ns::Foo;
}

// main.cpp
#include "bar.h"
void k() {
   ns2::Bar();  
}

[llvmbot]

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

[sam-mccall]

Because the AST representation of "constructor call" is varied and murky, this feels a bit underspecified.

Are we e.g. saying that this code isn't a use of Foo or any of its members?

// foo.h
class Foo { public: Foo(); };
// foo2.h
Foo returnsFoo();

// bar.h
#include "foo.h" // unused?
#include "foo2.h"
auto alsoReturnsFoo() -> decltype(returnsFoo()) { return {/*constructor call*/}; }

Or are we just saying that a constructor call is really a use of the type, and we should find the best form of the type?

[hokein]

To be clear, my original description was not correct (fixed), the current implementation treats a constructor call is a use of the constructor decl, rather than the class type. But I think it won't affect our discussion.

And it raises question 1: should a constructor call be considered as a use of the constructor decl, or a class type? I'm not sure the reason behind it (to align with the clangd implementation?), and given that we already treat all member expr (including destructor call) as a use of class type, constructor call seems to fit in this model as well.

Because the AST representation of "constructor call" is varied and murky, this feels a bit underspecified.

Yes, it is hard to distinguish an explicitly-spelled constructor call with the current clang AST, we have a simple heuristic in WalkAST.cpp, which is based on the validity of the parenthese or brace. (Question 2: can we improve it in the clang AST? E.g. adding an extra NameSpelled bit in the CXXConstructExpr class).

Are we e.g. saying that this code isn't a use of Foo or any of its members?

Given the Foo is not spelled in the construct call, I tend to say this code isn't a use of Foo according to the IWYS policy.

In today's implementation, we report an (implicit/explicit) reference of a constructor decl, this implies that we never remove the foo.h (and our simple heuristic fails on this case since we have a valid {, we report an explicit reference even the name is not spelled in the code).

Or are we just saying that a constructor call is really a use of the type, and we should find the best form of the type?

That's a good and higher-level question, possible answers:

• No regardless the constructor call is explicitly spelled or not => this is simple, and we don't need to solve the above questions, but it violates IWYS policy in some case where the constructor call is explicitly spelled.
• Yes only when the constructor call is explicitly spelled => this is ideal, and requires some complexity, we need to solve question 2 and question 3.

[kadircet]

I'd also like to highlight the other problem you've in the example above. I believe no matter what conclusions we come to regarding the constructor handling, in the presence of a type-alias all the usages should be attributed to it.

Now ignoring the type-alias case and focusing on the constructor calls;

per include-what-you-spell (IWYS) policy, unless the constructor call is explicitly spelled in code, with the type name, we shouldn't report the use. e.g. in Sam's example, foo.h is unused and it should be foo2.h's responsibility to include foo.h.
but I believe this goes against the handling of member calls (as they're ~always explicitly spelled and quite similar in nature to constructors).

Moreover I don't see useful invariant we can introduce into the code by special casing constructor calls that don't have the type name spelled (one that I can think of is, if the library moves from type Foo to Bar, users don't need to make any changes to their code, but if they're using any members it isn't possible, so this seems like a quite weak guarantee). Surely this is easier to explain and understand from library behaviour policy though.

I believe the rather useful guarantee here is not treating constructor calls as references when the user code's intent is solely as an opaque type (e.g. through implicit conversions/copies, if we had a takesFoo(returnsFoo()) like code snippet in the user code). Hence the heuristics in library only tries to catch these cases.

This is mostly my gut feeling though (and yes I am planning to write all of these rationale down, I promise soon), so I am also happy to go with other behaviors if there're more compelling arguments toward what invariant we can introduce to code bases with different treatments.

---

question 1: should a constructor call be considered as a use of the constructor decl, or a class type?

regarding the constructordecl being a reference to constructor vs type, we should be consistent here and make it a reference to its type as well (I don't think treating constructor and other members differently gives us any benefit apart from creating a harder to grasp mental model).

Question 2: can we improve it in the clang AST?

as mentioned, i don't think we need to figure out if a constructor call happened with the spelling of the type, i think we should rather try to figure out whether the user explicitly intended to call the constructor. For such cases, I think it's enough to look at the presence of a brace/paren.