Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-9629] Swift overlay method willChangeValue(for:withSetMutation:using:) has incorrect signature #3560

Open
mayoff opened this issue Jan 9, 2019 · 4 comments

Comments

@mayoff
Copy link

mayoff commented Jan 9, 2019

Previous ID SR-9629
Radar None
Original Reporter @mayoff
Type Bug
Environment

Apple Swift version 4.2.1 (swiftlang-1000.11.42 clang-1000.11.45.1)

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: f4712f1ea33c722bda45fa956c7ddb3e

Issue Description:

The Swift Foundation overlay defines these two methods:

    public func willChangeValue<Value>(for keyPath: __owned KeyPath<Self, Value>, withSetMutation mutation: NSKeyValueSetMutationKind, using set: Set<Value>) -> Void {
        (self as! NSObject).willChangeValue(forKey: _bridgeKeyPathToString(keyPath), withSetMutation: mutation, using: set)
    }

    public func didChangeValue<Value>(for keyPath: __owned KeyPath<Self, Value>, withSetMutation mutation: NSKeyValueSetMutationKind, using set: Set<Value>) -> Void {
        (self as! NSObject).didChangeValue(forKey: _bridgeKeyPathToString(keyPath), withSetMutation: mutation, using: set)
    }

The signatures of these methods are wrong.

Here's an example of how they ought to be used:

import Foundation

class MyStringCollection: NSObject {

    @objc var strings: Set<String> { return _strings }

    @objc class var automaticallyNotifiesObserversOfStrings: Bool { return false }

    func add(_ string: String) {
        if _strings.contains(string) { return }

        let keyPath = \MyStringCollection.strings
        let mutation = NSKeyValueSetMutationKind.union
        let set: Set<String> = [string]
        willChangeValue(for: keyPath, withSetMutation: mutation, using: set)
        _strings.insert(string)
        didChangeValue(for: keyPath, withSetMutation: mutation, using: set)
    }

    private var _strings = Set<String>()
}

But the compiler rejects this example:

withSetMutationBug.swift:15:73: error: cannot convert value of type 'Set<String>' to expected argument type 'Set<Set<String>>'
        willChangeValue(for: keyPath, withSetMutation: mutation, using: set)
                                                                        ^~~
withSetMutationBug.swift:17:72: error: cannot convert value of type 'Set<String>' to expected argument type 'Set<Set<String>>'
        didChangeValue(for: keyPath, withSetMutation: mutation, using: set)

The problem is that the keyPath argument's generic type is KeyPath<Self, Value> and the set argument's generic type is Set<Value>. Based on the keyPath, Value needs (in my example) to be Set<String> but based on the set, Value needs to be just String.

The methods need to be changed to have usable signatures. This should work:

    public func willChangeValue<Setlike, Value>(for keyPath: __owned KeyPath<Self, Setlike>, withSetMutation mutation: NSKeyValueSetMutationKind, using set: Set<Value>) -> Void where Setlike: Collection, Setlike.Element == Value {
        (self as! NSObject).willChangeValue(forKey: _bridgeKeyPathToString(keyPath), withSetMutation: mutation, using: set)
    }

    public func didChangeValue<Setlike, Value>(for keyPath: __owned KeyPath<Self, Setlike>, withSetMutation mutation: NSKeyValueSetMutationKind, using set: Set<Value>) -> Void where Setlike: Collection, Setlike.Element == Value {
        (self as! NSObject).didChangeValue(forKey: _bridgeKeyPathToString(keyPath), withSetMutation: mutation, using: set)
    }
@mayoff
Copy link
Author

mayoff commented Jan 9, 2019

The methods are defined in swift/stdlib/public/Darwin/Foundation/NSObject.swift.

@belkadan
Copy link

Seems like an accurate description to me (whoops!). This is source-breaking, but in practice the current signature seems impossible to use correctly, so it shouldn't actually break any working apps.

Rob's proposed signature is better written without the Value parameter at all:

    public func willChangeValue<Setlike>(for keyPath: __owned KeyPath<Self, Setlike>, withSetMutation mutation: NSKeyValueSetMutationKind, using set: Set<Setlike.Element>) -> Void where Setlike: Collection {

Another possible signature:

    public func willChangeValue<Value>(for keyPath: __owned KeyPath<Self, Set<Value>>, withSetMutation mutation: NSKeyValueSetMutationKind, using set: Set<Value>) -> Void {

cc @millenomi, @Catfish-Man

@Catfish-Man
Copy link
Member

Yup, that seems like an accurate assessment to me. Sigh… this is what happens when I do things in a hurry :/ thanks for catching this Rob!

@swift-ci
Copy link
Contributor

Comment by Karthikkeyan Bala Sundaram (JIRA)

Hi @Catfish-Man & @mayoff,

Looks like this bug is not assigned to no one, Can I pick this task to update the methods signatures and give a PR.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants