[C++20] [Modules] Merging of lambda types

[ilya-biryukov]

I believe this code should not produce the static assertion failure:

//--- lambda.h
#pragma once

inline auto cmp = [](auto l, auto r) {
  return l < r;
};
//--- A.cppm
module;
#include "lambda.h"

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

export module B;
export auto c2 = cmp;
export using ::cmp;
//--- use.cppm
module;

export module use;

import A;
import B;

static_assert(__is_same(decltype(c1), decltype(c2))); // should succeed.
auto x = cmp; // cmp must not be ambiguous,

// Build with:
// clang++ -std=c++20 --precompile -o A.pcm A.cppm
// clang++ -std=c++20 --precompile -o B.pcm B.cppm
// clang++ -std=c++20 -fprebuilt-module-path=. use.cppm

As the lambda meets all requirements specified in basic.def.odr and cmp is declared inline. I.e. this should be no different from explicit class declaration from lambda.

However, the static assertion currently fails and the use of 'cmp' produces an "ambiguous reference" error.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[llvmbot]

@llvm/issue-subscribers-c-20

[llvmbot]

@llvm/issue-subscribers-clang-modules

[ChuanqiXu9]

Confirmed. First I hesitated since I know each lambda has a unique closure type. But later I found https://eel.is/c++draft/basic.def.odr#14.4:

Each such definition shall consist of the same sequence of tokens, where the definition of a closure type is considered to consist of the sequence of tokens of the corresponding lambda-expression.

And https://eel.is/c++draft/basic.def.odr#14.6

In each such definition, except within the default arguments and default template arguments of D, corresponding lambda-expressions shall have the same closure type (see below).

So static_assert shouldn't fail.

Also I think the inline part may be redundant, no matter it is inline or not, the declaration is attached to global module fragment.

[aaronmondal]

I believe this is implicitly causing libcxx to fail when we include anything that makes use of __compare/synth_three_way.h in GMFs and the locations of the headers differ during compilation and precompilation. For instance if we include <compare> several times in the interface and/or implementation GMFs, we get something like this:

error: redeclaration of '__synth_three_way' with a different type:
    'const std::__1::(lambda at <path 1>/libcxx/include/__compare/synth_three_way.h:29:3)'
 vs 'const std::__1::(lambda at <path 2>/libcxx/include/__compare/synth_three_way.h:29:3)'

This patch seems to fix the issue, but I'm not sure what the implications of removing this lambda may be:

--git a/libcxx/include/__compare/synth_three_way.h b/libcxx/include/__compare/synth_three_way.h
index fa8cbda79ba2..a6df1b936c72 100644
--- a/libcxx/include/__compare/synth_three_way.h
+++ b/libcxx/include/__compare/synth_three_way.h
@@ -25,8 +25,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 // [expos.only.func]
 
-_LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way =
-  []<class _Tp, class _Up>(const _Tp& __t, const _Up& __u)
+template<class _Tp, class _Up>
+_LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way(const _Tp& __t, const _Up& __u)
     requires requires {
       { __t < __u } -> __boolean_testable;
       { __u < __t } -> __boolean_testable;
@@ -42,7 +42,7 @@ _LIBCPP_HIDE_FROM_ABI inline constexpr auto __synth_three_way =
   };
 
 template <class _Tp, class _Up = _Tp>
-using __synth_three_way_result = decltype(std::__synth_three_way(declval<_Tp&>(), declval<_Up&>()));
+using __synth_three_way_result = decltype(_VSTD::__synth_three_way(declval<_Tp&>(), declval<_Up&>()));
 
 #endif // _LIBCPP_STD_VER > 17

@mumbleskates Could you explain why this lambda is neccessary?

Apart from having to add a template deduction guide for back_insert_iterator this is the only blocker I found so far for using libcxx freely in global module fragments.

I initially tried only changing (in Line 29) inline constexpr to static inline constexpr, but then the using __synth_three_way_result-line raises this buggy error message which I think was already reported in some other module-related issue:

error: type alias template redefinition with different types
    ('decltype(__synth_three_way(declval<_Tp &>(), declval<_Up &>()))'
  vs 'decltype(__synth_three_way(declval<_Tp &>(), declval<_Up &>()))')

[mumbleskates]

@aaronmondal the lambda is transcribed pretty much verbatim from the draft standard. It was written here, and there was some discussion about whether or not to make it a lambda (cc @ldionne). we're supposed to have ODR protection with _LIBCPP_HIDE_FROM_ABI though i'm not familiar with the different situations that may be arising here.

it looks like arthur made the change e0e7bd1 in march this year updating the __synth_three_way_result definition to use the explicit namespace, which may have been good to have in there originally but also should have used _VSTD. it does not look like this commit went through review, and he's gone now.

@aaronmondal if you make only the latter change, to __synth_three_way_result to use _VSTD::, does that alone fix it?

[aaronmondal]

@mumbleskates Thanks for taking a look at this!

the lambda is transcribed pretty much verbatim from the draft standard

Ah ok found it: https://eel.is/c++draft/expos.only.func#conceptref:three_way_comparable_with

if you make only the latter change, to __synth_three_way_result to use _VSTD::, does that alone fix it?

Only changing this still raises the redeclaration of '__synth_three_way' with a different type error.

I think this is a bug with modules, not with libcxx. However, if the simpler non-lambda variant satisfies all tests (haven't checked yet), I think it may actually desirable to use that variant. From the linked discussion it seems to me that the lambda was more or less an arbitrary choice for the sake of sticking close to what the exposition in the standard looks like. I think the original argument had something to do with not having to use _VSTD, but since _VSTD should be used now anyways the non-lambda variant seems a lot easier to comprehend to me.

Fixing the lambda resolution in modules seems very hard in comparison to changing 3 lines in libcxx. I checked the cppcoreguidelines for stylistic reasons on why one may want to use a lambda here but couldn't find any.

So for the sake of readability and module compatibility (even if technically just a workaround) I think it would be nice to change this.

I'll send the patch for review 😊