This is an archive of the discontinued LLVM Phabricator instance.

[C++20][Modules] Improve handing of Private Module Fragment diagnostics.
ClosedPublic

Authored by iains on Jun 22 2022, 1:23 AM.

Details

Summary

This adds a check for exported inline functions, that there is a definition in
the definition domain (which, in practice, can only be the module purview but
before any PMF starts) since the PMF definition domain cannot contain exports.

This is:
[dcl.inline]/7
If an inline function or variable that is attached to a named module is declared in
a definition domain, it shall be defined in that domain.

The patch also amends diagnostic output by excluding the PMF sub-module from the
set considered as sources of missing decls. There is no point in telling the user
that the import of a PMF object is missing - since such objects are never reachable
to an importer. We still show the definition (as unreachable), to help point out
this.

Diff Detail

Event Timeline

iains created this revision.Jun 22 2022, 1:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 1:23 AM
iains published this revision for review.Jun 22 2022, 1:24 AM
iains added reviewers: urnathan, ChuanqiXu.
iains added a subscriber: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 1:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

It looks like we need to handle inline variable as well to match the intention.

clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

From my reading, 'exported' is not emphasized.

clang/lib/Sema/SemaModule.cpp
895–905

So we might need to move this to somewhere else.

iains marked 2 inline comments as done.Jun 22 2022, 12:10 PM

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

it is here:
https://eel.is/c++draft/module#private.frag-2.1
( I agree it is somewhat confusing, but the export makes the linkage external, which the example treats differently from the fn_m() case which has module linkage).

It is possible that we might need to pull together several pieces of the std and maybe ask core for clarification?

clang/lib/Sema/SemaModule.cpp
895–905

depending on reading of the cases that can have this effect.

iains marked 2 inline comments as done.Jun 22 2022, 12:29 PM
iains added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

it is here:
https://eel.is/c++draft/module#private.frag-2.1
( I agree it is somewhat confusing, but the export makes the linkage external, which the example treats differently from the fn_m() case which has module linkage).

hmm... my linkage comment is wrong - however the distinction between exported and odr-used seems to be made here (fn_m() and fn_e()).

It is possible that we might need to pull together several pieces of the std and maybe ask core for clarification?

ChuanqiXu added inline comments.Jun 22 2022, 8:10 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

What I read is:

[dcl.inline]p7: https://eel.is/c++draft/dcl.inline#7

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

and the definition of definition domain is:

[basic.def.odr]p12: https://eel.is/c++draft/basic#def.odr-12

A definition domain is a private-module-fragment or the portion of a translation unit excluding its private-module-fragment (if any).

The definition of "attached to a named module" is:

[module.unit]p7: https://eel.is/c++draft/module.unit#7

A module is either a named module or the global module. A declaration is attached to a module as follows: ...

So it is clearly not consistency with [module.private.frag]p2.1. I would send this to WG21.

From the perspective of handling err_export_inline_not_defined error as a developer what about the following option?

export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here

// ...
module :private;
void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private

My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

clang/test/Modules/cxx20-10-5-ex1.cpp
25–28

Can export inline function call other non-export inline functions? Sorry if it is tested somewhere else. Curious what are the transitive restrictions, so we test edge cases.

ChuanqiXu added a comment.EditedJun 22 2022, 8:31 PM

From the perspective of handling err_export_inline_not_defined error as a developer what about the following option?

export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here

// ...
module :private;
void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private

My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

The suggested diagnostic message might not be right since the following one should be legal:

export module m;
...
module :private;
// foo() is not declared earlier.
inline void foo() {...}

The definition of 'foo' here should be legal although it is inline and in private fragment.

clang/test/Modules/cxx20-10-5-ex1.cpp
25–28

I guess it is not tested. But a non-export function wouldn't be exported if it is called by export function from the perspective of std. Although more tests should be fine all the time, the current test case should come from the example in the standard. We could send another test if we want.

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

From the perspective of handling err_export_inline_not_defined error as a developer what about the following option?

export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here

// ...
module :private;
void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private

My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

I agree my error message is kinda "implementor-speak" really, there's a tension between using something that will allow the user to find the source of the problem with a google search and and avoiding that.

We could implement what you suggest (pending a resolution of what we're actually supposed to be implementing) - I guess - but we'd need to defer the check until the end of the TU - i.e. after any potential PMF. I think we can differentiate the case that @ChuanqiXu noted (definition local to the PMF) because that would not have an entry in the PendingInlineExports table.

As for transitive cases, I agree we need to defer consideration of that until we can discuss with core - otherwise this patch will become a rabbit hole ;)

clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

Yes, that was what I found - maybe we are missing something about the export that changes those rules.
.

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

The test may be something like:

export module A;
inline int a;
module :private
int a = 0; // expected-error

But I feel like we couldn't go on before we get response from WG21.

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

The test may be something like:

export module A;
inline int a;
module :private
int a = 0; // expected-error

but we reject this at the moment with "redefinition of 'a'" - so that implies we do not have fully correct C++17 handling here?

But I feel like we couldn't go on before we get response from WG21.

Agreed, and anyway I think we would want to add a new test case, not to amend the example from the std (otherwise that becomes confusing as well)

ChuanqiXu added a comment.EditedJun 23 2022, 12:43 AM

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

The test may be something like:

export module A;
inline int a;
module :private
int a = 0; // expected-error

but we reject this at the moment with "redefinition of 'a'" - so that implies we do not have fully correct C++17 handling here?

Sorry, my bad. The configuration of my godbolt was wrong (The input is LLVM IR). I feel like the following one should be the test case:

export module A;
[export] inline int a;

Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

The meaning of [export] depends on the result of the feedback from WG21.

But I feel like we couldn't go on before we get response from WG21.

Agreed, and anyway I think we would want to add a new test case, not to amend the example from the std (otherwise that becomes confusing as well)

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

I feel like the following one should be the test case:

export module A;
[export] inline int a;

Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

hmm ... isn't that implicitly initialised with 0?

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

The meaning of [export] depends on the result of the feedback from WG21.

yes, I saw your post to ext.

It looks like we need to handle inline variable as well to match the intention.

can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?

Did you have some ideas here?

I feel like the following one should be the test case:

export module A;
[export] inline int a;

Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:

If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.

hmm ... isn't that implicitly initialised with 0?

Oh, I just realized every declaration is a definition too except some cases: https://eel.is/c++draft/basic.def#2

And I failed to give an example that a undefined inline variable declaration attached to the named modules. I wondered:

export module A;
extern "C++" inline int a;
extern "C++" int a = 0;

But now 'a' is attached to global module instead of named modules. So I just failed to find a good example... very sorry for wasting the time

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.

iains added a comment.EditedJun 23 2022, 2:18 AM

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.

Right, that should be an error - we do not have a test for it.
ActOnEndOfTranslationUnitFragment should be called with "Kind =TUFragmentKind::Normal" whether there us a PMF or not.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L1131
so - modulo the underlying question about 'export' we probably just need a second test-case
(we could still defer the processing to the end of the TU and implement the error messages as suggested by @vsapsai )

Also, if [module.private.frag]p2.1 is changed into:

the point by which the definition of an [exported] inline function or variable is required

The test above would cover this too.

BTW, it shows we could lack test like:

export module A;
[export] inline void func(); // no definition in the definition domain

I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.

Right, that should be an error - we do not have a test for it.
ActOnEndOfTranslationUnitFragment should be called with "Kind =TUFragmentKind::Normal" whether there us a PMF or not.
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/Sema.cpp#L1131

I don't understand the intention of the suggestion. I feel like the current one looks right.

so - modulo the underlying question about 'export' we probably just need a second test-case

Agreed.

(we could still defer the processing to the end of the TU and implement the error messages as suggested by @vsapsai )

Oh, the intention of your suggestion is to implement the suggestion by @vsapsai, right? If it is, I would like to suggest to move the processing of PendingInlineExports to ActOnEndOfTranslationUnit instead of change the argument of ActOnEndOfTranslationUnitFragment.

Sorry for changing my mind. I've thought about the errors more and especially about the case mentioned by Chuanqi

export module A;
[export] inline void func();

I'm afraid it can complicate the implementation but we can achieve some consistency with errors like

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'

and

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'
//...
module :private;
void func() {}  // note: definition here is not reachable as it is private

I think it is useful to have connection between declaration and definition and to explain why the definition is no good.

Specific wording around "no definition | missing definition | definition required" is flexible.

From the discussion, it looks like the 'export' part is not necessary here and we don't need to care about linkage in this revision.

Sorry for changing my mind. I've thought about the errors more and especially about the case mentioned by Chuanqi

export module A;
[export] inline void func();

I'm afraid it can complicate the implementation but we can achieve some consistency with errors like

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'

