IWYU pragma: export not effective for forward declared enum class

[justusranvier]

IWYU adds forward declarations of enum classes to every file that references the enum even if those files already includes a header that has a forward declaration marked with // IWYU pragma: export

[kimgr]

I don't think pragma export applies to forward declarations. Not sure how involved it would be to make that happen.

[justusranvier]

I found this out because I was testing the develop branch of IWYU with clang-15 and found that it's now adding forward declarations for enum classes in a few hundred files that don't need the forward declarations because they already include the header that is intended for that purpose.

If we can't use the export then we'll need hundreds of no_forward_declare pragmas.

[justusranvier]

Actually the feature of forward declaring an enum class the way IWYU is doing it is pretty suspect in the first place.

It's not like forward declaring a regular class where you're only making the name available - the forward declarations include the underlying type, which raises the possibility that it might get changed in one place and now all the forward declarations are wrong.

[kimgr]

Sounds like something @bolshakov-a would be interested in. @justusranvier Do you see any pattern to the behavior? Can you reduce it to a freestanding testcase?

[bolshakov-a]

Yes, it seems that IWYU don't have an option to export something but #includes at now. @justusranvier, the problem is mainly with source files or header files? I have an idea that forward declarations should not appear in source files at all. Maybe, it could be done though command line switch in order not to break tests...

It's not like forward declaring a regular class where you're only making the name available - the forward declarations include the underlying type, which raises the possibility that it might get changed in one place and now all the forward declarations are wrong.

But you can forward-declare a regular class, then make it struct (or, likely, vice versa) and also have to change all the forward declarations. Or just change its name... My opinion is that the cases when one changes an underlying type are much less frequent as the cases when one adds a new enum item and recompiles many files which could not be recompiled. Maybe, IWYU didn't always report full enum type usages earlier, but there were inconsistencies with it.
I aggree that enum forward declarations could theoretically cause a problem: there are very specific cases when it is possible to link together files with different underlying types of single enum and to get a runtime crash. And I thought to make a command-line flag for this feature. But I think that the benefit outweights possible harm...

[justusranvier]

Can you reduce it to a freestanding testcase?

A freestanding test case is difficult, but the replication conditions aren't too terribly complex.

If you build this docker image at tag ci-37.0 and then run the run-iwyu.sh script against this repository's develop branch then you'll see the behavior.

[justusranvier]

Maybe, IWYU didn't always report full enum type usages earlier, but there were inconsistencies with it.

IWYU-0.18 and lower never tried to add forward declarations for enums. I never saw this behavior until I tried out a newer IWYU commit (1d053ed) because 0.18 wasn't compiling against llvm-15.

[bolshakov-a]

Yes, correct. I mean IWYU suggested earlier to add #includes for enums, but, maybe, not always, I don't remember. As I understand from your question, you have got many fwd-decl suggestions for enums, but not suggestions to remove enum header inclusions, right? There weren't enum header reportings in IWYU-0.18? If so, it was erroneous and inconsistent behavior, because if you have mentioned an enum name in the code, you should either include the header with full enum type definition or add forward- (opaque-) declaration.

[justusranvier]

If so, it was erroneous and inconsistent behavior, because if you have mentioned an enum name in the code, you should either include the header with full enum type definition or add forward- (opaque-) declaration.

The way the project is currently written I have opaque declarations in dedicated headers which all the files that do not require access to specific enum values include, and separate headers with the full definition which files that need specific values include.

This way if for some reason the underlying type needs to change then it only need to be changed in exactly two headers, not every place that mentions the enum.

IWYU-0.18 is mostly fine with this configuration, except that in a few places I have to use a no_include pragma to prevent it from pulling in the full definition header where it is not necessary.

It would be great if I could preserve this behavior while still using IWYU and the export pragma seems like the logical way to do it. As it stands now if I want to upgrade past IWYU-0.18 I'll need to add dozens to hundreds of no_forward_declare pragmas to my code first.

[kimgr]

@justusranvier I think your project makes benign assumptions, it feels like a normal design to me.

I was considering maybe we should loosen IWYU's approach to forward-decls entirely. Just as an idea, I'd like to pose the following policy:

• If a use does not require the full type, require a forward-declaration
• If a forward-declaration is already available in the same file, suggest no changes
• New: If a forward-declaration is already available in the direct includes, suggest no changes

This weakens the policy to accept included forward-declares as well as local forward-declares. But it wouldn't accept indirectly-included forward-decls. I'm not sure exactly how to implement that, or if it would have ill effects, but it seems to me it might fix issues with forwarding headers (e.g. <iosfwd>).

Actually <iosfwd> wouldn't be accepted by that policy either, since it pulls in forward-decls from other, private headers. So maybe the resolution would need to happen after mapping, if it doesn't already.

[justusranvier]

For regular classes I would prefer that all forward declares be put in any header that references the types.

If A.hpp includes B.hpp, and if some functions in A.hpp need class X;, then I want class X forward declared in A.hpp even if it is forward declared in B.hpp.

The reason for this is if some future refactoring removes the need for B.hpp then I don't want the build to break because X is now an unknown symbol.

"Forward declare as much as possible" is the behavior I want most of the time, the only problem is that unlike for regular classes there is no convenient way to manually override this policy with enum classes.

[justusranvier]

I can get by with // IWYU pragma: no_forward_declare at each and every point where the undesired forward declares are being added, but a single // IWYU pragma: export would be much nicer.

[kimgr]

the only problem is that unlike for regular classes there is no convenient way to manually override this policy with enum classes.

@justusranvier Sorry for maybe asking an obtuse question, but how do you override it for classes? I didn't think we could export any forward declarations?

[justusranvier]

