Merge pull request swiftlang#81191 from atrick/var-walker-recursion

LifetimeDependenceDiagnostics: bug fixes and output clarity

Author: atrick

SwiftCompilerSources/Sources/Optimizer/FunctionPasses/LifetimeDependenceDiagnostics.swift

@@ -238,37 +238,53 @@ 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 parentVar.isClosureCapture {
+        diagnose(argLoc, .lifetime_outside_scope_capture)
+      } else 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 +298,35 @@ 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 isAccessScope: Bool = false
+  var isArgument: Bool = false
+  var isClosureCapture: 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 +340,22 @@ private struct LifetimeVariable {
     return introducer
   }
 
-  private init(introducer: Value) {
-    if let arg = introducer as? Argument {
+  private init(introducer: Value, _ context: some Context) {
+    if let arg = introducer as? FunctionArgument {
       self.varDecl = arg.varDecl
-    } else {
-      self.sourceLoc = introducer.definingInstruction?.location.sourceLoc
-      self.varDecl = introducer.definingInstruction?.findVarDecl()
+      self.sourceLoc = arg.sourceLoc
+      self.isArgument = true
+      self.isClosureCapture = arg.isClosureCapture
+      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 +369,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)
   }
 
@@ -1285,7 +1277,7 @@ extension LifetimeDependenceUseDefAddressWalker {
       case let store as StoringInstruction:
         return walkUp(newLifetime: store.source)
       case let srcDestInst as SourceDestAddrInstruction:
-        return walkUp(newLifetime: srcDestInst.destination)
+        return walkUp(newLifetime: srcDestInst.source)
       case let apply as FullApplySite:
         if let f = apply.referencedFunction, f.isConvertPointerToPointerArgument {
           return walkUp(newLifetime: apply.parameterOperands[0].value)

SwiftCompilerSources/Sources/SIL/Argument.swift

@@ -34,7 +34,12 @@ public class Argument : Value, Hashable {
 
   public var isLexical: Bool { false }
 
-  public var varDecl: VarDecl? { bridged.getVarDecl().getAs(VarDecl.self) }
+  public var varDecl: VarDecl? {
+    if let varDecl = bridged.getVarDecl().getAs(VarDecl.self) {
+      return varDecl
+    }
+    return debugUserDecl
+  }
 
   public var sourceLoc: SourceLoc? { varDecl?.nameLoc }
 
@@ -56,6 +61,10 @@ final public class FunctionArgument : Argument {
     bridged.FunctionArgument_isLexical()
   }
 
+  public var isClosureCapture: Bool {
+    bridged.FunctionArgument_isClosureCapture()
+  }
+
   public var isSelf: Bool {
     parentFunction.argumentConventions.selfIndex == index
   }

SwiftCompilerSources/Sources/SIL/Function.swift

@@ -221,6 +221,13 @@ final public class Function : CustomStringConvertible, HasShortDescription, Hash
     }
   }
 
+  public var accessorKindName: String? {
+    guard bridged.isAccessor() else {
+      return nil
+    }
+    return StringRef(bridged: bridged.getAccessorName()).string
+  }
+
   /// True, if the function runs with a swift 5.1 runtime.
   /// Note that this is function specific, because inlinable functions are de-serialized
   /// in a client module, which might be compiled with a different deployment target.

SwiftCompilerSources/Sources/SIL/Instruction.swift

@@ -452,7 +452,7 @@ public enum VariableScopeInstruction {
 
   // TODO: with SIL verification, we might be able to make varDecl non-Optional.
   public var varDecl: VarDecl? {
-    if let debugVarDecl = instruction.debugVarDecl {
+    if let debugVarDecl = instruction.debugResultDecl {
       return debugVarDecl
     }
     // SILGen may produce double var_decl instructions for the same variable:
@@ -474,15 +474,31 @@ extension Instruction {
     if let varScopeInst = VariableScopeInstruction(self) {
       return varScopeInst.varDecl
     }
-    return debugVarDecl
+    return debugResultDecl
   }
 
-  var debugVarDecl: VarDecl? {
+  var debugResultDecl: VarDecl? {
     for result in results {
-      for use in result.uses {
-        if let debugVal = use.instruction as? DebugValueInst {
-          return debugVal.varDecl
-        }
+      if let varDecl = result.debugUserDecl {
+        return varDecl
+      }
+    }
+    return nil
+  }
+}
+
+extension Value {
+  var debugValDecl: VarDecl? {
+    if let arg = self as? Argument {
+      return arg.varDecl
+    }
+    return debugUserDecl
+  }
+
+  var debugUserDecl: VarDecl? {
+    for use in uses {
+      if let debugVal = use.instruction as? DebugValueInst {
+        return debugVal.varDecl
       }
     }
     return nil

include/swift/AST/ASTBridging.h

@@ -395,6 +395,7 @@ struct BridgedDeclObj {
   BRIDGED_INLINE bool ProtocolDecl_requiresClass() const;
   BRIDGED_INLINE bool AbstractFunction_isOverridden() const;
   BRIDGED_INLINE bool Destructor_isIsolated() const;
+  SWIFT_IMPORT_UNSAFE BRIDGED_INLINE BridgedStringRef AccessorDecl_getKindName() const;
 };
 
 enum ENUM_EXTENSIBILITY_ATTR(closed) BridgedASTNodeKind : uint8_t {