Fix assertion failure code completing after nil literal

We were asserting (and doing the wrong thing) when trying to code
complete
    nil #^HERE^#

The issue is that we tried to apply a solution to the expression that
contained free type variables (converted to generic parameters). This
trips us up when we expect the new type to conform to protocols. In code
completion we generally only need the type of the expression, so this
commit switches to getting that more explicitly.  That said, this did
cause us to drop non-API parameter names in call patterns after an
opening '(' in some cases (covered by rdar://20962472).

Thanks to Doug for suggesting this solution!

rdar://problem/20891867

Swift SVN r28584

Author: benlangmuir

include/swift/Sema/CodeCompletionTypeChecking.h

@@ -33,11 +33,18 @@ namespace swift {
   /// \returns true on success, false on error.
   bool typeCheckCompletionDecl(Decl *D);
 
-  /// \brief Typecheck an expression parsed during code completion.
+  /// \brief Return the type of an expression parsed during code completion, or
+  /// None on error.
+  Optional<Type> getTypeOfCompletionContextExpr(ASTContext &Ctx,
+                                                DeclContext *DC,
+                                                Expr *&parsedExpr);
+
+  /// \brief Typecheck a single parsed expression.
   ///
   /// \returns true on success, false on error.
-  bool typeCheckCompletionContextExpr(ASTContext &Ctx, DeclContext *DC,
-                                      Expr *&parsedExpr);
+  // FIXME: this is not used by code completion; we should probably move it.
+  bool typeCheckContextExpr(ASTContext &Ctx, DeclContext *DC,
+                            Expr *&parsedExpr);
 
   /// Partially typecheck the specified function body.
   bool typeCheckAbstractFunctionBodyUntil(AbstractFunctionDecl *AFD,

lib/IDE/CodeCompletion.cpp

@@ -741,20 +741,18 @@ class CodeCompletionCallbacksImpl : public CodeCompletionCallbacks {
     return typeCheckCompletionDecl(DelayedParsedDecl);
   }
 
-  /// \returns true on success, false on failure.
-  bool typecheckParsedExpr() {
+  Optional<Type> getTypeOfParsedExpr() {
     assert(ParsedExpr && "should have an expression");
-
-    Expr *TypecheckedExpr = ParsedExpr;
-    if (!typeCheckCompletionContextExpr(P.Context, CurDeclContext,
-                                        TypecheckedExpr))
-      return false;
-
-    if (TypecheckedExpr->getType()->is<ErrorType>())
-      return false;
-
-    ParsedExpr = TypecheckedExpr;
-    return true;
+    Expr *ModifiedExpr = ParsedExpr;
+    if (auto T = getTypeOfCompletionContextExpr(P.Context, CurDeclContext,
+                                                ModifiedExpr)) {
+      // FIXME: even though we don't apply the solution, the type checker may
+      // modify the original expression. We should understand what effect that
+      // may have on code completion.
+      ParsedExpr = ModifiedExpr;
+      return T;
+    }
+    return None;
   }
 
   /// \returns true on success, false on failure.
@@ -2600,8 +2598,10 @@ void CodeCompletionCallbacksImpl::doneParsing() {
   if (auto *AFD = dyn_cast_or_null<AbstractFunctionDecl>(DelayedParsedDecl))
     CurDeclContext = AFD;
 
-  if (ParsedExpr && !typecheckParsedExpr()) {
-    if (Kind != CompletionKind::PostfixExprParen)
+  Optional<Type> ExprType;
+  if (ParsedExpr) {
+    ExprType = getTypeOfParsedExpr();
+    if (!ExprType && Kind != CompletionKind::PostfixExprParen)
       return;
   }
 
@@ -2625,7 +2625,7 @@ void CodeCompletionCallbacksImpl::doneParsing() {
 
   case CompletionKind::DotExpr: {
     Lookup.setHaveDot(DotLoc);
-    Type OriginalType = ParsedExpr->getType();
+    Type OriginalType = *ExprType;
     Type ExprType = OriginalType;
 
     // If there is no nominal type in the expr, try to find nominal types
@@ -2651,10 +2651,9 @@ void CodeCompletionCallbacksImpl::doneParsing() {
   }
 
   case CompletionKind::PostfixExpr: {
-    Type ExprType = ParsedExpr->getType();
-    if (isDynamicLookup(ExprType))
+    if (isDynamicLookup(*ExprType))
       Lookup.setIsDynamicLookup();
-    Lookup.getValueExprCompletions(ExprType);
+    Lookup.getValueExprCompletions(*ExprType);
     break;
   }
 
@@ -2665,8 +2664,8 @@ void CodeCompletionCallbacksImpl::doneParsing() {
       if (auto *DRE = dyn_cast<DeclRefExpr>(AE->getFn()))
         VD = DRE->getDecl();
     }
-    if (ParsedExpr->getType())
-      Lookup.getValueExprCompletions(ParsedExpr->getType(), VD);
+    if (ExprType)
+      Lookup.getValueExprCompletions(*ExprType, VD);
 
     if (!Lookup.FoundFunctionCalls ||
         (Lookup.FoundFunctionCalls &&
@@ -2679,14 +2678,14 @@ void CodeCompletionCallbacksImpl::doneParsing() {
 
   case CompletionKind::SuperExpr: {
     Lookup.setIsSuperRefExpr();
-    Lookup.getValueExprCompletions(ParsedExpr->getType());
+    Lookup.getValueExprCompletions(*ExprType);
     break;
   }
 
   case CompletionKind::SuperExprDot: {
     Lookup.setIsSuperRefExpr();
     Lookup.setHaveDot(SourceLoc());
-    Lookup.getValueExprCompletions(ParsedExpr->getType());
+    Lookup.getValueExprCompletions(*ExprType);
     break;
   }
 

lib/Sema/PlaygroundTransform.cpp

@@ -818,7 +818,7 @@ class Instrumenter {
     Expr *SendDataCall = new (Context) CallExpr(SendDataRef, SendDataArgs, true,
                                                 Type());
 
-    if (!typeCheckCompletionContextExpr(Context, TypeCheckDC, SendDataCall)) {
+    if (!typeCheckContextExpr(Context, TypeCheckDC, SendDataCall)) {
       return nullptr;
     }
 

lib/Sema/TypeCheckConstraints.cpp

@@ -860,29 +860,21 @@ static void diagnoseExpr(TypeChecker &TC, const Expr *E,
   performExprDiagnostics(TC, E, DC);
 }
 
-#pragma mark High-level entry points
-bool TypeChecker::typeCheckExpression(
-       Expr *&expr, DeclContext *dc,
-       Type convertType,
-       Type contextualType,
-       bool discardedExpr,
-       FreeTypeVariableBinding allowFreeTypeVariables,
-       ExprTypeCheckListener *listener) {
-  PrettyStackTraceExpr stackTrace(Context, "type-checking", expr);
+bool TypeChecker::solveForExpression(
+    Expr *&expr, DeclContext *dc, Type convertType, Type contextualType,
+    bool discardedExpr, FreeTypeVariableBinding allowFreeTypeVariables,
+    ExprTypeCheckListener *listener, ConstraintSystem &cs,
+    SmallVectorImpl<Solution> &viable) {
 
   // First, pre-check the expression, validating any types that occur in the
   // expression and folding sequence expressions.
   if (preCheckExpression(*this, expr, dc))
     return true;
 
-  // Construct a constraint system from this expression.
-  ConstraintSystem cs(*this, dc, ConstraintSystemFlags::AllowFixes);
-  
   if (!contextualType.isNull()) {
     cs.setContextualType(expr, &contextualType);
   }
-  
-  CleanupIllFormedExpressionRAII cleanup(cs, expr);
+
   if (auto generatedExpr = cs.generateConstraints(expr))
     expr = generatedExpr;
   else {
@@ -911,7 +903,6 @@ bool TypeChecker::typeCheckExpression(
   }
 
   // Attempt to solve the constraint system.
-  SmallVector<Solution, 4> viable;
   if (cs.solve(viable, allowFreeTypeVariables)) {
     if (listener && listener->suppressDiagnostics())
       return true;
@@ -937,7 +928,31 @@ bool TypeChecker::typeCheckExpression(
     listener->solvedConstraints(solution);
   }
 
+  return false;
+}
+
+#pragma mark High-level entry points
+bool TypeChecker::typeCheckExpression(
+       Expr *&expr, DeclContext *dc,
+       Type convertType,
+       Type contextualType,
+       bool discardedExpr,
+       FreeTypeVariableBinding allowFreeTypeVariables,
+       ExprTypeCheckListener *listener) {
+  PrettyStackTraceExpr stackTrace(Context, "type-checking", expr);
+
+  // Construct a constraint system from this expression.
+  ConstraintSystem cs(*this, dc, ConstraintSystemFlags::AllowFixes);
+  CleanupIllFormedExpressionRAII cleanup(cs, expr);
+
+  // Attempt to solve the constraint system.
+  SmallVector<Solution, 4> viable;
+  if (solveForExpression(expr, dc, convertType, contextualType, discardedExpr,
+                         allowFreeTypeVariables, listener, cs, viable))
+    return true;
+
   // Apply the solution to the expression.
+  auto &solution = viable[0];
   auto result = cs.applySolution(solution, expr, convertType, discardedExpr,
                                  listener && listener->suppressDiagnostics());
   if (!result) {
@@ -968,6 +983,33 @@ bool TypeChecker::typeCheckExpression(
   return false;
 }
 
+Optional<Type> TypeChecker::getTypeOfExpressionWithoutApplying(
+    Expr *&expr, DeclContext *dc, Type convertType, Type contextualType,
+    bool discardedExpr, FreeTypeVariableBinding allowFreeTypeVariables,
+    ExprTypeCheckListener *listener) {
+  PrettyStackTraceExpr stackTrace(Context, "type-checking", expr);
+
+  // Construct a constraint system from this expression.
+  ConstraintSystem cs(*this, dc, ConstraintSystemFlags::AllowFixes);
+  CleanupIllFormedExpressionRAII cleanup(cs, expr);
+
+  // Attempt to solve the constraint system.
+  SmallVector<Solution, 4> viable;
+  if (solveForExpression(expr, dc, convertType, contextualType, discardedExpr,
+                         allowFreeTypeVariables, listener, cs, viable))
+    return None;
+
+  // Get the expression's simplified type.
+  auto &solution = viable[0];
+  Type exprType = solution.simplifyType(*this, expr->getType());
+
+  assert(exprType && !exprType->is<ErrorType>() && "erroneous solution?");
+  assert(!exprType->hasTypeVariable() &&
+         "free type variable with FreeTypeVariableBinding::GenericParameters?");
+
+  return exprType;
+}
+
 /// Private class to "cleanse" an expression tree of types. This is done in the
 /// case of a typecheck failure, where we may want to re-typecheck partially-
 /// typechecked subexpressions in a context-free manner.

lib/Sema/TypeChecker.cpp

@@ -674,8 +674,31 @@ bool swift::typeCheckCompletionDecl(Decl *D) {
   return true;
 }
 
-bool swift::typeCheckCompletionContextExpr(ASTContext &Ctx, DeclContext *DC,
-                                           Expr *&parsedExpr) {
+/// \brief Return the type of an expression parsed during code completion, or
+/// a null \c Type on error.
+Optional<Type> swift::getTypeOfCompletionContextExpr(ASTContext &Ctx,
+                                                     DeclContext *DC,
+                                                     Expr *&parsedExpr) {
+  // Set up a diagnostics engine that swallows diagnostics.
+  DiagnosticEngine diags(Ctx.SourceMgr);
+
+  TypeChecker TC(Ctx, diags);
+  // Try to solve for the actual type of the expression.
+  if (auto T = TC.getTypeOfExpressionWithoutApplying(
+          parsedExpr, DC, Type(), Type(), /*discardedExpr=*/true,
+          FreeTypeVariableBinding::GenericParameters))
+    return T;
+
+  // Try to recover by using the original unchecked type.
+  if (parsedExpr && !isa<ErrorExpr>(parsedExpr) && parsedExpr->getType() &&
+      !parsedExpr->getType()->is<ErrorType>())
+    return parsedExpr->getType();
+
+  return None;
+}
+
+bool swift::typeCheckContextExpr(ASTContext &Ctx, DeclContext *DC,
+                                 Expr *&parsedExpr) {
   // Set up a diagnostics engine that swallows diagnostics.
   DiagnosticEngine diags(Ctx.SourceMgr);
 

lib/Sema/TypeChecker.h

@@ -869,6 +869,19 @@ class TypeChecker final : public LazyResolver {
   /// which must be lazy.
   void completeLazyVarImplementation(VarDecl *lazyVar);
 
+  /// Sets up and solves the constraint system \p cs to type check the given
+  /// expression.
+  ///
+  /// \returns true if an error occurred, false otherwise.
+  ///
+  /// \see typeCheckExpression
+  bool solveForExpression(Expr *&expr, DeclContext *dc, Type convertType,
+                          Type contextualType, bool discardedExpr,
+                          FreeTypeVariableBinding allowFreeTypeVariables,
+                          ExprTypeCheckListener *listener,
+                          constraints::ConstraintSystem &cs,
+                          SmallVectorImpl<constraints::Solution> &viable);
+
   /// \name Name lookup
   ///
   /// Routines that perform name lookup.
@@ -881,7 +894,7 @@ class TypeChecker final : public LazyResolver {
   /// @{
 private:
   Optional<Type> boolType;
-  
+
 public:
   /// \brief Define the default constructor for the given struct or class.
   void defineDefaultConstructor(NominalTypeDecl *decl);
@@ -922,6 +935,37 @@ class TypeChecker final : public LazyResolver {
                              = FreeTypeVariableBinding::Disallow,
                            ExprTypeCheckListener *listener = nullptr);
 
+  /// \brief Type check the given expression and return its type without
+  /// applying the solution.
+  ///
+  /// \param expr The expression to type-check.
+  ///
+  /// \param convertType The type that the expression is being converted to,
+  /// or null if the expression is standalone.
+  ///
+  /// \param contextualType Contextual type information that can be applied to
+  /// the expression. (This is distinct from convertType in that while it can
+  /// inform the type of the expression, it won't result in a conversion
+  /// constraint being applied to the expression.)
+  ///
+  /// \param discardedExpr True if the result of this expression will be
+  /// discarded.
+  ///
+  /// \param allowFreeTypeVariables Whether free type variables are allowed in
+  /// the solution, and what to do with them.
+  ///
+  /// \param listener If non-null, a listener that will be notified of important
+  /// events in the type checking of this expression, and which can introduce
+  /// additional constraints.
+  ///
+  /// \returns the type of \p expr on success, None otherwise.
+  /// FIXME: expr may still be modified...
+  Optional<Type> getTypeOfExpressionWithoutApplying(
+      Expr *&expr, DeclContext *dc, Type convertType, Type contextualType,
+      bool discardedExpr, FreeTypeVariableBinding allowFreeTypeVariables =
+                              FreeTypeVariableBinding::Disallow,
+      ExprTypeCheckListener *listener = nullptr);
+
   /// \brief Type check the given expression assuming that its children
   /// have already been fully type-checked.
   ///