Merge pull request #18647 from slavapestov/inout-in-parens-is-still-inout

Easy InOutType removals and bug fix

Author: slavapestov

include/swift/AST/DiagnosticsSema.def

@@ -1064,16 +1064,6 @@ ERROR(c_function_pointer_from_function_with_context,none,
 // MARK: Type Check Declarations
 //------------------------------------------------------------------------------
 
-ERROR(var_type_not_materializable,none,
-      "variable has type %0 which includes nested inout parameters",
-      (Type))
-ERROR(param_type_non_materializable_tuple,none,
-      "named parameter has type %0 which includes nested inout parameters",
-      (Type))
-ERROR(enum_element_not_materializable,none,
-      "enum case has type %0 which includes nested inout parameters",
-      (Type))
-
 ERROR(missing_initializer_def,PointsToFirstBadToken,
       "initializer requires a body", ())
 

lib/SILGen/SILGenProlog.cpp

@@ -85,18 +85,19 @@ class EmitBBArguments : public CanTypeVisitor<EmitBBArguments,
   SILGenFunction &SGF;
   SILBasicBlock *parent;
   SILLocation loc;
-  bool functionArgs;
   ArrayRef<SILParameterInfo> &parameters;
 
-  EmitBBArguments(SILGenFunction &sgf, SILBasicBlock *parent,
-                  SILLocation l, bool functionArgs,
+  EmitBBArguments(SILGenFunction &sgf, SILBasicBlock *parent, SILLocation l,
                   ArrayRef<SILParameterInfo> &parameters)
-    : SGF(sgf), parent(parent), loc(l), functionArgs(functionArgs),
-      parameters(parameters) {}
+    : SGF(sgf), parent(parent), loc(l), parameters(parameters) {}
 
   ManagedValue visitType(CanType t) {
-    auto argType = SGF.getLoweredType(t->getInOutObjectType());
-    if (t->is<InOutType>())
+    return visitType(t, /*isInOut=*/false);
+  }
+
+  ManagedValue visitType(CanType t, bool isInOut) {
+    auto argType = SGF.getLoweredType(t);
+    if (isInOut)
       argType = SILType::getPrimitiveAddressType(argType.getASTType());
 
     // Pop the next parameter info.
@@ -112,16 +113,18 @@ class EmitBBArguments : public CanTypeVisitor<EmitBBArguments,
     ManagedValue mv = SGF.B.createInputFunctionArgument(
         argType, loc.getAsASTNode<ValueDecl>());
 
+    if (isInOut)
+      return mv;
+
     // If the value is a (possibly optional) ObjC block passed into the entry
     // point of the function, then copy it so we can treat the value reliably
     // as a heap object. Escape analysis can eliminate this copy if it's
     // unneeded during optimization.
     CanType objectType = t;
     if (auto theObjTy = t.getOptionalObjectType())
       objectType = theObjTy;
-    if (functionArgs
-        && isa<FunctionType>(objectType)
-        && cast<FunctionType>(objectType)->getRepresentation()
+    if (isa<FunctionType>(objectType) &&
+        cast<FunctionType>(objectType)->getRepresentation()
               == FunctionType::Representation::Block) {
       SILValue blockCopy = SGF.B.createCopyBlock(loc, mv.getValue());
       mv = SGF.emitManagedRValueWithCleanup(blockCopy);
@@ -208,13 +211,18 @@ struct ArgumentInitHelper {
 
   unsigned getNumArgs() const { return ArgNo; }
 
-  ManagedValue makeArgument(Type ty, SILBasicBlock *parent, SILLocation l) {
+  ManagedValue makeArgument(Type ty, bool isInOut, SILBasicBlock *parent,
+                            SILLocation l) {
     assert(ty && "no type?!");
 
     // Create an RValue by emitting destructured arguments into a basic block.
     CanType canTy = ty->eraseDynamicSelfType()->getCanonicalType();
-    return EmitBBArguments(SGF, parent, l, /*functionArgs*/ true,
-                           parameters).visit(canTy);
+    EmitBBArguments argEmitter(SGF, parent, l, parameters);
+
+    // Note: inouts of tuples are not exploded, so we bypass visit().
+    if (isInOut)
+      return argEmitter.visitType(canTy, /*isInOut=*/true);
+    return argEmitter.visit(canTy);
   }
 
   /// Create a SILArgument and store its value into the given Initialization,
@@ -223,23 +231,9 @@ struct ArgumentInitHelper {
     SILLocation loc(vd);
     loc.markAsPrologue();
 
-    ManagedValue argrv = makeArgument(ty, parent, loc);
+    ManagedValue argrv = makeArgument(ty, vd->isInOut(), parent, loc);
 
-    // Create a shadow copy of inout parameters so they can be captured
-    // by closures. The InOutDeshadowing guaranteed optimization will
-    // eliminate the variable if it is not needed.
     if (vd->isInOut()) {
-      SILValue address = argrv.getUnmanagedValue();
-
-      // As a special case, don't introduce a local variable for
-      // Builtin.UnsafeValueBuffer, which is not copyable.
-      if (isa<BuiltinUnsafeValueBufferType>(ty->getCanonicalType())) {
-        // FIXME: mark a debug location?
-        SGF.VarLocs[vd] = SILGenFunction::VarLoc::get(address);
-        SGF.B.createDebugValueAddr(loc, address,
-                                   SILDebugVariable(vd->isLet(), ArgNo));
-        return;
-      }
       assert(argrv.getType().isAddress() && "expected inout to be address");
     } else if (auto *metatypeTy = ty->getAs<MetatypeType>()) {
       // This is a hack to deal with the fact that Self.Type comes in as a
@@ -269,8 +263,8 @@ struct ArgumentInitHelper {
 
   void emitParam(ParamDecl *PD) {
     auto type = PD->getType();
-    if (PD->isInOut())
-      type = InOutType::get(type); // FIXME remove InOutType
+
+    assert(type->isMaterializable());
 
     ++ArgNo;
     if (PD->hasName()) {
@@ -282,26 +276,11 @@ struct ArgumentInitHelper {
   }
 
   void emitAnonymousParam(Type type, SILLocation paramLoc, ParamDecl *PD) {
-    // Allow non-materializable tuples to be bound to anonymous parameters.
-    //
-    // FIXME: Remove this once -swift-version 3 code generation goes away.
-    if (!type->isMaterializable()) {
-      if (auto tupleType = type->getAs<TupleType>()) {
-        for (auto eltType : tupleType->getElementTypes()) {
-          emitAnonymousParam(eltType, paramLoc, nullptr);
-        }
-        return;
-      }
-    }
-
     // A value bound to _ is unused and can be immediately released.
     Scope discardScope(SGF.Cleanups, CleanupLocation(PD));
 
     // Manage the parameter.
-    ManagedValue argrv = makeArgument(type, &*f.begin(), paramLoc);
-
-    // Don't do anything else if we don't have a parameter.
-    if (!PD) return;
+    auto argrv = makeArgument(type, PD->isInOut(), &*f.begin(), paramLoc);
 
     // Emit debug information for the argument.
     SILLocation loc(PD);

lib/Sema/TypeCheckConstraints.cpp

@@ -2419,27 +2419,7 @@ bool TypeChecker::typeCheckBinding(Pattern *&pattern, Expr *&initializer,
 
   auto resultTy = typeCheckExpression(initializer, DC, contextualType,
                                       contextualPurpose, flags, &listener);
-
-  // If we detected that the initializer would have a non-materializable type,
-  // complain.
-  if (resultTy && listener.isInOut()) {
-    diagnose(pattern->getStartLoc(), diag::var_type_not_materializable,
-             resultTy);
-
-    pattern->setType(ErrorType::get(Context));
-    pattern->forEachVariable([&](VarDecl *var) {
-      // Don't change the type of a variable that we've been able to
-      // compute a type for.
-      if (var->hasType() &&
-          !var->getType()->hasUnboundGenericType() &&
-          !var->getType()->hasError())
-        return;
-
-      var->markInvalid();
-    });
-
-    return true;
-  }
+  assert(!listener.isInOut());
 
   if (resultTy) {
     TypeResolutionOptions options = TypeResolutionFlags::OverrideType;

lib/Sema/TypeCheckDecl.cpp

@@ -4402,14 +4402,9 @@ void TypeChecker::validateDecl(ValueDecl *D) {
 
     EED->setSignatureIsValidated();
 
-    // Require the carried type to be materializable.
     if (auto argTy = EED->getArgumentInterfaceType()) {
-      assert(!argTy->hasLValueType() && "enum element cannot carry @lvalue");
-
-      if (!argTy->isMaterializable()) {
-        diagnose(EED->getLoc(), diag::enum_element_not_materializable, argTy);
-        EED->setInterfaceType(ErrorType::get(Context));
-      }
+      assert(argTy->isMaterializable());
+      (void) argTy;
     }
 
     break;

lib/Sema/TypeCheckPattern.cpp

@@ -789,19 +789,6 @@ static bool validateParameterType(ParamDecl *decl, DeclContext *DC,
     TL.setType(Ty);
   }
 
-  // If the user did not explicitly write 'let', 'var', or 'inout', we'll let
-  // type inference figure out what went wrong in detail.
-  if (decl->getSpecifierLoc().isValid()) {
-    // If the param is not a 'let' and it is not an 'inout'.
-    // It must be a 'var'. Provide helpful diagnostics like a shadow copy
-    // in the function body to fix the 'var' attribute.
-    if (!decl->isImmutable() && !decl->isImplicit() &&
-        (Ty.isNull() || !Ty->is<InOutType>()) && !hadError) {
-      decl->setInvalid();
-      hadError = true;
-    }
-  }
-
   if (hadError)
     TL.setInvalidType(TC.Context);
 
@@ -861,18 +848,26 @@ bool TypeChecker::typeCheckParameterList(ParameterList *PL, DeclContext *DC,
       param->markInvalid();
       hadError = true;
     } else {
-      if (type->is<InOutType>())
-        param->setSpecifier(VarDecl::Specifier::InOut);
-      param->setInterfaceType(type->getInOutObjectType());
+      param->setInterfaceType(type);
     }
 
     checkTypeModifyingDeclAttributes(param);
     if (!hadError) {
-      if (isa<InOutTypeRepr>(typeRepr)) {
+      auto *nestedRepr = typeRepr;
+
+      // Look through parens here; other than parens, specifiers
+      // must appear at the top level of a parameter type.
+      while (auto *tupleRepr = dyn_cast<TupleTypeRepr>(nestedRepr)) {
+        if (!tupleRepr->isParenType())
+          break;
+        nestedRepr = tupleRepr->getElementType(0);
+      }
+
+      if (isa<InOutTypeRepr>(nestedRepr)) {
         param->setSpecifier(VarDecl::Specifier::InOut);
-      } else if (isa<SharedTypeRepr>(typeRepr)) {
+      } else if (isa<SharedTypeRepr>(nestedRepr)) {
         param->setSpecifier(VarDecl::Specifier::Shared);
-      } else if (isa<OwnedTypeRepr>(typeRepr)) {
+      } else if (isa<OwnedTypeRepr>(nestedRepr)) {
         param->setSpecifier(VarDecl::Specifier::Owned);
       }
     }
@@ -1626,29 +1621,22 @@ bool TypeChecker::coerceParameterListToType(ParameterList *P, ClosureExpr *CE,
       // Coerce explicitly specified argument type to contextual type
       // only if both types are valid and do not match.
       if (!hadError && isValidType(ty) && !ty->isEqual(paramType)) {
-        assert(!param->isImmutable() || !ty->is<InOutType>());
-        param->setType(ty->getInOutObjectType());
-        param->setInterfaceType(ty->mapTypeOutOfContext()->getInOutObjectType());
+        param->setType(ty);
+        param->setInterfaceType(ty->mapTypeOutOfContext());
       }
     }
 
-    assert(!ty->hasLValueType() && "Bound param type to @lvalue?");
+    assert(ty->isMaterializable());
     if (forceMutable) {
       param->setSpecifier(VarDecl::Specifier::InOut);
-    } else if (auto *TTy = ty->getAs<TupleType>()) {
-      if (param->hasName() && TTy->hasInOutElement()) {
-        diagnose(param->getStartLoc(),
-                 diag::param_type_non_materializable_tuple, ty);
-      }
     }
 
     // If contextual type is invalid and we have a valid argument type
     // trying to coerce argument to contextual type would mean erasing
     // valuable diagnostic information.
     if (isValidType(ty) || shouldOverwriteParam(param)) {
-      assert(!param->isImmutable() || !ty->is<InOutType>());
-      param->setType(ty->getInOutObjectType());
-      param->setInterfaceType(ty->mapTypeOutOfContext()->getInOutObjectType());
+      param->setType(ty);
+      param->setInterfaceType(ty->mapTypeOutOfContext());
     }
 
     checkTypeModifyingDeclAttributes(param);
@@ -1667,9 +1655,6 @@ bool TypeChecker::coerceParameterListToType(ParameterList *P, ClosureExpr *CE,
   auto getType = [](const AnyFunctionType::Param &param) -> Type {
     auto type = param.getPlainType();
 
-    if (param.isInOut())
-      return InOutType::get(type);
-
     if (param.isVariadic())
       return ArraySliceType::get(type);
 

lib/Sema/TypeCheckType.cpp

@@ -2082,7 +2082,18 @@ bool TypeResolver::resolveASTFunctionTypeParams(
     }
 
     ValueOwnership ownership;
-    switch (eltTypeRepr->getKind()) {
+
+    auto *nestedRepr = eltTypeRepr;
+
+    // Look through parens here; other than parens, specifiers
+    // must appear at the top level of a parameter type.
+    while (auto *tupleRepr = dyn_cast<TupleTypeRepr>(nestedRepr)) {
+      if (!tupleRepr->isParenType())
+        break;
+      nestedRepr = tupleRepr->getElementType(0);
+    }
+
+    switch (nestedRepr->getKind()) {
     case TypeReprKind::Shared:
       ownership = ValueOwnership::Shared;
       break;
@@ -2098,7 +2109,7 @@ bool TypeResolver::resolveASTFunctionTypeParams(
     }
     ParameterTypeFlags paramFlags =
         ParameterTypeFlags::fromParameterType(ty, variadic, ownership);
-    elements.emplace_back(ty->getInOutObjectType(), Identifier(), paramFlags);
+    elements.emplace_back(ty, Identifier(), paramFlags);
   }
 
   return false;
@@ -2611,16 +2622,12 @@ Type TypeResolver::resolveSpecifierTypeRepr(SpecifierTypeRepr *repr,
     options -= TypeResolutionFlags::TypeAliasUnderlyingType;
   }
 
-  Type ty = resolveType(repr->getBase(), options);
-  if (!ty || ty->hasError()) return ty;
-  if (repr->getKind() == TypeReprKind::InOut) return InOutType::get(ty);
-  return ty;
+  return resolveType(repr->getBase(), options);
 }
 
 
 Type TypeResolver::resolveArrayType(ArrayTypeRepr *repr,
                                     TypeResolutionOptions options) {
-  // FIXME: diagnose non-materializability of element type!
   Type baseTy = resolveType(repr->getBase(), withoutContext(options));
   if (!baseTy || baseTy->hasError()) return baseTy;
 
@@ -2640,7 +2647,6 @@ Type TypeResolver::resolveDictionaryType(DictionaryTypeRepr *repr,
                                          TypeResolutionOptions options) {
   options = adjustOptionsForGenericArgs(options);
 
-  // FIXME: diagnose non-materializability of key/value type?
   Type keyTy = resolveType(repr->getKey(), withoutContext(options));
   if (!keyTy || keyTy->hasError()) return keyTy;
 
@@ -2761,8 +2767,7 @@ Type TypeResolver::resolveTupleType(TupleTypeRepr *repr,
     Type ty = resolveType(tyR, elementOptions);
     if (!ty || ty->hasError()) return ty;
 
-    elements.emplace_back(ty->getInOutObjectType(),
-                          repr->getElementName(i), ParameterTypeFlags());
+    elements.emplace_back(ty, repr->getElementName(i), ParameterTypeFlags());
   }
 
   // Single-element labeled tuples are not permitted outside of declarations

test/Constraints/closures.swift

@@ -712,3 +712,10 @@ extension Node {
       })!
   }
 }
+
+// Make sure we don't allow this anymore
+func takesTwo(_: (Int, Int) -> ()) {}
+func takesTwoInOut(_: (Int, inout Int) -> ()) {}
+
+takesTwo { _ in } // expected-error {{contextual closure type '(Int, Int) -> ()' expects 2 arguments, but 1 was used in closure body}}
+takesTwoInOut { _ in } // expected-error {{contextual closure type '(Int, inout Int) -> ()' expects 2 arguments, but 1 was used in closure body}}

test/decl/func/functions.swift

@@ -168,3 +168,14 @@ func testCurryFixits() {
 // Bogus diagnostic talking about a 'var' where there is none
 func invalidInOutParam(x: inout XYZ) {}
 // expected-error@-1{{use of undeclared type 'XYZ'}}
+
+// Parens around the 'inout'
+func parentheticalInout(_ x: ((inout Int))) {}
+
+var value = 0
+parentheticalInout(&value)
+
+func parentheticalInout2(_ fn: (((inout Int)), Int) -> ()) {
+  var value = 0
+  fn(&value, 0)
+}