Diagnose inout uses of 'lets' in constructors in the type checker.

Stored `let` properties of a struct, class, or actor permit
'inout' modification within the constructor body after they have been
initialized. Tentatively remove this rule, only allowing such `let`
properties to be initialized (assigned to) and not treated as `inout`.

Fixes rdar://127258363.

Author: DougGregor

include/swift/Sema/ConstraintSystem.h

@@ -2092,6 +2092,14 @@ struct ClosureIsolatedByPreconcurrency {
   bool operator()(const ClosureExpr *expr) const;
 };
 
+/// Determine whether the given expression is part of the left-hand side
+/// of an assignment expression.
+struct IsInLeftHandSideOfAssignment {
+  ConstraintSystem &cs;
+
+  bool operator()(Expr *expr) const;
+};
+
 /// Describes the type produced when referencing a declaration.
 struct DeclReferenceType {
   /// The "opened" type, which is the type of the declaration where any
@@ -4421,7 +4429,7 @@ class ConstraintSystem {
   /// \param wantInterfaceType Whether we want the interface type, if available.
   Type getUnopenedTypeOfReference(VarDecl *value, Type baseType,
                                   DeclContext *UseDC,
-                                  ConstraintLocator *memberLocator = nullptr,
+                                  ConstraintLocator *locator,
                                   bool wantInterfaceType = false,
                                   bool adjustForPreconcurrency = true);
 
@@ -4443,7 +4451,7 @@ class ConstraintSystem {
   getUnopenedTypeOfReference(
       VarDecl *value, Type baseType, DeclContext *UseDC,
       llvm::function_ref<Type(VarDecl *)> getType,
-      ConstraintLocator *memberLocator = nullptr,
+      ConstraintLocator *locator,
       bool wantInterfaceType = false,
       bool adjustForPreconcurrency = true,
       llvm::function_ref<Type(const AbstractClosureExpr *)> getClosureType =
@@ -4453,7 +4461,10 @@ class ConstraintSystem {
       llvm::function_ref<bool(const ClosureExpr *)> isolatedByPreconcurrency =
         [](const ClosureExpr *closure) {
           return closure->isIsolatedByPreconcurrency();
-        });
+        },
+      llvm::function_ref<bool(Expr *)> isAssignTarget = [](Expr *) {
+        return false;
+      });
 
   /// Given the opened type and a pile of information about a member reference,
   /// determine the reference type of the member reference.

lib/SILGen/SILGenConcurrency.cpp

@@ -647,7 +647,8 @@ bool SILGenFunction::unsafelyInheritsExecutor() {
 
 void ExecutorBreadcrumb::emit(SILGenFunction &SGF, SILLocation loc) {
   if (mustReturnToExecutor) {
-    assert(SGF.ExpectedExecutor || SGF.unsafelyInheritsExecutor());
+    assert(SGF.ExpectedExecutor || SGF.unsafelyInheritsExecutor() ||
+           SGF.isCtorWithHopsInjectedByDefiniteInit());
     if (auto executor = SGF.ExpectedExecutor)
       SGF.B.createHopToExecutor(
           RegularLocation::getDebugOnlyLocation(loc, SGF.getModule()), executor,

lib/SILGen/SILGenConstructor.cpp

@@ -618,6 +618,19 @@ static bool ctorHopsInjectedByDefiniteInit(ConstructorDecl *ctor,
   }
 }
 
+bool SILGenFunction::isCtorWithHopsInjectedByDefiniteInit() {
+  auto declRef = F.getDeclRef();
+  if (!declRef || !declRef.isConstructor())
+    return false;
+
+  auto ctor = dyn_cast_or_null<ConstructorDecl>(declRef.getDecl());
+  if (!ctor)
+    return false;
+
+  auto isolation = getActorIsolation(ctor);
+  return ctorHopsInjectedByDefiniteInit(ctor, isolation);
+}
+
 void SILGenFunction::emitValueConstructor(ConstructorDecl *ctor) {
   MagicFunctionName = SILGenModule::getMagicFunctionName(ctor);
 

lib/SILGen/SILGenFunction.h

@@ -820,6 +820,10 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   /// the fields.
   void emitDeallocatingMoveOnlyDestructor(DestructorDecl *dd);
 
+  /// Whether we are inside a constructor whose hops are injected by
+  /// definite initialization.
+  bool isCtorWithHopsInjectedByDefiniteInit();
+
   /// Generates code for a struct constructor.
   /// This allocates the new 'self' value, emits the
   /// body code, then returns the final initialized 'self'.

lib/Sema/CSApply.cpp

@@ -655,12 +655,33 @@ namespace {
       }
 
       auto ref = resolveConcreteDeclRef(decl, locator);
+
+      // If we have a variable that's treated as an rvalue but allows
+      // assignment (for initialization) in the current context,
+      // treat it as an rvalue that we immediately load. This is
+      // the AST that's expected by SILGen.
+      bool loadImmediately = false;
+      if (auto var = dyn_cast_or_null<VarDecl>(ref.getDecl())) {
+        if (!fullType->hasLValueType()) {
+          if (var->mutability(dc) == StorageMutability::Initializable) {
+            fullType = LValueType::get(fullType);
+            adjustedFullType = LValueType::get(adjustedFullType);
+            loadImmediately = true;
+          }
+        }
+      }
+
+
       auto declRefExpr =
           new (ctx) DeclRefExpr(ref, loc, implicit, semantics, fullType);
       cs.cacheType(declRefExpr);
       declRefExpr->setFunctionRefKind(choice.getFunctionRefKind());
       Expr *result = adjustTypeForDeclReference(
           declRefExpr, fullType, adjustedFullType, locator);
+      // If we have to load, do so now.
+      if (loadImmediately)
+        result = cs.addImplicitLoadExpr(result);
+
       result = forceUnwrapIfExpected(result, locator);
 
       if (auto *fnDecl = dyn_cast<AbstractFunctionDecl>(decl)) {

lib/Sema/ConstraintSystem.cpp

@@ -1434,12 +1434,14 @@ ConstraintSystem::getWrappedPropertyInformation(
 ///   is being accessed; must be null if and only if this is not
 ///   a type member
 static bool
-doesStorageProduceLValue(AbstractStorageDecl *storage, Type baseType,
-                         DeclContext *useDC,
-                         ConstraintLocator *memberLocator = nullptr) {
+doesStorageProduceLValue(
+    AbstractStorageDecl *storage, Type baseType,
+    DeclContext *useDC,
+    llvm::function_ref<bool(Expr *)> isAssignTarget,
+    ConstraintLocator *locator) {
   const DeclRefExpr *base = nullptr;
-  if (memberLocator) {
-    if (auto *const E = getAsExpr(memberLocator->getAnchor())) {
+  if (locator) {
+    if (auto *const E = getAsExpr(locator->getAnchor())) {
       if (auto *MRE = dyn_cast<MemberRefExpr>(E)) {
         base = dyn_cast<DeclRefExpr>(MRE->getBase());
       } else if (auto *UDE = dyn_cast<UnresolvedDotExpr>(E)) {
@@ -1448,9 +1450,33 @@ doesStorageProduceLValue(AbstractStorageDecl *storage, Type baseType,
     }
   }
 
-  // Unsettable storage decls always produce rvalues.
-  if (!storage->isSettableInSwift(useDC, base))
-    return false;
+  switch (storage->mutabilityInSwift(useDC, base)) {
+    case StorageMutability::Immutable:
+      // Immutable storage decls always produce rvalues.
+      return false;
+
+    case StorageMutability::Mutable:
+      break;
+
+    case StorageMutability::Initializable: {
+      // If the member is not known to be on the left-hand side of
+      // an assignment, treat it as an rvalue so we can't pass it
+      // inout.
+      if (!locator)
+        return false;
+
+      auto *anchor = getAsExpr(simplifyLocatorToAnchor(locator));
+      if (!anchor)
+        return false;
+
+      // If the anchor isn't known to be the target of an assignment,
+      // treat as immutable.
+      if (!isAssignTarget(anchor))
+        return false;
+
+      break;
+    }
+  }
 
   if (!storage->isSetterAccessibleFrom(useDC))
     return false;
@@ -1490,7 +1516,7 @@ ClosureIsolatedByPreconcurrency::operator()(const ClosureExpr *expr) const {
 
 Type ConstraintSystem::getUnopenedTypeOfReference(
     VarDecl *value, Type baseType, DeclContext *UseDC,
-    ConstraintLocator *memberLocator, bool wantInterfaceType,
+    ConstraintLocator *locator, bool wantInterfaceType,
     bool adjustForPreconcurrency) {
   return ConstraintSystem::getUnopenedTypeOfReference(
       value, baseType, UseDC,
@@ -1504,18 +1530,20 @@ Type ConstraintSystem::getUnopenedTypeOfReference(
 
         return wantInterfaceType ? var->getInterfaceType() : var->getTypeInContext();
       },
-      memberLocator, wantInterfaceType, adjustForPreconcurrency,
+      locator, wantInterfaceType, adjustForPreconcurrency,
       GetClosureType{*this},
-      ClosureIsolatedByPreconcurrency{*this});
+      ClosureIsolatedByPreconcurrency{*this},
+      IsInLeftHandSideOfAssignment{*this});
 }
 
 Type ConstraintSystem::getUnopenedTypeOfReference(
     VarDecl *value, Type baseType, DeclContext *UseDC,
     llvm::function_ref<Type(VarDecl *)> getType,
-    ConstraintLocator *memberLocator,
+    ConstraintLocator *locator,
     bool wantInterfaceType, bool adjustForPreconcurrency,
     llvm::function_ref<Type(const AbstractClosureExpr *)> getClosureType,
-    llvm::function_ref<bool(const ClosureExpr *)> isolatedByPreconcurrency) {
+    llvm::function_ref<bool(const ClosureExpr *)> isolatedByPreconcurrency,
+    llvm::function_ref<bool(Expr *)> isAssignTarget) {
   Type requestedType =
       getType(value)->getWithoutSpecifierType()->getReferenceStorageReferent();
 
@@ -1540,7 +1568,8 @@ Type ConstraintSystem::getUnopenedTypeOfReference(
 
   // Qualify storage declarations with an lvalue when appropriate.
   // Otherwise, they yield rvalues (and the access must be a load).
-  if (doesStorageProduceLValue(value, baseType, UseDC, memberLocator) &&
+  if (doesStorageProduceLValue(value, baseType, UseDC, isAssignTarget,
+                               locator) &&
       !requestedType->hasError()) {
     return LValueType::get(requestedType);
   }
@@ -1913,7 +1942,8 @@ ConstraintSystem::getTypeOfReference(ValueDecl *value,
   // FIXME: @preconcurrency
   bool wantInterfaceType = !varDecl->getDeclContext()->isLocalContext();
   Type valueType =
-      getUnopenedTypeOfReference(varDecl, Type(), useDC, /*base=*/nullptr,
+      getUnopenedTypeOfReference(varDecl, Type(), useDC, 
+                                 getConstraintLocator(locator),
                                  wantInterfaceType);
 
   Type thrownErrorType;
@@ -2608,6 +2638,48 @@ bool ConstraintSystem::isPartialApplication(ConstraintLocator *locator) {
   return level < (baseTy->is<MetatypeType>() ? 1 : 2);
 }
 
+bool IsInLeftHandSideOfAssignment::operator()(Expr *expr) const {
+  // Walk up the parent tree.
+  auto parent = cs.getParentExpr(expr);
+  if (!parent)
+    return false;
+
+  // If the parent is an assignment expression, check whether we are
+  // the left-hand side.
+  if (auto assignParent = dyn_cast<AssignExpr>(parent)) {
+    return expr == assignParent->getDest();
+  }
+
+  // Always look up through these parent kinds.
+  if (isa<TupleExpr>(parent) || isa<IdentityExpr>(parent) ||
+      isa<AnyTryExpr>(parent) || isa<BindOptionalExpr>(parent)) {
+    return (*this)(parent);
+  }
+
+  // If the parent is a lookup expression, only follow from the base.
+  if (auto lookupParent = dyn_cast<LookupExpr>(parent)) {
+    if (lookupParent->getBase() == expr)
+      return (*this)(parent);
+
+    // The expression wasn't in the base, so this isn't part of the
+    // left-hand side.
+    return false;
+  }
+
+  // If the parent is an unresolved member reference a.b, only follow
+  // from the base.
+  if (auto dotParent = dyn_cast<UnresolvedDotExpr>(parent)) {
+    if (dotParent->getBase() == expr)
+      return (*this)(parent);
+
+    // The expression wasn't in the base, so this isn't part of the
+    // left-hand side.
+    return false;
+  }
+
+  return false;
+}
+
 DeclReferenceType ConstraintSystem::getTypeOfMemberReference(
     Type baseTy, ValueDecl *value, DeclContext *useDC, bool isDynamicLookup,
     FunctionRefKind functionRefKind, ConstraintLocator *locator,
@@ -2711,7 +2783,9 @@ DeclReferenceType ConstraintSystem::getTypeOfMemberReference(
     if (auto *subscript = dyn_cast<SubscriptDecl>(value)) {
       auto elementTy = subscript->getElementInterfaceType();
 
-      if (doesStorageProduceLValue(subscript, baseTy, useDC, locator))
+      if (doesStorageProduceLValue(
+              subscript, baseTy, useDC,
+              IsInLeftHandSideOfAssignment{*this}, locator))
         elementTy = LValueType::get(elementTy);
 
       auto indices = subscript->getInterfaceType()
@@ -2966,7 +3040,10 @@ Type ConstraintSystem::getEffectiveOverloadType(ConstraintLocator *locator,
     if (auto subscript = dyn_cast<SubscriptDecl>(decl)) {
       auto elementTy = subscript->getElementInterfaceType();
 
-      if (doesStorageProduceLValue(subscript, overload.getBaseType(), useDC))
+      if (doesStorageProduceLValue(subscript, overload.getBaseType(),
+                                   useDC,
+                                   IsInLeftHandSideOfAssignment{*this},
+                                   locator))
         elementTy = LValueType::get(elementTy);
       else if (elementTy->hasDynamicSelfType()) {
         elementTy = withDynamicSelfResultReplaced(elementTy,
@@ -2990,7 +3067,9 @@ Type ConstraintSystem::getEffectiveOverloadType(ConstraintLocator *locator,
           locator);
     } else if (auto var = dyn_cast<VarDecl>(decl)) {
       type = var->getValueInterfaceType();
-      if (doesStorageProduceLValue(var, overload.getBaseType(), useDC)) {
+      if (doesStorageProduceLValue(
+              var, overload.getBaseType(), useDC,
+              IsInLeftHandSideOfAssignment{*this}, locator)) {
         type = LValueType::get(type);
       } else if (type->hasDynamicSelfType()) {
         type = withDynamicSelfResultReplaced(type, /*uncurryLevel=*/0);

lib/Sema/TypeCheckExpr.cpp

@@ -581,17 +581,6 @@ bool TypeChecker::requireArrayLiteralIntrinsics(ASTContext &ctx,
   return true;
 }
 
-Expr *TypeChecker::buildCheckedRefExpr(VarDecl *value, DeclContext *UseDC,
-                                       DeclNameLoc loc, bool Implicit) {
-  auto type = constraints::ConstraintSystem::getUnopenedTypeOfReference(
-      value, Type(), UseDC,
-      [&](VarDecl *var) -> Type { return value->getTypeInContext(); });
-  auto semantics = value->getAccessSemanticsFromContext(UseDC,
-                                                       /*isAccessOnSelf*/false);
-  return new (value->getASTContext())
-      DeclRefExpr(value, loc, Implicit, semantics, type);
-}
-
 Expr *TypeChecker::buildRefExpr(ArrayRef<ValueDecl *> Decls,
                                 DeclContext *UseDC, DeclNameLoc NameLoc,
                                 bool Implicit, FunctionRefKind functionRefKind) {

lib/Sema/TypeChecker.h

@@ -944,10 +944,6 @@ Comparison compareDeclarations(DeclContext *dc, ValueDecl *decl1,
 /// of the first one and the expression will still type check.
 bool isDeclRefinementOf(ValueDecl *declA, ValueDecl *declB);
 
-/// Build a type-checked reference to the given value.
-Expr *buildCheckedRefExpr(VarDecl *D, DeclContext *UseDC, DeclNameLoc nameLoc,
-                          bool Implicit);
-
 /// Build a reference to a declaration, where name lookup returned
 /// the given set of declarations.
 Expr *buildRefExpr(ArrayRef<ValueDecl *> Decls, DeclContext *UseDC,

test/Constraints/diagnostics.swift

@@ -1396,7 +1396,9 @@ do {
   func generic<T>(_ value: inout T, _ closure: (S<T>) -> Void) {}
 
   let arg: Int
+  // expected-note@-1{{change 'let' to 'var' to make it mutable}}
   generic(&arg) { (g: S<Double>) -> Void in } // expected-error {{cannot convert value of type '(S<Double>) -> Void' to expected argument type '(S<Int>) -> Void'}}
+  // expected-error@-1{{cannot pass immutable value as inout argument: 'arg' is a 'let' constant}}
 }
 
 // rdar://problem/62428353 - bad error message for passing `T` where `inout T` was expected
@@ -1556,18 +1558,17 @@ func testNilCoalescingOperatorRemoveFix() {
   let _ = "" /* This is a comment */ ?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{13-43=}}
 
   let _ = "" // This is a comment
-    ?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1558:13-1559:10=}}
+    ?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1560:13-1561:10=}}
 
   let _ = "" // This is a comment
     /*
      * The blank line below is part of the test case, do not delete it
      */
+    ?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1563:13-1567:10=}}
 
-    ?? "" // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1561:13-1566:10=}}
-
-  if ("" ?? // This is a comment // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{9-1569:9=}}
+  if ("" ?? // This is a comment // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{9-1570:9=}}
       "").isEmpty {}
 
   if ("" // This is a comment
-      ?? "").isEmpty {} // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1571:9-1572:12=}}
+      ?? "").isEmpty {} // expected-warning {{left side of nil coalescing operator '??' has non-optional type 'String', so the right side is never used}} {{1572:9-1573:12=}}
 }