In my last comment I was mixing up pragmas which apply to #include lines and mapping directives which can apply to symbols.

Probably the way to get the behavior I want is by adding every enum type to my imp file so that I can specify exactly which header to include combined with no_forward_declare pragmas in every spot where iwyu is going to add an undesired forward declaration.

[justusranvier]

This commit is a case study in iwyu-0.18 to 0.19 regressions.

It took over a thousand no_forward_declare pragmas to restore the desired behavior.

Many of the suggestions produced by this version were erroneous and broke the build:

• If the underlying type of an enum is bool then iwyu will generate forward declarations with an underlying type of _Bool
• iwyu will suggest illegal forward declarations of scoped enums that are inside a class

About half of the thousand new pragmas I needed to add were in source files which raises another long standing issue with iwyu: it suggests forward declarations in cpp files and not only in header files. There are zero valid reasons for this tool to add a forward declaration to a cpp file so every suggestion is generates is a false positive which must be manually suppressed.

[bolshakov-a]

If the underlying type of an enum is bool then iwyu will generate forward declarations with an underlying type of _Bool

It really does... Thank you! I hope I can take a look at the weekend.

iwyu will suggest illegal forward declarations of scoped enums that are inside a class

That's strange. I even wrote a test case that it doesn't occur. Could you please provide a minimal repro?

... another long standing issue with iwyu: it suggests forward declarations in cpp files and not only in header files. There are zero valid reasons for this tool to add a forward declaration to a cpp file so every suggestion is generates is a false positive which must be manually suppressed.

I agree. Maybe, I'll make a PR in some better times...

But I just wonder why you don't want to place opaque declarations instead of suppressing them? Do you really have to change enum underlying types often? And even if you really do, it could be easily done just with auto-replacing. Though, a lot of changes may be in git diff in that case...

[justusranvier]

That's strange. I even wrote a test case that it doesn't occur. Could you please provide a minimal repro?

I found the place where I though it happened and it turned out to be some ancient code that used old naming conventions so the namespace looked like a class name.

But I just wonder why you don't want to place opaque declarations instead of suppressing them? Do you really have to change enum underlying types often?

It's happened in the past but it is not frequent usually I'm just adding new enumerations without changing the underlying type. Mostly it's a concern for DRY. One enum in particular is referred to in over 230 source and header files. If the declaration is duplicated in each one of those files that's 230 new sources of potential future update mistakes that don't need to exist.

[kimgr]

I'm a little torn on this one:

• On the one hand, it sounds like IWYU is doing the right thing -- pushing forward-declarations to the leaf of the include graph wherever possible (this should give real savings in compile time)
• On the other hand, your use of bundling headers make sense to cut down on cruft (a single #include line can cover many forward-decls)

So it seems to me the problem is exactly what the issue title says: IWYU: pragma export doesn't work for forward-declarations. PR #1129 demonstrates the mechanics, I think that could be made to work for export too, but it's a little tricky how to maintain a mapping from export comments to the actual decls that need to be affected.

[kimgr]

@justusranvier I have a preliminary branch up at https://github.com/kimgr/include-what-you-use/tree/pragma-export-fwd to add support for IWYU pragma: export on forward-declarations.

I'm not sure if I've missed anything obvious, but it feels to me like it should work for the simplest case, at least.

If you could try it out in your context, that would be most helpful.

[justusranvier]

I should be able to test it either later this week or next week.

The test will involve deleting about 1500 no_forward_declare lines and seeing how many of them are no longer needed with the new version.

One question that has come up is this: when I opened the issue I wasn't thinking clearly about the fact that pragma: export was only for includes and not for symbols. Does your branch change that policy in general or just for forward declarations?

[kimgr]

@justusranvier Only for forward declarations. But I don't see a use for explicit reexport of other symbols (they're fully defined, right?). Am I missing something?

[justusranvier]

If IWYU pragma: export can apply to symbols then it could potentially replace uses of a mapping file, but when I look at the file I use for opentxs I don't see any entries that could potentially be removed because most of them are for headers I don't control (boost, qt, standard library) and so can't add export pragmas to them anyway.

[kimgr]

The use I intended was for exporting forward-declarations, e.g. something like:

// x.h
class MyIncompleteType;   // IWYU pragma:export

// main.cc
#include "x.h"

extern void bar(const MyIncompleteType&):

void foo(MyIncompleteType* p) {
    return bar(*p);
}

Symbol mappings are more for as-documented mappings of full uses, i.e. int8_t should always be provided by <cstdint>. In a forward-declare scenario, a fwd-decl will still be prefered to a symbol mapping, at least I think that's the case.

[justusranvier]

@justusranvier I have a preliminary branch up at https://github.com/kimgr/include-what-you-use/tree/pragma-export-fwd to add support for IWYU pragma: export on forward-declarations.

I'm not sure if I've missed anything obvious, but it feels to me like it should work for the simplest case, at least.

If you could try it out in your context, that would be most helpful.

It's working for me and that version allowed me to remove about 2400 lines of pragmas that are now no longer necessary.

[justusranvier]

Well now I found a problem.

I've been testing with --safe_headers but switching over to --nosafe_headers has been something I've intended to do for a while.

I tried it just now and iwyu is removing lines that are marked pragma: export.

Does pragma: export normally imply pragma: keep?

[kimgr]

Sounds like that is a separate issue from pragma: export on forward-declarations -- if so, let's open a new bug ticket for it so we don't conflate different things.

[kimgr]

Please give PR #1164 a whirl. I've refined the original branch and added some tests.

[kimgr]

(Actually, maybe that also fixes the removal of exported lines you saw. If not, please open a new ticket with more details)

[kimgr]

Merged #1164, closing this as fixed.