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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu planned changes to this revision.
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<
----------------
urnathan wrote:
> aaron.ballman wrote:
> > ChuanqiXu wrote:
> > > 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?
> > > I think we need to touch diagnoseQualifiedDeclaration() after all. Since there is a potential risk of crashing in it. 
> > 
> > My point is that we may have wanted to reject this code before ever needing to call `diagnoseQualifiedDeclaration()` in the first place. aka, it might be just as valid to assert we never see an `ExportDecl` here because the caller should have already handled that case.
> > 
> > > 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.
> > 
> > The declaration is invalid in C++20 because it does not declare anything (this is the same example with the export keywords removed): https://godbolt.org/z/8zc9q7fno
> > 
> > Clang fails the first one because we don't yet implement P0634R3, but the presence of the export keyword should not change diagnostic behavior here.
> > 
> > 
> Agreed, 'export' is only applicable to namespace-scope declarations.  We should reject it applying to non-namepace-scope entities.
Got it. It is important to make the code semantics consistent with the spec.


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

https://reviews.llvm.org/D112903