Skip to content

Conversation

@mizvekov
Copy link
Contributor

@mizvekov mizvekov commented Sep 1, 2024

We were incorrectly applying [temp.deduct]p5 to partial ordering.

Marked as NFCI as I don't think the difference is actually observable in practice.
During partial ordering, the deduced arguments will mostly be dependent and thus cannot be checked.
Otherwise, later during overload resolution, if deduction succeeds in both directions,
we will perform subsumption check for the constraints ([temp.func.order]p6).

We were incorrectly applying [temp.deduct]p5 to partial ordering.

Marked as NFCI as I don't think the difference is actually observable
in practice. During partial ordering, the deduced arguments will
mostly be dependent and thus cannot be checked. Otherwise, later
during overload resolution, if deduction succeeds in both directions,
we will perform subsumption check for the constraints ([temp.func.order]p6).
@mizvekov mizvekov self-assigned this Sep 1, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 1, 2024
@llvmbot
Copy link
Member

llvmbot commented Sep 1, 2024

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

We were incorrectly applying [temp.deduct]p5 to partial ordering.

Marked as NFCI as I don't think the difference is actually observable in practice.
During partial ordering, the deduced arguments will mostly be dependent and thus cannot be checked.
Otherwise, later during overload resolution, if deduction succeeds in both directions,
we will perform subsumption check for the constraints ([temp.func.order]p6).


Full diff: https://github.com/llvm/llvm-project/pull/106882.diff

1 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+15-21)
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index 11bc9f2d1e7484..01f18e5a325197 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -3313,10 +3313,12 @@ FinishTemplateArgumentDeduction(
   if (Trap.hasErrorOccurred())
     return TemplateDeductionResult::SubstitutionFailure;
 
-  if (auto Result = CheckDeducedArgumentConstraints(S, Partial, SugaredBuilder,
-                                                    CanonicalBuilder, Info);
-      Result != TemplateDeductionResult::Success)
-    return Result;
+  if (!IsPartialOrdering) {
+    if (auto Result = CheckDeducedArgumentConstraints(
+            S, Partial, SugaredBuilder, CanonicalBuilder, Info);
+        Result != TemplateDeductionResult::Success)
+      return Result;
+  }
 
   return TemplateDeductionResult::Success;
 }
@@ -3364,13 +3366,16 @@ static TemplateDeductionResult FinishTemplateArgumentDeduction(
   if (Trap.hasErrorOccurred())
     return TemplateDeductionResult::SubstitutionFailure;
 
-  if (auto Result = CheckDeducedArgumentConstraints(S, Template, SugaredBuilder,
-                                                    CanonicalBuilder, Info);
-      Result != TemplateDeductionResult::Success)
-    return Result;
+  if (!PartialOrdering) {
+    if (auto Result = CheckDeducedArgumentConstraints(
+            S, Template, SugaredBuilder, CanonicalBuilder, Info);
+        Result != TemplateDeductionResult::Success)
+      return Result;
+  }
 
   return TemplateDeductionResult::Success;
 }
+
 /// Complete template argument deduction for DeduceTemplateArgumentsFromType.
 /// FIXME: this is mostly duplicated with the above two versions. Deduplicate
 /// the three implementations.
@@ -5595,19 +5600,8 @@ static TemplateDeductionResult FinishTemplateArgumentDeduction(
       TDR != TemplateDeductionResult::Success)
     return TDR;
 
-  // C++20 [temp.deduct]p5 - Only check constraints when all parameters have
-  // been deduced.
-  if (!IsIncomplete) {
-    if (auto Result = CheckDeducedArgumentConstraints(S, FTD, SugaredBuilder,
-                                                      CanonicalBuilder, Info);
-        Result != TemplateDeductionResult::Success)
-      return Result;
-  }
-
-  if (Trap.hasErrorOccurred())
-    return TemplateDeductionResult::SubstitutionFailure;
-
-  return TemplateDeductionResult::Success;
+  return Trap.hasErrorOccurred() ? TemplateDeductionResult::SubstitutionFailure
+                                 : TemplateDeductionResult::Success;
 }
 
 /// Determine whether the function template \p FT1 is at least as

Copy link
Contributor

@zyn0217 zyn0217 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please wait for others' opinions before merging.

Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mizvekov mizvekov merged commit 840d4d9 into main Sep 1, 2024
@mizvekov mizvekov deleted the users/mizvekov/clang-fix-GH18291-4 branch September 1, 2024 08:11
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" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants