[clang] Use decltype((E)) for compound requirement type constraint

[mizvekov]

Summary

See PR45088.

Compound requirement type constraints were using decltype(E) instead of
decltype((E)), as per [expr.prim.req]p1.3.3.

Since neither instantiation nor type dependence should matter for
the constraints, this uses an approach where a decltype type is not built,
and just the canonical type of the expression after template instantiation
is used on the requirement.

Signed-off-by: Matheus Izvekov <[e-mail]>

[• Quuxplusone]

clang/lib/Sema/SemaType.cpp

8837

I think a better name for this might be getDecltypeForNonIdentifierExpr and/or getDecltypeForParenthesizedExpr.
Depending on the mental model you want people to have, ImplicitParens might be better named AsNonIdentifier.
The thing we're enabling/disabling here is "whether to treat an id-expression differently from any other kind of expression."

Also, FYI and FWIW, I have a proposal in the pipeline for C++23 that will make "the decltype of" the id-expression in return x into an rvalue (for the purposes of decltype(auto) return types). So it might be worth thinking about what that codepath would look like, since it's another example of "treating id-expressions differently from other kinds of expressions (but in a different way from how we already do)."
http://quuxplusone.github.io/draft/d2266-implicit-move-rvalue-ref.html
If I ran the zoo, I'd have a primitive function for "get the decltype of this expression, ignoring all crazy id-expression rules," and then I'd layer various helper functions on top of it that did, like, "if (is id-expression) craziness; else call the primitive function."

Finally, your comment on line 8837 ought to indicate what rules it's not accounting for. "The rules" is pretty vague. ;)

clang/test/CXX/expr/expr.prim/expr.prim.req/compound-requirement.cpp

89–90

I think this is too large of a change. How about just keeping the old test code but changing it to

template<typename T>
concept Large = sizeof(std::decay_t<T>) >= 4; // expected-note{{because... etc}}

[rsmith]

clang/lib/AST/ASTContext.cpp

5446 ↗

