[PATCH] Add stopgap option -fmodule-implementation-of

[Richard Smith]

On Wed, May 28, 2014 at 5:31 PM, Ben Langmuir <blangmuir at apple.com> wrote:

>
> On May 28, 2014, at 1:46 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Tue, May 27, 2014 at 8:15 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>
>> Hi Richard,
>>
>> Thanks for the review! As I said, this didn’t feel like a great fix to me
>> either.
>>
>> On May 27, 2014, at 3:00 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> On Tue, May 27, 2014 at 8:31 AM, Ben Langmuir <blangmuir at apple.com>wrote:
>>
>>> CC’ing djasper, since you made -fmodule-name a driver option, so you may
>>> be familiar with this code.
>>>
>>> I’m not super happy with this patch, but we really need some way to not
>>> use modular imports while compiling the implementation (e.g. .c/.cpp/.m
>>> files) files that correspond to module headers.  Both because we are not
>>> yet isolating submodules from each other enough to do incremental rebuilding
>>
>>
>> Can you explain what you mean by this? In particular, I'm very unclear on
>> how it the translation unit being compiled has any relation to this issue.
>> (I've not seen any problems that match this description so far.)
>>
>>
>> My understanding is that when we build a module with submodules, the
>> submodules can access the declarations/definitions in previously built
>> submodules.  Until that is fixed, it seems that to be safe we should
>> rebuild everything that depends on a module whenever any of its headers
>> change.  That ruins incremental rebuilding for the author of the module
>> whenever they change their headers since all of their implementation files
>> likely would depend on the module.
>>
>
> I don't think it ruins anything. But I agree that it's a case where a
> build with modules can sometimes be more expensive. I thought this was a
> well-known and expected property of modules builds?
>
>
> What’s unexpected to me is that changing a header whose contents are not
> usually visible may still require rebuilding all of my .cpp files.
> module Foo { module One { header “One.h” } module Two { header “Two.h” } }
>
> // One.cpp - I don’t want to rebuild when Two.h changes
> #import <Foo/One.h>
>
> Do we agree that this is unnecessary if submodules cannot accidentally be
> affected by changes in other submodules they don’t import (and we have some
> way to get the set of dependency files for just the submodule)?
>

No, I don't agree with that. One.cpp might inline some function definitions
from Two.h, for instance. Or it might fail to build because it declares
something that conflicts with something in Two.h.

  and because with the VFS added to the mix we may get a mix of textual and
>>> modular imports of the same headers when doing “quote”-style includes,
>>> which doesn’t yet work properly (multiple definitions, hidden definitions,
>>> fun stuff).
>>>
>>
>> I assume you intend for this bug to be fixed eventually. Once that's
>> done, is there still value in this flag?
>>
>>
>> Right, ideally we should still find the module with the VFS, and even if
>> we don’t it shouldn’t matter.  Once that (and the above) is fixed, the flag
>> has no use that I can think of.
>>
>>
>>
>>>     Add stopgap option -fmodule-implementation-of <name>
>>>
>>
>> Please do not add a stopgap workaround to our stable and
>> backwards-compatible driver interface; just add it to -cc1 instead.
>>
>>
>>     This flag specifies that we are building an implementation file of the
>>>     module <name>, preventing importing <name> as a module. This does not
>>>     consider this to be the 'current module' for the purposes of doing
>>>     modular checks like decluse or non-modular-include warnings, unlike
>>>     -fmodule-name.
>>
>>
>> I don't see any relation between the flag's name and its functionality;
>> there seems to be no reason for this to be linked to the translation unit
>> being the implementation of any particular module (and if there were,
>> that's what -fmodule-name is for). Instead, I think what you're trying to
>> specify is that a particular module is included textually for this
>> compilation. Please pick a name that suggests that functionality instead.
>>
>>
>> The name is related to the use-case, not the mechanism.  I originally
>> thought about something like “-no-module=<name>", but I didn’t want to use
>> a generic diagnostic e.g. “error: @import of module specified with
>> -no-module=”).  With the more specific flag I have a more specific
>> diagnostic that says not to @import the module you’re implementing and use
>> #import instead.
>>
>
> I don't think that is an appropriate diagnostic. There's no problem with
> importing the module you're building, and we shouldn't have a diagnostic
> that suggests there is.
>
>
> There is the VFS issue I mentioned causing non-modular includes with
> includer-relative lookups. In practice this should only be a problem when
> in the implementation files of the module.
>

You can certainly find modules other than "your own" via relative paths (to
the extent that "your own module" is a meaningful concept). Maybe it
doesn't happen for implicit framework modules, but they're not the only
case.

Since you also cannot use @import of your module’s headers within the
> module itself (yet), this gives a very simple and consistent rule: don’t
> @import from the current module/module being implemented, use
> #include/#import instead.
>

As an aside, since @import is already recognized by the lexer, have you
considered the possibility of treating it as a #include in that case?
Blocking @imports to submodules of your own module seems like a workaround
for an implementation weakness, rather than a sensible language design
choice.

This is just a tradeoff between (possibly) better build times when you have
> changed your headers and better build times when you haven't done so (or
> when you will rebuild lots of source files). We shouldn't be making that
> decision for our users.
>
>
> To be clear I see the incremental building of c/cpp/m file issues I
> described above as a bug, which I would like to fix when I have time.  Once
> that is fixed, the only tradeoff would be between the cost of building a
> pcm and importing it vs. the cost of textually including a subset of its
> headers.  I don’t have a good feel for whether that is something to worry
> about: do you think it will be valuable for users to be able to opt-out
> from auto-import for this purpose?
>

