[C++20][Modules] deleted definitions should be allowed in header units

[kaimfrai]

Since non-inline external definitions in header units were disallowed in commit/335668b, using standard header units has become impractical.
In particular, the example given at https://clang.llvm.org/docs/StandardCPlusPlusModules.html#header-units

clang++ -std=c++20 -xc++-system-header --precompile iostream -o iostream.pcm

no longer compiles with current Clang 16 (tried libc++ and libstdc++) while it did work before.

Errors include but are not limited to:

libstdc++

include/c++/12/new:206:8: error: non-inline external definitions are not permitted in C++ header units
  void launder(volatile void*) = delete;

include/c++/12/type_traits:2125:27: error: non-inline external definitions are not permitted in C++ header units
      static const size_t _S_alignment = 0;

libc++

 include/c++/v1/__type_traits/aligned_union.h:29:25: error: non-inline external definitions are not permitted in C++ header units
    static const size_t value = _I0;

include/c++/v1/__functional/reference_wrapper.h:100:27: error: non-inline external definitions are not permitted in C++ header units
template <class _Tp> void ref(const _Tp&&) = delete;

Unfortunately, this issue isn't limited to libstdc++ and libc++. Not only do other headers often include standard headers, but it's entirely resonable to have = delete; in header files. They are therefore also unusable as header units. Transitioning from headers to header units is thus limited to a small subset of headers, while Clang 15 allowed many more. While I think it's the correct default to error out in these cases, I would appreciate an explicit flag that skips over this rule, at least until P1502R1 (Standard library Header Units) or P2465R3 (std module) are implemented. For example:

clang++ -std=c++20 -xc++-system-header -fallow-non-inline-external-definitions-in-header-units --precompile iostream -o iostream.pcm

Without such a flag I would find it very hard to make use of standard header units as they are implemented right now.

[llvmbot]

@llvm/issue-subscribers-clang-modules

[dwblaikie]

Might be simpler to consider deleted definitions to be implicitly inline - it's more like how it works in practice.

[dwblaikie]

I think it's a pretty simple either implementation or spec bug/omission.

I doubt anyone intended to disallow deleted definitions in header units.

[ChuanqiXu9]

Might be simpler to consider deleted definitions to be implicitly inline - it's more like how it works in practice.

Yeah, it sounds good. In the language side, only the names have linkage instead of declarations. So from the perspective of language, the name of the deleted function can be external. But I guess it may be fine for the compiler to treat it as implicitly inline since the deleted definitions should never appear in object files.

I think it's a pretty simple either implementation or spec bug/omission.

According to @iains , some people of wg21 are discussing to remove such rules in future.

For this issue report, I feel it makes sense to skip the rule. Although LLVM16 is going to be branched, we don't need to be hurry since it should be possible to backport the patch since this is a (severe) regression bug.

---

(This section is not related to the issue report),

Out of curiosity, I want to ask how do you use header units? What is the build system you're using? Currently, wg21 is still discussing how to support header units. It looks like cmake wouldn't support header units until they get a conclusion. I want to say that I feel the header units is not well tested and used a lot in practice now. So I am curious about your practice and using experience for header units.

[ChuanqiXu9]

Sent https://reviews.llvm.org/D141894

[iains]

well, we anticipated that this could cause some code to flag an error.

For clarification: there are some WG21 members who are saying they think this rule should be removed. I am not aware of any paper that actually proposes its removal (so that as far as I know, the rule is the same in C++23).

I think that the
include/c++/12/type_traits:2125:27: error: non-inline external definitions are not permitted in C++ header units static const size_t _S_alignment = 0;

• cases look like a bug, tu-local declarations are allowed. (https://eel.is/c++draft/module#import-6
  ) I guess the const has somehow tripped up the logic.

• perhaps we should seek clarification from core on whether this was intended to apply to deleted functions.

Otherwise - from an engineering perspective

If a header contains a non-inline external definition, then it can only be used once (since multiple imports are an ODR violation), therefore actually it probably makes little sense to make it into a header unit, that is just extra build-time work - (it might as well be included textually in the single place it is used).

[iains]

The issue with the deleted function is that it is a definition - if it was a declaration, the rule would not fire.

I suppose that we could implement @dwblaikie 's suggestion to make them implicitly inline. I wonder if there are other similar cases that will trip us up.

I will file a question with core now - obviously, clang can elect to make a dialect as @ChuanqiXu9 has proposed in https://reviews.llvm.org/D141894 but let's see what answer we get first?

[ChuanqiXu9]

well, we anticipated that this could cause some code to flag an error.

For clarification: there are some WG21 members who are saying the think this rule should be removed. I am not aware of any paper that actually proposes its removal (so that as far as I know, the rule is the same in C++23).

Thanks for the clarification. That's what I understood. Maybe I didn't tell things clearly.

I think that the include/c++/12/type_traits:2125:27: error: non-inline external definitions are not permitted in C++ header units static const size_t _S_alignment = 0;

• cases look like a bug, tu-local declarations are allowed. (https://eel.is/c++draft/module#import-6
  ) I guess the const has somehow tripped up the logic.

I think the behavior is correct. Since the name '_S_alignment' has not internal linkage. According to http://eel.is/c++draft/basic.link, _S_alignment doesn't live in namespace scope so [basic.link]p3 doesn't apply for it. And _S_alignment lives in aligned_union and aligned_union has external linkage, so _S_alignment has external linkage according to [basic.link]p5. So I think the behavior here is fine.

• perhaps we should seek clarification from core on whether this was intended to apply to deleted functions.

Otherwise - from an engineering perspective

If a header contains a non-inline external definition, then it can only be used once (since multiple imports are an ODR violation), therefore actually it probably makes little sense to make it into a header unit, that is just extra build-time work - (it might as well be included textually in the single place it is used).

Year, since this is the defect of the language spec. It would be better to ask this in core. But clang16 is going to be branched. Even if we can backport patches before the formal release, the time is really limited. I really feel upset that we can't compile a hello world example for header units. So I think we should both accept the patch and ask questions in wg21. Otherwise, we have to mark header units as unusable in clang16, which seems like a big regression.

[ChuanqiXu9]

The issue with the deleted function is that it is a definition - if it was a declaration, the rule would not fire.

I suppose that we could implement @dwblaikie 's suggestion to make them implicitly inline. I wonder if there are other similar cases that will trip us up.

Since Clang16 is going to be branched. I don't think we should perform the action now. It has wider impact.

I will file a question with core now - obviously, clang can elect to make a dialect as @ChuanqiXu9 has proposed in https://reviews.llvm.org/D141894 but let's see what answer we get first?

Yeah, it should be fine to wait for few days.

[iains]

the response from core is:

They already are, see https://eel.is/c++draft/dcl.fct.def.delete#4

"A deleted function is implicitly an inline function ([dcl.inline])."

.. so that particular issue can be resolved (I will take a look at it today).