This is an archive of the discontinued LLVM Phabricator instance.

[modules] Handle modules with nonstandard names in module.private.modulemaps
ClosedPublic

Authored by graydon on Dec 16 2016, 9:04 AM.

Details

Summary

The module system supports accompanying a primary module (say Foo) with
an auxiliary "private" module (defined in an adjacent module.private.modulemap
file) that augments the primary module when associated private headers are
available. The feature is intended to be used to augment the primary
module with a submodule (say Foo.Private), however some users in the wild
are choosing to augment the primary module with an additional top-level module
with a "similar" name (in all cases so far: FooPrivate).

This "works" when a user of the module initially imports a private header,
such as '#import "Foo/something_private.h"' since the Foo import winds up
importing FooPrivate in passing. But if the import is subsequently recorded
in a PCH file, reloading the PCH will fail to validate because of a cross-check
that attempts to find the module.modulemap (or module.private.modulemap) using
HeaderSearch algorithm, applied to the "FooPrivate" name. Since it's stored in
Foo.framework/Modules, not FooPrivate.framework/Modules, the check fails and
the PCH is rejected.

This patch adds a compensatory workaround in the HeaderSearch algorithm
when searching (and failing to find) a module of the form FooPrivate: the
name used to derive filesystem paths is decoupled from the module name
being searched for, and if the initial search fails and the module is
named "FooPrivate", the filesystem search name is altered to remove the
"Private" suffix, and the algorithm is run a second time (still looking for
a module named FooPrivate, but looking in directories derived from Foo).

Accompanying this change is a new warning that triggers when a user loads
a module.private.modulemap that defines a top-level module with a different
name from the top-level module defined in its adjacent module.modulemap.

Diff Detail

Repository
rL LLVM

Event Timeline

graydon updated this revision to Diff 81765.Dec 16 2016, 9:04 AM
graydon retitled this revision from to [modules] Handle modules with nonstandard names in module.private.modulemaps.
graydon updated this object.
graydon added reviewers: manmanren, doug.gregor.
graydon added a subscriber: cfe-commits.
bruno added a subscriber: bruno.Dec 16 2016, 3:00 PM
manmanren accepted this revision.Dec 20 2016, 9:38 AM
manmanren edited edge metadata.

Thanks,
Manman

This revision is now accepted and ready to land.Dec 20 2016, 9:38 AM
bruno added a comment.Dec 20 2016, 9:59 AM

Hi,

Thanks for working on this.

include/clang/Basic/DiagnosticLexKinds.td
647 ↗(On Diff #81765)

It would be nice if we could also emit a FixIt to tell the user how should the modulemap be fixed.

lib/Lex/HeaderSearch.cpp
208 ↗(On Diff #81765)

Remove the curly braces here

211 ↗(On Diff #81765)

Remove this newline

216 ↗(On Diff #81765)

Remove this newline

test/Modules/implicit-private-with-different-name.m
9 ↗(On Diff #81765)

Can you check for the entire warning message?

graydon marked 5 inline comments as done.Dec 20 2016, 2:44 PM

Addressed review comments

graydon updated this revision to Diff 82161.Dec 20 2016, 2:45 PM
graydon edited edge metadata.

Updates to address review comments

bruno added a reviewer: bruno.Dec 20 2016, 2:59 PM
bruno added inline comments.
test/Modules/implicit-private-with-different-name.m
13 ↗(On Diff #82161)

I think we can enhance the usability a bit here, how about:

warning: top-level module 'APrivate' in private module map, expected a submodule of 'A'
note: make 'APrivate' a submodule of 'A' to ensure it can be found by name

framework module APrivate {
                 ^
              A.Private
graydon added inline comments.Dec 20 2016, 3:06 PM
test/Modules/implicit-private-with-different-name.m
13 ↗(On Diff #82161)

I'm not sure what you're requesting; as far as I can see that is exactly the diagnostic I'm emitting (along with the fixit)

bruno accepted this revision.Dec 20 2016, 4:00 PM
bruno edited edge metadata.

Right, I missed it, LGTM

This revision was automatically updated to reflect the committed changes.