LifetimeDependence: clarify diagnostics for many unusual cases.

Ensure that we always issue a diagnostic on error, but avoid emitting any notes that don't have source locations.

With implicit accessors and thunks, report the correct line number and indicate which accessor generates the error.

Always check for debug_value users.

Consistently handle access scopes across diagnostic analysis and diagnostic messages.

(cherry picked from commit ec51286)

Author: atrick

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceDiagnostics.swift

@@ -238,37 +238,51 @@ private struct DiagnoseDependence {
 
     // Identify the escaping variable.
     let escapingVar = LifetimeVariable(dependent: operand.value, context)
-    let varName = escapingVar.name
-    if let varName {
-      diagnose(escapingVar.sourceLoc, .lifetime_variable_outside_scope,
-               varName)
+    if let varDecl = escapingVar.varDecl {
+      // Use the variable location, not the access location.
+      diagnose(varDecl.nameLoc, .lifetime_variable_outside_scope, escapingVar.name ?? "")
+    } else if let sourceLoc = escapingVar.sourceLoc {
+      diagnose(sourceLoc, .lifetime_value_outside_scope)
     } else {
-      diagnose(escapingVar.sourceLoc, .lifetime_value_outside_scope)
+      // Always raise an error even if we can't find a source location.
+      let sourceLoc = function.location.sourceLoc
+      if let accessorKind =  escapingVar.accessorKind {
+        diagnose(sourceLoc, .lifetime_value_outside_accessor, accessorKind)
+      } else {
+        // Thunks do not have a source location, but we try to use the function location anyway.
+        let thunkSelect = dependence.function.thunkKind == .noThunk ? 0 : 1
+        diagnose(sourceLoc, .lifetime_value_outside_thunk, thunkSelect, function.name)
+      }
     }
     reportScope()
     // Identify the use point.
-    let userSourceLoc = operand.instruction.location.sourceLoc
-    diagnose(userSourceLoc, diagID)
+    if let userSourceLoc = operand.instruction.location.sourceLoc {
+      diagnose(userSourceLoc, diagID)
+    }
   }
 
-  // Identify the dependence scope.
+  // Identify the dependence scope. If no source location is found, bypass this diagnostic.
   func reportScope() {
-    if case let .access(beginAccess) = dependence.scope {
-      let parentVar = LifetimeVariable(dependent: beginAccess, context)
-      if let sourceLoc = beginAccess.location.sourceLoc ?? parentVar.sourceLoc {
-        diagnose(sourceLoc, .lifetime_outside_scope_access,
-                 parentVar.name ?? "")
-      }
+    let parentVar = LifetimeVariable(dependent: dependence.parentValue, context)
+    // First check if the dependency is limited to an access scope. If the access has no source location then
+    // fall-through to report possible dependence on an argument.
+    if parentVar.isAccessScope, let accessLoc = parentVar.sourceLoc {
+      diagnose(accessLoc, .lifetime_outside_scope_access, parentVar.name ?? "")
       return
     }
-    if let arg = dependence.parentValue as? Argument,
-       let varDecl = arg.varDecl,
-       let sourceLoc = arg.sourceLoc {
-      diagnose(sourceLoc, .lifetime_outside_scope_argument,
-               varDecl.userFacingName)
+    // If the argument does not have a source location (e.g. a synthesized accessor), report the function location. The
+    // function's source location is sufficient for argument diagnostics, but if the function has no location, don't
+    // report any scope.
+    if parentVar.isArgument, let argLoc = parentVar.sourceLoc ?? function.location.sourceLoc {
+      if let parentName = parentVar.name {
+        diagnose(argLoc, .lifetime_outside_scope_argument, parentName)
+      } else {
+        diagnose(argLoc, .lifetime_outside_scope_synthesized_argument, parentVar.accessorKind ?? function.name)
+      }
       return
     }
-    let parentVar = LifetimeVariable(dependent: dependence.parentValue, context)
+    // Now diagnose dependencies on regular variable and value scopes.
+    // Thunks do not have a function location, so any scopes inside the thunk will be ignored.
     if let parentLoc = parentVar.sourceLoc {
       if let parentName = parentVar.name {
         diagnose(parentLoc, .lifetime_outside_scope_variable, parentName)
@@ -282,24 +296,34 @@ private struct DiagnoseDependence {
 // Identify a best-effort variable declaration based on a defining SIL
 // value or any lifetime dependent use of that SIL value.
 private struct LifetimeVariable {
-  var varDecl: VarDecl?
-  var sourceLoc: SourceLoc?
+  var varDecl: VarDecl? = nil
+  var sourceLoc: SourceLoc? = nil
+  var isArgument: Bool = false
+  var isAccessScope: Bool = false
+  var accessorKind: String?
+  var thunkKind: Function.ThunkKind = .noThunk
 
   var name: StringRef? {
     return varDecl?.userFacingName
   }
 
   init(dependent value: Value, _ context: some Context) {
-    if value.type.isAddress {
-      self = Self(accessBase: value.accessBase, context)
+    guard let introducer = getFirstVariableIntroducer(of: value, context) else {
       return
     }
-    if let firstIntroducer = getFirstVariableIntroducer(of: value, context) {
-      self = Self(introducer: firstIntroducer)
+    if introducer.type.isAddress {
+      if let beginAccess = introducer as? BeginAccessInst {
+        // Recurse through beginAccess to find the variable introducer rather than the variable access.
+        self = .init(dependent: beginAccess.address, context)
+        self.isAccessScope = true
+        // However, remember source location of the innermost access.
+        self.sourceLoc = beginAccess.location.sourceLoc ?? self.sourceLoc
+        return
+      }
+      self = .init(accessBase: introducer.accessBase, context)
       return
     }
-    self.varDecl = nil
-    self.sourceLoc = nil
+    self = Self(introducer: introducer, context)
   }
 
   private func getFirstVariableIntroducer(of value: Value, _ context: some Context) -> Value? {
@@ -313,15 +337,21 @@ private struct LifetimeVariable {
     return introducer
   }
 
-  private init(introducer: Value) {
+  private init(introducer: Value, _ context: some Context) {
     if let arg = introducer as? Argument {
       self.varDecl = arg.varDecl
-    } else {
-      self.sourceLoc = introducer.definingInstruction?.location.sourceLoc
-      self.varDecl = introducer.definingInstruction?.findVarDecl()
+      self.sourceLoc = arg.sourceLoc
+      self.isArgument = true
+      return
     }
-    if let varDecl {
-      sourceLoc = varDecl.nameLoc
+    if let varDecl = introducer.definingInstruction?.findVarDecl() {
+      self.varDecl = varDecl
+      self.sourceLoc = varDecl.nameLoc
+    } else if let sourceLoc = introducer.definingInstruction?.location.sourceLoc {
+      self.sourceLoc = sourceLoc
+    } else {
+      self.accessorKind = introducer.parentFunction.accessorKindName
+      self.thunkKind = introducer.parentFunction.thunkKind
     }
   }
 
@@ -335,32 +365,27 @@ private struct LifetimeVariable {
       // never be produced by one of these, except when it is redundant with the `alloc_box` VarDecl. It does not seem
       // possible for a box to be moved/borrowed directly into another variable's box. Reassignment always loads/stores
       // the value.
-      self = Self(introducer: projectBox.box.referenceRoot)
+      self = .init(introducer: projectBox.box.referenceRoot, context)
     case .stack(let allocStack):
-      self = Self(introducer: allocStack)
+      self = .init(introducer: allocStack, context)
     case .global(let globalVar):
       self.varDecl = globalVar.varDecl
       self.sourceLoc = varDecl?.nameLoc
     case .class(let refAddr):
-      self.varDecl = refAddr.varDecl
-      self.sourceLoc = refAddr.location.sourceLoc
+      self = .init(introducer: refAddr, context)
     case .tail(let refTail):
-      self = Self(introducer: refTail.instance)
+      self = .init(introducer: refTail.instance, context)
     case .argument(let arg):
-      self.varDecl = arg.varDecl
-      self.sourceLoc = arg.sourceLoc
+      self = .init(introducer: arg, context)
     case .yield(let result):
       // TODO: bridge VarDecl for FunctionConvention.Yields
-      self.varDecl = nil
-      self.sourceLoc = result.parentInstruction.location.sourceLoc
+      self = .init(introducer: result, context)
     case .storeBorrow(let sb):
       self = .init(dependent: sb.source, context)
     case .pointer(let ptrToAddr):
-      self.varDecl = nil
-      self.sourceLoc = ptrToAddr.location.sourceLoc
+      self = .init(introducer: ptrToAddr, context)
     case .index, .unidentified:
-      self.varDecl = nil
-      self.sourceLoc = nil
+      break
     }
   }
 }

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceInsertion.swift

@@ -300,8 +300,8 @@ private func insertMarkDependencies(value: Value, initializer: Instruction?,
   }
 }
 
-/// Walk up the value dependence chain to find the best-effort variable declaration. Typically called while diagnosing
-/// an error.
+/// Walk up the value dependence chain to find the best-effort variable declaration. Used to find the source of a borrow
+/// dependence or to print the source variable in a diagnostic message.
 ///
 /// Returns an array with at least one introducer value.
 ///
@@ -440,11 +440,12 @@ struct VariableIntroducerUseDefWalker : LifetimeDependenceUseDefValueWalker, Lif
 
   /// Override to check for on-stack variables before following an initializer.
   mutating func walkUp(address: Value, access: AccessBaseAndScopes) -> WalkResult {
-    // Check for stack locations that correspond to an lvalue.
-    if case let .stack(allocStack) = access.base {
-      if allocStack.varDecl != nil {
-        // Report this variable's innermmost access scope.
-        return addressIntroducer(access.enclosingAccess.address ?? address, access: access)
+    // Check for stack locations that correspond to an lvalue if there isn't any nested access scope.
+    if access.innermostAccess == nil {
+      if case let .stack(allocStack) = access.base {
+        if allocStack.varDecl != nil {
+          return addressIntroducer(allocStack, access: access)
+        }
       }
     }
     return walkUpDefault(address: address, access: access)

SwiftCompilerSources/Sources/Optimizer/Utilities/LifetimeDependenceUtils.swift

@@ -1221,6 +1221,24 @@ extension LifetimeDependenceUseDefAddressWalker {
   }
 
   mutating func walkUpDefault(address: Value, access: AccessBaseAndScopes) -> WalkResult {
+    if let beginAccess = access.innermostAccess {
+      // Skip the access scope for unsafe[Mutable]Address. Treat it like a projection of 'self' rather than a separate
+      // variable access.
+      if let addressorSelf = beginAccess.unsafeAddressorSelf {
+        return walkUp(newLifetime: addressorSelf)
+      }
+      // Ignore the acces scope for trivial values regardless of whether it is singly-initialized. Trivial values do not
+      // need to be kept alive in memory and can be safely be overwritten in the same scope. Lifetime dependence only
+      // cares that the loaded value is within the lexical scope of the trivial value's variable declaration. Rather
+      // than skipping all access scopes, call 'walkUp' on each nested access in case one of them needs to redirect the
+      // walk, as required for 'access.unsafeAddressorSelf' or in case the implementation overrides certain accesses.
+      if isTrivialScope {
+        return walkUp(newLifetime: beginAccess.address)
+      }
+      // Generally assume an access scope introduces a variable borrow scope. And generally ignore address forwarding
+      // mark_dependence.
+      return addressIntroducer(beginAccess, access: access)
+    }
     // Continue walking for some kinds of access base.
     switch access.base {
     case .box, .global, .class, .tail, .pointer, .index, .unidentified:
@@ -1229,14 +1247,9 @@ extension LifetimeDependenceUseDefAddressWalker {
       // Ignore stack locations. Their access scopes do not affect lifetime dependence.
       return walkUp(stackInitializer: allocStack, at: address, access: access)
     case let .argument(arg):
-      // Ignore access scopes for @in or @in_guaranteed arguments when all scopes are reads. Do not ignore a [read]
-      // access of an inout argument or outer [modify]. Mutation later with the outer scope could invalidate the
-      // borrowed state in this narrow scope. Do not ignore any mark_depedence on the address.
-      if arg.convention.isIndirectIn && access.isOnlyReadAccess {
+      if arg.convention.isExclusiveIndirect {
         return addressIntroducer(arg, access: access)
       }
-      // @inout arguments may be singly initialized (when no modification exists in this function), but this is not
-      // relevant here because they require nested access scopes which can never be ignored.
     case let .yield(yieldedAddress):
       // Ignore access scopes for @in or @in_guaranteed yields when all scopes are reads.
       let apply = yieldedAddress.definingInstruction as! FullApplySite
@@ -1250,27 +1263,6 @@ extension LifetimeDependenceUseDefAddressWalker {
         return walkUp(newLifetime: sb.source)
       }
     }
-    // Skip the access scope for unsafe[Mutable]Address. Treat it like a projection of 'self' rather than a separate
-    // variable access.
-    if case let .access(innerAccess) = access.scopes.first,
-       let addressorSelf = innerAccess.unsafeAddressorSelf {
-      return walkUp(newLifetime: addressorSelf)
-    }
-    // Ignore the acces scope for trivial values regardless of whether it is singly-initialized. Trivial values do not
-    // need to be kept alive in memory and can be safely be overwritten in the same scope. Lifetime dependence only
-    // cares that the loaded value is within the lexical scope of the trivial value's variable declaration. Rather than
-    // skipping all access scopes, call 'walkUp' on each nested access in case one of them needs to redirect the walk,
-    // as required for 'access.unsafeAddressorSelf'.
-    if isTrivialScope {
-      switch access.scopes.first {
-      case .none, .base:
-        break
-      case let .access(beginAccess):
-        return walkUp(newLifetime: beginAccess.address)
-      case let .dependence(markDep):
-        return walkUp(newLifetime: markDep.value)
-      }
-    }
     return addressIntroducer(access.enclosingAccess.address ?? address, access: access)
   }
 

include/swift/AST/DiagnosticsSIL.def

@@ -1157,8 +1157,14 @@ ERROR(lifetime_variable_outside_scope, none,
       "lifetime-dependent variable '%0' escapes its scope", (Identifier))
 ERROR(lifetime_value_outside_scope, none,
       "lifetime-dependent value escapes its scope", ())
+ERROR(lifetime_value_outside_accessor, none,
+      "lifetime-dependent value returned by generated accessor '%0'", (StringRef))
+ERROR(lifetime_value_outside_thunk, none,
+      "lifetime-dependent value returned by generated %select{function|thunk}0 '%1'", (bool, StringRef))
 NOTE(lifetime_outside_scope_argument, none,
      "it depends on the lifetime of argument '%0'", (Identifier))
+NOTE(lifetime_outside_scope_synthesized_argument, none,
+     "it depends on the lifetime of an argument of '%0'", (Identifier))
 NOTE(lifetime_outside_scope_access, none,
      "it depends on this scoped access to variable '%0'", (Identifier))
 NOTE(lifetime_outside_scope_variable, none,

test/SILOptimizer/lifetime_dependence/verify_diagnostics.sil

@@ -176,6 +176,7 @@ bb0(%0 : @guaranteed $NE):
   %2 = function_ref @reborrowNE : $@convention(thin) (@guaranteed NE) -> @lifetime(borrow 0) @owned NE
   %3 = apply %2(%0) : $@convention(thin) (@guaranteed NE) -> @lifetime(borrow 0) @owned NE
   // expected-error @-1{{lifetime-dependent value escapes its scope}}
+  // expected-note @-6{{it depends on the lifetime of an argument of 'testReborrow'}}
   %4 = mark_dependence [unresolved] %3 on %0
   return %4 // expected-note {{this use causes the lifetime-dependent value to escape}}
 }
@@ -185,6 +186,7 @@ bb0(%0 : @guaranteed $NE):
   %2 = function_ref @reborrowNE : $@convention(thin) (@guaranteed NE) -> @lifetime(borrow 0) @owned NE
   %3 = apply %2(%0) : $@convention(thin) (@guaranteed NE) -> @lifetime(borrow 0) @owned NE
   // expected-error @-1{{lifetime-dependent value escapes its scope}}
+  // expected-note @-5{{it depends on the lifetime of an argument of 'testBorrowValue'}}
   %4 = mark_dependence [unresolved] %3 on %0
   return %4 // expected-note {{this use causes the lifetime-dependent value to escape}}
 }
@@ -229,6 +231,7 @@ bb0(%0 : $*NE, %1 : $*Holder):
   %5 = alloc_stack $Holder
   %6 = store_borrow %4 to %5
   %7 = alloc_stack $NE // expected-error {{lifetime-dependent value escapes its scope}}
+  // expected-note @-8{{it depends on the lifetime of an argument of 'testCopyInit'}}
 
   %8 = function_ref @getNEIndirect : $@convention(thin) (@in_guaranteed Holder) -> @lifetime(borrow address_for_deps 0) @out NE
   %9 = apply %8(%7, %6) : $@convention(thin) (@in_guaranteed Holder) -> @lifetime(borrow address_for_deps 0) @out NE

test/SILOptimizer/lifetime_dependence/verify_diagnostics.swift

@@ -36,9 +36,15 @@ struct A {}
 
 func useA(_:A){}
 
-struct NE : ~Escapable {}
+public struct NE : ~Escapable {}
+
+public struct NEImmortal: ~Escapable {
+  @lifetime(immortal)
+  public init() {}
+}
 
 class C {}
+
 struct Holder {
   var c: C? = nil
 }
@@ -91,3 +97,30 @@ public func testGenericDep<T: ~Escapable>(type: T.Type) -> T {
   return result
   // expected-note @-1{{this use causes the lifetime-dependent value to escape}}
 }
+
+// Test diagnostics on implicit accessors.
+public struct ImplicitAccessors {
+  let c: C
+
+  public var neComputed: NEImmortal {
+    get {
+      NEImmortal()
+    }
+    set {
+    }
+  }
+}
+
+public struct NoncopyableImplicitAccessors : ~Copyable & ~Escapable {
+  public var ne: NE
+
+  public var neComputedBorrow: NE {
+    @lifetime(borrow self)
+    get { ne }
+
+    @lifetime(&self)
+    set {
+      ne = newValue
+    }
+  }
+}