[cfe-dev] [Modules] Silent textual inclusion when files not found.

[Richard Smith]

On Tue, Jun 9, 2015 at 8:01 PM, Sean Silva <chisophugis at gmail.com> wrote:

>
>
> On Thu, Jun 4, 2015 at 1:27 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On Wed, Jun 3, 2015 at 6:24 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>
>>> On Wed, Jun 3, 2015 at 8:40 AM, Ben Langmuir <blangmuir at apple.com>
>>> wrote:
>>>
>>>>
>>>> On Jun 2, 2015, at 7:57 PM, Sean Silva <chisophugis at gmail.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Currently we do not seem to issue any form of diagnostic when there are
>>>> missing headers named in a module map. See the attached test case. Even
>>>> worse, we will just treat all headers in the module as textual. Hilarity
>>>> ensues.
>>>>
>>>>
>>>> Yep, this is llvm.org/PR20507
>>>>
>>>>
>>>> Some prior art in this area:
>>>> Daniel - r197485
>>>> Ben - r206664
>>>>
>>>> Based on these commits, it ostensibly seems that clang does *some* sort
>>>> of checking for missing files in a module map, but I can't seem to coax
>>>> clang into doing this in C++ language mode.
>>>>
>>>> Looking at the source code, it seems like we end up conflating
>>>> "unavailable" due to a failed `requires` with "a header is missing". It
>>>> sounds like we essentially need two notions "unsatisfied `requires`" and
>>>> "necessary header is missing" (a header guarded by an unsatisfied
>>>> `requires` does not count as "necessary"). Haven't dug in deep yet, but my
>>>> hypothesis is that somewhere along the way we silently treat a module with
>>>> missing headers as though it had an unsatisfied `requires`, leading to us
>>>> silently neglecting that it was ever in a module at all.
>>>>
>>>>
>>>> Yes, they both come out as the module being “unvavailable”, which is
>>>> why we don’t import it.  I think the right answer is that attempting to
>>>> import any unavailable module (even via an auto-import) should be an
>>>> error.  That is:
>>>>
>>>> module MissingHeader {
>>>>     header “exists.h"
>>>>     header “doesnt_exist.h”
>>>> }
>>>>
>>>> #include <exists.h> // error, this would import MissingHeader, which is
>>>> unavailable because we're missing header “doesnt_exist.h"
>>>>
>>>> module Top {
>>>>     header “Top.h”
>>>>     module A {
>>>>         requires non_existent
>>>>        header “A.h”
>>>>     }
>>>>     module B {
>>>>         header “B.h”
>>>>     }
>>>> }
>>>>
>>>> #include <Top.h> // OK
>>>> #include <A.h> // error, this would import Top.A, which is unavailable
>>>> because of the missing requirment
>>>> #include <B.h> // OK
>>>>
>>>> But handling the first case is more important I think.
>>>>
>>>>
>>>> I'm glad to put some effort into fixing this; I spent a good part of
>>>> today with a bizarre error that I traced back to this and I don't want my
>>>> users to have to deal with the same. Any pointers would be appreciated.
>>>>
>>>>
>>>> That would be great! I was coincidentally hoping to fix this myself in
>>>> the next few weeks, but if you beat me to it even better.
>>>>
>>>>
>>> I'm still acclimating myself to the modules code, do you have any
>>> concrete thoughts on how to do this? I was just planning on looking through
>>> for everywhere we use "unavailable" happens and figure out a pattern, but
>>> if you already have an idea, it might save me some time.
>>>
>>
>> Something like:
>>  * make ModuleMap::findModuleForHeader able to produce an unavailable
>> module, but treat it as being worse than any available module
>>  * make Preprocessor::HandleIncludeDirective issue an error if its call
>> to LookupFile produces a SuggestedModule that is unavailable
>>
>
> I ran into a bit of a snag when doing this. It looks
> like diag::err_module_header_missing (and diag::err_module_unavailable) are
> considered to be frontend diagnostics, so we can't issue them from Lex. Any
> ideas on how to proceed? It doesn't feel right to duplicate a diagnostic,
> it suggests we're missing a single point of truth for this.
>

Such shared diagnostics go in DiagnosticCommonKinds.td.


> Doing `git grep 'isAvailable([^)]'` suggests to me that we are checking
> the availability way too "deep" in the frontend code. It seems the only
> centralized place to check this would be in the module map parser, so maybe
> we can reinstate this check in the module map parser like it was before
> r197485 (the commit that regressed our diagnosing missing headers)? It
> seems like that commit removed this functionality as work towards not
> stating headers while reading the module map, but nothing seems to have
> come of that.
>

That sounds like it'd cause us to reject a module map covering two modules,
if one of them is missing a header but we only care about using the other
one.


> -- Sean Silva
>
>
>>
>> This should probably happen even if modules is disabled (that is, when we
>> are parsing module maps but not actually building / using precompiled
>> module files).
>>
>> Also, any preferred bikeshed for the names for "unvailable because of
>>> unsatisfied requires" and "necessary header is missing" concepts in the
>>> source?
>>>
>>
>> If the response to both is the same (as Ben suggested, and I agree) then
>> I'm not sure that we need to distinguish them anywhere other than the place
>> where we produce the diagnostic.
>>
>>
>>> -- Sean Silva
>>>
>>>
>>>>
>>>> -- Sean Silva
>>>> <testmoduledepbuildfail.tar>
>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150611/c58d06fe/attachment.html>