[ASTImporter] Do not look up lambda classes

[martong]

Summary

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.

We have the same problem in this case:

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

In StructuralEquivalenceContext we could distinquish lambdas only by
their source location in these cases. But we the lambdas are actually
structrually equivalent they differn only by the source location.

Thus, the solution is to disable lookup completely if the decl in
the "from" context is a lambda.
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.

[a_sidorin]

Hello Gabor!
I think it's a correct solution for the analyzer: usually, we cannot import a lambda until we have to import some enclosing expression - which means that the lambdas are actually not the same. But I'm not so sure about how it can affect the LLDB logic. @shafik Shafik, could you please take a look?

[shafik]

I am not enthusiastic about this solution but I need to think about it some more.

We can see that p0624r2 added copy assignable lambdas:

bool f1() {
    auto x = []{} = {}; auto x2 = x;

    return x == x2;
}

bool f2() {
    auto x = []{} = {};
    auto xb = []{} = {};

    return x == xb;
}

see godbolt

So I don't think this is a long-term solution, although I don't know what clang is doing to make this work yet.

[martong]

• Add tests for default constructible and assignable stateless lambdas

[martong]

In D66348#1636564, @shafik wrote:

I am not enthusiastic about this solution but I need to think about it some more.

We can see that p0624r2 added copy assignable lambdas:

bool f1() {
    auto x = []{} = {}; auto x2 = x;

    return x == x2;
}

bool f2() {
    auto x = []{} = {};
    auto xb = []{} = {};

    return x == xb;
}

see godbolt

So I don't think this is a long-term solution, although I don't know what clang is doing to make this work yet.

Hi Shafik, thank you for looking into this and for the links.
I have added two test cases, which demonstrates that the solution will work even with copy assignable lambdas.

The reason why this works is that when we import "x2" then there is a DeclRefExpr node which points the the lambda class CXXRecordDecl.
And that had been already imported when we imported "x", so it is mapped by MapImported. This means we just return with the mapped definition of the lambda class before the code would reach the part when we skip the lookup.

[shafik]

I was concerned about how this would affect LLDB but after thinking about it I realized that in the DWARF we will just end up with one DW_TAG_class_type.

clang/lib/AST/ASTImporter.cpp

if (Error Err = importInto(
         SearchName, D->getTypedefNameForAnonDecl()->getDeclName()))
       return std::move(Err);
     IDNS = Decl::IDNS_Ordinary;
   } else if (Importer.getToContext().getLangOpts().CPlusPlus)
     IDNS |= Decl::IDNS_Ordinary | Decl::IDNS_TagFriend;
 
   // We may already have a record of the same name; try to find and match it.
   RecordDecl *PrevDecl = nullptr;
-  if (!DC->isFunctionOrMethod()) {
+  if (!DC->isFunctionOrMethod() && !D->isLambda()) {
     SmallVector<NamedDecl *, 4> ConflictingDecls;
     auto FoundDecls =
         Importer.findDeclsInToCtx(DC, SearchName);
     if (!FoundDecls.empty()) {
       // We're going to have to compare D against potentially conflicting Decls,
       // so complete it.
       if (D->hasExternalLexicalStorage() && !D->isCompleteDefinition())
         D->getASTContext().getExternalSource()->CompleteType(D);

clang/unittests/AST/ASTImporterTest.cpp

TEST_P(ASTImporterOptionSpecificTestBase,
   EXPECT_TRUE(Constr2->isDeletedAsWritten());
   EXPECT_EQ(ImportedX->isAggregate(), FromX->isAggregate());
 }
 
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins,
                         ::testing::Values(ArgVector{"-target",
                                                     "aarch64-linux-gnu"}), );
 
+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, );