New contributions should be allowed to not rely on transitive includes

[vittorioromeo]

tl;dr

I propose that new contributions to the Godot engine should not be blocked or rejected on the basis that the contributor prefers to not rely on transitive includes.

Context

This discussion is prompted by this PR I submitted to the Godot engine:
godotengine/godot#87893

For your convenience, here are some screenshots of the relevant discussion:

As you can see from the above screenshots, I am being asked by @AThousandShips to intentionally remove the

// safe_binary_mutex.h
#include "core/error/error_macros.h"
#include "core/typedefs.h"

lines of code, despite the fact that DEV_ASSERT and _ALWAYS_INLINE are directly mentioned in the code and respectively defined in error_macros.h and typedefs.h.

The reasoning behind this request is that:

1. error_macros.h and typedefs.h are included in mutex.h, which is also included in safe_binary_mutex.h, therefore they are already transitively included;

2. "that's how we do things, we don't "include what we use"", which is not a compelling technical argumentation.

Drawbacks of transitive includes

Below is a non-exhaustive list of the drawbacks of relying on transitive includes:

• Hidden dependencies: if a file relies on headers included by other headers, it can be difficult to identify which headers are actually required by the file. This can make the code harder to understand and maintain. I've personally faced this while attempting to speed up Godot's compilation times -- it was extremely painful to make changes in headers due to the presence of many hidden dependencies.

• Potentially increased compilation times: every time a header file is included, the compiler must parse it and all of its dependencies. If some required dependencies are transitively included through an header that exposes more than it should, rather than included directly through the minimal subset of required headers, this can lead to additional unnecesary work by the compiler.

• Risk of name clashes: transitive includes can introduce names into the global namespace that you didn’t expect, increasing the risk of name clashes.

• Reduced portability: if a header file changes its own includes, it can break code in other files that were relying on those transitive includes.

• Inflexible code structure: relying on transitive includes can lead to a tightly-coupled code structure, making it harder to reuse code in different contexts.

Benefits of transitive includes

There are genuinely no real technical advantages that I can think of. Maybe conciseness and saving a few lines of code? Or maybe slightly lowering the mental overhead of thinking what includes are actually required when writing new code?

Neither of these are compelling technical reasons to adopt transitive includes.

My proposals

As Godot is a very large codebase, it is unrealistic to propose a full refactoring of the engine to avoid transitive includes, which are unfortunately in widespread use throughout its codebase.

However, I believe that every good long-term change starts as a small, gradual effort. Therefore, I mainly propose two things, in order of importance:

1. New contributions to the Godot engine should not be blocked or rejected on the basis that the contributor prefers to not rely on transitive includes. If the contributor decides to explicitly specify every include file required for the proposed code contribution, the PR should not be held because of that decision.

   Rationale: new code doesn't have to repeat the mistake of the pasts just because "that's how things have always been done". New code is an opportunity to start doing the right thing and gradually improve the engine's codebase quality over time. This demotivational image is also very relevant here: https://despair.com/cdn/shop/products/consistencydemotivator_grande.jpeg?v=1414004030

2. Contributions to the Godot engine that specifically aim to reduce the use of transitive includes should be accepted and encouraged, even if the contribution does not introduce any new feature or bugfix.

   Rationale: contributions that improve the internal structure of the engine's codebase might not seem worthwhile, but they compound over time, making it easier for the engine to be refactored and improved. Future contributors will avoid the pain of dealing with issues caused by transitive includes, and the engine will become easier to maintain over time.

Of course, I believe that (1) is much more valuable than (2), but both proposals should be considered.

[KoBeWi]

There is a tool literally called include-what-you-use and we could apply it to the whole codebase if we wanted, but it leads to things like this godotengine/godot#57734
30 lines of includes on top of each file aren't exactly friendly.

[vittorioromeo]

@KoBeWi

---

There is a tool literally called include-what-you-use and we could apply it to the whole codebase if we wanted

That would be a different proposal, with much larger ramifications. What I am asking here is basically to allow new contributions to not make the already terribly header hygiene situation worse than it already is, just because it is already bad.

In my own experience, include-what-you-use also needs human intervention -- sometimes it is too aggressive, but it's a good start to figure out where to begin untangling things.

---

30 lines of includes on top of each file aren't exactly friendly.

Completely agreed. That's when you realize that maybe your class is doing too much, and should be split into separate components :)

Transitive includes hide these problems, they do not cause them. If your file needs 30 separate includes, you don't sweep that under the rug, you start investigating why you need that many includes and you'll likely realize that there's an opportunity for a better architecture.

I'm also not saying that transitive includes never make sense -- a helper header that is clearly documented as a transitive inclusion header and that bundles a few headers that are commonly used together might be a reasonable and pragmatic solution in a few specific scenarios.

[timothyqiu]

I prefer non-transitive includes too.

In practice, when making PRs, I usually manually remove direct includes generated by autocompletion. It's because I want to avoid long discussions like you've encountered. Without a written rule, it's just a competition of who want to repeat their opinion again.

I think either way, it's better to put some written rules about includes.