llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.86k stars 11.48k forks source link

Clang complains about concept depending on itself #62096

Open craffael opened 1 year ago

craffael commented 1 year ago

Version of Clang

clang version 17.0.0 (https://github.com/llvm/llvm-project.git 5b461d5ec172d21029da492064704fe3da6f8bab)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /opt/compiler-explorer/clang-trunk/bin

Steps to reproduce the problem

Try to compile the following program with clang++ main.cpp -std=c++20 -ftemplate-backtrace-limit=0:

#include <type_traits>

template <class OPERATOR>
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;

struct AnyOperator {

  template <Operator OP>
  explicit AnyOperator(OP op) noexcept {}

  /// Copy constructor
  AnyOperator(const AnyOperator&) noexcept = default;
};

static_assert(Operator<AnyOperator>);

It produces the following output (see also Godbolt):

<source>:4:20: error: satisfaction of constraint 'std::is_nothrow_copy_constructible_v<OPERATOR>' depends on itself
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:4:20: note: while substituting template arguments into constraint expression here
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:8:13: note: while checking the satisfaction of concept 'Operator<AnyOperator>' requested here
  template <Operator OP>
            ^
<source>:8:13: note: while substituting template arguments into constraint expression here
  template <Operator OP>
            ^~~~~~~~
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.1/../../../../include/c++/13.0.1/type_traits:3306:7: note: while checking constraint satisfaction for template 'AnyOperator<AnyOperator>' required here
    = __is_nothrow_constructible(_Tp, __add_lval_ref_t<const _Tp>);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/compiler-explorer/gcc-snapshot/lib/gcc/x86_64-linux-gnu/13.0.1/../../../../include/c++/13.0.1/type_traits:3306:7: note: in instantiation of function template specialization 'AnyOperator::AnyOperator<AnyOperator>' requested here
<source>:4:25: note: in instantiation of variable template specialization 'std::is_nothrow_copy_constructible_v<AnyOperator>' requested here
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;
                        ^
<source>:4:20: note: while substituting template arguments into constraint expression here
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<source>:16:15: note: while checking the satisfaction of concept 'Operator<AnyOperator>' requested here
static_assert(Operator<AnyOperator>);
              ^~~~~~~~~~~~~~~~~~~~~
<source>:16:1: error: static assertion failed
static_assert(Operator<AnyOperator>);
^             ~~~~~~~~~~~~~~~~~~~~~
<source>:16:15: note: because substituted constraint expression is ill-formed: constraint depends on a previously diagnosed expression
static_assert(Operator<AnyOperator>);
              ^
2 errors generated.

Correct behavior

Clang should accept the code. As far as I know the code is valid. It compiles fine with MSVC and GCC.

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-frontend

llvmbot commented 1 year ago

@llvm/issue-subscribers-c-20

shafik commented 1 year ago

I think this should work but really @erichkeane should take a look

erichkeane commented 1 year ago

I don't see any reason this shouldn't work, that looks right? And I admit the error message doesn't do a great job telling us what went wrong unfortunately. However, can anyone create a min-repro for this for me that doesn't include the include of the type trait? I wonder if the type trait is doing something goofy here.

erichkeane commented 1 year ago

Actually, I got it down to:

template <class OPERATOR>
concept Operator =
  __is_nothrow_constructible(OPERATOR, OPERATOR&);

struct AnyOperator {

  template <Operator OP>
  explicit AnyOperator(OP op) noexcept {}

  AnyOperator(const AnyOperator&) noexcept = default;
};

static_assert(Operator<AnyOperator>);

Unless that builtin has something wacky happening, I don't see the recursion. The code that does just checks to see if we're still in the evaluation of something while being asked to evaluate the same constraint, but I'm not sure where that could have come from.

craffael commented 1 year ago

I can only guess: If your remove the templated constructor it works. So I assume that: 1) We check whether AnyOperator fulfills the Operator concept. 2) I.e. we have to check whether AnyOperator is copy constructible. But for some reason not the copy constructor is selected, but the templated constructor: AnyOperator::AnyOperator<AnyOperator> 3) Now the compiler checks again whether AnyOperator fulfills the Operator concept. Now we are back at step 1 and we have a recursion...

erichkeane commented 1 year ago

AAh, so that builtin DOES cause this problem, and this is a legitimate diagnostic as far as I can tell. If I remove the diagnostic, we hit our template implementation limit.

