Page MenuHomePhabricator

Warning for framework include violation from Headers to PrivateHeaders
ClosedPublic

Authored by bruno on May 23 2018, 4:36 PM.

Details

Summary

Framework vendors usually layout their framework headers in the following way:

Foo.framework/Headers -> "public" headers
Foo.framework/PrivateHeader -> "private" headers

Since both headers in both directories can be found with #import <Foo/some-header.h>, it's easy to make mistakes and include headers in Foo.framework/PrivateHeader from headers in Foo.framework/Headers, which usually configures a layering violation on Darwin ecosystems. One of the problem this causes is dep cycles when modules are used, since it's very common for "private" modules to include from the "public" ones; adding an edge the other way around will trigger cycles.

Add a warning to catch those cases such that:

In file included from test.m:1:
./A.framework/Headers/A.h:2:10: warning: public framework header includes private framework header 'A/APriv.h' [-Wframework-include-private-from-public]

^

This only works for the layering violation within a framework.

rdar://problem/38712182

Depends on D47157

Diff Detail

Event Timeline

bruno updated this revision to Diff 148317.May 23 2018, 4:36 PM
bruno created this revision.

Update to a more recent version of the patch.

vsapsai added inline comments.May 24 2018, 12:42 PM
include/clang/Basic/DiagnosticGroups.td
34–35

It might be convenient for users to have a warning group that will cover different framework warnings, something like -Wframework-hygiene. But it's out of scope for this review, more as an idea for future improvements.

lib/Lex/HeaderSearch.cpp
625

In this function we accept some paths that aren't valid framework paths. Need to think more if it is OK or if we want to be stricter.

893

s/api/API/

895

My opinion on readability is personal, so take it with a grain of salt. What if you make variable names more consistent, like

  • IsIncluderPrivateHeader
  • IsIncluderFromFramework
  • IsIncludeePrivateHeader
test/Modules/framework-public-includes-private.m
34

I'm not entirely sure it's not covered by existing test. It might be worth testing private header including public header and private header including another private header.

bruno updated this revision to Diff 149361.May 31 2018, 2:52 PM

Update patch after changes to D47157. Also address some of Volodymyr feedback.

bruno marked an inline comment as done.May 31 2018, 2:58 PM
bruno added inline comments.
include/clang/Basic/DiagnosticGroups.td
34–35

Yep, that should come once we land all other bits.

lib/Lex/HeaderSearch.cpp
625

It should be ok at this point, otherwise the framework style path would have failed before finding this header.

test/Modules/framework-public-includes-private.m
34

The warning is on by default and clang already have the scenario you described in other module tests - those would fail if there's a bug in the logic here.

vsapsai added inline comments.Jun 8 2018, 3:25 PM
lib/Lex/HeaderSearch.cpp
683

Please capitalize "api". The rest looks good.

bruno updated this revision to Diff 152198.Jun 20 2018, 4:22 PM

Address Volodymyr's comments

vsapsai accepted this revision.Jun 22 2018, 3:22 PM

Looks good to me. The only problem is I'm not entirely sure this warning will interact with real code the way I expect it to. We'll need to keep an eye on it and tweak if necessary.

This revision is now accepted and ready to land.Jun 22 2018, 3:22 PM
bruno closed this revision.Jun 25 2018, 3:36 PM

Committed r335542