[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857

zyn0217 · 2024-08-12T07:35:23Z

This patch partially implements CWG 2369 for non-lambda-constrained functions.

Lambdas are left intact at this point because we need extra work to correctly instantiate captures before the function instantiation.

As a premise of CWG2369, this patch also implements CWG2770 to ensure the function parameters are instantiated on demand.

Closes #54440

clang/lib/Sema/SemaConcept.cpp

github-actions · 2024-08-18T10:45:39Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 4336f00f2156970cc0af2816331387a0a4039317 6fc2f7de9235499320514a70ee8d3a95f57deccb --extensions cpp,h -- clang/include/clang/Sema/Sema.h clang/include/clang/Sema/Template.h clang/lib/Sema/SemaConcept.cpp clang/lib/Sema/SemaTemplateDeduction.cpp clang/lib/Sema/SemaTemplateDeductionGuide.cpp clang/lib/Sema/SemaTemplateInstantiate.cpp clang/lib/Sema/TreeTransform.h clang/test/CXX/drs/cwg23xx.cpp clang/test/CXX/drs/cwg26xx.cpp clang/test/CXX/drs/cwg27xx.cpp clang/test/CXX/expr/expr.prim/expr.prim.req/nested-requirement.cpp clang/test/CXX/temp/temp.constr/temp.constr.atomic/constrant-satisfaction-conversions.cpp clang/test/SemaCXX/concept-crash-on-diagnostic.cpp clang/test/SemaCXX/cxx20-ctad-type-alias.cpp clang/test/SemaCXX/cxx23-assume.cpp clang/test/SemaCXX/cxx2c-fold-exprs.cpp clang/test/SemaCXX/lambda-unevaluated.cpp clang/test/SemaTemplate/concepts-recursive-inst.cpp clang/test/SemaTemplate/cxx2a-constraint-exprs.cpp clang/test/SemaTemplate/deduction-guide.cpp clang/test/SemaTemplate/nested-implicit-deduction-guides.cpp

View the diff from clang-format here.

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 20a8b5c369..fab460dbd8 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13008,7 +13008,8 @@ public:
   ///
   /// \param DC In the event we don't HAVE a declaration yet, we instead provide
   ///  the decl context where it will be created.  In this case, the \p
-  ///  Innermost should likely be provided.  If \p ND is non-null, this is ignored.
+  ///  Innermost should likely be provided.  If \p ND is non-null, this is
+  ///  ignored.
   ///
   /// \param Innermost if non-NULL, specifies a template argument list for the
   /// template declaration passed as \p ND.

The function template<class Duration> requires three_way_comparable_with<sys_seconds, sys_time<Duration>> constexpr auto operator<=>(const leap_second& x, const sys_time<Duration>& y) noexcept; Has a recursive constrained. This caused an infinite loop in GCC and is now hit by llvm#102857. A fix would be to make this function a hidden friend, this solution is propsed in LWG4139. For consistency all comparisons are made hidden friends. Since the issue causes compilation failures no additional test are needed. Fixes: llvm#104700

The function template<class Duration> requires three_way_comparable_with<sys_seconds, sys_time<Duration>> constexpr auto operator<=>(const leap_second& x, const sys_time<Duration>& y) noexcept; Has a recursive constrained. This caused an infinite loop in GCC and is now hit by llvm#102857. A fix would be to make this function a hidden friend, this solution is propsed in LWG4139. For consistency all comparisons are made hidden friends. Since the issue causes compilation failures no additional test are needed.

zyn0217 · 2024-08-19T09:52:04Z

@mordante
I saw some tests are "unexpectedly passed" on ARM platforms: https://buildkite.com/llvm-project/libcxx-ci/builds/37165#0191685f-c770-41d1-a8a4-8819da1b1802
Is that something undesired / why did we mark them XFAIL previously?

The function template<class Duration> requires three_way_comparable_with<sys_seconds, sys_time<Duration>> constexpr auto operator<=>(const leap_second& x, const sys_time<Duration>& y) noexcept; Has a recursive constrained. This caused an infinite loop in GCC and is now hit by #102857. A fix would be to make this function a hidden friend, this solution is propsed in LWG4139. For consistency all comparisons are made hidden friends. Since the issue causes compilation failures no additional test are needed. Fixes: #104700

clang/test/CXX/drs/cwg27xx.cpp

Sirraide · 2024-08-28T11:14:27Z

(Unassigning myself because I’m not familiar enough w/ the details of substitution to review this in a meaningful way).

That said, I think this is still missing a release note isn’t it? :)

