Merge pull request #17111 from jckarter/defer-let-inout-bug-4.2

[4.2] DI: Consider capture of `let`s to be a read-only use.

Author: jckarter

lib/SILOptimizer/Mandatory/DIMemoryUseCollectorOwnership.cpp

@@ -877,9 +877,32 @@ void ElementUseCollector::collectUses(SILValue Pointer, unsigned BaseEltNo) {
         addElementUses(BaseEltNo, PointeeType, User, DIUseKind::IndirectIn);
         continue;
 
+      
       // If this is an @inout parameter, it is like both a load and store.
-      case ParameterConvention::Indirect_Inout:
       case ParameterConvention::Indirect_InoutAliasable: {
+        // FIXME: The @inout_aliasable convention is used for indirect captures
+        // of both 'let' and 'var' variables. Using a more specific convention
+        // for 'let' properties like @in_guaranteed unfortunately exposes bugs
+        // elsewhere in the pipeline. A 'let' capture cannot really be mutated
+        // by the callee, and this is enforced by sema, so we can consider it
+        // a nonmutating use.
+        bool isLet = true;
+        
+        for (unsigned i = 0; i < TheMemory.NumElements; ++i) {
+          if (!TheMemory.isElementLetProperty(i)) {
+            isLet = false;
+            break;
+          }
+        }
+        
+        if (isLet) {
+          addElementUses(BaseEltNo, PointeeType, User, DIUseKind::IndirectIn);
+          continue;
+        }
+        
+        LLVM_FALLTHROUGH;
+      }
+      case ParameterConvention::Indirect_Inout: {
         // If we're in the initializer for a struct, and this is a call to a
         // mutating method, we model that as an escape of self.  If an
         // individual sub-member is passed as inout, then we model that as an

lib/Sema/ConstraintSystem.cpp

@@ -873,6 +873,7 @@ std::pair<Type, Type>
 ConstraintSystem::getTypeOfReference(ValueDecl *value,
                                      FunctionRefKind functionRefKind,
                                      ConstraintLocatorBuilder locator,
+                                     DeclContext *useDC,
                                      const DeclRefExpr *base) {
   if (value->getDeclContext()->isTypeContext() && isa<FuncDecl>(value)) {
     // Unqualified lookup can find operator names within nominal types.
@@ -937,7 +938,7 @@ ConstraintSystem::getTypeOfReference(ValueDecl *value,
   // Unqualified reference to a type.
   if (auto typeDecl = dyn_cast<TypeDecl>(value)) {
     // Resolve the reference to this type declaration in our current context.
-    auto type = TC.resolveTypeInContext(typeDecl, nullptr, DC,
+    auto type = TC.resolveTypeInContext(typeDecl, nullptr, useDC,
                                         TypeResolutionFlags::InExpression,
                                         /*isSpecialized=*/false);
 
@@ -958,7 +959,7 @@ ConstraintSystem::getTypeOfReference(ValueDecl *value,
 
   // Determine the type of the value, opening up that type if necessary.
   bool wantInterfaceType = !varDecl->getDeclContext()->isLocalContext();
-  Type valueType = TC.getUnopenedTypeOfReference(varDecl, Type(), DC, base,
+  Type valueType = TC.getUnopenedTypeOfReference(varDecl, Type(), useDC, base,
                                                  wantInterfaceType);
 
   assert(!valueType->hasUnboundGenericType() &&
@@ -1222,7 +1223,7 @@ ConstraintSystem::getTypeOfMemberReference(
 
   // If the base is a module type, just use the type of the decl.
   if (baseObjTy->is<ModuleType>()) {
-    return getTypeOfReference(value, functionRefKind, locator, base);
+    return getTypeOfReference(value, functionRefKind, locator, useDC, base);
   }
 
   // Don't open existentials when accessing typealias members of
@@ -1633,7 +1634,7 @@ void ConstraintSystem::resolveOverload(ConstraintLocator *locator,
     } else {
       std::tie(openedFullType, refType)
         = getTypeOfReference(choice.getDecl(),
-                             choice.getFunctionRefKind(), locator);
+                             choice.getFunctionRefKind(), locator, useDC);
     }
 
     // For a non-subscript declaration found via dynamic lookup, strip

lib/Sema/ConstraintSystem.h

@@ -2188,6 +2188,7 @@ class ConstraintSystem {
                           ValueDecl *decl,
                           FunctionRefKind functionRefKind,
                           ConstraintLocatorBuilder locator,
+                          DeclContext *useDC,
                           const DeclRefExpr *base = nullptr);
 
   /// \brief Retrieve the type of a reference to the given value declaration,

test/SILGen/closures.swift

@@ -380,13 +380,11 @@ func closeOverLetLValue() {
 // CHECK:   [[TMP_CLASS_ADDR:%.*]] = alloc_stack $ClassWithIntProperty, let, name "a", argno 1
 // CHECK:   [[COPY_ARG:%.*]] = copy_value [[ARG]]
 // CHECK:   store [[COPY_ARG]] to [init] [[TMP_CLASS_ADDR]] : $*ClassWithIntProperty
-// CHECK:   [[LOADED_CLASS:%.*]] = load [copy] [[TMP_CLASS_ADDR]] : $*ClassWithIntProperty
-// CHECK:   [[BORROWED_LOADED_CLASS:%.*]] = begin_borrow [[LOADED_CLASS]]
+// CHECK:   [[BORROWED_LOADED_CLASS:%.*]] = load_borrow [[TMP_CLASS_ADDR]] : $*ClassWithIntProperty
 // CHECK:   [[INT_IN_CLASS_ADDR:%.*]] = ref_element_addr [[BORROWED_LOADED_CLASS]] : $ClassWithIntProperty, #ClassWithIntProperty.x
 // CHECK:   [[ACCESS:%.*]] = begin_access [read] [dynamic] [[INT_IN_CLASS_ADDR]] : $*Int
 // CHECK:   [[INT_IN_CLASS:%.*]] = load [trivial] [[ACCESS]] : $*Int
-// CHECK:   end_borrow [[BORROWED_LOADED_CLASS]] from [[LOADED_CLASS]]
-// CHECK:   destroy_value [[LOADED_CLASS]]
+// CHECK:   end_borrow [[BORROWED_LOADED_CLASS]] from [[TMP_CLASS_ADDR]]
 // CHECK:   destroy_addr [[TMP_CLASS_ADDR]] : $*ClassWithIntProperty
 // CHECK:   dealloc_stack %1 : $*ClassWithIntProperty
 // CHECK:   return [[INT_IN_CLASS]]
@@ -397,10 +395,8 @@ func closeOverLetLValue() {
 // GUARANTEED:   [[TMP:%.*]] = alloc_stack $ClassWithIntProperty
 // GUARANTEED:   [[COPY:%.*]] = copy_value %0 : $ClassWithIntProperty
 // GUARANTEED:   store [[COPY]] to [init] [[TMP]] : $*ClassWithIntProperty
-// GUARANTEED:   [[LOADED_COPY:%.*]] = load [copy] [[TMP]]
-// GUARANTEED:   [[BORROWED:%.*]] = begin_borrow [[LOADED_COPY]]
-// GUARANTEED:   end_borrow [[BORROWED]] from [[LOADED_COPY]]
-// GUARANTEED:   destroy_value [[LOADED_COPY]]
+// GUARANTEED:   [[BORROWED:%.*]] = load_borrow [[TMP]]
+// GUARANTEED:   end_borrow [[BORROWED]] from [[TMP]]
 // GUARANTEED:   destroy_addr [[TMP]]
 // GUARANTEED: } // end sil function '$S8closures18closeOverLetLValueyyFSiyXEfU_'
 

test/SILOptimizer/definite_init_address_only_let.swift

@@ -0,0 +1,31 @@
+// RUN: %target-swift-frontend -emit-sil -verify %s
+
+func foo<T>(a: Bool, t: T) {
+  let x: T
+  defer { print(x) }
+
+  x = t
+  return
+}
+
+func bar<T>(a: Bool, t: T) {
+  let x: T // expected-note {{defined here}}
+  defer { print(x) } //expected-error{{constant 'x' used before being initialized}}
+
+  if a {
+    x = t
+    return
+  }
+}
+
+func bas<T>(a: Bool, t: T) {
+  let x: T 
+  defer { print(x) }
+
+  if a {
+    x = t
+    return
+  }
+
+  x = t
+}

test/SILOptimizer/definite_init_diagnostics.swift

@@ -83,11 +83,6 @@ func test2() {
   }
   b4 = 7
 
-  let b5: Any
-  b5 = "x"   
-  { takes_inout_any(&b5) }()   // expected-error {{immutable value 'b5' must not be passed inout}}
-  ({ takes_inout_any(&b5) })()   // expected-error {{immutable value 'b5' must not be passed inout}}
-
   // Structs
   var s1 : SomeStruct
   s1 = SomeStruct()   // ok

test/expr/closure/let.swift

@@ -0,0 +1,13 @@
+// RUN: %target-swift-frontend -typecheck -verify %s
+
+func frob(x: inout Int) {}
+
+func foo() {
+  let x: Int // expected-note* {{}}
+
+  x = 0
+
+  _ = { frob(x: x) }() // expected-error{{'x' is a 'let'}}
+  _ = { x = 0 }() // expected-error{{'x' is a 'let'}}
+  _ = { frob(x: x); x = 0 }() // expected-error 2 {{'x' is a 'let'}}
+}