(On Diff #328918)

If we don't track the extra parens here too, we can end up giving the same canonical type to dependent decltype types with and without implicit parens, even though they can instantiate to different types. (But I think the simplest way to handle this would be to include the parens in the expression; see other comment.)

clang/lib/Sema/SemaExprCXX.cpp

8664–8665

Instead of adding complexity to the type system to deal with this special case, can you directly create a ParenExpr here?

[mizvekov]

Addressing Arthur's concerns:

• Changed getBaseDecltypeForExpr to getDecltypeForParenthesizedExpr.
• Changed ImplicitParens to Parenthesized.
• Clarified comment.

[mizvekov]

clang/lib/AST/ASTContext.cpp

5446 ↗

(On Diff #328918)

Good catch, but yeah I'll see about the other solution.

clang/lib/Sema/SemaExprCXX.cpp

8664–8665

Hmm I thought about that. Since I am a bit unfamiliar with the code there, I was not sure it was going to end up being more or less complicated than the proposed solution. But I'll give it a shot if that looks simpler to you.

clang/test/CXX/expr/expr.prim/expr.prim.req/compound-requirement.cpp

89–90

Yeah I thought about that, but on the other hand changing the test to make it a bit more strict did not look like a problem to me.
Hmm, we cover this case in the test below anyway, might as well reduce the amount of changes I guess.

[mizvekov]

• Uses yet another simpler approach: Since dependence should not matter (we were using the type of the expression after template instantiation anyway), we don't even need to build a decltype type here, and just get the canonical type from the (expr) instead. Kudos to rsmith for this suggestion in private.
• Adds some more tests, testing more kinds of values.

[mizvekov]

lint

[mizvekov]

broken bot rebuild repush

[mizvekov]

I don't know what is going on with the clang-format linux failure here.
The patch it suggests looks awful...

[mizvekov]

clang/lib/Sema/SemaExprCXX.cpp

8664–8665

So yeah, now we just get the type of the expression directly, by using the now exposed underlying code from decltype.
This is OK since the expressions in the requires clause were never dependent in the first place.
Even though just creating the parensexpr would likely lead to a smaller change as in number of lines of code modified, I think not using it makes the fact that dependency is not an issue here more clear in the code than just reusing the full decltype machinery.

[rsmith]

Thanks, I like this approach.

clang/include/clang/Basic/DiagnosticSemaKinds.td

2761

I don't think we need to talk about the mechanics of how we formed the type here. Also, adding a manual 'aka' like this will result in a double-aka (decltype((foo))' (aka 'bar' (aka 'baz'))) in some cases.

Aside [not something to address in this patch]: this (and other diagnostics nearby) also violate our diagnostics best practices by redundantly including in the diagnostic the source expression that the caret will be pointing to, but perhaps that's justifiable in this case because we expect these expressions to be short and they're so central to what's being diagnosed.

clang/lib/Sema/SemaConcept.cpp

448

Please don't canonicalize the type here. If we've retained type sugar for the type of e, it'll be more helpful to the user if we present that type sugar.

[mizvekov]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2761

Though the original code just built a decltype and printed it here. This format string, as far as I remember, matches exactly what the original code printed, so that a sweeping change to update all the tests would not have to be made.
I vaguely remember following the code of the type printer here and seeing that it would stop from printing an aka chain by just canonicalizing the type on the first aka.

So yeah, the original code ironically violated this best practice by just printing a type, but which in this case contains the expression.

But I agree we do not necessarily need to explain the mechanics of this type here, though I think the standard does explain how the type is obtained here in terms of decltype(()).

But since you mentioned this, I think the best approach here might be to just print the non canonical type, and not print the expression.
Will have to update all the tests but no big deal. Agreed?

[mizvekov]

Changes per rsmith review:

Just print the non-canonical type in the diagnostics.
Instead of printing the expression, make the caret point to it, instead of
pointing to the constraint.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def note_expr_requirement_noexcept_not_met : Note<
   "%select{and|because}0 '%1' may throw an exception">;
 def note_expr_requirement_type_requirement_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid: %2">;
 def note_expr_requirement_type_requirement_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;
 def note_expr_requirement_constraints_not_satisfied : Note<
   "%select{and|because}0 type constraint '%1' was not satisfied:">;
 def note_expr_requirement_constraints_not_satisfied_simple : Note<
-  "%select{and|because}0 %1 does not satisfy %2:">;
+  "%select{and|because}0 'decltype((%1))' (aka %2) does not satisfy %3:">;
 def note_type_requirement_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid: %2">;
 def note_type_requirement_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;
 def note_nested_requirement_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid: %2">;
 def note_nested_requirement_unknown_substitution_error : Note<
   "%select{and|because}0 '%1' would be invalid">;

clang/include/clang/Sema/Sema.h

bool RequireLiteralType(SourceLocation Loc, QualType T, unsigned DiagID,
     BoundTypeDiagnoser<Ts...> Diagnoser(DiagID, Args...);
     return RequireLiteralType(Loc, T, Diagnoser);
   }
 
   QualType getElaboratedType(ElaboratedTypeKeyword Keyword,
                              const CXXScopeSpec &SS, QualType T,
                              TagDecl *OwnedTagDecl = nullptr);
 
+  QualType getCanonicalTypeForParenthesizedExpr(Expr *E);
   QualType BuildTypeofExprType(Expr *E, SourceLocation Loc);
   /// If AsUnevaluated is false, E is treated as though it were an evaluated
   /// context, such as when building a type for decltype(auto).
   QualType BuildDecltypeType(Expr *E, SourceLocation Loc,
                              bool AsUnevaluated = true);
   QualType BuildUnaryTransformType(QualType BaseType,
                                    UnaryTransformType::UTTKind UKind,
                                    SourceLocation Loc);

clang/lib/Sema/SemaConcept.cpp

case concepts::ExprRequirement::SS_TypeRequirementSubstitutionFailure: {
         S.Diag(SubstDiag->DiagLoc,
                diag::note_expr_requirement_type_requirement_unknown_substitution_error)
             << (int)First << SubstDiag->SubstitutedEntity;
       break;
     }
     case concepts::ExprRequirement::SS_ConstraintsNotSatisfied: {
       ConceptSpecializationExpr *ConstraintExpr =
           Req->getReturnTypeRequirementSubstitutedConstraintExpr();
-      if (ConstraintExpr->getTemplateArgsAsWritten()->NumTemplateArgs == 1)
+      if (ConstraintExpr->getTemplateArgsAsWritten()->NumTemplateArgs == 1) {
         // A simple case - expr type is the type being constrained and the concept
         // was not provided arguments.
+        Expr *e = Req->getExpr();
         S.Diag(ConstraintExpr->getBeginLoc(),
                diag::note_expr_requirement_constraints_not_satisfied_simple)
-            << (int)First << S.BuildDecltypeType(Req->getExpr(),
-                                                 Req->getExpr()->getBeginLoc())
+            << (int)First << e << S.getCanonicalTypeForParenthesizedExpr(e)
             << ConstraintExpr->getNamedConcept();
-      else
+      } else {
         S.Diag(ConstraintExpr->getBeginLoc(),
                diag::note_expr_requirement_constraints_not_satisfied)
             << (int)First << ConstraintExpr;
+      }
       S.DiagnoseUnsatisfiedConstraint(ConstraintExpr->getSatisfaction());
       break;
     }
     case concepts::ExprRequirement::SS_Satisfied:
       llvm_unreachable("We checked this above");
   }
 }
 

