[PATCH] D75922: [ASTImporter] Compare the DC of the underlying type of a FriendDecl

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D75922#1915918 <https://reviews.llvm.org/D75922#1915918>, @shafik wrote:

> I believe that your main example violates basic.scope.class p2 <http://eel.is/c++draft/basic.scope.class#2>:
>
> > A name N used in a class S shall refer to the same declaration in its context and when re-evaluated in the completed scope of S.
> >  No diagnostic is required for a violation of this rule.
>
> Although it is ill-formed no diagnostic required.


Thanks Shafik for pointing this out! I thought that this kind of scoping problems could happen only with FriendDecls.

Unfortunately, similar code constructs to the main example are there in live projects, the example is a reduced case from Google's Protobuf. Also, I understand that giving a diagnostic would require an additional round of analysis (in Sema) of the scope after that is completed and that could cause performance issues. This is a good target for a clang-tidy check though.

So, I think this leaves us with 3 possible directions:

1. Do not handle this case, i.e. leaving one node missing from the imported AST of `Container` (this means abandoning this patch). This would be OK since we are not required to handle ill-formed code. The problem with this option is that when we want to import `Container` again then we will find that structurally nonequivalent to the previously imported `Container`, because it has one less FriendDecl.
2. Do handle the case. This option is justified from the view of the task of the ASTImporter that is to copy ASTs (maybe even if they result from ill-formed code). But perhaps it is not the best idea to blindly copy all kind of malformed ASTs. E.g. ASTs built from debug info could be malformed and then it is better to have diagnostics from the Importer (next option).
3. Emit a diagnostic here from ASTImporter and return with an `Error` because the code is ill-formed and we can diagnose that. Still, we would be able to diagnose these kind of errors only in case of FriendDecls. But perhaps the task of reporting these kind of errors should be in clang-tidy rather.

This is a hard decision to make, but I vote for 2) then 3).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75922