Re: IWYU annotations

On 09.12.24 17:37, Tom Lane wrote:
> Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
>> I have done a pass over much of the source code with
>> include-what-you-use (IWYU) to remove superfluous includes (commits
>> dbbca2cf299, 9be4e5d293b, ecb5af77987). Along the way I have collected
>> some pragma annotations to deal with exceptions and special cases and
>> peculiarities of the PostgreSQL source code header structures (see [0]
>> for description). Here I'm proposing a set of patches to add such
>> annotations in commonly useful cases that should deal with most of the
>> noise.
>
> This seems to be going in the direction that there will be Yet Another
> tool that committers have to know everything about in order to not
> commit bad code. I'm feeling resistant to that, mainly because I'm
> far from convinced that IWYU brings us enough value to justify
> everybody having to learn about it.

It's not realistic at the moment for it to be a tool that everyone needs
to use and everyone needs to keep 100% clean. We're certainly very far
from that being feasible at the moment.

I see it useful in two areas:

First, when you add new files or move lots of code around, you can have
it provide an informed opinion about what includes to keep and add.
Because in practice that's mostly a crapshoot nowadays. An example from
a current patch under review:

$ iwyu_tool.py -p build src/backend/libpq/auth-oauth.c
...
../src/backend/libpq/auth-oauth.c should remove these lines:
- #include <fcntl.h> // lines 19-19
- #include <unistd.h> // lines 18-18
- #include "storage/fd.h" // lines 28-28

Second, clangd (the language server) has support for this also, and so
depending on local configuration and preferences, it can highlight
missing or redundant includes or even add some automatically as you
edit. (The latter obviously needs some manual supervision, but it is
arguably kind of neat that you don't need to jump to the top and
manually add includes as you type in new code that needs an additional
header.)

But in order for either of this to work, it needs to have some
information about basic PostgreSQL code conventions. Otherwise, it will
also do this:

../src/backend/libpq/auth-oauth.c should add these lines:
...
#include <stdbool.h> // for false, bool, true
#include <stddef.h> // for NULL, size_t
#include "c.h" // for Assert, explicit_bzero,
pg_strncasecmp
...

because it doesn't know that the convention is that you are supposed to
include "postgres.h" (or one of the other always-first headers) and then
everything that it brings it should usually not be included again
directly. (Or conversely in some cases it will suggest to remove the
include of "postgres.h" because it doesn't provide anything of use.)

So right now you get a bunch of noise and misleading information for
each file and the whole clangd support is of limited use. That's what
my patch set is mainly trying to address.

> In particular, this patchset introduces what seem like very
> error-prone setups, such as in rmgrdesc.c where there's now one
> group of #include's with "pragma: begin_keep/pragma: end_keep"
> around it and another group without. Most of us are likely
> to just blindly stick a new #include into alphabetical order
> somewhere in there and not notice that there's now an additional
> concern. The fact that that you've added precisely zero
> documentation about what these pragmas are doesn't help.

It's a fair point that some documentation could be provided. I suppose
we don't want to verbosely explain each pragma individually. Should
there be some central explanation, maybe in src/tools/pginclude/README?

Note that if you google like "IWYU pragma: export" it will take you to
the upstream documentation as the first hit, so the full documentation
is pretty easy to find.