[Sema] Diagnose 'nonisolated' attribute on wrapped properties

[theblixguy]

Follow up to #60421.

Diagnose use of nonisolated on wrapped properties, because we currently do not support it. This is because the backing property is a stored var and so cannot be marked as nonisolated (otherwise you would be able to mutate the state from outside the actor, which is an unsafe operation). We could potentially support it in the future by allowing users to mark the backing property as a stored let instead, then it would be safe to add nonisolated to the wrapped property.

Resolves #59380
Resolves #59494

[hborla]

Thank you!

[theblixguy]

@swift-ci please smoke test

[theblixguy]

I assume we need to take this into 5.7 too?

[xedin]

Yes, please open 5.7 PR for this!

[b8591340]

Sorry, but this is not an acceptable solution to me (author of both #59380 and #59494).

It forbids behavior that was sound with understanding of Swift concurrency.

I am making nontrivial use of nonisolated property wrappers to streamline access to effectively-@unchecked Sendable remote properties within a @MainActor class.

The compiler could detect nonmutating setters within the property wrapper and only error (or warn until Swift 6) on stored variables.

[hborla]

It forbids behavior that was sound with understanding of Swift concurrency.

This behavior is not sound, that's why we must ban it until we have a way to make the property wrapper storage immutable actor state. Otherwise, this is fundamentally unsafe.

The compiler could detect nonmutating setters within the property wrapper and only error (or warn until Swift 6) on stored variables.

This still isn't safe because the accessor will still read from mutable actor-isolated state.

@theblixguy is going to start a discussion on the Swift Forums to brainstorm ways to enable using nonisolated property wrappers with actor state safely.

[b8591340]

Thanks for the comment Holly.

I see that proper safety would be ideal, but in the meantime applying @unchecked Sendable to the property wrapper and protecting reads and writes through concurrency-compatible locking such as os_unfair_lock wouldn’t work?

[hborla]

@b8591340 FWIW I agree that this is not a solution to the issues that you filed. We should keep those open.

[hborla]

I see that proper safety would be ideal, but in the meantime applying @unchecked Sendable to the property wrapper and protecting reads and writes through concurrency-compatible locking such as os_unfair_lock wouldn’t work?

The problem is that there isn't a way to protect access to the backing storage in the actor. I don't believe there's a way to mark variables as nonisolated today, even acknowledging that it's unsafe. We would need to bring back nonisolated(unsafe) or similar to apply to the backing property wrapper storage in the actor, but nonisolated(unsafe) was deliberately removed from SE-0306: Actors during the evolution review process.

[b8591340]

I guess I could have a faulty model of nonisolated.

When you say backing storage do you mean that computed vars in nonisolated properties store actor-local state? That is, is the following concoction unsound assuming UIViewSubclass is installed within a view hierarchy and its offset property is mutated from another thread or task?

import Foundation
import UIKit
import os

let offsets = Offsets()

final class Offsets: @unchecked Sendable {
    var lock = os_unfair_lock_s()
    var data = [Int]()

    func new(offset: Int) -> Int {
                os_unfair_lock_lock  (&lock)
        defer { os_unfair_lock_unlock(&lock) }
        let index = data.count
        data.append(offset)
        return index
    }

    func write(offset value: Int, at index: Int) {
                os_unfair_lock_lock  (&lock)
        defer { os_unfair_lock_unlock(&lock) }
        data[index] = value
    }

    func read(at index: Int) -> Int {
                os_unfair_lock_lock  (&lock)
        defer { os_unfair_lock_unlock(&lock) }
        return data[index]
    }
}

@propertyWrapper
struct Wrapper: @unchecked Sendable {
    var wrappedValue: Int
      {               _read { yield  offsets.read(at: index) }
        nonmutating     set {        offsets.write(offset: newValue, at: index) } }

    @inlinable
    public init(wrappedValue offset: Int)
      { index = offsets.new(offset: offset) }

    public let index: Int
}

final class UIViewSubclass: UIView {
    @Wrapper nonisolated var offset = 0
}

[hborla]

When you say backing storage do you mean that computed vars in nonisolated properties store actor-local state?

For nonisolated property wrappers, yes. The problem is here:

final class UIViewSubclass: UIView {
  @Wrapper nonisolated var offset = 0
}

The property wrapper transformation does this:

// Implicitly @MainActor isolated
final class UIViewSubclass: UIView {
  private var _offset: Wrapper<Int> = Wrapper(wrappedValue: 0) // <--- MainActor-isolated mutable state!

  nonisolated var offset: Int {
    get { return _offset.wrappedValue } // <--- error! access to MainActor-isolated state in a nonisolated context
  }
}

The fact that the computed property was synthesized was circumventing the regular actor isolation checking, which is the bug that was fixed by this PR.

[b8591340]

I see, thanks Holly!

Would making the property wrapper implementation emit a let for immutable structs (like the above) entail significant (i.e. out-of-scope) effort?

It seems like it would be a first-step measure that would be as safe as @unchecked Sendable for nonmutating setters. It could rely on the property wrapper being Sendable when a nonmutating setter is both found within the wrapper and used in a nonisolated capacity.