and

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'
//...
module :private;
void func() {}  // note: definition here is not reachable as it is private

I think it is useful to have connection between declaration and definition and to explain why the definition is no good.

Specific wording around "no definition | missing definition | definition required" is flexible.

It makes sense to me.

iains planned changes to this revision.Jun 27 2022, 5:25 AM

From the discussion, it looks like the 'export' part is not necessary here and we don't need to care about linkage in this revision.

Indeed.

Sorry for changing my mind. I've thought about the errors more and especially about the case mentioned by Chuanqi

export module A;
[export] inline void func();

I'm afraid it can complicate the implementation but we can achieve some consistency with errors like

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'

and

export module A;
export inline void func(); // error: no definition for exported inline function 'func' in module 'A'
//...
module :private;
void func() {}  // note: definition here is not reachable as it is private

I think it is useful to have connection between declaration and definition and to explain why the definition is no good.

Specific wording around "no definition | missing definition | definition required" is flexible.

It makes sense to me.

So I will re-work this patch to deal with the two changes (I think that the proposed merge of changes to the example should be enough to go on).

iains updated this revision to Diff 442942.Jul 7 2022, 9:06 AM
iains marked 3 inline comments as done.

rebased, reworked

  • to follow the changes proposed by core
  • to make the diagnostics follow that and a compromise for the proposed revision before the core amendment.
iains added a comment.Jul 7 2022, 9:09 AM

the revised diagnostics look like this:

error: {un-}exported inline function not defined before the private module fragment

with
note: private module fragment begins here pointing to the start of the PMF

If there is no PMF then we just say:

error: {un-}exported inline function not defined

clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

I think that we can consider this closed by the question to the ext reflector and the amendment proposed by core.

clang/test/Modules/cxx20-10-5-ex1.cpp
25–28

I've changed the example to match the proposed amendment to the standard.

If you think we should have some other test case (additional to Modules/Reachability-Private), that's fine - would you like to propose one (or maybe add to an existing)?

ChuanqiXu added inline comments.Jul 7 2022, 7:13 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

I prefer inline function attached to a named module not defined %select{| before the private module fragment}1. Since the export part is not important here and the important part is whether or not they are attached to a named module.

clang/lib/Sema/SemaDecl.cpp
9701 ↗(On Diff #442942)
clang/test/Modules/cxx20-10-5-ex1.cpp
25–28

The current one looks fine to me.

iains added inline comments.Jul 8 2022, 12:48 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

I took the error message from here:
https://github.com/cplusplus/draft/pull/5537
which was prepared after the dicussion that you started on the ext reflector. Actually, I do not have a strong feeling either way.

ChuanqiXu added inline comments.Jul 8 2022, 12:53 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

Oh, I didn't track that. I feel my suggested version is slightly better. Otherwise users might want to make it work by adding/removing export clause.

iains updated this revision to Diff 447062.Jul 23 2022, 5:59 AM

rebased, retested.

iains updated this revision to Diff 452617.Aug 15 2022, 2:56 AM

rebased, addressed review comments

iains marked an inline comment as done.Aug 15 2022, 2:58 AM
iains added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
11155

I removed the reference to un/exported.

ChuanqiXu accepted this revision.Aug 15 2022, 9:44 PM
ChuanqiXu added inline comments.
clang/include/clang/Sema/Sema.h
2251

I'm curious why the element is not FunctionDecl?

clang/lib/Sema/Sema.cpp
1285

nit:

This revision is now accepted and ready to land.Aug 15 2022, 9:44 PM
iains updated this revision to Diff 454205.Aug 20 2022, 6:29 AM
iains marked an inline comment as done.

rebased, addressed remaing comments.

iains marked 2 inline comments as done.Aug 20 2022, 6:32 AM
iains added inline comments.
clang/lib/Sema/Sema.cpp
1285

(I added this one)
I wonder if we could try to reduce the number of asserts included by considering making a test-case to cover the condition that is of concern - since most devs build with assertions enabled, we should ensure that they are only covering some case we really cannot test directly.

This revision was automatically updated to reflect the committed changes.
iains marked an inline comment as done.
ChuanqiXu added inline comments.Aug 21 2022, 7:17 PM
clang/lib/Sema/Sema.cpp
1285

I think the test cases and assertion are not conflicted with each other. For example, when I made a change and a test case fails, I may spent a lot of time to locate the problem. But if the assertion fails, I will get the problem more quickly. And for the specific case, it looks like not easy to construct a test case.