[C++20] [Modules] We didn't implement lookup in module level actually

[ChuanqiXu9]

Minimal reproducer:

// a.cppm
export module a;
namespace a { int a = 43; }

// use.cc
import a;
namespace a {
    double a = 43.0
}

We should accept the example since the a::a in module a is not visible from use.cc and also the a::a in module a is a different entity with a::a in use.cc so they don't violate the ODR rule.

The root reason is that how do we handle the module's visibility in clang. Now in clang, the process to judge the visibility of declarations is:

lookup  a name -> lookup the name in **all** imported modules -> judge if all the found declarations are valid (ODR Check, etc...) -> check the visibility of found declarations

Then the reason about the failure is clear. We check the visibility after we check the validness of found declarations. Then it booms out in the checking validness phase. So the visibility doesn't get involved here.

The reason why we implement it as this is that:

• We can only get the visibility of a declaration after deserializing it. So we must check the visibility after deserializing it.
• If we deserialize something, we'd better to check its validness immediately. Otherwise we may be in unsafe state.

To solve the first problem, we need to be able to get the module file of a declaration before deserializing it. This requires us to change the encodings of serialized declaration ID. See #86912 for example.

For the second problem, maybe we have to refactor the current design of declarations to make a::a@a to be a well-defined different entity with a::a in the global module.

[llvmbot]

@llvm/issue-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Minimal reproducer:

// a.cppm
export module a;
namespace a { int a = 43; }

// use.cc
import a;
namespace a {
    double a = 43.0
}

We should accept the example since the a::a in module a is not visible from use.cc and also the a::a in module a is a different entity with a::a in use.cc so they don't violate the ODR rule.

The root reason is that how do we handle the module's visibility in clang. Now in clang, the process to judge the visibility of declarations is:

lookup  a name -> lookup the name in **all** imported modules -> judge if all the found declarations are valid (ODR Check, etc...) -> check the visibility of found declarations

Then the reason about the failure is clear. We check the visibility after we check the validness of found declarations. Then it booms out in the checking validness phase. So the visibility doesn't get involved here.

The reason why we implement it as this is that:

• We can only get the visibility of a declaration after deserializing it. So we must check the visibility after deserializing it.
• If we deserialize something, we'd better to check its validness immediately. Otherwise we may be in unsafe state.

To solve the first problem, we need to be able to get the module file of a declaration before deserializing it. This requires us to change the encodings of serialized declaration ID. See #86912 for example.

For the second problem, maybe we have to refactor the current design of declarations to make a::a@<!-- -->a to be a well-defined different entity with a::a in the global module.

[ChuanqiXu9]

Oh, I need to recheck the wording to make sure if the reproducer is valid. Since it looks like it violates the rules in https://eel.is/c++draft/basic.def.odr#15.3:

Each such definition shall not be attached to a named module

[ChuanqiXu9]

After identified, the reproducer should be accepted. According to https://eel.is/c++draft/basic.pre#10 and https://eel.is/c++draft/basic.link#2, we judge the same definable item by the name, and these name have different linkages, so https://eel.is/c++draft/basic.def.odr#15.3 may not fall in here.

[ChuanqiXu9]

One possible implementation: we should divide a name lookup table for reachable but not visible names than the current lookup table. And due to we've already encoded the module file information in the Decl ID, so we can know whether a Decl belongs to which declaration before deserializing.

So the lookup implementation will become to:

the current process lookup + the module local lookup.

[ChuanqiXu9]

It still looks tricky since there are declaration contexts across multiple modules (e.g, namespace).

[ChuanqiXu9]

One possible implementation: we should divide a name lookup table for reachable but not visible names than the current lookup table. And due to we've already encoded the module file information in the Decl ID, so we can know whether a Decl belongs to which declaration before deserializing.

So the lookup implementation will become to:

the current process lookup + the module local lookup.

And this can't implement it 100% percently. Since it just tried to not load the declaration from other modules. But it doesn't handle the case that both the names are loaded.

After all, we need to face the case that these two names are loaded.

[ChuanqiXu9]

The fundamental reason for this is that: clang didn't attach linkages to names but attach linkages to declarations. But in the spec, the linkages are attached to the names. So here is the root of the mismatch.

One fundamental solution is to refactor DeclarationName to make it tied with modules. This may be pretty foundamental.

The other one is keep the current implementation direction to mimic the standard behavior.