Fix handling of overriden methods during ASTImport

[shafik]

Summary

When importing classes we may add a CXXMethodDecl more than once to a CXXRecordDecl. This patch will fix the cases we currently know about and handle both the case where we are only dealing with declarations and when we have to merge a definition into an existing declaration.

[shafik]

@martong the only issue is that I am seeing a regression on Analysis/ctu-main.cpp when I run check-clang. I am going to look into it but if you have any insights that would be helpful.

[balazske]

lib/AST/ASTImporter.cpp

3058

The cast<Decl> should not be needed here (and is not done at the other return places). The const_cast can be omitted too (FoundByLookup is not const now).

unittests/AST/ASTImporterTest.cpp

2242

I think names like BP and BFP could be better (P should stand for "Pattern" and always the last part of the name, so use BFP and BFIsDefP). And for storing B::F a BF can be better name than B.

2259

Use CodeWithoutDef.

2311

It is ensured only by FirstDeclMatcher that not the definition is found (this pattern may match the definition too) for BFP. It may be more accurate to include in the pattern that this is not a definition.

2324

For the out-of-class definition the hasParent(cxxRecordDecl(hasName("B"))) does not match? (Otherwise this count should be 1.)

2343

The previous tests use two TUs too so the name for this is not exact (here the code is different, maybe use ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode?). This test does roughly the same as ImportOverriddenMethodTwiceDefinitionFirst but with the definition last and out-of-class version. (Name can be ImportOverriddenMethodTwiceOutOfClassDefLast too.) I am not sure why is the foo used here (B::F can be imported instead).

2365

hasName("B") is wrong here, should be "D"?

[martong]

Hi Shafik, thank you for this patch! Generally it looks quite okay to me, but I have a few minor comments.

lib/AST/ASTImporter.cpp

3061

Could you please do this refactor and remove this sentence from the comment? (At line 3208 we repeat the same logic.)

unittests/AST/ASTImporterTest.cpp

2242

I agree. I think the contractions we use in ImportOverriddenMethodTwiceOutOfClassDef are more consistent.

2324

That does not match, because hasParent matches based on the lexical decl context. In the case of the out-of-class definition, the lexical decl context is the global namespace (the TranslationUnitDecl).

However, the semantical decl context of the out-of-class definition is the CXXRecordDecl with the name "B". But this relation is not symmetric. The definition is not the child (i.e. DeclContext::containsDecl is false) of the CXXRecordDecl rather the child of the TranslataionUnitDecl.

2384

Perhaps it would make sense to have expectations on DFP and on DFPIsDef too.

2392

Perhaps it would make sense to have expectations on D::f() {} too. Even better could be to have a lambda which takes a few Decls as parameters and does the expectations on those.

[martong]

@martong the only issue is that I am seeing a regression on Analysis/ctu-main.cpp when I run check-clang. I am going to look into it but if you have any insights that would be helpful.

I am looking into it and will come back with what I have found.

[martong]

I have found some other minor things in the tests.

unittests/AST/ASTImporterTest.cpp

2274

I think the parent should have hasName("D") here. Seems like a copy paste error.

2281

ImportedD seems to be unused. (And in the next test case too.)

2399

Could we check the redecl chain of D::f() too?

[martong]

Shafik,
I have realized what's the problem with the ctu-main test. When we import the body and set the new body to the existing FunctionDecl then the parameters are still the old parameters so the new body does not refer to the formal parameters! This way the analyzer legally thinks that the parameter is not used inside the function because there is no DeclRef to that :(
This could be solved only if we merge *every* parts precisely, including the parameters, body, noexcept specifier, etc. But this would be a huge work.

Also, the test in ctu-main contains ODR violations. E.g, below fcl is first just a protoype, then it is defined in-class in the second TU.