__is_nothrow_constructible is causing us to evaluate whether AnyOperator is constructible from an AnyOperator, which causes it to have to re-evaluate Operator with AnyOperator (for the constraint on AnyOperator::AnyOperator). I'm not sure how GCC is managing this, unless __is_nothrow_constructible has a way of constructing it without evaluating the constraint on the constructor.

So at the moment, I don't think this is a problem with the concepts stuff, but with that evaluation of the builtin. SO I'm not sure what I can do about it. @shafik : Perhaps its worth looking into that builtin/how GCC behaves with it?

erichkeane commented 1 year ago

I can only guess: If your remove the templated constructor it works. So I assume that:

1. We check whether AnyOperator fulfills the Operator concept.

2. I.e. we have to check whether AnyOperator is copy constructible. But for some reason not the copy constructor is selected, but the templated constructor: `AnyOperator::AnyOperator<AnyOperator>`

3. Now the compiler checks again whether `AnyOperator` fulfills the Operator concept. Now we are back at step 1 and we have a recursion...

Yep! Looks like we simul-posted, thats exactly what is happening. The builtin is effectively doing OPERATOR(OPERATOR&);, which is re-evaluating the constraint on the ctor. So I'm not sure what we can do about that. I still don't know how GCC figures that out, even using the same builtin.

craffael commented 1 year ago

I think the problem is that __is_nothrow_constructible selects the wrong overload. If I write

AnyOperator o = ...; // somehow initialize it
AnyOperator q(o);

then the copy constructor should be selected (and not the template constructor). GCC and Clang (for Clang we must remove the concept constraint) both select the copy constructor, i.e. they are both correct. But __is_nothrow_constructible somehow has problems, respectively seems to use some other logic to make the decision...

erichkeane commented 1 year ago

Hmm... yes, I think thats it. We've somehow decided that our initialization here requires evaluating the template, even though there is already a non-template match. But I don't know our initialization code well enough to know the answer to that. Perhaps there is a missing DR here that we aren't implementing @hubert-reinterpretcast ?

JVApen commented 1 year ago

Reading this, it reminds me of the following issues: https://github.com/llvm/llvm-project/issues/40268 and https://github.com/llvm/llvm-project/issues/38149 There is something specific about the overload resolving that does not choose the copy constructor if not called with a const instance.

tigrkoshka commented 1 year ago

Encountered a similar issue, and noticed something curious. In terms of the original code, it (expectedly) can be fixed by std::enable_if:

template <class OPERATOR>
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;

struct AnyOperator {

    template <Operator OP,
              std::enable_if<!std::is_same_v<std::decay_t<OP>, AnyOperator>, bool> = true>
    explicit AnyOperator(OP op) noexcept {}

    AnyOperator(const AnyOperator&) noexcept = default;
};

static_assert(Operator<AnyOperator>);

This snippet compiles fine.

However, replacing std::enable_if with an identical requires clause does not seem to fix the issue:

template <class OPERATOR>
concept Operator = std::is_nothrow_copy_constructible_v<OPERATOR>;

struct AnyOperator {
    template <Operator OP>
    explicit AnyOperator(OP op) noexcept
        requires(!std::same_as<std::decay_t<OP>, AnyOperator>)
    {}

    AnyOperator(const AnyOperator&) noexcept = default;
};

static_assert(Operator<AnyOperator>);

Compiling this snippet with Clang 16.0.2 produces the same error messages as in the original question.

This seemed like a related bug, so I thought I'd comment it here. If this is an unrelated problem, please point that out and I would be happy to open a separate issue:)

erichkeane commented 1 year ago

Thats the same problem, the Operator constraint in the AnyOperator constructor checks the concept Operator, which our implementation of is_nothrow_copy_constructible (via the builtin) is causing us to try to construct AnyOperator, which again, causes us to check the constraint.

ckwastra commented 3 months ago

I encountered a similar issue regarding operator overloading where MSVC/GCC accept the code (Godbolt):

template <class T, class U, class = void> struct A : false_type {};
template <class T, class U>
struct A<T, U, void_t<decltype(declval<T>() + declval<U>())>> : true_type {};
template <class T, class U> void operator+(T, U);
template <class T, class U>
  requires A<T, U>::value
  // ^ Satisfaction of constraint 'A<T, U>::value' depends on itself
void operator+(T, U);
int main() {
  auto a = {1, 2, 3};
  auto b = {1, 2, 3};
  a + b; // Invalid operands to binary expression
}

Using std::enable_if_t instead of requires seems to resolve this issue.

