[cfe-commits] [PATCH] Implement C++0x deduced auto type

[Richard Smith]

On Fri, February 18, 2011 17:52, Douglas Gregor wrote:
> On Feb 17, 2011, at 6:26 PM, Richard Smith wrote:
>> The attached patch implements the C++0x deduced 'auto' type feature.
>> This should fix the following PRs:
>>
>> Bug 8738 - C++'0x auto not implemented
>> Bug 9060 - Late-specified return type accepted for parenthesized
>> declarator Bug 9132 - clang accepts declarators other than a literal
>> 'auto' as the return type of a function with a trailing-return-type
>>
>> (The latter two were fixed in order to allow more comprehensive testing
>> of deduced auto!)

[snip]

> I have a bunch of comments below, most of which
> are for relatively small issues or questions. In all cases, I'm perfectly
> happy to have this patch go in as-is (or with the tiny tweaks I've
> requested), and save some of the non-trivial things for follow-up
> commits.

Great, I've checked it in with a few of the tweaks you suggest below as
r126069.

> Specific comments on your patch:
>
>
> Index: test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p6.cpp
> ===================================================================
> --- test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p6.cpp	(revision 0)
> +++ test/CXX/dcl.dcl/dcl.spec/dcl.type/dcl.spec.auto/p6.cpp	(revision 0)
> @@ -0,0 +1,60 @@
> [snip]
> +  const auto **fail2(p); // desired-error {{variable 'fail2' with type
> 'auto const **' has incompatible initializer of type 'int **'}}
> expected-error {{cannot initialize}} +}
>
>
> The "desired-error" is cute, but please put a FIXME to indicate that the
> diagnostic here isn't any good. This is due to PR9233, right?

Done. And the same for a couple of other poor diagnostics.

> Index: include/clang/AST/Decl.h
> ===================================================================
> --- include/clang/AST/Decl.h	(revision 125795)
> +++ include/clang/AST/Decl.h	(working copy)
> @@ -646,6 +646,10 @@
> /// \brief Whether this local variable could be allocated in the return
> /// slot of its function, enabling the named return value optimization
> (NRVO).
> bool NRVOVariable : 1; +
> +  /// \brief Whether this variable has a deduced C++0x auto type for
> which we're +  /// currently parsing the initializer.
> +  bool ParsingAutoInit : 1;
>
>
> It's unfortunate that this temporary bit has to live forever in the AST.
> Did you consider an alternative, such as a SmallPtrSet inside Sema that
> tracks the (few, relatively rare) VarDecls that are waiting for their
> initializers? (I guess in the future, FieldDecls must also be
> considered).

Using a spare bit on VarDecl seemed slightly simpler, but FieldDecls have
me sold on SmallPtrSet now :) I'll put a patch together for that.

> Index: include/clang/AST/Type.h
> ===================================================================
> --- include/clang/AST/Type.h	(revision 125795)
> +++ include/clang/AST/Type.h	(working copy)
> @@ -3014,6 +3015,45 @@
> static bool classof(const SubstTemplateTypeParmPackType *T) { return true;
> }
> };
>
>
> +/// \brief Represents a C++0x auto type.
> +///
> +/// These types are usually a placeholder for a deduced type. However,
> within +/// templates and before the initializer is attached, there is no
> deduced type +/// and an auto type is type-dependent and canonical.
> +class AutoType : public Type, public llvm::FoldingSetNode {
> +  AutoType(QualType DeducedType)
> +    : Type(Auto, DeducedType.isNull() ? QualType(this, 0) : DeducedType,
> +           DeducedType.isNull() || DeducedType->isDependentType(),
> +           /*VariablyModified=*/false, /*ContainsParameterPack=*/false) {
> }
>
>
> The deduced type can never be dependent, because we shouldn't be trying
> to deduce anything from a type-dependent initializer. So, I think the "||
> DeducedType->isDependentType()" be be removed and, instead, add an assert
> for DeducedType.isNull() || !DeducedType->isDependentType().

Done. I had hoped we could deduce type T for "auto v = T();" (within a
template<typename T>) but I don't think that can work.

> Index: lib/Sema/SemaDecl.cpp
> ===================================================================
> --- lib/Sema/SemaDecl.cpp	(revision 125795)
> +++ lib/Sema/SemaDecl.cpp	(working copy)
> @@ -4500,6 +4525,24 @@
> return; }
>
>
> +  if (TypeMayContainAuto && VDecl->getType()->getContainedAutoType()) {
> +    VDecl->setParsingAutoInit(false);
> +
> +    QualType DeducedType;
> +    if (!DeduceAutoType(VDecl->getType(), Init, DeducedType)) {
> +      Diag(VDecl->getLocation(), diag::err_auto_var_deduction_failure)
> +        << VDecl->getDeclName() << VDecl->getType() << Init->getType()
> +        << Init->getSourceRange();
> +      RealDecl->setInvalidDecl();
> +      return;
> +    }
> +    VDecl->setType(DeducedType);
> +
> +    // If this is a redeclaration, check that the type we just deduced
> matches +    // the previously declared type.
> +    if (VarDecl *Old = VDecl->getPreviousDeclaration())
> +      MergeVarDeclTypes(VDecl, Old);
> +  }
>
>
> It would be useful to cite the appropriate C++0x standard clause here.

