[C++20] [Modules] The constructor of lambda is broken after deserialization

[ChuanqiXu9]

Reproducer:

//--- lambda.h
auto cmp = [](auto l, auto r) {
  return l < r;
};

//--- A.cppm
module;
#include "lambda.h"
export module A;
export using ::cmp;

//--- use.cpp
import A;
auto x = cmp;

Compile it with:

clang++ -std=c++20 --precompile -o A.pcm A.cppm
clang++ -std=c++20 -fprebuilt-module-path=. use.cpp

It would emit the following errors:

use.cpp:5:6: error: no matching constructor for initialization of '(lambda at /home/chuanqi.xcq/workspace.xuchuanqi/llvm-project-for-work/build/57222/lambda.h:1:12)'
auto x = cmp; // cmp must not be ambiguous,
     ^   ~~~
/home/chuanqi.xcq/workspace.xuchuanqi/llvm-project-for-work/build/57222/lambda.h:1:12: note: candidate template ignored: could not match 'auto (*)(type-parameter-0-0, type-parameter-0-1)' against '(lambda at /home/chuanqi.xcq/workspace.xuchuanqi/llvm-project-for-work/build/57222/lambda.h:1:12)'
auto cmp = [](auto l, auto r) {
           ^
1 error generated.

[llvmbot]

@llvm/issue-subscribers-c-20

[llvmbot]

@llvm/issue-subscribers-clang-modules

[Bigcheese]

Shouldn't cmp not be visible at all in use.cpp here? It's not exported by A.

[ChuanqiXu9]

Thanks. I just verified it still exists on trunk with the updated reproducer.

[jer-irl]

@ChuanqiXu9 This issue is titled with a reference to serialization/deserialization, do you suspect that the serialization functionality itself is involved in the problem? After dumping the clang AST from the primary module interface unit and from the downstream dependent TU (with -ast-dump-all to force eager parsing), it looks like both have the same constructors and special member functions, all also with the same node attributes. Writing a similar case with plain PCH files also doesn't hit the same sort of visibility problem, but that might be unrelated.

I'm thinking this is deeper in the "Sema" system, but please let me know if you think I might be down the wrong path.

[jer-irl]

@Bigcheese, as you noted, it doesn’t seem like the original reproducer before Chuanqi's edit was actually expected to be acceptable code judging by https://eel.is/c++draft/module.global.frag#4, and I think it shows there’s another related issue that could cause invalid code being accepted, demonstrated (leading to a bad diagnostic) with the following:

// RUN: rm -rf %t
// RUN: mkdir %t
// RUN: split-file %s %t

// RUN: %clang_cc1 -std=c++20 -emit-module-interface %t/A.cppm -o %t/A.pcm -verify
// RUN: %clang_cc1 -std=c++20 -fprebuilt-module-path=%t %t/useA.cpp -fsyntax-only -verify

//--- global.h
namespace foo {
int x = 42;
}

//--- A.cppm
// expected-no-diagnostics
module;
#include "global.h"
export module A;

//--- useA.cpp
// expected-no-diagnostics
import A;

namespace foo {
double x = 99.9;
}

error: 'expected-error' diagnostics seen but not expected: 
  File /Users/jeremy/Programming/llvm-project/build/tools/clang/test/CXX/module/module.global.frag/Output/p3copy.cpp.tmp/useA.cpp Line 5: redefinition of 'x' with a different type: 'double' vs 'int'
error: 'expected-note' diagnostics seen but not expected: 
  File /Users/jeremy/Programming/llvm-project/build/tools/clang/test/CXX/module/module.global.frag/Output/p3copy.cpp.tmp/global.h Line 2: previous definition is here

Is this worth filing separately?

[ChuanqiXu9]

@ChuanqiXu9 This issue is titled with a reference to serialization/deserialization, do you suspect that the serialization functionality itself is involved in the problem?

No, it was my direct feelings when seeing the problem. I never looked into it. And I feel it might be a Sema issues too.

[ChuanqiXu9]

I think it shows there’s another related issue that could cause invalid code being accepted, demonstrated (leading to a bad diagnostic) with the following:

I guess you're saying valid being rejected?

And this is a known issue that we didn't implement discarding unreachable things in GMFs. I have a draft on this: #76930

[Bigcheese]

The visibility issue is definitely separate, and different from not including things the the PCM, as there are cases where we do need to include entities in the GMF in the PCM, but they still need to not be visible. I think visibility to name lookup should be handled the same way that it's handled for submodules under local submodule visibility (LSV) (but don't actually require LSV).

We can ignore visibility for this issue, and just focus on the updated testcase which definitely should work.