[PATCH] D112903: [C++20] [Module] Fix front end crashes when trying to export a qualified entity which is already declared

Wed Nov 3 20:27:50 PDT 2021

ChuanqiXu added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7784-7785
   "because namespace %1 does not enclose namespace %2">;
+def err_invalid_declarator_in_export : Error<"cannot export %0 here "
+  "because it had be declared in %1.">;
 def err_invalid_declarator_global_scope : Error<
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > I think this diagnostic text is more clear based on the standards text you cited. This would also come with a note diagnostic to point to the previous declaration.
> Given that this is not intended for p6 specifically, I think my suggestion is incorrect. But I am also not certain your diagnostic is either, but it's really hard to tell given where our current support is for modules. All of the other compilers suggest that an unqualified id is expected to be found, and I tend to agree -- there's no declaration there *to* export, just the type specifier for a declaration. This makes me think the issue is elsewhere and perhaps we shouldn't even be getting into `diagnoseQualifiedDeclaration()`.
I think we need to touch `diagnoseQualifiedDeclaration()` after all. Since there is a potential risk of crashing in it. If we didn't fix it in `diagnoseQualifiedDeclaration()` and find other place  to fix the issue, I think it may crash potentially one day in the process of developing or we might ignore some paths to  diagnoseQualifiedDeclaration()`. It would be a disaster.
And you said "there's no declaration there *to* export". And I noticed that there is error/warning in `Sema::ParsedFreeStandingDeclSpec` which would emit this kind of error/warning. But as the title describes, it only works for free standing declaration, which is not the same with the issue in bug47715. And `diagnoseQualifiedDeclaration()` would handle the qualified redeclaration. So it looks a good place to me.
BTW, I found the current patch could handle [module.interface]/p6 partially for qualified redeclaration surprisingly. See the newly added test case for example.
Finally, given that we need to touch `diagnoseQualifiedDeclaration()` to fix the crash, I think the patch should be good and we could try to cover [module.interface]/p6 in successive patches. Do you think so?

================
Comment at: clang/lib/Sema/SemaDecl.cpp:5748-5750
+    else if (isa<ExportDecl>(Cur))
+      Diag(Loc, diag::err_invalid_declarator_in_export)
+          << Name << cast<NamedDecl>(DC) << SS.getRange();
----------------
aaron.ballman wrote:
> ChuanqiXu wrote:
> > aaron.ballman wrote:
> > > I don't believe this is sufficient to cover [module.interface]p6. I tried out the example from the paragraph in the standard and we still silently accept it.
> > Yes, this patch is not intended to cover [module.interface]p6. The intention is to fix the crash since I feel it would take a longer time to support [module.interface]p6. And crash is not good any way. I plan to support [module.interface]p6 in successive patches. Do you happy with this?
> > BTW, I have a plan to support clang's c++20 module to a workable state. And I am working on the reachable definition in https://eel.is/c++draft/module.reach#3 and  https://eel.is/c++draft/module.global.frag#3.
> Thank you for the clarification! Fixing the crash is definitely a step in the right direction.
BYW, I added a new issue for the example in [module.interface]p6 here: https://bugs.llvm.org/show_bug.cgi?id=52395.

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

https://reviews.llvm.org/D112903