Probably not, and since it's not entirely semantically equivalent, I'm
worried that it'd lead to subtle problems if we allowed this.

The reason -fmodule-name didn’t seem right here is that it treats the
>> current TU as being part of a module build (i.e. we are inside
>> compileModule and/or have set -emit-module).  We look at CurrentModule
>> (which is what -fmodule-name sets) in a number of places and can get odd
>> behaviour if you aren’t actually compiling a module.
>>
>
> We already have tests that use it in this way, and that's how
> -fmodules-decluse is intended to work. If -fmodule-name doesn't work when
> not compiling a module (and it sounds like there are cases where it
> doesn't), that seems like a bug.
>
>
> Okay, that’s what I thought, but it’s good to know for sure that was the
> intent.
>
> If we clean up the various “CurrentModule”, “SourceModule”,
> “CompilingModule” etc so that -fmodule-name does not do more than necessary
> we could alternately express what I’m doing as -fmodule-name=Blah
> -fimplementation-is-not-in-module or something like that, but I’m not sure
> that helps.
>
>
> For example, when we call loadModule, we skip loading the named module and
>> making it visible, but we still add it to the list of imports.  That causes
>> trouble with PCH, which then think that this module was actually loaded
>> already.  I also don’t want to unconditionally do decluse checking or give
>> non-modular include warnings, etc when building an implementation file.
>>
>> Maybe when we really are inside a module build, a better way to spell
>> -fmodule-name=Blah -emit-module would be -emit-module=Blah? But we already
>> have -fmodule-name as a driver option so I don’t know if we would really
>> want to change this.
>>
>>
>> I'm also not sure how this is supposed to work in general: suppose the
>> implementation of module A imports module B, and module B includes a header
>> from module A. Do we get another variant of module B, or do we textually
>> include A into B (generating probably-broken submodule visibility), or is
>> that now an error, or something else?
>>
>>
>> Module B is unaffected by this option, since it gets reset in
>> LangOptions::resetNonModularOptions(), which admittedly would end up going
>> back to mixing modular and non-modular includes so maybe an error is more
>> appropriate.   I’m curious: what would the use case for this pattern be? I
>>  can see why you would want a submodule of A (e.g. A.Internal) to do this,
>> but why some other top-level module B?
>>
>
> Because proper layering is hard :)
>
>
> I can agree with that ;)
>
>
> Take a look at LLVM: our module maps there have separate modules for
> Analysis and Transforms (which seems reasonable to me), and yet source
> files in lib/Transforms use headers from Analysis and vice versa.
>
>
> This may not be a great example; I only see one reverse dependency between
> Analysis and Transforms:
> lib/Analysis/MemoryBuiltins.cpp -> "llvm/Transforms/Utils/Local.h”
> which is only being used for EmitGEPOffset.
>

Right, but it only took me a few seconds of grepping to find that one. I'm
sure there are lots more. If this happens in LLVM (which, for projects of
its size, has very disciplined layering), I'm very confident it'll happen
for most large projects.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140528/a422d0f4/attachment.html>