[PATCH] D146406: [IncludeCleaner][clangd] Mark umbrella headers as users of private

Thu Mar 23 05:08:08 PDT 2023

kadircet marked 2 inline comments as done.
kadircet added a comment.

thanks a lot for the review!

================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:100
+      // Check if main file is the public interface for a private header. If so
+      // we shouldn't diagnose it as unused.
+      if (auto PHeader = PI->getPublic(I.Resolved); !PHeader.empty()) {
----------------
hokein wrote:
> kadircet wrote:
> > hokein wrote:
> > > The beahvior (no showing the diagnostic) seems reasonable to me as we can infer that the included header is supposed to be exported by the main file. Just explore this a bit more.
> > > 
> > > The sad bit is that we start diverging from the classical IWYU tool (I have check it, for this case, it gives an unused-include unless you add an export pragma).
> > > 
> > > I think the main cause is that we're missing the `// IWYU pragma: export`, we should still want to recommend users to add the pragma export, right?
> > > 
> > > My only concern is that without the `export` pragma, whether the include is used can't be reason about by just "reading" the main-file content by human, e.g. for the case below, there is no usage of the `private.h` (previously the trailing `// IWYU pragma: export` comment is considered a usage), we have to open the private.h and find the private mapping to verify the usage.
> > > 
> > > ```
> > > // public.h
> > > 
> > > #include "private.h"
> > > ```
> > > 
> > > It seems better to provide an `adding //IWYU pragma: export` FIXIT.
> > > 
> > > The sad bit is that we start diverging from the classical IWYU tool (I have check it, for this case, it gives an unused-include unless you add an export pragma).
> > 
> > we're not providing the same behavior with IWYU tool, it's nice to be compatible with it and this change won't break that compatibility (i.e. if IWYU tool drops this include, we're not suggesting to insert or while IWYU tool is suggesting to insert we're not suggesting to remove). so i don't think this argument applies here.
> > 
> > > I think the main cause is that we're missing the // IWYU pragma: export, we should still want to recommend users to add the pragma export, right?
> > 
> > i don't see the reason why. i think we're actually losing value by enforcing people to annotate headers in both places. if the library provider already put a pragma in the included header, marking the umbrella header as the private header, what's the extra value we're getting by making them also annotate the public header with a bunch of export pragmas? i feel like this goes against the nature of letting tooling automate things as much as possible.
> > 
> > > My only concern is that without the export pragma, whether the include is used can't be reason about by just "reading" the main-file content by human, e.g. for the case below, there is no usage of the private.h (previously the trailing // IWYU pragma: export comment is considered a usage), we have to open the private.h and find the private mapping to verify the usage.
> > 
> > this definitely makes sense, and having an explicit IWYU export/keep pragma on these includes are definitely nice but I believe in the common case, because private-public matching is 1:1, the reason why a private header is included inside a public header is quite obvious hence absence of these pragmas won't really cause confusion, to the contrary forcing people to introduce them will actually create frustration.
> > 
> > WDYT?
> > this change won't break that compatibility (i.e. if IWYU tool drops this include, we're not suggesting to insert or while IWYU tool is suggesting to insert we're not suggesting to remove). so i don't think this argument applies here.
> 
> Agree on this point. This is a safe change. But now clangd is diverging from the clang-include-cleaner tool.
> 
> > i feel like this goes against the nature of letting tooling automate things as much as possible
> 
> I think the proposed change is good.
> 
> From the tooling automation, for example, we're doing a large cleanup (remove unused includes),  we should not remove this include. 
> 
> For interactive tools (like clangd), I actually think in addition to that, clangd should tell the user that they are missing an IWYU export pragma here, and suggest a FIXIT (but that's probably another thread).
>  
> > but I believe in the common case, because private-public matching is 1:1, the reason why a private header is included inside a public header is quite obvious hence absence of these pragmas won't really cause confusion
> 
> I'm not sure this is true, I think private=>public is 1:1, but public=>private is not always 1:1 (e.g. public header use `begin_export`/`end_export` to export a bunch of headers),
> for these public headers, missing export pragmas is hard to justify the purpose.
> 
> 
> 
> But now clangd is diverging from the clang-include-cleaner tool.

And I was trying to make the point that we're already not trying to avoid that, deliberately. As our use definitions & objectives around automation are drastically different.

> For interactive tools (like clangd), I actually think in addition to that, clangd should tell the user that they are missing an IWYU export pragma here, and suggest a FIXIT (but that's probably another thread).

I think there's harm rather than value in enabling people to introduce annotations, especially when they're not needed. Happy to discuss this more.

> I think private=>public is 1:1, but public=>private is not always 1:1

There's no matching like public "header" => private "header". The mapping goes from "an include in a header" to "a private header", which is still 1:1 and I believe even in that case it's obvious (regular case for an umbrella header)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146406/new/

https://reviews.llvm.org/D146406