[Modules] Silent textual inclusion when files not found.

[_sean_silva]

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.

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.

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.

– Sean Silva

testmoduledepbuildfail.tar (5.5 KB)

[blangmuir]

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.

[_sean_silva]

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.

Also, any preferred bikeshed for the names for "unvailable because of
unsatisfied requires" and "necessary header is missing" concepts in the
source?

-- Sean Silva

[zygoloid]

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

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]

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

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

Thanks for the pointers. I'll take a look at doing this.

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.

But aren't they different when it comes time for clang to build the module?
We want to build a top-level module even if it has a submodule with an
unsatisfied "requires" (e.g. _Builtin_intrinsics has submodules with
contradictory `requires`), taking appropriate action to exclude any headers
with unsatisfied `requires`. On the other hand (modulo submodules already
removed from consideration due to unsatisfied `requires`), a missing header
should be treated as a hard error. I guess this latter case will already
cause an error due to a failed inclusion, but just from a QoI perspective
it seems preferable to diagnose this immediately up front before diving
into building the module (so that it dominates any other diagnostics that
might be produced while building the module).

-- Sean Silva

[zygoloid]

I think you're right, but I think that will be the emergent behavior from
treating the two cases the same: if a module's header is missing, the
corresponding *top-level* module (and all its submodules) gets marked
unavailable, so we should never try to implicitly build it.

There might be a file system race here, where the main build process thinks
the module is available, but the module build finds a header is missing and
builds an empty module because all the submodules are unavailable. That's
probably worth fixing, but again the fix doesn't really depend on the
different kinds of availability: if the top-level module is unavailable, we
should produce an error if we try to build a module file for it. (This is
probably easy to test by building an unavailable module with -cc1
-emit-module.)

[_sean_silva]

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

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

Thanks for the pointers. I'll take a look at doing this.

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.

But aren't they different when it comes time for clang to build the
module? We want to build a top-level module even if it has a submodule with
an unsatisfied "requires" (e.g. _Builtin_intrinsics has submodules with
contradictory `requires`), taking appropriate action to exclude any headers
with unsatisfied `requires`. On the other hand (modulo submodules already
removed from consideration due to unsatisfied `requires`), a missing header
should be treated as a hard error.

I think you're right, but I think that will be the emergent behavior from
treating the two cases the same: if a module's header is missing, the
corresponding *top-level* module (and all its submodules) gets marked
unavailable, so we should never try to implicitly build it.

Thanks for the explanation, that makes perfect sense.

-- Sean Silva

[_sean_silva]

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.

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.

-- Sean Silva

[zygoloid]

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]

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