This is an archive of the discontinued LLVM Phabricator instance.

[C++20] [Modules] Merge same concept decls in global module fragment
ClosedPublic

Authored by ChuanqiXu on Jul 26 2022, 10:46 PM.

Details

Summary

I found this one when I try to add a C++20 Modules example for D130585. Then I found there are something to improve:
(1) When the declaration is shadowed by using decls, we could try to find its targeted decl. This is fine since we would check Previous.isSingleResult() before we process anything.
(2) When we complain about the redefinition, we need to check for the existence of global module fragment. GMF is designed to make C++20 Named modules to be compatible with headers.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 26 2022, 10:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2022, 10:46 PM
ChuanqiXu requested review of this revision.Jul 26 2022, 10:46 PM

Thanks! Mostly questions to better understand the spec and clang. Please excuse me if they sound too basic.
I have read the modules section of the standard, but I couldn't find where it specifies whether these redefinition errors should be present or not. Any guidance of where to look at?

Could we add a few examples where legitimate redefinition errors do happen?
(private module fragment?

clang/lib/Sema/SemaTemplate.cpp
8727

This follows what getFoundDecl() does, but still allows to show errors when lookup results contain multiple entities.

A question too: is true that concept is always reachable if it has a reachable using decl?
I wonder whether hasReachableDefinition should be called on UsingShadowDecl instead?

8748

Would we allow redefinitions inside the same global module fragment?

module;

template<class T> concept ok = true;
template<class T> concept ok = true; // should be an error.

could we add a test for that?
or do we only mark declarations coming from actual pcm files as having as owning module fragment?

  • Address comment.
  • Add Sema::CheckRedefinitionInModule
ChuanqiXu marked 2 inline comments as done.EditedJul 28 2022, 12:53 AM

Thanks! Mostly questions to better understand the spec and clang. Please excuse me if they sound too basic.
I have read the modules section of the standard, but I couldn't find where it specifies whether these redefinition errors should be present or not. Any guidance of where to look at?

This is primarily at: http://eel.is/c++draft/basic.def.odr#14:

[basic.def.odr]p14:
For any definable item D with definitions in multiple translation units,

  • if D is a non-inline non-templated function or variable, or
  • if the definitions in different translation units do not satisfy the

following requirements,

the program is ill-formed; a diagnostic is required only if the definable
item is attached to a named module and a prior definition is reachable at
the point where a later definition occurs.
  • Each such definition shall not be attached to a named module

([module.unit]).

  • Each such definition shall consist of the same sequence of tokens, ...

... (following off is about the ODR checks)

So except the ODR checking, the redefinition errors should be present if:

  • The redefinitions lives in one TU. or,
  • Any of the redefinitions get attached to a named modules.

So we can get some examples:


// foo.cpp
template <class T, class U>
concept same_as = __is_same(T, U);
template <class T, class U>
concept same_as = __is_same(T, U);

This is disallowed. Since there are definitions of same_as in the same TU for foo.cpp. Note that headers won't present a TU. So the following one is disallowed too:

// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cpp
#include "foo.h"
#include "foo.h"

The definitions lives in the same TU of foo.cpp too. So this is problematic.


Note that a named module unit (*.cppm files) presents a TU. So the following one is disallowed:

C++
// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cppm
module;
#include "foo.h"
#include "foo.h"
export module foo;

Since there are two definitions in the same TU of foo.cppm.


And the following one should be allowed:

C++
// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cppm
module;
#include "foo.h"
export module foo;
export using :: same_as;
// use.cpp
#include "foo.h"
import foo;
template <class T> void foo()
  requires same_as<T, int>
{}

This is valid since the two definitions lives in two TUs (use.cpp and foo.cppm) and neither of them lives in named modules (the same_as definition in foo.cppm lives in global module instead of a named module).


And this one is disallowed

C++
// foo.h
template <class T, class U>
concept same_as = __is_same(T, U);
// foo.cppm
export module foo;
export template <class T, class U>
concept same_as = __is_same(T, U);
// use.cpp
#include "foo.h"
import foo;
template <class T> void foo()
  requires same_as<T, int>
{}

Although the two definitions live in 2 TUs, but one of them is attached to the named module foo. So it is problematic.


Overall, I think the design intuition as follows:
the biggest different of C++20 Named Modules between clang modules (or the standardized one, header units) is that the C++ Named Modules owns a unique TU, while the design of clang header modules (or header units) tries to mimic headers as much as they can. However, there are full of legacy codes using headers. The C++20 Named modules are hard to use in real projects if they can't be compatible with existing headers. So here is the global module fragment, which is designed for compatibility.
It might be easier to understand the rules now.

I implemented Sema::CheckRedefinitionInModule by the way to mimic the rules. Although there is a FIXME, I guess the FIXME itself is not too bad. At least it won't make things worse : )

Could we add a few examples where legitimate redefinition errors do happen?

Yeah, added.

(private module fragment?

In the above wording, there is a requirement reachable. But the declarations in the private module fragment is not reachable to the users of the module. So it won't be a legitimate redefinition error.

clang/lib/Sema/SemaTemplate.cpp
8727

This follows what getFoundDecl() does, but still allows to show errors when lookup results contain multiple entities.

Yeah, this is much better.

is true that concept is always reachable if it has a reachable using decl?

Yes.

I wonder whether hasReachableDefinition should be called on UsingShadowDecl instead?

We don't need too.

8748

I've added this as D.cppm in the tests.

ChuanqiXu marked 2 inline comments as done.Jul 28 2022, 1:57 AM

BTW, I've sent a NFC test to show the current behavior for functions, classes and variables are basically fine: https://github.com/llvm/llvm-project/commit/fe1887da36c63f64903de112f2a8e88f973318fa

Thanks for the thorough explanation and testing.
LGTM.

clang/lib/Sema/SemaDecl.cpp
1738

NIT: maybe rename to IsRedefinitionInModule?
I would expect Check... to produce a diagnostic itself. Having Is... in the name also removes any confusion about whether the true as a return value indicates an error or not.

1747

NIT: maybe accept the modules directly?

clang/test/Modules/merge-concepts.cppm
22

NIT on wording

ilya-biryukov accepted this revision.Jul 28 2022, 7:59 AM
This revision is now accepted and ready to land.Jul 28 2022, 7:59 AM
ChuanqiXu updated this revision to Diff 448504.Jul 28 2022, 7:45 PM
ChuanqiXu marked 3 inline comments as done.

Address comments.

Thanks for the thorough explanation and testing.
LGTM.

Thanks for reviewing!

clang/lib/Sema/SemaDecl.cpp
1747

I feel like the current style saves more typing.