Nested wrapper header support

[jbytheway]

I ran into a problem attempting to use IWYU on a project with the following pattern:

• a 3rd-party library private header, included by
• a 3rd party library public header, included by
• a local project header intended to be the only way to include headers from that 3rd party library (and which also uses symbols from that library), included by
• a local project source file.

The existing special case code for handling private headers forces the last header to include the first one, even when a mapping exists from the first to the second.

This PR fixes that, by tweaking the logic so that we will retain private includes when they exist, but not necessarily add them whenever we are a potential public header for them.

The test I've added piggy-backs on an existing test to take advantage of its mapping file. Let me know if that's inappropriate and I should make this test independent of existing test files.

[kimgr]

Thank you, this looks interesting. I can't claim to understand the premise of the original code, but the improvement as described sounds useful.

I would much prefer if the test was free-standing -- is it possible to create this double re-export without mappings, e.g. with nested IWYU pragma: export?

I'll try and get my head around this in the meantime.

[jbytheway]

Sure. Test rewritten to be simpler (using just pragmas) and independent of the other tests.

[kimgr]

I've been looking at this for a while, and I honestly can't get my head around it :)

Can you explain in more detail why that direct-includes check solves the problem of nested exporting? It seems to me this indicates a problem in IncludePicker::GetCandidateHeadersForFilepathIncludedFrom (called via public_headers), and that you're now just papering over its mistake.

