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

[Sean Silva]

On Thu, Jun 11, 2015 at 3:27 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> 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.
>

Thanks.

-- Sean Silva


>
>
>> 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/3a4ea829/attachment.html>