[clang] [clang][docs] assert.h is not a good candidate for a textual header (PR #165057)

[Richard Smith via cfe-commits]

zygoloid wrote:

> We've (Apple) found `textual header`s to be very limited in practice. Basically they can't cause a declaration to exist in multiple modules. Swift will see those declarations as distinct because the module name is part of the type/function name, and as distinct things they're by definition incompatible. Sometimes it kind of works out through the type merging that clang does, but mostly it doesn't.

Conversely, we've (Google) found them to be essential for modeling dependency relationships between notional modules, where not all `#include`d files are modular headers. And we do have situations where we have the same header in multiple modules (even cases where it's textual in one and modular in another), and it works in practice for us.

To what extent is this a Swift import problem rather than a Clang modules problem? (To be clear: I'm not suggesting that would make it any less severe, but it might allow us to better separate the concerns.) The treatment of the module name as part of the entity name sounds Swift-specific, at least.

> What even _should_ happen if modules A and B include <assert.h>, but B defines NDEBUG before doing so? If I just import A and B and don't include <assert.h> myself, which definition of `assert` do I see?

If you try to use the `assert` macro in that state, I believe you would get an error because it doesn't have a consistent definition. If you do include `<assert.h>` yourself, it will `#undef` and re-`#define` the macro, and you'll get that macro instead of the imported ones.

> I thought even strict `use` checking rules don't complain about headers that aren't covered by any module, it only flags module imports that aren't listed?

That's the difference between `-fmodules-decluse` and `-fmodules-strict-decluse` -- the latter also [rejects undeclared inclusions](https://github.com/llvm/llvm-project/blob/3f26d567535b0b06824a2470ae3b4e02288615e6/clang/lib/Lex/ModuleMap.cpp#L552).

> I'm trying to say that `<modular_header_that_has_an_assert.h>` doesn't have a good way to know that using `assert` will be a bit weird with modules. If `<assert.h>` wasn't a modular header (textual or otherwise), then `<modular_header_that_has_an_assert.h>` could get a `-Wnon-modular-include-in-module` to let it know. Since the owner of `<modular_header_that_has_an_assert.h>` probably isn't aware of the subtle behavior difference, and probably never really thought about picking up `NDEBUG`, it's just what happens to happen in regular C without modules. Wheras if `<assert.h>` is textual modular, the behavior changes but it's not obvious.

I mean, I suppose, but this would be a potential risk any time you include a textual header from a non-textual header in a module, just like it'd be a risk any time you include an entirely undeclared header from a modular header, because a textual header could likewise depend on enclosing state.

If you want a warning for this kind of issue, I wonder if really we have two different kinds of textual headers we should be distinguishing here -- those like `<assert.h>` that are unsafe to `#include` into a modular header (but fine to include from another textual header of the same kind or from a source file), and those like `.def` files that are largely safe to `#include` into a modular header. Maybe this should be expressed as an attribute on the `textual header` declaration?

That said, it seems that when you mark `modular_header_that_has_an_assert.h` as being a modular header, you are explicitly requesting that the state of `NDEBUG` in an includer of that header does not affect the behavior of the header, and that's the behavior you get. Being a modular header means you get compiled as-if from a clean preprocessor + AST state, so there simply isn't an `NDEBUG` that you could inherit from anywhere other than the command line. So the purpose of such checking would be to detect cases where your modularization accidentally did the wrong thing -- where you thought a header was modular but it wasn't. And maybe there's something more general we can do about that:

If we want some detection of cases where a modular build behaves differently from a textual inclusion build, I think we could probably build that without building too much new infrastructure, at least for local submodule visibility builds. In essence, we'd enable use of module maps but not precompiled modules, and textually enter all modular headers even if they're from a different top-level module. Then if we see a use of an identifier that has a macro definition that isn't visible, but would be visible if all modules were visible, we produce an error (and likewise for name lookup and reachability checks).

https://github.com/llvm/llvm-project/pull/165057