zyn0217 · 2024-08-28T11:17:54Z

(Unassigning myself because I’m not familiar enough w/ the details of substitution to review this in a meaningful way).

No problem :)

That said, I think this is still missing a release note isn’t it? :)

Yeah, I realized it the moment after I pushed the last commit, and I decided to leave that in the next push so I don't make commits piecemeal.

cor3ntin

Thanks!
I did a first pass on the review, I have a few questions

clang/include/clang/Sema/Sema.h

cor3ntin · 2024-08-29T13:07:00Z

clang/include/clang/Sema/Sema.h

+  /// template declaration passed as \p ND. The next declaration context would
+  /// be switched to \p DC if present; otherwise, it would be the semantic


I am not sure what "Next Declaration Context" refers too here

It means the visitation order. In order to collect the instantiation arguments, we start with a declaration and walk up toward its template parents. This function could use a refactor and be smarter in figuring out how to walk up from ND alone, so that the DC parameter becomes unnecessary and could be removed.

clang/lib/Sema/SemaConcept.cpp

cor3ntin · 2024-08-29T13:19:00Z

clang/lib/Sema/SemaTemplate.cpp

+    MultiLevelTemplateArgumentList MLTAL =
+        getTemplateInstantiationArgs(Template, Template->getDeclContext(),
+                                     /*Final=*/false, CanonicalConverted,
+                                     /*RelativeToPrimary=*/true,
+                                     /*Pattern=*/nullptr,
+                                     /*ForConceptInstantiation=*/true);


Can you explain why we do not use NewContext anymore?

The NewContext argument didn't actually make any difference to getTemplateInstantiationArgs() previously, as it would always move to Template->getDeclContext() for the next traversal if the Innermost parameter (which is CanonicalConverted) was present. So I changed it to Template->getDeclContext() to clarify the behavior.

However, I can revert the changes on getTemplateInstantiationArgs() after @sdkrystian's refactoring patch, so this is left only for the time being.

clang/lib/Sema/SemaTemplateDeduction.cpp

clang/lib/Sema/SemaTemplateInstantiate.cpp

clang/lib/Sema/SemaConcept.cpp

…104713) The function template<class Duration> requires three_way_comparable_with<sys_seconds, sys_time<Duration>> constexpr auto operator<=>(const leap_second& x, const sys_time<Duration>& y) noexcept; Has a recursive constrained. This caused an infinite loop in GCC and is now hit by llvm#102857. A fix would be to make this function a hidden friend, this solution is propsed in LWG4139. For consistency all comparisons are made hidden friends. Since the issue causes compilation failures no additional test are needed. Fixes: llvm#104700

zyn0217 · 2024-10-08T11:30:12Z

(Sorry for picking up this late, and this is ready for the second round of review - apart from the getTemplateInstantiationArgs() pieces, which can be ditched after the MLTAL refactoring patch lands)

cor3ntin · 2024-10-09T07:22:26Z

clang/lib/Sema/SemaTemplateInstantiate.cpp

+  if (SemaRef.CurrentInstantiationScope->findInstantiationUnsafe(OldParm))
+    return false;
+  // The current context might have been changed in the process of transforming
+  // lambda expression. So resume it before we substitute into the parameter.


Suggested change

// lambda expression. So resume it before we substitute into the parameter.

// lambda expression. So restore it before we substitute into the parameter.

cor3ntin · 2024-10-09T07:24:40Z

clang/lib/Sema/SemaTemplateDeduction.cpp