hubert-reinterpretcast commented 2 months ago

Perhaps there is a missing DR here that we aren't implementing @hubert-reinterpretcast ?

Clang is doing the "directly conforming thing". The template is a candidate (https://wg21.link/over.match.ctor), and deduction (including checking of constraints; https://wg21.link/temp.deduct.general#5) happens before the rest of overload resolution. The only de jure reason that the copy constructor is the better candidate is that it is not a template (the template results in a viable candidate).

GCC seems to "figure it out" by applying a special (ad hoc) rule against constructors that take their own class by value as the sole argument. It never bothered to check the constraints.

https://godbolt.org/z/4nqETK5ba

template <typename T>
struct Oops {
  static_assert(sizeof(T) == 0);  // GCC never sees this
  static constexpr bool value = true;
};

template <class OPERATOR>
concept Operator = Oops<OPERATOR>::value;

struct AnyOperator {
  template <Operator OP>
  AnyOperator(OP op) noexcept {}

  explicit AnyOperator(const AnyOperator&) noexcept = default;
};

void f(AnyOperator *ap) {
  AnyOperator a = *ap;
}
hubert-reinterpretcast commented 2 months ago

I don't see a defect here: The original reporter should simply have added a helper concept that checks that the constructor template argument is not AnyOperator prior to checking the constructibility.

hubert-reinterpretcast commented 2 months ago

The closest "rule" appears to be https://wg21.link/class.copy.ctor#5.

However, the words:

A member function template is never instantiated to produce such a constructor signature.

technically mean that the definition is not produced (not the declaration, despite references to "signature").

There is also nothing in those words that indicate where in the deduction/substitution/constraint checking/etc. process this kicks in.

hubert-reinterpretcast commented 2 months ago

It appears that Clang also has some version of the "rule".

https://godbolt.org/z/hT4nfvxnW

struct AnyOperator {
  template <typename OP>
  AnyOperator(OP op, int = 0) noexcept {}  // Clang rejects this candidate for the copy initialization of `op`

  explicit AnyOperator(const AnyOperator&) noexcept {}
};

void f(AnyOperator *ap) {
  AnyOperator a(*ap, 0);
}
hubert-reinterpretcast commented 2 months ago

I encountered a similar issue regarding operator overloading where MSVC/GCC accept the code (Godbolt):

template <class T, class U, class = void> struct A : false_type {};
template <class T, class U>
struct A<T, U, void_t<decltype(declval<T>() + declval<U>())>> : true_type {};
template <class T, class U> void operator+(T, U);
template <class T, class U>
  requires A<T, U>::value
  // ^ Satisfaction of constraint 'A<T, U>::value' depends on itself
void operator+(T, U);
int main() {
  auto a = {1, 2, 3};
  auto b = {1, 2, 3};
  a + b; // Invalid operands to binary expression
}

Using std::enable_if_t instead of requires seems to resolve this issue.

Yours is a different issue @ckwastra. The originally-reported issue hinges on when a constructor template candidate should be rejected. Your issue is a bug in Clang's name lookup for dependent operator function calls using the overloaded operator syntax.

namespace Q {
  struct A {};
}

template <class T>
int operator+(T, char (*)[0 ?
  T{} + static_cast<char (*)[1]>(0) :
  1]);  // Not recursive: `operator+` only available to unqualified name lookup following the end of the declarators

int f() {
  return Q::A{} + static_cast<char (*)[1]>(0);
}

https://godbolt.org/z/K3zxGPn1z

hubert-reinterpretcast commented 2 months ago

However, the words:

A member function template is never instantiated to produce such a constructor signature.

technically mean that the definition is not produced (not the declaration, despite references to "signature").

There is also nothing in those words that indicate where in the deduction/substitution/constraint checking/etc. process this kicks in.

https://github.com/cplusplus/CWG/issues/474

zygoloid commented 2 months ago

In addition to the question of when the "substitution produces a C(C) constructor" check takes place, there's another GCC / Clang difference here: GCC appears to not consider template candidates at all if there's an exact match among the non-template candidates:

template <typename T>
struct Oops {
  static_assert(sizeof(T) == 0);
  static constexpr bool value = true;
};

template <class OPERATOR>
concept Operator = Oops<OPERATOR>::value;

template <Operator OP> void f(OP op);
void f(int);

// Clang rejects, GCC accepts
void g(int n) { f(n); }
// Both reject
void h(short n) { f(n); }