Re: IWYU annotations - Mailing list pgsql-hackers

On 09.12.24 17:37, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.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.