Page MenuHomePhabricator

[ASTImporter] Compare the DC of the underlying type of a FriendDecl
Needs ReviewPublic

Authored by martong on Mar 10 2020, 7:09 AM.

Details

Summary

Currently, we do not check the scope of friend classes. This can result in some
cases with missing nodes in the imported AST. E.g.:

class Container {
  friend class X; // A friend class ::X
  class X{ };
  friend class X; // Friend class Container::X
};

... here either ::X or Container::X is missing after importing Container.
This patch fixes the problem.

Diff Detail

Event Timeline

martong created this revision.Mar 10 2020, 7:09 AM
shafik added a subscriber: rsmith.Mar 10 2020, 4:21 PM

I believe that your main example violates basic.scope.class p2:

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.

CC @rsmith

clang/lib/AST/ASTImporter.cpp
3637

It looks like we use FriendDecl *D in other places, we should try to be consistent across the file.

I would not be against changing it but perhaps in a different PR?

3656

return {};

balazske added inline comments.
clang/lib/AST/ASTImporter.cpp
3679

Computing FriendDC can be moved out of the loop.

clang/unittests/AST/ASTImporterTest.cpp
4035

Use ToTU and FromTU as is in other tests.

I believe that your main example violates basic.scope.class p2:

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).