Done.

> Index: lib/Sema/SemaTemplateDeduction.cpp
> ===================================================================
> --- lib/Sema/SemaTemplateDeduction.cpp	(revision 125795)
> +++ lib/Sema/SemaTemplateDeduction.cpp	(working copy)
> @@ -22,6 +22,7 @@
> #include "clang/AST/Expr.h"
> #include "clang/AST/ExprCXX.h"
> #include "llvm/ADT/BitVector.h"
> +#include "TreeTransform.h"
> #include <algorithm>
>
>
> namespace clang { @@ -2445,21 +2446,22 @@
> ParamType = ParamType.getLocalUnqualifiedType();
> const ReferenceType *ParamRefType = ParamType->getAs<ReferenceType>(); if
> (ParamRefType) {
> +    QualType PointeeType = ParamRefType->getPointeeType();
> +
> //   [C++0x] If P is an rvalue reference to a cv-unqualified
> //   template parameter and the argument is an lvalue, the type
> //   "lvalue reference to A" is used in place of A for type
> //   deduction.
> -    if (const RValueReferenceType *RValueRef
> -                                   =
> dyn_cast<RValueReferenceType>(ParamType)) { -      if
> (!RValueRef->getPointeeType().getQualifiers() &&
> -          isa<TemplateTypeParmType>(RValueRef->getPointeeType()) &&
> +    if (isa<RValueReferenceType>(ParamType)) {
> +      if (!PointeeType.getQualifiers() &&
> +          isa<TemplateTypeParmType>(PointeeType) &&
> Arg->Classify(S.Context).isLValue())
> ArgType = S.Context.getLValueReferenceType(ArgType);
> }
>
>
> //   [...] If P is a reference type, the type referred to by P is used
> //   for type deduction.
> -    ParamType = ParamRefType->getPointeeType();
> +    ParamType = PointeeType;
> }
>
>
> // Overload sets usually make this parameter an undeduced
>
>
> This is unrelated to auto, right? It also seems slightly dangerous, since
> we're really looking for the specific written form "T&&" (hence the
> dyn_cast on ParamType), rather than something that might be a typedef of
> "T&&".

I was in this area trying to fix deduction of "auto &&x = lvalue;", but
ended up not putting the fix here. The changes above should be a no-op,
just changing an unnecessary dyn_cast to isa.

Incidentally, are we really supposed to treat:

  template<typename T> void f(T &&v);

differently from:

  template<typename T> void f(T (&&v));

? FWIW, g++ treats them the same.

> @@ -2946,6 +2948,88 @@
> QualType(), Specialization, Info);
> }
>
>
> +namespace {
> +  class SubstituteAutoTransform :
> +    public TreeTransform<SubstituteAutoTransform> {
> +    QualType Replacement;
> +  public:
> +    SubstituteAutoTransform(Sema &SemaRef, QualType Replacement) :
> +      TreeTransform<SubstituteAutoTransform>(SemaRef),
> Replacement(Replacement) {
> +    }
>
>
> Please add a comment for the SubstituteAutoTransform class.

Done.

> +    QualType TransformAutoType(TypeLocBuilder &TLB, AutoTypeLoc TL) {
> +      // Some of the template deduction code expects to see unadorned
> +      // TemplateTypeParmTypes, so don't wrap them in AutoType.
> +      if (isa<TemplateTypeParmType>(Replacement)) {
> +        QualType Result = Replacement;
> +        TemplateTypeParmTypeLoc NewTL =
> TLB.push<TemplateTypeParmTypeLoc>(Result);
> +        NewTL.setNameLoc(TL.getNameLoc());
> +        return Result;
> +      } else {
> +        QualType Result = RebuildAutoType(Replacement);
> +        AutoTypeLoc NewTL = TLB.push<AutoTypeLoc>(Result);
> +        NewTL.setNameLoc(TL.getNameLoc());
> +        return Result;
> +      }
> +    }
> +  };
> +}
>
>
> Is the comment on the  "if" branch wrong? It seems that we wouldn't be
> performing template argument deduction using something that was
> originally an 'auto' type as the "P" in the deduction. However, it does
> seem that this "if" branch is critical for Sema::DeduceAutoType to work
> properly.

