[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

@rsmith Thanks for your valuable input!



================
Comment at: clang/lib/Sema/SemaLookup.cpp:2000-2004
+  // Class and enumeration member names can be found by name lookup in any
+  // context in which a definition of the type is reachable.
+  if (auto *ECD = dyn_cast<EnumConstantDecl>(ND))
+    return getSema().hasReachableDeclaration(
+        cast<NamedDecl>(ECD->getDeclContext()));
----------------
rsmith wrote:
> I don't think this is quite right. Given:
> 
> ```
> export module M {
> export enum E1 { e1 };
> enum E2 { e2 };
> export using E2U = E2;
> enum E3 { e3 };
> export E3 f();
> ```
> 
> ... the intent is:
> 
> ```
> import M;
> int main() {
>   auto a = E1::e1; // OK, namespace-scope name E1 is visible and e1 is reachable
>   auto b = e1; // OK, namespace-scope name e1 is visible
>   auto c = E2::e2; // error, namespace-scope name E2 is not visible
>   auto d = e2; // error, namespace-scope name e2 is not visible
>   auto e = E2U::e2; // OK, namespace-scope name E2U is visible and E2::e2 is reachable
>   auto f = E3::e3; // error, namespace-scope name E3 is not visible
>   auto g = e3; // error, namespace-scope name e3 is not visible
>   auto h = decltype(f())::e3; // OK, namespace-scope name f is visible and E3::e3 is reachable
> }
> ```
> 
> Instead of doing the check in this function, I think we need to consider the scope in which we're doing a lookup: if that scope is a class or enumeration scope, and the class or enumeration has a reachable definition, then we don't do any visibility checks for its members.
Oh, your example makes sense. The current implementation would accept `d` and `g` unexpectedly. I spent some time to look into this one. And I find it is not so easy to fix. I left a FIXME below and I would file an issue if this patch landed. Do you think this is OK?

BTW, I feel like the wording of spec need to be adjusted too. From the wording, I feel like `d` and `g` should be accepted.


================
Comment at: clang/lib/Sema/SemaLookup.cpp:3754
+          //  reachable definition in the set of associated entities,
+          if (AssociatedClasses.count(RD) && hasReachableDeclaration(D)) {
             Visible = true;
----------------
rsmith wrote:
> We should look for a reachable definition here, not a reachable declaration.
Yeah, from the wording it should be reachable definition. But it would cause many failures if I write hasReachableDefinition/hasVisibleDefinition here. So it looks like a legacy defect. I chose to follow the original style here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113545/new/

https://reviews.llvm.org/D113545