clang/lib/Sema/SemaExprCXX.cpp

else if (NoexceptLoc.isValid() && canThrow(E) == CanThrowResult::CT_Can)
     Status = concepts::ExprRequirement::SS_NoexceptNotMet;
   else if (ReturnTypeRequirement.isSubstitutionFailure())
     Status = concepts::ExprRequirement::SS_TypeRequirementSubstitutionFailure;
   else if (ReturnTypeRequirement.isTypeConstraint()) {
     // C++2a [expr.prim.req]p1.3.3
     //     The immediately-declared constraint ([temp]) of decltype((E)) shall
     //     be satisfied.
     TemplateParameterList *TPL =
         ReturnTypeRequirement.getTypeConstraintTemplateParameterList();
-    QualType MatchedType =
-        BuildDecltypeType(E, E->getBeginLoc()).getCanonicalType();
     llvm::SmallVector<TemplateArgument, 1> Args;
-    Args.push_back(TemplateArgument(MatchedType));
+    Args.push_back(TemplateArgument(getCanonicalTypeForParenthesizedExpr(E)));
     TemplateArgumentList TAL(TemplateArgumentList::OnStack, Args);
     MultiLevelTemplateArgumentList MLTAL(TAL);
     for (unsigned I = 0; I < TPL->getDepth(); ++I)
       MLTAL.addOuterRetainedLevel();
     Expr *IDC =
         cast<TemplateTypeParmDecl>(TPL->getParam(0))->getTypeConstraint()
             ->getImmediatelyDeclaredConstraint();
     ExprResult Constraint = SubstExpr(IDC, MLTAL);

clang/lib/Sema/SemaType.cpp

QualType Sema::BuildTypeofExprType(Expr *E, SourceLocation Loc) {
   if (!E->isTypeDependent()) {
     QualType T = E->getType();
     if (const TagType *TT = T->getAs<TagType>())
       DiagnoseUseOfDecl(TT->getDecl(), E->getExprLoc());
   }
   return Context.getTypeOfExprType(E);
 }
 
+/// getDecltypeForParenthesizedExpr - Given an expr, will return the type for
+/// that expression, as in [dcl.type.simple]p4 but without taking id-expressions
+/// and class member access into account.
+static QualType getDecltypeForParenthesizedExpr(Sema &S, Expr *E) {
+  // C++11 [dcl.type.simple]p4:
+  //   [...]
+  QualType T = E->getType();
+  switch (E->getValueKind()) {
+  //     - otherwise, if e is an xvalue, decltype(e) is T&&, where T is the
+  //       type of e;
+  case VK_XValue:
+    return S.Context.getRValueReferenceType(T);
+  //     - otherwise, if e is an lvalue, decltype(e) is T&, where T is the
+  //       type of e;
+  case VK_LValue:
+    return S.Context.getLValueReferenceType(T);
+  //  - otherwise, decltype(e) is the type of e.
+  case VK_RValue:
+    return T;
+  }
+}
+
 /// getDecltypeForExpr - Given an expr, will return the decltype for
 /// that expression, according to the rules in C++11
 /// [dcl.type.simple]p4 and C++11 [expr.lambda.prim]p18.
 static QualType getDecltypeForExpr(Sema &S, Expr *E) {
   if (E->isTypeDependent())
     return S.Context.DependentTy;
 
   // C++11 [dcl.type.simple]p4:
if (isa<ParenExpr>(E)) {
           QualType T = S.getCapturedDeclRefType(Var, DRE->getLocation());
           if (!T.isNull())
             return S.Context.getLValueReferenceType(T);
         }
       }
     }
   }
 