// ctu-main.cpp
class mycls {
public:
  int fcl(int x);
  //...

// ctu-other.cpp
 class mycls {
 public:
  int fcl(int x) {
    return x + 5;
  }
  //...

In the second TU it should be defined out-of-class.

So my suggestion is to

1. use out-of-class definition of functions in ctu-other.cpp. Since I tried it already i have the diff for the lit tests, attached.
   lit_tests.patch1 KBDownload
2. skip the case when there is a definition in the 'from' context and let it do the redecl chain.

For 2) I was thinking about this:

if (FoundByLookup) {
  if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
    if (D->getLexicalDeclContext() == D->getDeclContext()) {
      if (!D->doesThisDeclarationHaveABody())
        return cast<Decl>(const_cast<FunctionDecl *>(FoundByLookup));
      else {
        // Let's continue and build up the redecl chain in this case.
        // FIXME Merge the functions into one decl.
      }
    }
  }
}

Later, we must implement the case when we have to merge the definition into the prototype properly, but that should be definitely another patch.
Actually, this case comes up mostly with class template specializations , because different specializations may have a function prototype in one TU, but a definition for that in another TU (see e.g. the test MergeFieldDeclsOfClassTemplateSpecialization).

lib/AST/ASTImporter.cpp

3054

It is not trivial why we add this block to the code, so I think a comment would be really appreciated here.
I was thinking about something like this:

+  // We do not allow more than one in-class prototype of a function.  This is
+  // because AST clients like VTableBuilder asserts on this.  VTableBuilder
+  // assumes that only one function can override a function. Building a redecl
+  // chain would result there are more than one function which override the
+  // base function (even if they are part of the same redecl chain inside the
+  // derived class.)

[martong]

lib/AST/ASTImporter.cpp

3058

We must also register the decl into the map of imported decls as we do in all the other cases.

return Importer.MapImported(D, FoundByLookup);

[balazske]

So the problem is that there are references to ParmVarDecl from inside function body and at import of ParmVarDecl always a new one is created even if there is an already existing (in the existing function prototype)? Maybe it works in VisitParmVarDecl to search for already existing ParmVarDecl (not already imported) and return if found.

[martong]

So the problem is that there are references to ParmVarDecl from inside function body and at import of ParmVarDecl always a new one is created even if there is an already existing (in the existing function prototype)?

Yes. During the import of the body we import a DeclRefExpr which refers to a ParmVarDecl but not the existing one, thus a new is created.

Maybe it works in VisitParmVarDecl to search for already existing ParmVarDecl (not already imported) and return if found.

Yes it would worth to try, indeed.

[shafik]

@martong I don't follow the discussion on VisitParmVarDecl an what seems like an alternate fix. Can you elaborate?

lib/AST/ASTImporter.cpp

3054

Just for clarification, from a C++ perspective, I would say "in-class declaration" rather than "in-class prototype" and therefore VTableBuilder assumes only one in class declaration and building a redecal chain would result in more than one in-class declaration being present.

Does that makes sense?

unittests/AST/ASTImporterTest.cpp

2343

I originally thought I did not need foo either but I could get it to import the definition of B::f() any other way.

I also saw similar behavior with clang-import-test where I need an expression that included something like foo() in order to obtain the out-of-line definition. It is not obvious to me why.

So in this case if I switch to using BFP instead of FooDef then this fails:

auto *ToBFOutOfClass = FirstDeclMatcher<CXXMethodDecl>().match(
  ToTU, cxxMethodDecl(hasName("f"), isDefinition()));

[shafik]

Addressing comments on naming in the unit test and refactoring of duplicated code.

[balazske]

lib/AST/ASTImporter.cpp

2949

It would be better to return Error instead of Expected. The return value (Stmt*) is not used and it may be nullptr if the FromFD does not have a body.

2958

Code formatting is not OK in this function (and other places). (Indentation and spaces around "(" ")".)

unittests/AST/ASTImporterTest.cpp

2310

This line is too long. clang-format can be used to format the code automatically (or git-clang-format).

[shafik]

Changes based on comments

• Refactoring ImportFunctionDeclBody to return Error
• Refactoring test/Analysis/ctu-main.cpp based on Gabor's patch
• Removing merging of function body for later PR