Note that your testcase would succeed even without the IWYU pragma: export in export_nesting.h because of the associated-header rule (a.cpp always retains a.h, and they're analyzed as a unit), so I wonder if it reflects the original problem perfectly.

Thanks for any clarifications.

[jbytheway]

Can you explain in more detail why that direct-includes check solves the problem of nested exporting? It seems to me this indicates a problem in IncludePicker::GetCandidateHeadersForFilepathIncludedFrom (called via public_headers), and that you're now just papering over its mistake.

So, firstly, a private header can only be included from headers which are somehow explicitly marked up as exporting that private header (by export pragmas, a mapping file, or other means). The code computes the transitive closure of these relationships, and all transitive public headers are correctly returned from GetCandidateHeadersForFilepathIncludedFrom. That works perfectly fine.

The problem is that the code prior to this change also enforces the reverse relationship: any header which is considered to export a particular private header must include that private header (at least, if it happens to use any symbols from it). This pre-empts most of the other logic about deciding what header is suitable for a symbol. That is not desirable in this use case.

Here's the relevant logic (as on current master); this is the very first thing that happens to assign an include to a use:

include-what-you-use/iwyu_output.cc

Lines 740 to 766 in cd358ea

	// Step (1) The easy case: decls that map to just one file. This
	// captures both decls that aren't in private header files, and
	// those in private header files that only map to one public file.
	// For every other decl, we store the (decl, public-headers) pair.
	for (OneUse& use : *uses) {
	// We don't need to add any #includes for non-full-use.
	if (use.ignore_use() || !use.is_full_use())
	continue;
	// Special case #1: Some uses come with a suggested header already picked.
	if (use.has_suggested_header()) {
	desired_headers.insert(use.suggested_header());
	continue;
	}
	// Special case #2: if the dfn-file maps to the use-file, then
	// this is a file that the use-file is re-exporting symbols for,
	// and we should keep the #include as-is.
	const string use_file = ConvertToQuotedInclude(GetFilePath(use.use_loc()));
	if (use.PublicHeadersContain(use_file)) {
	use.set_suggested_header(ConvertToQuotedInclude(use.decl_filepath()));
	desired_headers.insert(use.suggested_header());
	LogIncludeMapping("private header", use);
	} else if (use.public_headers().size() == 1) {
	use.set_suggested_header(use.public_headers()[0]);
	desired_headers.insert(use.suggested_header());
	LogIncludeMapping("only candidate", use);
	}
	}

So for any full use without an existing suggested header, we hit the problematic code; this part:

include-what-you-use/iwyu_output.cc

Lines 757 to 759 in cd358ea

	if (use.PublicHeadersContain(use_file)) {
	use.set_suggested_header(ConvertToQuotedInclude(use.decl_filepath()));
	desired_headers.insert(use.suggested_header());

i.e. if the decl was in a private header and we export that header then we are forced to include it.

My change is attempting to weaken that. So now, any header that already includes a private header which it exports can keep that include (whereas it would have been removed were the included file a private header which it doesn't export), but it is not forced to be added when not already present.

Note that your testcase would succeed even without the IWYU pragma: export in export_nesting.h because of the associated-header rule (a.cpp always retains a.h, and they're analyzed as a unit), so I wonder if it reflects the original problem perfectly.

The includes of export_nesting.cc are irrelevant for this test. It's the includes of export_nesting.h that matter. Indeed the test would pass without that pragma, but that doesn't matter. The important thing is that it wouldn't fail without that pragma (and without the code change).

[kimgr]

Thanks for the elaboration.

i.e. if the decl was in a private header and we export that header then we are forced to include it.

I think this may be include-what-you-use policy -- IWYU does its best to flatten the header dependency graph. What if we included <sstream> and only used std::string -- wouldn't we want to get rid of <sstream> in favor of <string>?

I guess explicit exports/mappings could override that, but I guess I don't understand how your change takes them into account.

My change is attempting to weaken that. So now, any header that already includes a private header
which it exports can keep that include (whereas it would have been removed were the included file a
private header which it doesn't export), but it is not forced to be added when not already present.

OK, so reading the implementation with your explanation in mind, I think I understand what you're changing, at least :)

Current behavior: if a mapping leads back from decl-file to use-file, attribute use to decl-file.
New behavior: if a mapping leads back from decl-file to use-file and use-file directly includes decl-file, attribute use to decl-file.

This implies there's some case where a mapping can occur from decl-file to use-file, without use-file directly including decl-file. Why can we assume this happened because of nested re-exports?

Sorry for being slow, I just want to make sure we don't regress some other common scenario.

[jbytheway]

i.e. if the decl was in a private header and we export that header then we are forced to include it.

I think this may be include-what-you-use policy -- IWYU does its best to flatten the header dependency graph. What if we included <sstream> and only used std::string -- wouldn't we want to get rid of <sstream> in favor of <string>?

Yes, and we would, because we don't export <string> (i.e. we are not a public header for <string>). This logic only applies in the specific case where we are a public header for some decl we use in a private header. This is a niche situation.

Even if we were a public header for <string>, the later logic will ensure that we will end up including some header which is a public header for <string>, so in your scenario it would only remain unchanged if <sstream> was also a public header for <string>, and in that case I think it should be fine to just include <sstream>.

I guess explicit exports/mappings could override that, but I guess I don't understand how your change takes them into account.

The explicit mappings are taken account inside the call to use.PublicHeadersContain(use_file).

Current behavior: if a mapping leads back from decl-file to use-file, attribute use to decl-file.
New behavior: if a mapping leads back from decl-file to use-file and use-file directly includes decl-file, attribute use to decl-file.

This implies there's some case where a mapping can occur from decl-file to use-file, without use-file directly including decl-file. Why can we assume this happened because of nested re-exports?

Well, there are other ways. It can happen via exports, mapping files, pragma: private, or some of the IWYU heuristics. But I think all should be handled similarly.

Sorry for being slow, I just want to make sure we don't regress some other common scenario.

In any code that was previously IWYU-compliant, this logic (before the change) will have ensured that use-file already includes decl-file. And in that situation the new behaviour matches the current behaviour. So, this change ought not to have any impact on code already using IWYU; it only matters for new code.

(It's possible there's some corner case e.g. when including decl-file through a symlink that something might go wrong; I don't understand IWYU's path canonicalization well enough to be certain what will happen there)

[kimgr]

Thanks for the extensive explanation, now I understand.

I have some style comments on the tests, I'll get back to that later today. Otherwise, looks good!

[kimgr]

Additional includes are usually called -d1.h and -i1.h, I think for "direct" and "indirect".

So I think these two supporting headers should be renamed:

• export_nesting_1.h -> export_nesting-d1.h
• export_nesting_2.h -> export_nesting-i1.h

[kimgr]

Left a few nits inline, otherwise I think this is ready to go.

[kimgr]

Full stop at end of sentence

[kimgr]

Oh, and please squash the commits together.

[kimgr]

Bingo, thanks!