[Clang] [C++20] regression in Clang 16

[vient]

Hi, I believe I encountered a regression with Clang 16 and Boost: https://godbolt.org/z/nzePcnGjj
Clang 15 with --std=c++20 compiles this code fine, Clang 16 with --std=c++17 works fine too but Clang 16 with --std=c++20 reports ambiguous symbol in a place where I can't see any ambiguity:

/opt/compiler-explorer/libs/boost_1_81_0/boost/beast/ssl/ssl_stream.hpp:679:5: note: candidate function [with AsyncStream = boost::beast::basic_stream<boost::asio::ip::tcp>, TeardownHandler = boost::beast::websocket::stream<boost::beast::ssl_stream<boost::beast::basic_stream<boost::asio::ip::tcp>>>::read_some_op<boost::beast::websocket::stream<boost::beast::ssl_stream<boost::beast::basic_stream<boost::asio::ip::tcp>>>::read_op<(lambda at <source>:19:29), boost::beast::basic_flat_buffer<std::allocator<char>>>, boost::asio::mutable_buffer>]
    async_teardown(
    ^
/opt/compiler-explorer/libs/boost_1_81_0/boost/beast/ssl/ssl_stream.hpp:702:1: note: candidate function [with AsyncStream = boost::beast::basic_stream<boost::asio::ip::tcp>, TeardownHandler = boost::beast::websocket::stream<boost::beast::ssl_stream<boost::beast::basic_stream<boost::asio::ip::tcp>>>::read_some_op<boost::beast::websocket::stream<boost::beast::ssl_stream<boost::beast::basic_stream<boost::asio::ip::tcp>>>::read_op<(lambda at <source>:19:29), boost::beast::basic_flat_buffer<std::allocator<char>>>, boost::asio::mutable_buffer>]
async_teardown(
^

in source code it looks like this:

    676     template<class AsyncStream, BOOST_BEAST_ASYNC_TPARAM1 TeardownHandler>
    677     friend
    678     void
    679     async_teardown(
    680         boost::beast::role_type role,
    681         ssl_stream<AsyncStream>& stream,
    682         TeardownHandler&& handler);

    699 template<class AsyncStream,
    700         BOOST_BEAST_ASYNC_TPARAM1 TeardownHandler = net::default_completion_token_t<beast::executor_type<AsyncStream>>>
    701 void
    702 async_teardown(
    703     boost::beast::role_type role,
    704     ssl_stream<AsyncStream>& stream,
    705     TeardownHandler&& handler = net::default_completion_token_t<beast::executor_type<AsyncStream>>{})
    706 {
    707     // Just forward it to the underlying ssl::stream
    708     using boost::beast::websocket::async_teardown;
    709     async_teardown(role, *stream.p_,
    710         std::forward<TeardownHandler>(handler));
    711 }

So the first one is declaration but Clang treats it as implementation?

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[llvmbot]

@llvm/issue-subscribers-c-20

[vient]

I can try to make reduced source

[AaronBallman]

Something looks amiss here to me, but it'd be helpful if we can devise a smaller reproducer than "use boost".

[AaronBallman]

I can try to make reduced source

That would be really helpful for us, thank you for the offer!

[royjacobson]

The essence of the bug is something like this, https://godbolt.org/z/ePsa3z9qb

template <class T, class... Ts>
concept AlwaysTrue = true;

template <class T>
class A {
    template<class S, AlwaysTrue<void(int)> T2>
    friend void f(S s, A<S> a, T2&& t);
};

template<class S, AlwaysTrue<void(int)> T2>
void f(S s, A<S> a, T2&& t) {
    return;
}

void f2() {
    A<int> a;
    f(1, a, 3);
}

This might be reduced further a bit, I think.

We don't reconcile the friend declaration with the definition: we think they're separate functions and then the call is ambiguous.
It's a bit similar to another regression we have here: #58896

[erichkeane]

Urgh... I looked at #58896 at one point, and couldn't make heads/tails of it at the time.

[usx95]

Reduced further https://godbolt.org/z/9Ys4G7nWf

template <class T>
concept True = true;

template <class T>
class A {
    template<True T2>
    friend void f(A<T2>);
};

template<True T2>
void f(A<T2>) { return; }

void f2() {
    f(A<int>{});
}

Interestingly changing f(A<T2>) to f(T2) resolves the issue. https://godbolt.org/z/Pd43cMcY6
Moving the constraint to requires also resolves template<typename T2> requires True<T2> https://godbolt.org/z/jTKE8vK59

[vient]

Okay, I guess reduction is no longer needed 😄
My cvise was too slow.

During the process I noticed that Boost 1.71 does not trigger the bug, I guess they moved to concepts somewhere between 1.72 and 1.81.

[vient]

So I waited for cvise to complete, here is the result

constexpr bool a = 1;

template <typename...>
concept b = a;

enum c {};

template <class> struct A {
  template <class h, b<void(int)> g> friend void d(c, A<h> &, g &&);
};

template <class h, b<void(int)> g> void d(c, A<h> &, g &&);

c e;

void f() {
  A<int> j;
  d(e, j, 0);
}

which gives

# clang++-16 -std=c++20 -c test.cpp -o lul
test.cpp:18:3: error: call to 'd' is ambiguous
  d(e, j, 0);
  ^
test.cpp:9:50: note: candidate function [with h = int, g = int]
  template <class h, b<void(int)> g> friend void d(c, A<h> &, g &&);
                                                 ^
test.cpp:12:41: note: candidate function [with h = int, g = int]
template <class h, b<void(int)> g> void d(c, A<h> &, g &&);
                                        ^
1 error generated.

Looks like the same idea as in your snippet but I am not an expert 🙂

[royjacobson]

@erichkeane - I think this is similar to something you had to do in your deferred concepts patch. From what I could gather, when we compare signatures, AreConstraintExpressionsEqual returns false because of depth difference on the constraints from the enclosing template.

There's a code path there that adjusts the depth, but it's called only for template instantiations, not when we coalesce the still uninstantiated template declarations.

Any idea for a quick fix here?

[erichkeane]

Yeah, I wouldn't be surprised, the 'making the depths equal' thing probably needs to be done if you can calculate it here.

I don't have a good idea of a quick fix, we MIGHT have to do something similar, we did some work at one point for partial specializations, though again, I'm not sure if that just needs to be applied here. I originally compared the size given by the getInstantiationArgs function to calculate the depth, but a followup patch by another contributor having to do with partial specializations changed that.

[erichkeane]

It's a bit similar to another regression we have here: #58896

Now looking at the two, I don't believe these are particularly related?

[erichkeane]

So since this is a C++20 only-issue, I don't think this ends up needing to be 'blocking' for the 16.0 release, so I'm opting to not make it do so.

[emmenlau]

Dear all, could this issue kindly be considered soon? It breaks building boost in c++20 mode, and boost is quite an essential library :-( :-(

[erichkeane]

So all the reductions no work in trunk, just not the initial example. So this once again needs reduction to see what the issue is. The problem is no longer with the beast library (its with basic_string being defined multiple times, and format being non-consteval, so the original problem is fixed.

So we need a reduction on those other two errors.

[vient]

Clang packages in apt repo are still 16.0.1, I've built 16.0.2 and it still shows same error so the fix is only in 17 branch at the moment. Will build trunk and run reduction with it then.

[vient]

Here is the reduced test case for out-of-line definition error https://godbolt.org/z/hP8We93vv

template <typename> struct a;
template <typename... b>
concept c = a<b...>::e;
using d = int;
template <class, bool> struct f { template <c<void(d)>> auto i(); };
template <class g, bool h> template <c<void(d)>> auto f<g, h>::i() {}

Clang trunk produces

<source>:6:64: error: out-of-line definition of 'i' does not match any declaration in 'f<type-parameter-0-0, >'
template <class g, bool h> template <c<void(d)>> auto f<g, h>::i() {}
                                                               ^
1 error generated.

while Clang 16 does not report errors

[erichkeane]

@alexander-shaposhnikov : Another out-of-line concepts bug for you to consider.

[alexander-shaposhnikov]

@erichkeane the examples from the comments above seem to compile with https://reviews.llvm.org/D146178

[erichkeane]

Cool, please put this on your list to 'close' when we commit that patch!

[vient]

I've bisected the fix of original problem to be 60bee9f — how to ensure it is backported to 16 release?

[AaronBallman]

Fixed in 60bee9f

[AaronBallman]

I've bisected the fix of original problem to be 60bee9f — how to ensure it is backported to 16 release?

The changes in that commit are large/non-obvious enough that it might not be backported to 16.x (we are conservative about what fixes we backport because we want each point release to be more stable than the last). We usually want those to "bake" a little longer in the tree. That said, if @erichkeane and @alexander-shaposhnikov think this fix is highly likely to be correct and not cause other problems, it is certainly worth considering it.

[vient]

As I understand 60bee9f together with https://reviews.llvm.org/D146178 completely fixes compilation of original code so either both should be backported or maybe none at all.

we are conservative about what fixes we backport because we want each point release to be more stable than the last

Reasonable, but in current state Clang 16 can't compile valid code from Boost while Clang 15 can. If fixes would not be backported, can you advise how Boost devs can hotfix it on their side?

[royjacobson]

After looking at the patch, I think it's safe enough to backport.

[erichkeane]

60bee9f was reverted because it causes a number of regressions. The author and I are working through them on D146178, but the changes are significant and there are still a few large hurdles to go through before we can approve D146178.

I do NOT think this is safe to backport. While I'm sympathetic to breaking boost like this, this is a part of concepts support, which is still experimental.

[erichkeane]

Also, re-opening bug, since the original patch that fixed it was reverted.

[vient]

@klemens-morgenstern FYI, seems that something needs to be done from Boost side if we want boostorg/beast#2648 fixed.

[vient]

CE links above work with trunk, seems to be fixed.