Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CTAD: implement DR2628 implicit deduction guides should propagate constraints #98592

Closed
hokein opened this issue Jul 12, 2024 · 6 comments · Fixed by #111143
Closed

CTAD: implement DR2628 implicit deduction guides should propagate constraints #98592

hokein opened this issue Jul 12, 2024 · 6 comments · Fixed by #111143
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@hokein
Copy link
Collaborator

hokein commented Jul 12, 2024

https://cplusplus.github.io/CWG/issues/2628.html

@hokein hokein added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Jul 12, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 12, 2024

@llvm/issue-subscribers-clang-frontend

Author: Haojian Wu (hokein)

https://cplusplus.github.io/CWG/issues/2628.html

@shafik
Copy link
Collaborator

shafik commented Jul 12, 2024

Can you provide and example that would demonstrate the difference? I am looking at the example from the DR: https://godbolt.org/z/WEaqzd9he

and it is not obvious what behavior I should see and how I would determine it is correct.

@hokein
Copy link
Collaborator Author

hokein commented Jul 15, 2024

If we look at it from a different angle (the AST point of view), it becomes more obvious https://godbolt.org/z/zed6846re:

template<class T> concept True = true;
template<class T> concept True2 = true;

template<class T>
requires True2<T>
 struct X {
  template<class U> requires True<U> X(T, U(&)[3]); 
};

double arr3[3];
X z(3, arr3);  

The implicit deduction guide is:

|-FunctionTemplateDecl <line:8:3, col:50> col:38 implicit <deduction guide for X>
| |-TemplateTypeParmDecl <line:5:10, col:16> col:16 referenced class depth 0 index 0 T
| |-TemplateTypeParmDecl <line:8:12, col:18> col:18 class depth 0 index 1 U
| |-ConceptSpecializationExpr <col:30, col:36> 'bool' Concept 0xcfa0880 'True'
| | |-ImplicitConceptSpecializationDecl <line:2:27> col:27
| | | `-TemplateArgument type 'type-parameter-0-1'
| | |   `-TemplateTypeParmType 'type-parameter-0-1' dependent depth 0 index 1
| | `-TemplateArgument <line:8:35> type 'type-parameter-0-1'
| |   `-TemplateTypeParmType 'type-parameter-0-1' dependent depth 0 index 1
| |-CXXDeductionGuideDecl <col:38, col:50> col:38 implicit <deduction guide for X> 'auto (T, type-parameter-0-1 (&)[3]) -> X<T>'
| | |-ParmVarDecl <col:40> col:41 'T'
| | `-ParmVarDecl <col:43, col:49> col:46 'type-parameter-0-1 (&)[3]'

The associated constraints is True<U>, which is wrong. It should be True<U> && True2<T>.

@zyn0217
Copy link
Contributor

zyn0217 commented Sep 30, 2024

I think the behavior of clang is already conforming now because in FindInstantiatedDecl(), we always call CheckTemplateIdType(), which at any rate checks the constraints for the deducing class template. This matches the footnote:

[Note 1: A constraint-expression in the template-head of C is checked for satisfaction before any constraints from the template-head or trailing requires-clause of the constructor. — end note]

Regarding the AST part, it's probably unnecessary for the synthesized declaration to always have a conjunction form. We have checked the constraints of C before checking that of the CTAD guide, so the two approaches are essentially equivalent. While it is doable to synthesize a BinaryOperator in that case, it would also bring up a redundant substitution.

Perhaps the nuance the user might note is the implicit guide printed in diagnostics. However, the issue would not be so much outstanding in that regard.

@hokein
Copy link
Collaborator Author

hokein commented Sep 30, 2024

Thanks for taking a look on this.

I think the form of the attached constraint directly affects the final result of template argument deduction. Consider the following example:

template<class T> concept True = true;
double arr3[3];

template<class T> 
requires True<T>
struct X {
  const int size;
  template<class U> 
  constexpr X(T, U(&)[3]) : size(sizeof(T)) {};
};
template <typename T, typename U> X(T, U (&)[3]) -> X<U>;

constexpr X z(3, arr3); 
static_assert(z.size == 4);

If I understand the standard correctly, the implicit deduction guide should be chosen because it is more constrained than the explicitly written deduction guide, according to over.match.best.general. However, it seems that all compilers fail on this case, https://godbolt.org/z/PKhK1WvTo.

The following example works as expected, https://godbolt.org/z/cMEjvrb4n

template<class T> concept True = true;
double arr3[3];

template<class T> 
struct X {
  const int size;
  template<class U> 
  requires True<T>
  constexpr X(T, U(&)[3]) : size(sizeof(T)) {};
};
template <typename T, typename U> X(T, U (&)[3]) -> X<U>;

constexpr X z(3, arr3); 
static_assert(z.size == 4);

However, when the constraint is moved after the constructor declaration, it doesn’t work in Clang (looks like a different bug in clang), https://godbolt.org/z/xKM6MPobT

template<class T> concept True = true;
double arr3[3];

template<class T> 
struct X {
  const int size;
  template<class U> 
  constexpr X(T, U(&)[3])  requires True<T> : size(sizeof(T)) {};
};
template <typename T, typename U> X(T, U (&)[3]) -> X<U>;

constexpr X z(3, arr3); 
static_assert(z.size == 4);

@zyn0217
Copy link
Contributor

zyn0217 commented Oct 1, 2024

Thanks for the clarification. I tested locally, and indeed if we combine these constraints together, then the first example compiles.

I suspect the third one is not working because we only preserved constraints from the template parameters, but we didn't do that for the function declarations in the process of transforming the constructor.

zyn0217 added a commit that referenced this issue Jan 23, 2025
…e constraints" (#111143)

Closes #98592
github-actions bot pushed a commit to arm/arm-toolchain that referenced this issue Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
4 participants