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-9076] NSSortDescriptor.keyPath is extremely broken #3607

Closed
lilyball mannequin opened this issue Oct 25, 2018 · 5 comments
Closed

[SR-9076] NSSortDescriptor.keyPath is extremely broken #3607

lilyball mannequin opened this issue Oct 25, 2018 · 5 comments
Assignees

Comments

@lilyball
Copy link
Mannequin

lilyball mannequin commented Oct 25, 2018

Previous ID SR-9076
Radar None
Original Reporter @lilyball
Type Bug
Status Resolved
Resolution Done
Environment

Tested in Apple Swift version 4.2 (swiftlang-1000.11.37.1 clang-1000.11.45.1)
Confirmed by inspecting the source of the latest Swift master (14d4235b571d7a4e2ae51854f0d665f96959d182)

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

md5: 88ed436db4b4df7ee10944ba48007057

Issue Description:

The NSSortDescriptor extension for working with KeyPath values is quite broken. The computed keyPath property relies on converting the String back into an AnyKeyPath by using a global table that is populated when converting the KeyPath into a String in the first place. The problem is, the String can't include the root type, so two different KeyPath values that have the same Obj-C keypath string will overwrite each other, and cause the NSSortDescriptor.keyPath property to return the wrong value.

Example:

import Foundation
class Foo: NSObject {
    @objc var name: String?
}
class Bar: NSObject {
    @objc var name: String?
}
let desc = NSSortDescriptor(keyPath: \Foo.name, ascending: true)
print(desc.keyPath == \Foo.name) // true
_ = NSSortDescriptor(keyPath: \Bar.name, ascending: true)
print(desc.keyPath == \Foo.name) // false
print(desc.keyPath == \Bar.name) // true

Besides equality, this also affects the behavior of the AnyKeyPath.appending(path:) method.

Personally, I think we should just do away with the keyPath property entirely. Barring that, I believe the correct solution here is to use Obj-C associated objects to associate the original KeyPath value with the NSSortDescriptor and do away with the global table.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

Upon second look, this is not actually the only client of the global keypath table, it was simply the only client of the global _bridgeStringToKeyPath(_: String) function. Unfortunately, the global keypath table is still used in the implementation of automaticallyNotifiesObservers(for:), and it's not clear how to fix that. I will file a separate ticket for that.

@belkadan
Copy link

cc @millenomi

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 25, 2018

I've got a proof-of-concept patch written for this already, but without tests yet. If I have time I'll publish it tonight.

@lilyball
Copy link
Mannequin Author

lilyball mannequin commented Oct 27, 2018

I've submitted PR #20103 for this issue.

@millenomi
Copy link
Contributor

Merged apple/swift@7c51419

@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
This issue was closed.
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

2 participants