[ASTImporter] Fix isa cast assert

[martong]

Summary

Do early return if we can't import the found decl for a member expr.
This follows the pre-existing scheme, e.g with E->getMemberDecl().
E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. It may involve a lookup in case of a using directive which refers to a member function in a base class template.

We faced this assert during the CTU analysis of google::protobuf v3.5.2. We tried hard to synthesize a minimal test example both by hand and by executing creduce on multiple files. Unfortunately, we were unable to reduce to such a minimal example, yet. Nevertheless, this fix solved the problem in protobuf.
To reproduce the error one must execute the analyzer with -Xclang -analyzer-config -Xclang experimental-enable-naive-ctu-analysis=true -Xclang -analyzer-config -Xclang ctu-dir=/path/to/ctu_dir

[a.sidorin]

This LGTM, but could you please add a test?

[martong]

Hi Aleksei,

Thanks for the review.

We faced this assert during the CTU analysis of protobuf. We tried hard to synthesize a minimal test example both by hand and by executing creduce on multiple files. Unfortunately, we were unable to reduce to such a minimal example, yet. Nevertheless, this fix solved the problem in protobuf.

E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. (Note, it may involve a lookup in case of a using directive which refers to a member function in a base class template.)

I hoped that this patch could be accepted without tests, since it is very small and the changes are pretty straight forward, so I thought it can be easily verified by a review.

[a.sidorin]

E->getFoundDecl().getDecl() can be null when a member expression does not involve lookup. (Note, it may involve a lookup in case of a using directive which refers to a member function in a base class template.)

Yes, a pretty weird example. Unfortunately, they are pretty common for XTU.

I hoped that this patch could be accepted without tests, since it is very small and the changes are pretty straight forward, so I thought it can be easily verified by a review.

Yes, this is the reason why I have accepted it initially.
I think we can accept this patch without a test but I'd like to get a somebody else's review. @xazax.hun, @szepet, could you take a look?
@martong Could you include the reason why this patch lacks tests into the commit message (including description of the situation where the code fails)?

[xazax.hun]

With a sufficiently detailed commit message, i.e.: what version of a project should be cheked out and how the analyzer needs to be ivoked to reproduce the problem I am ok with committing this without a test.

[szepet]

Yepp, pretty straightforward check for something we were not aware previously (but unfortunately encountered it).

[a.sidorin]

Confirming LGTM, no objections. Thank you!

[martong]

Guys, I still don't have commit rights, could you please help me with the commit? (I think one more good quality patch and then I could request commit rights for myself ...)

[martong]

ping...
@xazax.hun could you please help me with the commit?

lib/AST/ASTImporter.cpp

Expr *ASTNodeImporter::VisitMemberExpr(MemberExpr *E) {
   Expr *ToBase = Importer.Import(E->getBase());
   if (!ToBase && E->getBase())
     return nullptr;
 
   auto *ToMember = dyn_cast<ValueDecl>(Importer.Import(E->getMemberDecl()));
   if (!ToMember && E->getMemberDecl())
     return nullptr;
 
-  DeclAccessPair ToFoundDecl = DeclAccessPair::make(
-    dyn_cast<NamedDecl>(Importer.Import(E->getFoundDecl().getDecl())),
-    E->getFoundDecl().getAccess());
+  auto *ToDecl =
+      dyn_cast_or_null<NamedDecl>(Importer.Import(E->getFoundDecl().getDecl()));
+  if (!ToDecl && E->getFoundDecl().getDecl())
+    return nullptr;
+
+  DeclAccessPair ToFoundDecl =
+      DeclAccessPair::make(ToDecl, E->getFoundDecl().getAccess());
 
   DeclarationNameInfo ToMemberNameInfo(
     Importer.Import(E->getMemberNameInfo().getName()),
     Importer.Import(E->getMemberNameInfo().getLoc()));
 
   if (E->hasExplicitTemplateArgs()) {
     return nullptr; // FIXME: handle template arguments
   }