@@ -4042,8 +4062,8 @@ TemplateDeductionResult Sema::FinishTemplateArgumentDeduction(
  //   ([temp.constr.decl]), those constraints are checked for satisfaction
  //   ([temp.constr.constr]). If the constraints are not satisfied, type
  //   deduction fails.
-  if (!IsIncomplete) {
-    if (CheckInstantiatedFunctionTemplateConstraints(
+  if (IsLambda && !IsIncomplete) {


can you add a fixme comment here?

There's one on line 4003, maybe that just suffices?

cor3ntin · 2024-10-09T07:24:59Z

clang/lib/Sema/SemaTemplateDeduction.cpp

+      return TemplateDeductionResult::ConstraintsNotSatisfied;
+    }
+  }
+  // C++ [temp.deduct.call]p10: [DR1391]


Suggested change

// C++ [temp.deduct.call]p10: [DR1391]

// CWG1391

Oops, I thought I have fixed it everywhere...

cor3ntin · 2024-10-09T07:27:58Z

clang/lib/Sema/SemaTemplateDeduction.cpp

+      Info.reset(Info.takeSugared(),
+                 TemplateArgumentList::CreateCopy(Context, CanonicalBuilder));


Why is that necessary?

cor3ntin · 2024-10-09T07:28:39Z

clang/lib/Sema/SemaConcept.cpp

@@ -1517,7 +1555,7 @@ substituteParameterMappings(Sema &S, NormalizedConstraint &N,
 static bool substituteParameterMappings(Sema &S, NormalizedConstraint &N,
                                        const ConceptSpecializationExpr *CSE) {
  MultiLevelTemplateArgumentList MLTAL = S.getTemplateInstantiationArgs(
-      CSE->getNamedConcept(), CSE->getNamedConcept()->getLexicalDeclContext(),
+      CSE->getNamedConcept(), CSE->getNamedConcept()->getDeclContext(),


Do you have tests with friend functions?

This is another leftover of my previous changes on getTemplateInstantiationArgs()... Will revert it

cor3ntin · 2024-10-09T07:33:09Z

clang/lib/Sema/SemaConcept.cpp

+  if (auto *Method = dyn_cast<CXXMethodDecl>(FD))
+    ThisScope.emplace(SemaRef, /*Record=*/Method->getParent(),
+                      /*ThisQuals=*/Method->getMethodQualifiers());


When is that useful?

Honestly, I don't know. :(

I copied that from the previous implementation, and I was expecting it to make a difference in situations where member function calls are involved in a constraint evaluation. But I ran the regression tests with that removed and everything seemed fine.

I will have another try on a build without this patch. It could also be the case we're missing some tests, but I don't know...

Huh, I removed these CXXThisScopeRAII lines in CheckInstantiatedFunctionTemplateConstraints() and all libcxx and clang regressions still passed. I guess we probably could remove these setups there

mizvekov · 2024-10-10T05:10:18Z

clang/lib/Sema/SemaConcept.cpp

+                                           /*RelativeToPrimary=*/true,
+                                           /*ForConstraintInstantiation=*/true);
+
+  std::optional<Sema::CXXThisScopeRAII> ThisScope;


Do we need to point CurContext to the function here as well?

Yes, I think so - but I couldn't find a case where that affects anything.

zyn0217 requested a review from cor3ntin August 12, 2024 07:35

zyn0217 added 5 commits August 13, 2024 09:30

[Clang][NFCI] Slightly refactor getTemplateInstantiationArgs()

1119f0a

Merge branch 'main' into cwg-2369-2

ca00718

Resolve a merge conflict

2dd772d

CWG 2369

96bf64c

Rectify diagnostics

6b7072d

zyn0217 mentioned this pull request Aug 14, 2024

[libc++][chrono] Loads leap-seconds.list in tzdb. #82113

Merged

Instantiate function parameters as needed

bb31d36

zyn0217 force-pushed the cwg-2369 branch from 3aa27cc to bb31d36 Compare August 17, 2024 14:26

zyn0217 added 4 commits August 17, 2024 22:27

Remove debugging logs

631be75

Revert unrelated changes

7d484e2

Fix tests

c515407

Fix libcxx tests

a9da5a3

cor3ntin reviewed Aug 17, 2024

View reviewed changes

clang/lib/Sema/SemaConcept.cpp Outdated Show resolved Hide resolved

Instantiate the decls as we substituting constraints

cebe706

mordante mentioned this pull request Aug 18, 2024

[libc++][chono] Use hidden friends for leap_second comparison. #104713

Merged

mordante and others added 2 commits August 19, 2024 09:51

clang-format

30dc1d1

zyn0217 added 5 commits August 21, 2024 09:31

Merge branch 'main' into cwg-2369

9d3981f

Merge branch 'main' into cwg-2369

afb8a90

Quote the standard wordings, update the DR status page

6a5085a

Revert unrelated changes

974a0ad

oops, I forgot it

d19e700

zyn0217 marked this pull request as ready for review August 28, 2024 10:40

zyn0217 requested a review from Endilll as a code owner August 28, 2024 10:40

zyn0217 requested a review from mizvekov August 28, 2024 10:40

zyn0217 requested a review from Sirraide August 28, 2024 10:40

Endilll reviewed Aug 28, 2024

View reviewed changes

clang/test/CXX/drs/cwg27xx.cpp Outdated Show resolved Hide resolved

Sirraide removed their request for review August 28, 2024 11:13

cor3ntin added c++20 clang:frontend Language frontend issues, e.g. anything involving "Sema" concepts C++20 concepts labels Aug 29, 2024

cor3ntin reviewed Aug 29, 2024

View reviewed changes

mizvekov reviewed Aug 29, 2024

View reviewed changes

clang/lib/Sema/SemaConcept.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaConcept.cpp Outdated Show resolved Hide resolved

zyn0217 added 2 commits October 8, 2024 13:12

Merge branch 'main' into cwg-2369

5be9921

Simplify the implementation

8ace714

zyn0217 requested a review from erichkeane October 8, 2024 11:28

zyn0217 added 5 commits October 8, 2024 19:31

Add parentheses to sizeof expression

da61fab

Merge branch 'main' into cwg-2369

18da86a

Revert comment changes

1b8f0f6

Rebase on top of the new getTemplateInstantiationArgs()

ef5acad

Merge branch 'main' into cwg-2369

6fc2f7d

cor3ntin reviewed Oct 9, 2024

View reviewed changes

mizvekov reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857

[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857

zyn0217 commented Aug 12, 2024 •

edited

Loading

github-actions bot commented Aug 18, 2024 •

edited

Loading

zyn0217 commented Aug 19, 2024

Sirraide commented Aug 28, 2024

zyn0217 commented Aug 28, 2024

cor3ntin left a comment

cor3ntin Aug 29, 2024

mizvekov Aug 29, 2024

cor3ntin Aug 29, 2024

zyn0217 Oct 8, 2024

zyn0217 commented Oct 8, 2024

cor3ntin Oct 9, 2024

cor3ntin Oct 9, 2024

zyn0217 Oct 9, 2024

cor3ntin Oct 9, 2024

zyn0217 Oct 9, 2024

cor3ntin Oct 9, 2024

cor3ntin Oct 9, 2024

zyn0217 Oct 9, 2024

cor3ntin Oct 9, 2024

zyn0217 Oct 12, 2024

zyn0217 Oct 12, 2024

mizvekov Oct 10, 2024

zyn0217 Oct 12, 2024

		/// template declaration passed as \p ND. The next declaration context would
		/// be switched to \p DC if present; otherwise, it would be the semantic

	// lambda expression. So resume it before we substitute into the parameter.
	// lambda expression. So restore it before we substitute into the parameter.

		Info.reset(Info.takeSugared(),
		TemplateArgumentList::CreateCopy(Context, CanonicalBuilder));

[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857

Are you sure you want to change the base?

[Clang] Implement CWG2369 "Ordering between constraints and substitution" #102857

Conversation

zyn0217 commented Aug 12, 2024 • edited Loading

github-actions bot commented Aug 18, 2024 • edited Loading

zyn0217 commented Aug 19, 2024

Sirraide commented Aug 28, 2024

zyn0217 commented Aug 28, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyn0217 commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyn0217 commented Aug 12, 2024 •

edited

Loading

github-actions bot commented Aug 18, 2024 •

edited

Loading