[shafik]

@martong so I just tested your lit-tests.patch and it looks good!

lib/AST/ASTImporter.cpp

2958

Apologies, I forgot to run clang-format

[martong]

Shafik, thanks for addressing the comments. This looks good to me now!

lib/AST/ASTImporter.cpp

3054

Yes, absolutely. Please change the comment as you suggest.

[balazske]

unittests/AST/ASTImporterTest.cpp

2311

Looks mainly OK now. Only a small problem, I do not like names like BFPIsDefP here and later, in the previous test it is correctly named BFDefP (could be BFIsDefP too).

[shafik]

Addressing comments on commenting and naming.

[teemperor]

LGTM, thanks!

unittests/AST/ASTImporterTest.cpp

2270

I think this should be formatted in the same way as the snippet above.

2343

I'm actually wondering if we should do a full redecl chain check in the ASTImporter tests. E.g. iterating from the most recent decl and just make sure our redecl chain invariants are met (first one returns null and all getMostRecentDecl() calls returns the same decl). i assume usually this is implicitly done when we code gen or so, but this doesn't happen in the ASTImporter tests.

lib/AST/ASTImporter.cpp

public:
     using FunctionTemplateAndArgsTy =
         std::tuple<FunctionTemplateDecl *, TemplateArgsTy>;
     Expected<FunctionTemplateAndArgsTy>
     ImportFunctionTemplateWithTemplateArgsFromSpecialization(
         FunctionDecl *FromFD);
 
     Error ImportTemplateInformation(FunctionDecl *FromFD, FunctionDecl *ToFD);
 