-
-  // C++11 [dcl.type.simple]p4:
-  //   [...]
-  QualType T = E->getType();
-  switch (E->getValueKind()) {
-  //     - otherwise, if e is an xvalue, decltype(e) is T&&, where T is the
-  //       type of e;
-  case VK_XValue: T = S.Context.getRValueReferenceType(T); break;
-  //     - otherwise, if e is an lvalue, decltype(e) is T&, where T is the
-  //       type of e;
-  case VK_LValue: T = S.Context.getLValueReferenceType(T); break;
-  //  - otherwise, decltype(e) is the type of e.
-  case VK_RValue: break;
-  }
-
-  return T;
+  return getDecltypeForParenthesizedExpr(S, E);
 }
 
 QualType Sema::BuildDecltypeType(Expr *E, SourceLocation Loc,
                                  bool AsUnevaluated) {
   assert(!E->hasPlaceholderType() && "unexpected placeholder");
 
   if (AsUnevaluated && CodeSynthesisContexts.empty() &&
       !E->isInstantiationDependent() && E->HasSideEffects(Context, false)) {
     // The expression operand for decltype is in an unevaluated expression
     // context, so side effects could result in unintended consequences.
     // Exclude instantiation-dependent expressions, because 'decltype' is often
     // used to build SFINAE gadgets.
     Diag(E->getExprLoc(), diag::warn_side_effects_unevaluated_context);
   }
 
   return Context.getDecltypeType(E, getDecltypeForExpr(*this, E));
 }
 
