[ASTImporter] Fix structural ineq of lambdas with different sloc

[martong]

Summary

Currently we do not differentiate lambda classes based on their source
location, so during the import we will create only one lambda class for
lambdas which are defined on different slocs.
We should create two distinct classes though, this commit fixes it.

[a_sidorin]

Hi Gabor,
it is a nice design question if source locations can participate in structural match or not. The comparison tells us that the same code written in different files is not structurally equivalent and I cannot agree with it. They can be not the same, but their structure is the same. The question is: why do we get to this comparison? Do we find a non-equivalent decl by lookup? I guess there can be another way to resolve this issue, and I'll be happy to help if you share what is the problem behind the patch.

[martong]

In D64078#1572675, @a_sidorin wrote:

Hi Gabor,
it is a nice design question if source locations can participate in structural match or not. The comparison tells us that the same code written in different files is not structurally equivalent and I cannot agree with it. They can be not the same, but their structure is the same. The question is: why do we get to this comparison? Do we find a non-equivalent decl by lookup? I guess there can be another way to resolve this issue, and I'll be happy to help if you share what is the problem behind the patch.

Hey Alexei, thanks for looking into this. I agree, that those lambdas are actually "structurally" equivalent even if they are defined in different locations...
But still we have to differentiate them somehow.
Consider this code:

void f() {
  auto L0 = [](){};
  auto L1 = [](){};
}

First we import L0 then L1. Currently we end up having only one CXXRecordDecl for the two different lambdas. And that is a problem if the body of their op() is different.
This happens because when we import L1 then lookup finds the existing L0 and since they are structurally equivalent we just map the imported L0 to be the counterpart of L1.

I tried to use getLambdaManglingNumber() as you suggested in https://reviews.llvm.org/D64075 and that does solve this particular case, but cannot solve to other issue we have in the test case LambdasInFunctionParamsAreDifferentiated:

template <typename F0, typename F1>
void f(F0 L0 = [](){}, F1 L1 = [](){}) {}

Here after importing L0, L1 is mapped to L0 again.

I tried an alternative solution, which is to disable lookup completely if the decl in the "from" context is a lambda. That passed all tests.
However, that could have other problems: what if the lambda is defined in a header file and included in several TUs? I think we'd have as many duplicates as many includes we have. I think we could live with that, because the lambda classes are TU local anyway, we cannot just access them from another TU.
I have created another patch for that solution: https://reviews.llvm.org/D66348

clang/lib/AST/ASTStructuralEquivalence.cpp

 IsStructurallyEquivalentLambdas(StructuralEquivalenceContext &Context,
                                 CXXRecordDecl *D1, CXXRecordDecl *D2) {
   assert(D1->isLambda() && D2->isLambda() &&
          "Must be called on lambda classes");
   if (!IsStructurallyEquivalent(Context, D1->getLambdaCallOperator(),
                                 D2->getLambdaCallOperator()))
     return false;
 
+  auto &SM1 = Context.FromCtx.getSourceManager();
+  auto &SM2 = Context.ToCtx.getSourceManager();
+  auto Loc1 = SM1.getSpellingLoc(SM1.getExpansionLoc(D1->getLocation()));
+  auto Loc2 = SM2.getSpellingLoc(SM2.getExpansionLoc(D2->getLocation()));
+  if (SM1.getSpellingLineNumber(Loc1) != SM2.getSpellingLineNumber(Loc2))
+    return false;
+  if (SM1.getSpellingColumnNumber(Loc1) != SM2.getSpellingColumnNumber(Loc2))
+    return false;
+  if (SM1.getFilename(Loc1) != SM2.getFilename(Loc2))
+    return false;
+
   return true;
 }
 
 /// Determine structural equivalence of two records.
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      RecordDecl *D1, RecordDecl *D2) {
   if (D1->isUnion() != D2->isUnion()) {
     if (Context.Complain) {

clang/unittests/AST/ASTImporterTest.cpp

INSTANTIATE_TEST_CASE_P(ParameterizedTests, ErrorHandlingTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, DeclContextTest,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, CanonicalRedeclChain,
                         ::testing::Values(ArgVector()), );
 
+TEST_P(ASTImporterOptionSpecificTestBase, LambdasAreDifferentiated) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      void f() {
+        auto L0 = [](){};
+        auto L1 = [](){};
+      }
+      )",
+      Lang_CXX11, "input0.cc");
+  auto Pattern = lambdaExpr();
+  CXXRecordDecl *FromL0 =
+      FirstDeclMatcher<LambdaExpr>().match(FromTU, Pattern)->getLambdaClass();
+  CXXRecordDecl *FromL1 =
+      LastDeclMatcher<LambdaExpr>().match(FromTU, Pattern)->getLambdaClass();
+  ASSERT_NE(FromL0, FromL1);
+
+  CXXRecordDecl *ToL0 = Import(FromL0, Lang_CXX11);
+  CXXRecordDecl *ToL1 = Import(FromL1, Lang_CXX11);
+  EXPECT_NE(ToL0, ToL1);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+       LambdasInFunctionParamsAreDifferentiated) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      template <typename F0, typename F1>
+      void f(F0 L0 = [](){}, F1 L1 = [](){}) {}
+      )",
+      Lang_CXX11, "input0.cc");
+  auto Pattern = cxxRecordDecl(isLambda());
+  CXXRecordDecl *FromL0 =
+      FirstDeclMatcher<CXXRecordDecl>().match(FromTU, Pattern);
+  CXXRecordDecl *FromL1 =
+      LastDeclMatcher<CXXRecordDecl>().match(FromTU, Pattern);
+  ASSERT_NE(FromL0, FromL1);
+
+  CXXRecordDecl *ToL0 = Import(FromL0, Lang_CXX11);
+  CXXRecordDecl *ToL1 = Import(FromL1, Lang_CXX11);
+  ASSERT_NE(ToL0, ToL1);
+}
+
+TEST_P(ASTImporterOptionSpecificTestBase,
+       LambdasInFunctionParamsAreDifferentiatedWhenMacroIsUsed) {
+  Decl *FromTU = getTuDecl(
+      R"(
+      #define LAMBDA [](){}
+      template <typename F0, typename F1>
+      void f(F0 L0 = LAMBDA, F1 L1 = LAMBDA) {}
+      )",
+      Lang_CXX11, "input0.cc");
+  auto Pattern = cxxRecordDecl(isLambda());
+  CXXRecordDecl *FromL0 =
+      FirstDeclMatcher<CXXRecordDecl>().match(FromTU, Pattern);
+  CXXRecordDecl *FromL1 =
+      LastDeclMatcher<CXXRecordDecl>().match(FromTU, Pattern);
+  ASSERT_NE(FromL0, FromL1);
+
+  Import(FromL0, Lang_CXX11);
+  Import(FromL1, Lang_CXX11);
+  CXXRecordDecl *ToL0 = Import(FromL0, Lang_CXX11);
+  CXXRecordDecl *ToL1 = Import(FromL1, Lang_CXX11);
+  ASSERT_NE(ToL0, ToL1);
+}
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
                         DefaultTestValuesForRunOptions, );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportPath,
                         ::testing::Values(ArgVector()), );
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ImportExpr,
                         DefaultTestValuesForRunOptions, );

clang/unittests/AST/StructuralEquivalenceTest.cpp

auto t = makeDecls<LambdaExpr>(
       Lang_CXX11,
       lambdaExpr(),
       lambdaExpr());
   CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
   CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
   EXPECT_TRUE(testStructuralMatch(L0, L1));
 }
 
+TEST_F(StructuralEquivalenceLambdaTest,
+       LambdaClassesWithDifferentSpellingLocations) {
+  auto t = makeDecls<LambdaExpr>("void f() { auto L = [](int){}; }",
+                                 "void f() { auto L =  [](int){}; }",
+                                 Lang_CXX11, lambdaExpr(), lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
+TEST_F(StructuralEquivalenceLambdaTest,
+       LambdaClassesWithDifferentExpansionLocations) {
+  auto t = makeDecls<LambdaExpr>(
+      R"(
+      #define LAMBDA [](){}
+      void f() { auto L = LAMBDA; }
+      )",
+      R"(
+      #define LAMBDA [](){}
+      void f() { auto L =       LAMBDA; }
+      )",
+      Lang_CXX11, lambdaExpr(), lambdaExpr());
+  CXXRecordDecl *L0 = get<0>(t)->getLambdaClass();
+  CXXRecordDecl *L1 = get<1>(t)->getLambdaClass();
+  EXPECT_FALSE(testStructuralMatch(L0, L1));
+}
+
 TEST_F(StructuralEquivalenceTest, CompareSameDeclWithMultiple) {
   auto t = makeNamedDecls(
       "struct A{ }; struct B{ }; void foo(A a, A b);",
       "struct A{ }; struct B{ }; void foo(A a, B b);",
       Lang_CXX);
   EXPECT_FALSE(testStructuralMatch(t));
 }