Yes, the comment is right. I've expanded it somewhat in the checked-in code:

// If we're building the type pattern to deduce against, don't wrap the
// substituted type in an AutoType. Certain template deduction rules
// apply only when a template type parameter appears directly (and not if
// the parameter is found through desugaring). For instance:
//   auto &&lref = lvalue;
// must transform into "rvalue reference to T" not "rvalue reference to
// auto type deduced as T" in order for [temp.deduct.call]p3 to apply.

> +/// \brief Deduce the type for an auto type-specifier (C++0x
> [dcl.spec.auto]p6)
> +///
> +/// \param Type the type pattern using the auto type-specifier.
> +///
> +/// \param Init the initializer for the variable whose type is to be
> deduced. +///
> +/// \param Result if type deduction was successful, this will be set to
> the +/// deduced type. This may still contain undeduced autos if the type
> is +/// dependent.
> +///
> +/// \returns true if deduction succeeded, false if it failed.
> +bool
> +Sema::DeduceAutoType(QualType Type, Expr *Init, QualType &Result) {
> +  if (Init->isTypeDependent()) {
> +    Result = Type;
> +    return true;
> +  }
> +
> +  SourceLocation Loc = Init->getExprLoc();
> +
> +  LocalInstantiationScope InstScope(*this);
> +
> +  // Build template<class TemplParam> void Func(FuncParam);
> +  NamedDecl *TemplParam
> +    = TemplateTypeParmDecl::Create(Context, 0, Loc, 0, 0, 0, false,
> false); +  TemplateParameterList *TemplateParams
> +    = TemplateParameterList::Create(Context, Loc, Loc, &TemplParam, 1,
> Loc);
> +
> +  QualType TemplArg = Context.getTemplateTypeParmType(0, 0, false);
> +  QualType FuncParam =
> +    SubstituteAutoTransform(*this, TemplArg).TransformType(Type);
> +
> +  // Deduce type of TemplParam in Func(Init)
> +  llvm::SmallVector<DeducedTemplateArgument, 1> Deduced;
> +  Deduced.resize(1);
> +  QualType InitType = Init->getType();
> +  unsigned TDF = 0;
> +  if (AdjustFunctionParmAndArgTypesForDeduction(*this, TemplateParams,
> +                                                FuncParam, InitType,
> Init,
> +                                                TDF))
> +    return false;
> +
> +  TemplateDeductionInfo Info(Context, Loc);
> +  if (::DeduceTemplateArguments(*this, TemplateParams,
> +                                FuncParam, InitType, Info, Deduced,
> +                                TDF))
> +    return false;
> +
> +  QualType DeducedType = Deduced[0].getAsType();
> +  if (DeducedType.isNull())
> +    return false;
> +
> +  Result = SubstituteAutoTransform(*this,
> DeducedType).TransformType(Type);
> +  return true;
> +}
> +
>
>
> 1-1 mapping between standard and implementation, I like it! I do wonder
> if we could reduce the memory footprint a bit by, e.g., allocating the
> template parameter list and template type parameter on the stack rather
> than in the never-to-be-freed ASTContext. This could be a follow-up
> optimization.

OK, I'll look into it.

> Index: lib/Sema/SemaTemplate.cpp
> ===================================================================
> --- lib/Sema/SemaTemplate.cpp	(revision 125795)
> +++ lib/Sema/SemaTemplate.cpp	(working copy)
> @@ -2778,6 +2778,10 @@
> return false; }
>
>
> +bool UnnamedLocalNoLinkageFinder::VisitAutoType(const AutoType*) {
> +  return false;
> +}
> +
>
>
> Shouldn't this visit the deduced type, if there is one?

Yes, but this code is unreachable in c++0x mode. I've changed it anyway,
in case auto-the-type-specifier is ever made available to c++03.

> Index: lib/Sema/SemaType.cpp
> ===================================================================
> --- lib/Sema/SemaType.cpp	(revision 125795)
> +++ lib/Sema/SemaType.cpp	(working copy)
> @@ -1631,6 +1615,32 @@
> D.setInvalidType(true);
> }
>
>
> +      // Check for auto functions and trailing return type and adjust
> the +      // return type accordingly.
> +      if (getLangOptions().CPlusPlus0x && !D.isInvalidType()) {
> +        // trailing-return-type is only required if we're declaring a
> function, +        // and not, for instance, a pointer to a function.
> +        if (D.getDeclSpec().getTypeSpecType() == DeclSpec::TST_auto &&
> +            !FTI.TrailingReturnType && chunkIndex == 0) {
> +          Diag(D.getDeclSpec().getTypeSpecTypeLoc(),
> +               diag::err_auto_missing_trailing_return);
> +          T = Context.IntTy;
> +          D.setInvalidType(true);
> +        } else if (FTI.TrailingReturnType) {
> +          if (T.hasQualifiers() || !isa<AutoType>(T)) {
> +            // T must be exactly 'auto' at this point. See CWG issue 681.
>  +            Diag(D.getDeclSpec().getTypeSpecTypeLoc(),
> +                 diag::err_trailing_return_without_auto)
> +              << T << D.getDeclSpec().getSourceRange();
> +            D.setInvalidType(true);
> +          }
> +
> +          T = GetTypeFromParser(
> +            ParsedType::getFromOpaquePtr(FTI.TrailingReturnType),
> +            &ReturnTypeInfo);
> +        }
> +      }
> +
>
>
> Only a trivial comment here: I know the check was there before, there's
> no need to check the CPlusPlus0x flag, since we won't see TST_auto or
> FTI.TrailingReturnType unless the parser wanted it. By not checking for
> C++0x here, we have the option of teaching the parser to be smarter about
> dialect issues, e.g., if it figures out that "auto" is being used as a
> type-specifier in C++98/03 code, it could warn and then parse it as a
> type-specifier.

Done.

> Index: lib/AST/ItaniumMangle.cpp
> ===================================================================
> --- lib/AST/ItaniumMangle.cpp	(revision 125795)
> +++ lib/AST/ItaniumMangle.cpp	(working copy)
> @@ -1313,9 +1313,6 @@
> assert(false && "Overloaded and dependent types shouldn't get to name
> mangling"); break; -  case BuiltinType::UndeducedAuto:
> -    assert(0 && "Should not see undeduced auto here");
> -    break;
> case BuiltinType::ObjCId: Out << "11objc_object"; break; case
> BuiltinType::ObjCClass: Out << "10objc_class"; break;
> case BuiltinType::ObjCSel: Out << "13objc_selector"; break; @@ -1648,6
> +1645,12 @@
> Out << 'E';
> }
>
>
> +void CXXNameMangler::mangleType(const AutoType *T) {
> +  QualType D = T->getDeducedType();
> +  assert(!D.isNull() && "can't mangle undeduced auto type");
> +  mangleType(D);
> +}
>
>
> Please make this an error (via the Diagnostics subsystem) rather than an
> assertion... I can think of evil ways to write code that would trip up
> here, which we should (but don't) diagnose as errors. I'd rather fail
> gracefully.

I'll tackle this as a follow-up. Can you suggest a test case?

> Index: lib/AST/ASTContext.cpp
> ===================================================================
> --- lib/AST/ASTContext.cpp	(revision 125795)
> +++ lib/AST/ASTContext.cpp	(working copy)
> @@ -2674,6 +2677,14 @@
> return QualType(dt, 0); }
>
>
> +/// getAutoType - Unlike many "get<Type>" functions, we don't unique
> +/// AutoType AST's.
> +QualType ASTContext::getAutoType(QualType DeducedType) const {
> +  AutoType *at = new (*this, TypeAlignment) AutoType(DeducedType);
> +  Types.push_back(at);
> +  return QualType(at, 0);
> +}
>
>
> Why not unique 'auto' types when we have a deduced type?
>
>
> I can understand not uniquing when we have a dependent initializer (so
> the 'auto' has no deduced type for a "long" time), since we don't want
> 'x' and 'y' to look like they have the same type in:
>
>
> template<typename T, typename U> void f() { auto x = T(); auto y = U(); }
>
>
> If we could unique in the common cases---where we have a deduced type, or
> where we have no deduced type and we know we're not anywhere inside a
> template---we could save ourselves some memory.

Uniquing in the deduced case seems straightforward. I'll put together a
patch for that. Uniquing in the not-yet-initialized case seems as
dangerous as uniquing in the template case: x and y should have distinct
types here:

auto x = ({ auto y =

> None of the tests seem to cover the pointer-to-member case, e.g.,
>
> struct X { int &f(int); };
>
> void test(X &x) { auto X::*fp = &X::f; int &ir = (x.*fp)(17); }
>
> even though it seems to work perfectly well.

I've added some tests for this case.

Thanks,
Richard