[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

8838

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

97

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

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

8643

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

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

clang/lib/Sema/SemaExprCXX.cpp

8643

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

97

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

8643

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 ↗

(On Diff #330775)

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

447–449

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 ↗

(On Diff #330775)

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/AST/ASTContext.h

public:
   /// Return a ObjCObjectPointerType type for the given ObjCObjectType.
   QualType getObjCObjectPointerType(QualType OIT) const;
 
   /// GCC extension.
   QualType getTypeOfExprType(Expr *e) const;
   QualType getTypeOfType(QualType t) const;
 
   /// C++11 decltype.
-  QualType getDecltypeType(Expr *e, QualType UnderlyingType) const;
+  QualType getDecltypeType(Expr *e, QualType UnderlyingType,
+                           bool ImplicitParens = false) const;
 
   /// Unary type transforms
   QualType getUnaryTransformType(QualType BaseType, QualType UnderlyingType,
                                  UnaryTransformType::UTTKind UKind) const;
 
   /// C++11 deduced auto type.
   QualType getAutoType(QualType DeducedType, AutoTypeKeyword Keyword,
                        bool IsDependent, bool IsPack = false,

clang/include/clang/AST/Type.h

public:
 
   static bool classof(const Type *T) { return T->getTypeClass() == TypeOf; }
 };
 
 /// Represents the type `decltype(expr)` (C++11).
 class DecltypeType : public Type {
   Expr *E;
   QualType UnderlyingType;
+  bool ImplicitParens;
 
 protected:
   friend class ASTContext; // ASTContext creates these.
 
-  DecltypeType(Expr *E, QualType underlyingType, QualType can = QualType());
+  DecltypeType(Expr *E, QualType underlyingType, QualType can = QualType(),
+               bool ImplicitParens = false);
 
 public:
   Expr *getUnderlyingExpr() const { return E; }
   QualType getUnderlyingType() const { return UnderlyingType; }
+  bool getImplicitParens() const { return ImplicitParens; }
 
   /// Remove a single level of sugar.
   QualType desugar() const;
 
   /// Returns whether this type directly provides sugar.
   bool isSugared() const;
 
   static bool classof(const Type *T) { return T->getTypeClass() == Decltype; }

clang/include/clang/Sema/Sema.h

public:
   QualType getElaboratedType(ElaboratedTypeKeyword Keyword,
                              const CXXScopeSpec &SS, QualType T,
                              TagDecl *OwnedTagDecl = nullptr);
 
   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);
+                             bool AsUnevaluated = true,
+                             bool ImplicitParens = false);
   QualType BuildUnaryTransformType(QualType BaseType,
                                    UnaryTransformType::UTTKind UKind,
                                    SourceLocation Loc);
 
   //===--------------------------------------------------------------------===//
   // Symbol table / Decl tracking callbacks: SemaDecl.cpp.
   //
 

clang/lib/AST/ASTContext.cpp

QualType ASTContext::getTypeOfType(QualType tofType) const {
   Types.push_back(tot);
   return QualType(tot, 0);
 }
 
 /// Unlike many "get<Type>" functions, we don't unique DecltypeType
 /// nodes. This would never be helpful, since each such type has its own
 /// expression, and would not give a significant memory saving, since there
 /// is an Expr tree under each such type.
-QualType ASTContext::getDecltypeType(Expr *e, QualType UnderlyingType) const {
+QualType ASTContext::getDecltypeType(Expr *e, QualType UnderlyingType,
+                                     bool ImplicitParens) const {
   DecltypeType *dt;
 
   // C++11 [temp.type]p2:
   //   If an expression e involves a template parameter, decltype(e) denotes a
   //   unique dependent type. Two such decltype-specifiers refer to the same
   //   type only if their expressions are equivalent (14.5.6.1).
   if (e->isInstantiationDependent()) {
     llvm::FoldingSetNodeID ID;
     DependentDecltypeType::Profile(ID, *this, e);
 
     void *InsertPos = nullptr;
     DependentDecltypeType *Canon
       = DependentDecltypeTypes.FindNodeOrInsertPos(ID, InsertPos);
     if (!Canon) {
       // Build a new, canonical decltype(expr) type.
       Canon = new (*this, TypeAlignment) DependentDecltypeType(*this, e);
       DependentDecltypeTypes.InsertNode(Canon, InsertPos);
     }
-    dt = new (*this, TypeAlignment)
-        DecltypeType(e, UnderlyingType, QualType((DecltypeType *)Canon, 0));
+    dt = new (*this, TypeAlignment) DecltypeType(
+        e, UnderlyingType, QualType((DecltypeType *)Canon, 0), ImplicitParens);
   } else {
-    dt = new (*this, TypeAlignment)
-        DecltypeType(e, UnderlyingType, getCanonicalType(UnderlyingType));
+    dt = new (*this, TypeAlignment) DecltypeType(
+        e, UnderlyingType, getCanonicalType(UnderlyingType), ImplicitParens);
   }
   Types.push_back(dt);
   return QualType(dt, 0);
 }
 
 /// getUnaryTransformationType - We don't unique these, since the memory
 /// savings are minimal and these are rare.
 QualType ASTContext::getUnaryTransformType(QualType BaseType,

clang/lib/AST/Type.cpp

QualType TypeOfExprType::desugar() const {
   return QualType(this, 0);
 }
 
 void DependentTypeOfExprType::Profile(llvm::FoldingSetNodeID &ID,
                                       const ASTContext &Context, Expr *E) {
   E->Profile(ID, Context, true);
 }
 
-DecltypeType::DecltypeType(Expr *E, QualType underlyingType, QualType can)
+DecltypeType::DecltypeType(Expr *E, QualType underlyingType, QualType can,
+                           bool ImplicitParens)
     // C++11 [temp.type]p2: "If an expression e involves a template parameter,
     // decltype(e) denotes a unique dependent type." Hence a decltype type is
     // type-dependent even if its expression is only instantiation-dependent.
     : Type(Decltype, can,
            toTypeDependence(E->getDependence()) |
                (E->isInstantiationDependent() ? TypeDependence::Dependent
                                               : TypeDependence::None) |
                (E->getType()->getDependence() &
                 TypeDependence::VariablyModified)),
-      E(E), UnderlyingType(underlyingType) {}
+      E(E), UnderlyingType(underlyingType), ImplicitParens(ImplicitParens) {}
 
 bool DecltypeType::isSugared() const { return !E->isInstantiationDependent(); }
 
 QualType DecltypeType::desugar() const {
   if (isSugared())
     return getUnderlyingType();
 
   return QualType(this, 0);

clang/lib/AST/TypePrinter.cpp

void TypePrinter::printTypeOfBefore(const TypeOfType *T, raw_ostream &OS) {
   OS << ')';
   spaceBeforePlaceHolder(OS);
 }
 
 void TypePrinter::printTypeOfAfter(const TypeOfType *T, raw_ostream &OS) {}
 
 void TypePrinter::printDecltypeBefore(const DecltypeType *T, raw_ostream &OS) {
   OS << "decltype(";
+  if (T->getImplicitParens())
+    OS << "(";
   if (T->getUnderlyingExpr())
     T->getUnderlyingExpr()->printPretty(OS, nullptr, Policy);
+  if (T->getImplicitParens())
+    OS << ")";
   OS << ')';
   spaceBeforePlaceHolder(OS);
 }
 
 void TypePrinter::printDecltypeAfter(const DecltypeType *T, raw_ostream &OS) {}
 
 void TypePrinter::printUnaryTransformBefore(const UnaryTransformType *T,
                                             raw_ostream &OS) {

clang/lib/Sema/SemaConcept.cpp

switch (Req->getSatisfactionStatus()) {
     case concepts::ExprRequirement::SS_ConstraintsNotSatisfied: {
       ConceptSpecializationExpr *ConstraintExpr =
           Req->getReturnTypeRequirementSubstitutedConstraintExpr();
       if (ConstraintExpr->getTemplateArgsAsWritten()->NumTemplateArgs == 1)
         // A simple case - expr type is the type being constrained and the concept
         // was not provided arguments.
         S.Diag(ConstraintExpr->getBeginLoc(),
                diag::note_expr_requirement_constraints_not_satisfied_simple)
-            << (int)First << S.BuildDecltypeType(Req->getExpr(),
-                                                 Req->getExpr()->getBeginLoc())
+            << (int)First
+            << S.BuildDecltypeType(Req->getExpr(),
+                                   Req->getExpr()->getBeginLoc(), true, true)
             << ConstraintExpr->getNamedConcept();
       else
         S.Diag(ConstraintExpr->getBeginLoc(),
                diag::note_expr_requirement_constraints_not_satisfied)
             << (int)First << ConstraintExpr;
       S.DiagnoseUnsatisfiedConstraint(ConstraintExpr->getSatisfaction());
       break;
     }

clang/lib/Sema/SemaExprCXX.cpp

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();
+        BuildDecltypeType(E, E->getBeginLoc(), true, true).getCanonicalType();
     llvm::SmallVector<TemplateArgument, 1> Args;
     Args.push_back(TemplateArgument(MatchedType));
     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()

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);
 }
 
+/// getBaseDecltypeForExpr - Given an expr, will return the decltype for
+/// that expression, without accounting for the rules.
+static QualType getBaseDecltypeForExpr(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:
+    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;
+}
+
 /// 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:
   //   The type denoted by decltype(e) is defined as follows:
 
   // C++20:
   //     - if E is an unparenthesized id-expression naming a non-type
   //       template-parameter (13.2), decltype(E) is the type of the
   //       template-parameter after performing any necessary type deduction
   // Note that this does not pick up the implicit 'const' for a template
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 getBaseDecltypeForExpr(S, E);
 }
 
 QualType Sema::BuildDecltypeType(Expr *E, SourceLocation Loc,
-                                 bool AsUnevaluated) {
+                                 bool AsUnevaluated, bool ImplicitParens) {
   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 Type = E->isTypeDependent()
+                      ? Context.DependentTy
+                      : (ImplicitParens ? getBaseDecltypeForExpr
+                                        : getDecltypeForExpr)(*this, E);
+  return Context.getDecltypeType(E, Type, ImplicitParens);
 }
 
 QualType Sema::BuildUnaryTransformType(QualType BaseType,
                                        UnaryTransformType::UTTKind UKind,
                                        SourceLocation Loc) {
   switch (UKind) {
   case UnaryTransformType::EnumUnderlyingType:
     if (!BaseType->isDependentType() && !BaseType->isEnumeralType()) {

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 reference {
+  using type = T;
+  static constexpr bool value = false;
+};
+template<typename T> struct reference<T&> {
+  using type = T;
+  static constexpr bool value = true;
+};
+
 template<typename T, typename U>
 concept Same = is_same_v<T, U>;
 
+template<typename T> concept Ref = reference<T>::value;
+
 template<typename T>
-concept Large = sizeof(T) >= 4; // expected-note{{because 'sizeof(short) >= 4' (2 >= 4) evaluated to false}}
+concept LargeRef = sizeof(typename reference<T>::type) >= 4;
+// expected-note@-1{{because 'sizeof(typename 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 } -> Ref;
+  { t } -> LargeRef; // expected-note{{because 'decltype((t))' (aka 'short &') does not satisfy 'LargeRef':}}
+}
 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
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]}}
-}
+}