In blocks, only pretend that enum constants have enum type if necessary.

In C, enum constants have the type of the enum's underlying integer type,
rather than the type of the enum. (This is not true in C++.) Thus, when a
block's return type is inferred from an enum constant, it is incompatible
with expressions that return the enum type.

In r158899, I told block returns to pretend that enum constants have enum
type, like in C++. Doug Gregor pointed out that this can break existing code.

Now, we don't check the types of return statements until the end of the block.
This lets us go back and add implicit casts in blocks with mixed enum
constants and enum-typed expressions.

<rdar://problem/11662489> (again)

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@159591 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/Sema/ScopeInfo.h

@@ -93,7 +93,7 @@ public:
 
   /// \brief The list of return statements that occur within the function or
   /// block, if there is any chance of applying the named return value
-  /// optimization.
+  /// optimization, or if we need to infer a return type.
   SmallVector<ReturnStmt*, 4> Returns;
 
   /// \brief The stack of currently active compound stamement scopes in the

include/clang/Sema/Sema.h

@@ -168,6 +168,7 @@ namespace clang {
 namespace sema {
   class AccessedEntity;
   class BlockScopeInfo;
+  class CapturingScopeInfo;
   class CompoundScopeInfo;
   class DelayedDiagnostic;
   class DelayedDiagnosticPool;
@@ -4016,6 +4017,10 @@ public:
   
   /// \brief Introduce the lambda parameters into scope.
   void addLambdaParameters(CXXMethodDecl *CallOperator, Scope *CurScope);
+
+  /// \brief Deduce a block or lambda's return type based on the return
+  /// statements present in the body.
+  void deduceClosureReturnType(sema::CapturingScopeInfo &CSI);
   
   /// ActOnStartOfLambdaDefinition - This is called just before we start
   /// parsing the body of a lambda; it analyzes the explicit captures and 

lib/Sema/SemaDecl.cpp

@@ -7659,7 +7659,12 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
       if (CXXConstructorDecl *Constructor = dyn_cast<CXXConstructorDecl>(FD))
         MarkVTableUsed(FD->getLocation(), Constructor->getParent());
       
-      computeNRVO(Body, getCurFunction());
+      // Try to apply the named return value optimization. We have to check
+      // if we can do this here because lambdas keep return statements around
+      // to deduce an implicit return type.
+      if (getLangOpts().CPlusPlus && FD->getResultType()->isRecordType() &&
+          !FD->isDependentContext())
+        computeNRVO(Body, getCurFunction());
     }
     
     assert((FD == getCurFunctionDecl() || getCurLambda()->CallOperator == FD) &&

lib/Sema/SemaExpr.cpp

@@ -9350,7 +9350,10 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
   PopExpressionEvaluationContext();
 
   BlockScopeInfo *BSI = cast<BlockScopeInfo>(FunctionScopes.back());
-  
+
+  if (BSI->HasImplicitReturnType)
+    deduceClosureReturnType(*BSI);
+
   PopDeclContext();
 
   QualType RetTy = Context.VoidTy;
@@ -9423,7 +9426,12 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
 
   BSI->TheDecl->setBody(cast<CompoundStmt>(Body));
 
-  computeNRVO(Body, getCurBlock());
+  // Try to apply the named return value optimization. We have to check again
+  // if we can do this, though, because blocks keep return statements around
+  // to deduce an implicit return type.
+  if (getLangOpts().CPlusPlus && RetTy->isRecordType() &&
+      !BSI->TheDecl->isDependentContext())
+    computeNRVO(Body, getCurBlock());
   
   BlockExpr *Result = new (Context) BlockExpr(BSI->TheDecl, BlockTy);
   const AnalysisBasedWarnings::Policy &WP = AnalysisWarnings.getDefaultPolicy();

lib/Sema/SemaLambda.cpp

@@ -214,6 +214,141 @@ void Sema::addLambdaParameters(CXXMethodDecl *CallOperator, Scope *CurScope) {
   }
 }
 
+static bool checkReturnValueType(const ASTContext &Ctx, const Expr *E,
+                                 QualType &DeducedType,
+                                 QualType &AlternateType) {
+  // Handle ReturnStmts with no expressions.
+  if (!E) {
+    if (AlternateType.isNull())
+      AlternateType = Ctx.VoidTy;
+
+    return Ctx.hasSameType(DeducedType, Ctx.VoidTy);
+  }
+
+  QualType StrictType = E->getType();
+  QualType LooseType = StrictType;
+
+  // In C, enum constants have the type of their underlying integer type,
+  // not the enum. When inferring block return types, we should allow
+  // the enum type if an enum constant is used, unless the enum is
+  // anonymous (in which case there can be no variables of its type).
+  if (!Ctx.getLangOpts().CPlusPlus) {
+    const DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E->IgnoreParenImpCasts());
+    if (DRE) {
+      const Decl *D = DRE->getDecl();
+      if (const EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(D)) {
+        const EnumDecl *Enum = cast<EnumDecl>(ECD->getDeclContext());
+        if (Enum->getDeclName() || Enum->getTypedefNameForAnonDecl())
+          LooseType = Ctx.getTypeDeclType(Enum);
+      }
+    }
+  }
+
+  // Special case for the first return statement we find.
+  // The return type has already been tentatively set, but we might still
+  // have an alternate type we should prefer.
+  if (AlternateType.isNull())
+    AlternateType = LooseType;
+
+  if (Ctx.hasSameType(DeducedType, StrictType)) {
+    // FIXME: The loose type is different when there are constants from two
+    // different enums. We could consider warning here.
+    if (AlternateType != Ctx.DependentTy)
+      if (!Ctx.hasSameType(AlternateType, LooseType))
+        AlternateType = Ctx.VoidTy;
+    return true;
+  }
+
+  if (Ctx.hasSameType(DeducedType, LooseType)) {
+    // Use DependentTy to signal that we're using an alternate type and may
+    // need to add casts somewhere.
+    AlternateType = Ctx.DependentTy;
+    return true;
+  }
+
+  if (Ctx.hasSameType(AlternateType, StrictType) ||
+      Ctx.hasSameType(AlternateType, LooseType)) {
+    DeducedType = AlternateType;
+    // Use DependentTy to signal that we're using an alternate type and may
+    // need to add casts somewhere.
+    AlternateType = Ctx.DependentTy;
+    return true;
+  }
+
+  return false;
+}
+
+void Sema::deduceClosureReturnType(CapturingScopeInfo &CSI) {
+  assert(CSI.HasImplicitReturnType);
+
+  // First case: no return statements, implicit void return type.
+  ASTContext &Ctx = getASTContext();
+  if (CSI.Returns.empty()) {
+    // It's possible there were simply no /valid/ return statements.
+    // In this case, the first one we found may have at least given us a type.
+    if (CSI.ReturnType.isNull())
+      CSI.ReturnType = Ctx.VoidTy;
+    return;
+  }
+
+  // Second case: at least one return statement has dependent type.
+  // Delay type checking until instantiation.
+  assert(!CSI.ReturnType.isNull() && "We should have a tentative return type.");
+  if (CSI.ReturnType->isDependentType())
+    return;
+
+  // Third case: only one return statement. Don't bother doing extra work!
+  SmallVectorImpl<ReturnStmt*>::iterator I = CSI.Returns.begin(),
+                                         E = CSI.Returns.end();
+  if (I+1 == E)
+    return;
+
+  // General case: many return statements.
+  // Check that they all have compatible return types.
+  // For now, that means "identical", with an exception for enum constants.
+  // (In C, enum constants have the type of their underlying integer type,
+  // not the type of the enum. C++ uses the type of the enum.)
+  QualType AlternateType;
+
+  // We require the return types to strictly match here.
+  for (; I != E; ++I) {
+    const ReturnStmt *RS = *I;
+    const Expr *RetE = RS->getRetValue();
+    if (!checkReturnValueType(Ctx, RetE, CSI.ReturnType, AlternateType)) {
+      // FIXME: This is a poor diagnostic for ReturnStmts without expressions.
+      Diag(RS->getLocStart(),
+           diag::err_typecheck_missing_return_type_incompatible)
+        << (RetE ? RetE->getType() : Ctx.VoidTy) << CSI.ReturnType
+        << isa<LambdaScopeInfo>(CSI);
+      // Don't bother fixing up the return statements in the block if some of
+      // them are unfixable anyway.
+      AlternateType = Ctx.VoidTy;
+      // Continue iterating so that we keep emitting diagnostics.
+    }
+  }
+
+  // If our return statements turned out to be compatible, but we needed to
+  // pick a different return type, go through and fix the ones that need it.
+  if (AlternateType == Ctx.DependentTy) {
+    for (SmallVectorImpl<ReturnStmt*>::iterator I = CSI.Returns.begin(),
+                                                E = CSI.Returns.end();
+         I != E; ++I) {
+      ReturnStmt *RS = *I;
+      Expr *RetE = RS->getRetValue();
+      if (RetE->getType() == CSI.ReturnType)
+        continue;
+
+      // Right now we only support integral fixup casts.
+      assert(CSI.ReturnType->isIntegralOrUnscopedEnumerationType());
+      assert(RetE->getType()->isIntegralOrUnscopedEnumerationType());
+      ExprResult Casted = ImpCastExprToType(RetE, CSI.ReturnType,
+                                            CK_IntegralCast);
+      assert(Casted.isUsable());
+      RS->setRetValue(Casted.take());
+    }
+  }
+}
+
 void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
                                         Declarator &ParamInfo,
                                         Scope *CurScope) {
@@ -659,6 +794,8 @@ ExprResult Sema::ActOnLambdaExpr(SourceLocation StartLoc, Stmt *Body,
     //   denotes the following type:
     // FIXME: Assumes current resolution to core issue 975.
     if (LSI->HasImplicitReturnType) {
+      deduceClosureReturnType(*LSI);
+
       //   - if there are no return statements in the
       //     compound-statement, or all return statements return
       //     either an expression of type void or no expression or

lib/Sema/SemaStmt.cpp

@@ -2120,40 +2120,22 @@ Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
   // [expr.prim.lambda]p4 in C++11; block literals follow a superset of those
   // rules which allows multiple return statements.
   CapturingScopeInfo *CurCap = cast<CapturingScopeInfo>(getCurFunction());
+  QualType FnRetType = CurCap->ReturnType;
+
+  // For blocks/lambdas with implicit return types, we check each return
+  // statement individually, and deduce the common return type when the block
+  // or lambda is completed.
   if (CurCap->HasImplicitReturnType) {
-    QualType ReturnT;
     if (RetValExp && !isa<InitListExpr>(RetValExp)) {
       ExprResult Result = DefaultFunctionArrayLvalueConversion(RetValExp);
       if (Result.isInvalid())
         return StmtError();
       RetValExp = Result.take();
 
-      if (!RetValExp->isTypeDependent()) {
-        ReturnT = RetValExp->getType();
-
-        // In C, enum constants have the type of their underlying integer type,
-        // not the enum. When inferring block return values, we should infer
-        // the enum type if an enum constant is used, unless the enum is
-        // anonymous (in which case there can be no variables of its type).
-        if (!getLangOpts().CPlusPlus) {
-          Expr *InsideExpr = RetValExp->IgnoreParenImpCasts();
-          if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(InsideExpr)) {
-            Decl *D = DRE->getDecl();
-            if (EnumConstantDecl *ECD = dyn_cast<EnumConstantDecl>(D)) {
-              EnumDecl *Enum = cast<EnumDecl>(ECD->getDeclContext());
-              if (Enum->getDeclName() || Enum->getTypedefNameForAnonDecl()) {
-                ReturnT = Context.getTypeDeclType(Enum);
-                ExprResult Casted = ImpCastExprToType(RetValExp, ReturnT,
-                                                      CK_IntegralCast);
-                assert(Casted.isUsable());
-                RetValExp = Casted.take();
-              }
-            }
-          }
-        }
-      } else {
-        ReturnT = Context.DependentTy;
-      }
+      if (!RetValExp->isTypeDependent())
+        FnRetType = RetValExp->getType();
+      else
+        FnRetType = CurCap->ReturnType = Context.DependentTy;
     } else { 
       if (RetValExp) {
         // C++11 [expr.lambda.prim]p4 bans inferring the result from an
@@ -2163,21 +2145,14 @@ Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
           << RetValExp->getSourceRange();
       }
 
-      ReturnT = Context.VoidTy;
+      FnRetType = Context.VoidTy;
     }
-    // We require the return types to strictly match here.
-    if (!CurCap->ReturnType.isNull() &&
-        !CurCap->ReturnType->isDependentType() &&
-        !ReturnT->isDependentType() &&
-        !Context.hasSameType(ReturnT, CurCap->ReturnType)) {
-      Diag(ReturnLoc, diag::err_typecheck_missing_return_type_incompatible) 
-          << ReturnT << CurCap->ReturnType
-          << (getCurLambda() != 0);
-      return StmtError();
-    }
-    CurCap->ReturnType = ReturnT;
+
+    // Although we'll properly infer the type of the block once it's completed,
+    // make sure we provide a return type now for better error recovery.
+    if (CurCap->ReturnType.isNull())
+      CurCap->ReturnType = FnRetType;
   }
-  QualType FnRetType = CurCap->ReturnType;
   assert(!FnRetType.isNull());
 
   if (BlockScopeInfo *CurBlock = dyn_cast<BlockScopeInfo>(CurCap)) {
@@ -2245,10 +2220,12 @@ Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
   ReturnStmt *Result = new (Context) ReturnStmt(ReturnLoc, RetValExp,
                                                 NRVOCandidate);
 
-  // If we need to check for the named return value optimization, save the
-  // return statement in our scope for later processing.
-  if (getLangOpts().CPlusPlus && FnRetType->isRecordType() && 
-      !CurContext->isDependentContext())
+  // If we need to check for the named return value optimization,
+  // or if we need to infer the return type,
+  // save the return statement in our scope for later processing.
+  if (CurCap->HasImplicitReturnType ||
+      (getLangOpts().CPlusPlus && FnRetType->isRecordType() &&
+       !CurContext->isDependentContext()))
     FunctionScopes.back()->Returns.push_back(Result);
 
   return Owned(Result);

test/SemaObjC/blocks.m

@@ -76,8 +76,7 @@ void foo10() {
 
 // In C, enum constants have the type of the underlying integer type, not the
 // enumeration they are part of. We pretend the constants have enum type when
-// inferring block return types, so that they can be mixed-and-matched with
-// other expressions of enum type.
+// they are mixed with other expressions of enum type.
 enum CStyleEnum {
   CSE_Value = 1
 };
@@ -88,37 +87,32 @@ void testCStyleEnumInference(bool arg) {
   cse_block_t a;
 
   // No warnings here.
-  a = ^{ return CSE_Value; };
   a = ^{ return getCSE(); };
 
   a = ^{ // expected-error {{incompatible block pointer types assigning to 'cse_block_t' (aka 'enum CStyleEnum (^)()') from 'int (^)(void)'}}
     return 1;
   };
+  a = ^{ // expected-error {{incompatible block pointer types assigning to 'cse_block_t' (aka 'enum CStyleEnum (^)()') from 'int (^)(void)'}}
+    return CSE_Value;
+  };
 
   // No warnings here.
-  a = ^{ if (arg) return CSE_Value; else return CSE_Value; };
-  a = ^{ if (arg) return getCSE();  else return getCSE();  };
   a = ^{ if (arg) return CSE_Value; else return getCSE();  };
   a = ^{ if (arg) return getCSE();  else return CSE_Value; };
 
-  // Technically these two blocks should return 'int'.
-  // The first case is easy to handle -- just don't cast the enum constant
-  // to the enum type. However, the second guess would require going back
-  // and REMOVING the cast from the first return statement, which isn't really
-  // feasible (there may be more than one previous return statement with enum
-  // type). For symmetry, we just treat them the same way.
+  // These two blocks actually return 'int'
   a = ^{ // expected-error {{incompatible block pointer types assigning to 'cse_block_t' (aka 'enum CStyleEnum (^)()') from 'int (^)(void)'}}
     if (arg)
       return 1;
     else
-      return CSE_Value; // expected-error {{return type 'enum CStyleEnum' must match previous return type 'int'}}
+      return CSE_Value;
   };
 
-  a = ^{
+  a = ^{ // expected-error {{incompatible block pointer types assigning to 'cse_block_t' (aka 'enum CStyleEnum (^)()') from 'int (^)(void)'}}
     if (arg)
       return CSE_Value;
     else
-      return 1; // expected-error {{return type 'int' must match previous return type 'enum CStyleEnum'}}
+      return 1;
   };
 }
 
@@ -133,7 +127,6 @@ void testFixedTypeEnumInference(bool arg) {
   fte_block_t a;
   
   // No warnings here.
-  a = ^{ return FTE_Value; };
   a = ^{ return getFTE(); };
 
   // Since we fixed the underlying type of the enum, this is considered a
@@ -141,31 +134,29 @@ void testFixedTypeEnumInference(bool arg) {
   a = ^{
     return 1U;
   };
-  
+  a = ^{
+    return FTE_Value;
+  };
+
   // No warnings here.
   a = ^{ if (arg) return FTE_Value; else return FTE_Value; };
   a = ^{ if (arg) return getFTE();  else return getFTE();  };
   a = ^{ if (arg) return FTE_Value; else return getFTE();  };
   a = ^{ if (arg) return getFTE();  else return FTE_Value; };
   
-  // Technically these two blocks should return 'unsigned'.
-  // The first case is easy to handle -- just don't cast the enum constant
-  // to the enum type. However, the second guess would require going back
-  // and REMOVING the cast from the first return statement, which isn't really
-  // feasible (there may be more than one previous return statement with enum
-  // type). For symmetry, we just treat them the same way.
+  // These two blocks actually return 'unsigned'.
   a = ^{
     if (arg)
       return 1U;
     else
-      return FTE_Value; // expected-error{{return type 'enum FixedTypeEnum' must match previous return type 'unsigned int'}}
+      return FTE_Value;
   };
   
   a = ^{
     if (arg)
       return FTE_Value;
     else
-      return 1U; // expected-error{{return type 'unsigned int' must match previous return type 'enum FixedTypeEnum'}}
+      return 1U;
   };
 }
 
@@ -181,22 +172,25 @@ enum : short {
 typedef enum {
   TDE_Value
 } TypeDefEnum;
+TypeDefEnum getTDE();
 
 typedef enum : short {
   TDFTE_Value
 } TypeDefFixedTypeEnum;
-
+TypeDefFixedTypeEnum getTDFTE();
 
 typedef int (^int_block_t)();
 typedef short (^short_block_t)();
-void testAnonymousEnumTypes() {
+void testAnonymousEnumTypes(int arg) {
   int_block_t IB;
   IB = ^{ return AnonymousValue; };
-  IB = ^{ return TDE_Value; }; // expected-error {{incompatible block pointer types assigning to 'int_block_t' (aka 'int (^)()') from 'TypeDefEnum (^)(void)'}}
-  IB = ^{ return CSE_Value; }; // expected-error {{incompatible block pointer types assigning to 'int_block_t' (aka 'int (^)()') from 'enum CStyleEnum (^)(void)'}}
+  IB = ^{ if (arg) return TDE_Value; else return getTDE(); }; // expected-error {{incompatible block pointer}}
+  IB = ^{ if (arg) return getTDE(); else return TDE_Value; }; // expected-error {{incompatible block pointer}}
 
+  // Since we fixed the underlying type of the enum, these are considered
+  // compatible block types anyway.
   short_block_t SB;
   SB = ^{ return FixedAnonymousValue; };
-  // This is not an error anyway since the enum has a fixed underlying type.
-  SB = ^{ return TDFTE_Value; };
+  SB = ^{ if (arg) return TDFTE_Value; else return getTDFTE(); };
+  SB = ^{ if (arg) return getTDFTE(); else return TDFTE_Value; };
 }