[SR-9999] Runtime assertion hit when using an 'deallocated' unowned reference in a -O build

[hamishknight]

Previous ID	SR-9999
Radar	None
Original Reporter	@hamishknight
Type	Bug

Environment

Swift version 5.0-dev (LLVM 6ddb64316c, Swift d17602a)
Target: x86_64-apple-darwin18.2.0

Additional Detail from JIRA

Votes	0
Component/s	Compiler
Labels	Bug, Runtime
Assignee	None
Priority	Medium

md5: 3dc353b8937cf7587df306c968f94c7f

Issue Description:

In a -O build, the following trips a runtime assertion:

import Dispatch
import Foundation

func foo() {
  class MyObject {
    var value = 0
  }

  var o = MyObject()
  unowned var u = o
  o = MyObject()

  DispatchQueue.main.asyncAfter(deadline: .now()) {
    print("old unowned u = \(u)")
  }
}
foo()

RunLoop.main.run()

Output:

./swiftc /Users/hamishknight/Desktop/Stochastic\ Projects/unowned\ unowned/unowned\ unowned/main.swift -sdk /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk -O; and ./main
/Users/hamishknight/Desktop/Stochastic Projects/unowned unowned/unowned unowned/main.swift:18:15: warning: variable 'u' was never mutated; consider changing to 'let' constant
  unowned var u = o
          ~~~ ^
          let
Assertion failed: (object->refCounts.getUnownedCount() && "object is not currently unowned-retained"), function swift_unownedRetainStrong, file /Users/hamishknight/Desktop/swift-dev/swift/stdlib/public/runtime/HeapObject.cpp, line 516.
fish: './main' terminated by signal SIGABRT (Abort)

[belkadan]

cc @rjmccall, @jckarter. How do we capture unowned variables?

[jckarter]

It ought to be captured as unowned into the closure. It's fishy that it crashes with an assertion failure rather than a proper runtime trap, but it seems like this code ought to trap regardless, because the owning reference goes away before even the closure is formed.

[rjmccall]

Agreed. We should do a better job of diagnosing this at runtime, but essentially we're complaining when we copy the unowned reference into the closure that the reference is already dead, which seems like the right thing to do.

[hamishknight]

Indeed it should raise a runtime error – for context, the original issue was discovered when building without assertions where the code would sometimes trigger the correct runtime error and sometimes abort with EXC_BAD_ACCESS, which makes me feel like we have a race condition going on somewhere.