Merge pull request swiftlang#31554 from ravikandhadai/definite-init-failable-init-diag

[Definite Init] Improve diagnostics for delegating, failable initializers that do not initialize along all paths.

Author: ravikandhadai

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

@@ -1233,6 +1233,67 @@ void LifetimeChecker::handleInOutUse(const DIMemoryUse &Use) {
   }
 }
 
+/// Failable enum initializer produce a CFG for the return that looks like this,
+/// where the load is the use of 'self'.  Detect this pattern so we can consider
+/// it a 'return' use of self.
+///
+///   %3 = load %2 : $*Enum
+///   %4 = enum $Optional<Enum>, #Optional.Some!enumelt, %3 : $Enum
+///   br bb2(%4 : $Optional<Enum>)                    // id: %5
+/// bb1:
+///   %6 = enum $Optional<Enum>, #Optional.None!enumelt // user: %7
+///   br bb2(%6 : $Optional<Enum>)                    // id: %7
+/// bb2(%8 : $Optional<Enum>):                        // Preds: bb0 bb1
+///   dealloc_stack %1 : $*Enum                       // id: %9
+///   return %8 : $Optional<Enum>                     // id: %10
+///
+static bool isFailableInitReturnUseOfEnum(EnumInst *EI) {
+  // Only allow enums forming an optional.
+  if (!EI->getType().getOptionalObjectType())
+    return false;
+
+  if (!EI->hasOneUse())
+    return false;
+  auto *BI = dyn_cast<BranchInst>(EI->use_begin()->getUser());
+  if (!BI || BI->getNumArgs() != 1)
+    return false;
+
+  auto *TargetArg = BI->getDestBB()->getArgument(0);
+  if (!TargetArg->hasOneUse())
+    return false;
+  return isa<ReturnInst>(TargetArg->use_begin()->getUser());
+}
+
+/// Given a load instruction, return true iff the result of the load is used
+/// in a return instruction directly or is lifted to an optional (i.e., wrapped
+/// into .some) and returned. These conditions are used to detect whether the
+/// given load instruction is autogenerated for a return from the initalizers:
+/// `init` or `init?`, respectively. In such cases, the load should not be
+/// considered as a use of the value but rather as a part of the return
+/// instruction. We emit a specific diagnostic in this case.
+static bool isLoadForReturn(SingleValueInstruction *loadInst) {
+  bool hasReturnUse = false, hasUnknownUses = false;
+
+  for (auto LoadUse : loadInst->getUses()) {
+    auto *User = LoadUse->getUser();
+    // Ignore retains of the struct/enum before the return.
+    if (isa<RetainValueInst>(User))
+      continue;
+    if (isa<ReturnInst>(User)) {
+      hasReturnUse = true;
+      continue;
+    }
+    if (auto *EI = dyn_cast<EnumInst>(User))
+      if (isFailableInitReturnUseOfEnum(EI)) {
+        hasReturnUse = true;
+        continue;
+      }
+    hasUnknownUses = true;
+    break;
+  }
+  return hasReturnUse && !hasUnknownUses;
+}
+
 void LifetimeChecker::handleEscapeUse(const DIMemoryUse &Use) {
   // The value must be fully initialized at all escape points.  If not, diagnose
   // the error.
@@ -1260,8 +1321,7 @@ void LifetimeChecker::handleEscapeUse(const DIMemoryUse &Use) {
     // If this is a load with a single user that is a return, then this is
     // a return before self.init.   Emit a specific diagnostic.
     if (auto *LI = dyn_cast<LoadInst>(Inst))
-      if (LI->hasOneUse() &&
-          isa<ReturnInst>((*LI->use_begin())->getUser())) {
+      if (isLoadForReturn(LI)) {
         diagnose(Module, Inst->getLoc(),
                  diag::superselfinit_not_called_before_return,
                  (unsigned)TheMemory.isDelegatingInit());
@@ -1359,35 +1419,6 @@ void LifetimeChecker::handleEscapeUse(const DIMemoryUse &Use) {
   diagnoseInitError(Use, DiagMessage);
 }
 
-
-/// Failable enum initializer produce a CFG for the return that looks like this,
-/// where the load is the use of 'self'.  Detect this pattern so we can consider
-/// it a 'return' use of self.
-///
-///   %3 = load %2 : $*Enum
-///   %4 = enum $Optional<Enum>, #Optional.Some!enumelt, %3 : $Enum
-///   br bb2(%4 : $Optional<Enum>)                    // id: %5
-/// bb1:
-///   %6 = enum $Optional<Enum>, #Optional.None!enumelt // user: %7
-///   br bb2(%6 : $Optional<Enum>)                    // id: %7
-/// bb2(%8 : $Optional<Enum>):                        // Preds: bb0 bb1
-///   dealloc_stack %1 : $*Enum                       // id: %9
-///   return %8 : $Optional<Enum>                     // id: %10
-///
-static bool isFailableInitReturnUseOfEnum(EnumInst *EI) {
-  // Only allow enums forming an optional.
-  if (!EI->getType().getOptionalObjectType())
-    return false;
-
-  if (!EI->hasOneUse()) return false;
-  auto *BI = dyn_cast<BranchInst>(EI->use_begin()->getUser());
-  if (!BI || BI->getNumArgs() != 1) return false;
-
-  auto *TargetArg = BI->getDestBB()->getArgument(0);
-  if (!TargetArg->hasOneUse()) return false;
-  return isa<ReturnInst>(TargetArg->use_begin()->getUser());
-}
-
 enum BadSelfUseKind {
   BeforeStoredPropertyInit,
   BeforeSuperInit,
@@ -1681,31 +1712,8 @@ void LifetimeChecker::handleLoadUseFailure(const DIMemoryUse &Use,
   // diagnostic.
   if (isa<LoadInst>(Inst) || isa<LoadBorrowInst>(Inst)) {
     auto *LI = Inst;
-    bool hasReturnUse = false, hasUnknownUses = false;
-    
-    for (auto LoadUse : cast<SingleValueInstruction>(LI)->getUses()) {
-      auto *User = LoadUse->getUser();
-      
-      // Ignore retains of the struct/enum before the return.
-      if (isa<RetainValueInst>(User))
-        continue;
-      if (isa<ReturnInst>(User)) {
-        hasReturnUse = true;
-        continue;
-      }
-
-      if (auto *EI = dyn_cast<EnumInst>(User))
-        if (isFailableInitReturnUseOfEnum(EI)) {
-          hasReturnUse = true;
-          continue;
-        }
-
-      hasUnknownUses = true;
-      break;
-    }
-    
-    // Okay, this load is part of a return sequence, diagnose it specially.
-    if (hasReturnUse && !hasUnknownUses) {
+    // If this load is part of a return sequence, diagnose it specially.
+    if (isLoadForReturn(cast<SingleValueInstruction>(LI))) {
       // The load is probably part of the common epilog for the function, try to
       // find a more useful source location than the syntactic end of the
       // function.

test/SILOptimizer/definite_init_cross_module.swift

@@ -50,6 +50,13 @@ extension Point {
     self.x = xx // expected-error {{'self' used before 'self.init' call or assignment to 'self'}}
     self.y = 0 // expected-error {{'self' used before 'self.init' call or assignment to 'self'}}
   } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
+
+  // Test failable initializer.
+  init?(p: Point) {
+    if p.x > 0 {
+      self = p
+    }
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
 }
 
 extension GenericPoint {

test/SILOptimizer/definite_init_diagnostics.swift

@@ -1003,7 +1003,7 @@ enum MyAwesomeEnum {
 
   init?() {
 
-  } // expected-error {{'self' used before 'self.init' call or assignment to 'self'}}
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
 }
 
 // <rdar://problem/20679379> DI crashes on initializers on protocol extensions
@@ -1204,15 +1204,15 @@ enum SR1469_Enum1 {
     }
     // many lines later
     self = .A
-  } // expected-error {{'self' used before 'self.init' call or assignment to 'self'}}
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
 }
 
 enum SR1469_Enum2 {
   case A, B
 
   init?() {
     return
-  } // expected-error {{'self' used before 'self.init' call or assignment to 'self'}}
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
 }
 enum SR1469_Enum3 {
   case A, B
@@ -1222,7 +1222,7 @@ enum SR1469_Enum3 {
       self = .A
       return
     }
-  } // expected-error {{'self' used before 'self.init' call or assignment to 'self'}}
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
 }
 
 class BadFooSuper {
@@ -1589,3 +1589,20 @@ class DelegatingInitTest {
       // expected-error@-1 {{'self' used before 'self.init' call or assignment to 'self'}}
   } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
 }
+
+class A {
+  var a: Int
+
+  init(x: Int) {
+    self.a = x
+  }
+
+  convenience init(i: Int) {
+    if i > 0 {
+      self.init(x: i)
+    }
+    if i > -100 {
+      self.init(x: i)
+    }
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
+ }

test/SILOptimizer/definite_init_failable_initializers_diagnostics.swift

@@ -11,6 +11,24 @@ class FailableInitThatFailsReallyHard {
   }
 }
 
+// Failable initializers must produce correct diagnostics
+struct A {
+  var x: Int  // expected-note {{'self.x' not initialized}}
+  init?(i: Int) {
+    if i > 0 {
+      self.x = i
+    }
+  } // expected-error {{return from initializer without initializing all stored properties}}
+}
+
+// Delegating, failable initializers that doesn't initialize along all paths must produce correct diagnostics.
+extension Int {
+  init?(i: Int) {
+    if i > 0 {
+      self.init(i)
+    }
+  } // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
+}
 
 class BaseClass {}
 final class DerivedClass : BaseClass {

test/SILOptimizer/definite_init_markuninitialized_delegatingself.sil

@@ -292,3 +292,36 @@ bb0(%1 : $@thick MyClass4.Type):
   dealloc_stack %2 : $*MyClass4
   return %7 : $MyClass4
 }
+
+// A failable initializer: Int.init?(i:) that doesn't initialize along all paths.
+sil hidden [ossa] @intInitExtension : $@convention(method) (Int, Bool) -> Optional<Int> {
+bb0(%0 : $Int, %1 : $Bool):
+  %2 = alloc_stack $Int, var, name "self"
+  %3 = mark_uninitialized [delegatingself] %2 : $*Int
+  %5 = metatype $@thin Int.Type
+  %6 = struct_extract %1 : $Bool, #Bool._value
+  cond_br %6, bb1, bb2
+
+bb1:
+  %14 = begin_access [modify] [static] %3 : $*Int
+  assign %0 to %14 : $*Int
+  end_access %14 : $*Int
+  br bb3
+
+bb2:
+  br bb3
+
+bb3:
+  %19 = load [trivial] %3 : $*Int // expected-error {{'self.init' isn't called on all paths before returning from initializer}}
+  %20 = enum $Optional<Int>, #Optional.some!enumelt, %19 : $Int
+  dealloc_stack %2 : $*Int
+  br bb5(%20 : $Optional<Int>)
+
+bb4:
+  dealloc_stack %2 : $*Int
+  %24 = enum $Optional<Int>, #Optional.none!enumelt
+  br bb5(%24 : $Optional<Int>)
+
+bb5(%26 : $Optional<Int>):
+    return %26 : $Optional<Int>
+}