+QualType Sema::getCanonicalTypeForParenthesizedExpr(Expr *E) {
+  assert(!E->isInstantiationDependent() && "expression must be non-dependant");
+  return getDecltypeForParenthesizedExpr(*this, E).getCanonicalType();
+}
+
 QualType Sema::BuildUnaryTransformType(QualType BaseType,
                                        UnaryTransformType::UTTKind UKind,
                                        SourceLocation Loc) {
   switch (UKind) {
   case UnaryTransformType::EnumUnderlyingType:
     if (!BaseType->isDependentType() && !BaseType->isEnumeralType()) {
       Diag(Loc, diag::err_only_enums_have_underlying_types);
       return QualType();

clang/test/CXX/expr/expr.prim/expr.prim.req/compound-requirement.cpp

 using r6i = r6<E>; // expected-error{{constraints not satisfied for class template 'r6' [with T = E]}}
 
 template<typename T, typename U>
 constexpr bool is_same_v = false;
 
 template<typename T>
 constexpr bool is_same_v<T, T> = true;
 
+template<typename T> struct remove_reference { using type = T; };
+template<typename T> struct remove_reference<T&> { using type = T; };
+
 template<typename T, typename U>
 concept Same = is_same_v<T, U>;
 
 template<typename T>
-concept Large = sizeof(T) >= 4; // expected-note{{because 'sizeof(short) >= 4' (2 >= 4) evaluated to false}}
+concept Large = sizeof(typename remove_reference<T>::type) >= 4;
+// expected-note@-1{{because 'sizeof(typename remove_reference<short &>::type) >= 4' (2 >= 4) evaluated to false}}
 
-template<typename T> requires requires (T t) { { t } -> Large; } // expected-note{{because 'decltype(t)' (aka 'short') does not satisfy 'Large':}}
+template<typename T> requires requires (T t) { { t } -> Large; } // expected-note{{because 'decltype((t))' (aka 'short &') does not satisfy 'Large':}}
 struct r7 {};
 
 using r7i1 = r7<int>;
 using r7i2 = r7<short>; // expected-error{{constraints not satisfied for class template 'r7' [with T = short]}}
 
-template<typename T> requires requires (T t) { { t } -> Same<T>; }
+template<typename T> requires requires (T t) { { t } -> Same<T&>; }
 struct r8 {};
 
 using r8i1 = r8<int>;
 using r8i2 = r8<short*>;
 
 // Substitution failure in type constraint
 
-template<typename T> requires requires (T t) { { t } -> Same<typename T::type>; } // expected-note{{because 'Same<expr-type, typename T::type>' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
+template<typename T> requires requires (T t) { { t } -> Same<typename T::type&>; }
+// expected-note@-1{{because 'Same<expr-type, typename T::type &>' would be invalid: type 'int' cannot be used prior to '::' because it has no members}}
 struct r9 {};
 
 struct M { using type = M; };
 
 using r9i1 = r9<M>;
 using r9i2 = r9<int>; // expected-error{{constraints not satisfied for class template 'r9' [with T = int]}}
 
 // Substitution failure in both expression and return type requirement
 
 template<typename T> requires requires (T t) { { t.foo() } -> Same<typename T::type>; } // expected-note{{because 't.foo()' would be invalid: member reference base type 'int' is not a structure or union}}
 struct r10 {};
 
 using r10i = r10<int>; // expected-error{{constraints not satisfied for class template 'r10' [with T = int]}}
 
 // Non-type concept in type constraint
 
 template<int T>
 concept IsEven = (T % 2) == 0;
 
 template<typename T> requires requires (T t) { { t } -> IsEven; } // expected-error{{concept named in type constraint is not a type concept}}
 struct r11 {};
 
+// Value categories
+
+template<auto a = 0>
+requires requires (int b) {
+  { a } -> Same<int>;
+  { b } -> Same<int&>;
+  { 0 } -> Same<int>;
+  { static_cast<int&&>(a) } -> Same<int&&>;
+} void f1() {}
+template void f1<>();
+
 // C++ [expr.prim.req.compound] Example
 namespace std_example {
   template<typename T> concept C1 =
     requires(T x) {
       {x++};
     };
 
   template<typename T, typename U> constexpr bool is_same_v = false;
namespace std_example {
   template<typename T> concept C5 =
     requires(T x) {
       {g(x)} noexcept; // expected-note{{because 'g(x)' may throw an exception}}
     };
 
   static_assert(C5<char>);
   template<C5 T> struct C5_check {}; // expected-note{{because 'short' does not satisfy 'C5'}}
   using c5 = C5_check<short>; // expected-error{{constraints not satisfied for class template 'C5_check' [with T = short]}}
-}
+}