+    Error ImportFunctionDeclBody(FunctionDecl *FromFD, FunctionDecl *ToFD);
+
     bool IsStructuralMatch(Decl *From, Decl *To, bool Complain);
     bool IsStructuralMatch(RecordDecl *FromRecord, RecordDecl *ToRecord,
                            bool Complain = true);
     bool IsStructuralMatch(VarDecl *FromVar, VarDecl *ToVar,
                            bool Complain = true);
     bool IsStructuralMatch(EnumDecl *FromEnum, EnumDecl *ToRecord);
     bool IsStructuralMatch(EnumConstantDecl *FromEC, EnumConstantDecl *ToEC);
     bool IsStructuralMatch(FunctionTemplateDecl *From,
ASTNodeImporter::FindFunctionTemplateSpecialization(FunctionDecl *FromFD) {
   FunctionTemplateDecl *Template;
   TemplateArgsTy ToTemplArgs;
   std::tie(Template, ToTemplArgs) = *FunctionAndArgsOrErr;
   void *InsertPos = nullptr;
   auto *FoundSpec = Template->findSpecialization(ToTemplArgs, InsertPos);
   return FoundSpec;
 }
 
+Error ASTNodeImporter::ImportFunctionDeclBody(FunctionDecl *FromFD,
+                                              FunctionDecl *ToFD) {
+  if (Stmt *FromBody = FromFD->getBody()) {
+    if (ExpectedStmt ToBodyOrErr = import(FromBody))
+      ToFD->setBody(*ToBodyOrErr);
+    else
+      return ToBodyOrErr.takeError();
+  }
+  return Error::success();
+}
+
 ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
 
   SmallVector<Decl *, 2> Redecls = getCanonicalForwardRedeclChain(D);
   auto RedeclIt = Redecls.begin();
   // Import the first part of the decl chain. I.e. import all previous
   // declarations starting from the canonical decl.
   for (; RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt) {
     ExpectedDecl ToRedeclOrErr = import(*RedeclIt);
     if (!ToRedeclOrErr)
       return ToRedeclOrErr.takeError();
   }
   assert(*RedeclIt == D);
 
   // Import the major distinguishing characteristics of this function.
   DeclContext *DC, *LexicalDC;
   DeclarationName Name;
   SourceLocation Loc;
   NamedDecl *ToD;
   if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
     return std::move(Err);
   if (ToD)
     return ToD;
 
-  const FunctionDecl *FoundByLookup = nullptr;
+  FunctionDecl *FoundByLookup = nullptr;
   FunctionTemplateDecl *FromFT = D->getDescribedFunctionTemplate();
 
   // If this is a function template specialization, then try to find the same
   // existing specialization in the "to" context. The lookup below will not
   // find any specialization, but would find the primary template; thus, we
   // have to skip normal lookup in case of specializations.
   // FIXME handle member function templates (TK_MemberSpecialization) similarly?
   if (D->getTemplatedKind() ==
if (!ConflictingDecls.empty()) {
       Name = Importer.HandleNameConflict(Name, DC, IDNS,
                                          ConflictingDecls.data(),
                                          ConflictingDecls.size());
       if (!Name)
         return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
+  // We do not allow more than one in-class declaration of a function. This is
+  // because AST clients like VTableBuilder asserts on this. VTableBuilder
+  // assumes there is only one in-class declaration. Building a redecl
+  // chain would result in more than one in-class declaration for
+  // overrides (even if they are part of the same redecl chain inside the
+  // derived class.)
+  if (FoundByLookup) {
+    if (auto *MD = dyn_cast<CXXMethodDecl>(FoundByLookup)) {
+      if (D->getLexicalDeclContext() == D->getDeclContext()) {
+        if (!D->doesThisDeclarationHaveABody())
+          return Importer.MapImported(D, FoundByLookup);
+        else {
+          // Let's continue and build up the redecl chain in this case.
+          // FIXME Merge the functions into one decl.
+        }
+      }
+    }
+  }
+
   DeclarationNameInfo NameInfo(Name, Loc);
   // Import additional name location/type info.
   if (Error Err = ImportDeclarationNameLoc(D->getNameInfo(), NameInfo))
     return std::move(Err);
 
   QualType FromTy = D->getType();
   bool usedDifferentExceptionSpec = false;
 
ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
   // Import the describing template function, if any.
   if (FromFT) {
     auto ToFTOrErr = import(FromFT);
     if (!ToFTOrErr)
       return ToFTOrErr.takeError();
   }
 
   if (D->doesThisDeclarationHaveABody()) {
-    if (Stmt *FromBody = D->getBody()) {
-      if (ExpectedStmt ToBodyOrErr = import(FromBody))
-        ToFunction->setBody(*ToBodyOrErr);
-      else
-        return ToBodyOrErr.takeError();
-    }
+    Error Err = ImportFunctionDeclBody(D, ToFunction);
+
+    if (Err)
+      return std::move(Err);
   }
 
   // FIXME: Other bits to merge?
 
   // If it is a template, import all related things.
   if (Error Err = ImportTemplateInformation(D, ToFunction))
     return std::move(Err);
 

test/Analysis/Inputs/ctu-other.cpp

 int fns(int x) {
   return x + 7;
 }
 
 namespace embed_ns {
 int fens(int x) {
   return x - 3;
 }
-}
+} // namespace embed_ns
 
 class embed_cls {
 public:
-  int fecl(int x) {
-    return x - 7;
-  }
+  int fecl(int x);
 };
+int embed_cls::fecl(int x) {
+  return x - 7;
 }
+} // namespace myns
 
 class mycls {
 public:
-  int fcl(int x) {
-    return x + 5;
-  }
-  static int fscl(int x) {
-    return x + 6;
-  }
-
-  class embed_cls2 {
-  public:
-    int fecl2(int x) {
-      return x - 11;
-    }
-  };
-};
+  int fcl(int x);
+  static int fscl(int x);
+
+  class embed_cls2 {
+  public:
+    int fecl2(int x);
+  };
+};
+
+int mycls::fcl(int x) {
+int mycls::fscl(int x) {
+int mycls::embed_cls2::fecl2(int x) {
 
 namespace chns {
 int chf2(int x);
 
 class chcls {
 public:
   int chf4(int x);
 };

test/Analysis/ctu-main.cpp

int main() {
   clang_analyzer_eval(mycls::fscl(1) == 7);                 // expected-warning{{TRUE}}
   clang_analyzer_eval(myns::embed_cls().fecl(1) == -6);     // expected-warning{{TRUE}}
   clang_analyzer_eval(mycls::embed_cls2().fecl2(0) == -11); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(chns::chf1(4) == 12); // expected-warning{{TRUE}}
   clang_analyzer_eval(fun_using_anon_struct(8) == 8); // expected-warning{{TRUE}}
 
   clang_analyzer_eval(other_macro_diag(1) == 1); // expected-warning{{TRUE}}
-  // expected-warning@Inputs/ctu-other.cpp:75{{REACHABLE}}
+  // expected-warning@Inputs/ctu-other.cpp:80{{REACHABLE}}
   MACRODIAG(); // expected-warning{{REACHABLE}}
 }

unittests/AST/ASTImporterTest.cpp

TEST_P(ImportFunctions,
   Import(FromD, Lang_CXX);
 
   EXPECT_EQ(1u,
             DeclCounterWithPredicate<FunctionDecl>([](const FunctionDecl *FD) {
               return FD->doesThisDeclarationHaveABody();
             }).match(ToTU, functionDecl()));
 }
 
+TEST_P(ImportFunctions, ImportOverriddenMethodTwice) {
+  auto Code =
+      R"(
+      struct B { virtual void f(); };
+      struct D:B { void f(); };
+      )";
+  auto BFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B"))));
+  auto DFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))));
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *DF = FirstDeclMatcher<CXXMethodDecl>().match(FromTU0, DFP);
+  Import(DF, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *BF = FirstDeclMatcher<CXXMethodDecl>().match(FromTU1, BFP);
+  Import(BF, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, DFP), 1u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceDefinitionFirst) {
+  auto CodeWithoutDef =
+      R"(
+      struct B { virtual void f(); };
+      struct D:B { void f(); };
+      )";
+  auto CodeWithDef =
+      R"(
+    struct B { virtual void f(){}; };
+    struct D:B { void f(){}; };
+  )";
+  auto BFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B"))));
+  auto DFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))));
+  auto BFDefP = cxxMethodDecl(
+      hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFDefP = cxxMethodDecl(
+      hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FDefAllP = cxxMethodDecl(hasName("f"), isDefinition());
+
+  {
+    Decl *FromTU = getTuDecl(CodeWithDef, Lang_CXX, "input0.cc");
+    auto *FromD = FirstDeclMatcher<CXXMethodDecl>().match(FromTU, DFP);
+    Import(FromD, Lang_CXX);
+  }
+  {
+    Decl *FromTU = getTuDecl(CodeWithoutDef, Lang_CXX, "input1.cc");
+    auto *FromB = FirstDeclMatcher<CXXMethodDecl>().match(FromTU, BFP);
+    Import(FromB, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFDefP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, DFDefP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, FDefAllP), 2u);
+}
+
+TEST_P(ImportFunctions, ImportOverriddenMethodTwiceOutOfClassDef) {
+  auto Code =
+      R"(
+      struct B { virtual void f(); };
+      struct D:B { void f(); };
+      void B::f(){};
+      )";
+
+  auto BFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B"))));
+  auto BFDefP = cxxMethodDecl(
+      hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP = cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))),
+                           unless(isDefinition()));
+
+  Decl *FromTU0 = getTuDecl(Code, Lang_CXX);
+  auto *D = FirstDeclMatcher<CXXMethodDecl>().match(FromTU0, DFP);
+  Import(D, Lang_CXX);
+
+  Decl *FromTU1 = getTuDecl(Code, Lang_CXX, "input1.cc");
+  auto *B = FirstDeclMatcher<CXXMethodDecl>().match(FromTU1, BFP);
+  Import(B, Lang_CXX);
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToBFInClass = FirstDeclMatcher<CXXMethodDecl>().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher<CXXMethodDecl>().match(
+      ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+            ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+}
+
+TEST_P(ImportFunctions,
+       ImportOverriddenMethodTwiceOutOfClassDefInSeparateCode) {
+  auto CodeTU0 =
+      R"(
+      struct B { virtual void f(); };
+      struct D:B { void f(); };
+      )";
+  auto CodeTU1 =
+      R"(
+      struct B { virtual void f(); };
+      struct D:B { void f(); };
+      void B::f(){}
+      void D::f(){}
+      void foo(B &b, D &d) { b.f(); d.f(); }
+      )";
+
+  auto BFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("B"))));
+  auto BFDefP = cxxMethodDecl(
+      hasName("f"), hasParent(cxxRecordDecl(hasName("B"))), isDefinition());
+  auto DFP =
+      cxxMethodDecl(hasName("f"), hasParent(cxxRecordDecl(hasName("D"))));
+  auto DFDefP = cxxMethodDecl(
+      hasName("f"), hasParent(cxxRecordDecl(hasName("D"))), isDefinition());
+  auto FooDef = functionDecl(hasName("foo"));
+
+  {
+    Decl *FromTU0 = getTuDecl(CodeTU0, Lang_CXX, "input0.cc");
+    auto *D = FirstDeclMatcher<CXXMethodDecl>().match(FromTU0, DFP);
+    Import(D, Lang_CXX);
+  }
+
+  {
+    Decl *FromTU1 = getTuDecl(CodeTU1, Lang_CXX, "input1.cc");
+    auto *Foo = FirstDeclMatcher<FunctionDecl>().match(FromTU1, FooDef);
+    Import(Foo, Lang_CXX);
+  }
+
+  auto *ToTU = ToAST->getASTContext().getTranslationUnitDecl();
+
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, DFP), 1u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, BFDefP), 0u);
+  EXPECT_EQ(DeclCounter<FunctionDecl>().match(ToTU, DFDefP), 0u);
+
+  auto *ToB = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("B")));
+  auto *ToD = FirstDeclMatcher<CXXRecordDecl>().match(
+      ToTU, cxxRecordDecl(hasName("D")));
+  auto *ToBFInClass = FirstDeclMatcher<CXXMethodDecl>().match(ToTU, BFP);
+  auto *ToBFOutOfClass = FirstDeclMatcher<CXXMethodDecl>().match(
+      ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+  auto *ToDFInClass = FirstDeclMatcher<CXXMethodDecl>().match(ToTU, DFP);
+  auto *ToDFOutOfClass = LastDeclMatcher<CXXMethodDecl>().match(
+      ToTU, cxxMethodDecl(hasName("f"), isDefinition()));
+
+  // The definition should be out-of-class.
+  EXPECT_NE(ToBFInClass, ToBFOutOfClass);
+  EXPECT_NE(ToBFInClass->getLexicalDeclContext(),
+            ToBFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToBFOutOfClass->getDeclContext(), ToB);
+  EXPECT_EQ(ToBFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  EXPECT_NE(ToDFInClass, ToDFOutOfClass);
+  EXPECT_NE(ToDFInClass->getLexicalDeclContext(),
+            ToDFOutOfClass->getLexicalDeclContext());
+  EXPECT_EQ(ToDFOutOfClass->getDeclContext(), ToD);
+  EXPECT_EQ(ToDFOutOfClass->getLexicalDeclContext(), ToTU);
+
+  // Check that the redecl chain is intact.
+  EXPECT_EQ(ToBFOutOfClass->getPreviousDecl(), ToBFInClass);
+  EXPECT_EQ(ToDFOutOfClass->getPreviousDecl(), ToDFInClass);
+}
+
 struct ImportFriendFunctions : ImportFunctions {};
 
 TEST_P(ImportFriendFunctions, ImportFriendFunctionRedeclChainProto) {
   auto Pattern = functionDecl(hasName("f"));
 
   Decl *FromTU = getTuDecl("struct X { friend void f(); };"
                            "void f();",
                            Lang_CXX,