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

Thu Nov 4 21:36:02 PDT 2021

ChuanqiXu requested review of 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<
----------------
ChuanqiXu wrote:
> 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.
@aaron.ballman @urnathan I have sent a draft to reject the codes before `diagnoseQualifiedDeclaration()`. The draft is here: https://reviews.llvm.org/D113239. The draft is not completed to review the details. The key idea is to insert checks for `ExportDecl` on the fly to construct the AST. Due to the complexity of semantics analysis , we might need to insert the checks many times. It may not be too terrible since I found many `Check*` functions in `Sema`. So may be this is what we used to do.

And the draft could reject the example in  https://eel.is/c++draft/module.interface#6. It looks like not so hard as I imaged.

BTW, I have tried to let the compiler to emit "declaration does not declare anything" when it sees "export template <typename T> X<T>::Y". But I failed. It is not easy for me. And I found that the compiler wouldn't emit "declaration does not declare anything" when the  declaration  is in a `DeclContext`. Here is an example: https://godbolt.org/z/3j63PnTc7. So the compiler used to  handle this case in `diagnoseQualifiedDeclaration()`. Personally, it looks not bad to handle `export template<> X::Y` in `diagnoseQualifiedDeclaration()` at least from the names. And I am wondering if we could export a non-namespace-scope name without using qualifiers? If no, it looks better for me to handle this in `diagnoseQualifiedDeclaration()`. BTW, the following example is **not** what I meant:
```
struct X { struct Y {}; };
export using Z = typename X::Y;
```

Finally, it looks like we need to touch `diagnoseQualifiedDeclaration()` after all. Otherwise, it would crash for that example.

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

https://reviews.llvm.org/D112903