NSDictionary fix when key is a String rather than NSString. #1438
Conversation
|
Can you add a unit test that would crash if this fix wasn't applied to catch it accidentally getting reverted in the future, thanks. |
|
@swift-ci please test |
johnno1962
changed the title
Foundation/NSDictionary.swift
Outdated
| @@ -586,7 +586,8 @@ open class NSMutableDictionary : NSDictionary { | |||
| guard type(of: self) === NSDictionary.self || type(of: self) === NSMutableDictionary.self else { | |||
| NSRequiresConcreteImplementation() | |||
| } | |||
| _storage[(aKey as! NSObject)] = _SwiftValue.store(anObject) | |||
| let aKey = (aKey as? NSObject) ?? NSString(string: aKey as! String) | |||
There was a problem hiding this comment.
Please do not special-case String/NSString here. Can you use _SwiftValue.store(aKey) instead?
There was a problem hiding this comment.
I agree, _SwiftValue.store is the way we are trying to work-around not having bridges and all bridged conversions should be written into the same funnel point
|
cc @phausler — any reason why we shouldn't use SwiftValue here that I'm not seeing? |
|
@swift-ci please test |
|
It looks like the test failure is unrelated. |
| @@ -222,6 +223,12 @@ class TestNSDictionary : XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| func test_settingWithStringKey() { | |||
| let dict = NSDictionary() | |||
There was a problem hiding this comment.
Don't you mean NSMutableDictionary()?
On macOS, the test code as given results in a compile-time error: error: cannot assign through subscript: subscript is get-only.
There was a problem hiding this comment.
Whoops. Thanks @xwu. Fixed and squashed.
…get the following crash: Could not cast value of type 'Swift.AnyHashable' (0x1027b3b10) to 'SwiftFoundation.NSObject' (0x101a56668). 2018-02-16 14:30:46.355774+0000 TestFoundation[7306:8715333] Could not cast value of type 'Swift.AnyHashable' (0x1027b3b10) to 'SwiftFoundation.NSObject' (0x101a56668). (lldb) up frame swiftlang#9: 0x000000010165b114 SwiftFoundation`NSMutableDictionary.subscript.setter(newValue=some, key=Swift.AnyHashable @ 0x00007fff5fbfcd88, self=0x000060800e181fe0) at NSDictionary.swift:649 646 } 647 set { 648 if let val = newValue { -> 649 �[4ms�[0metObject(val, forKey: key) 650 } else { 651 removeObject(forKey: key) 652 } (lldb) up frame swiftlang#10: 0x00000001004c0443 TestFoundation`static Dictionary.twEncode(data=TestFoundation.TwoWayMirror @ 0x00007fff5fbfce00, self=[Key : Value]) at TwoWayMirror.swift:386 383 #if os(Linux) 384 let key = NSString(string: key as! String) 385 #endif -> 386 dict[key] �[4m=�[0m TwoWayMirror.encode(mirror: &mirror) 387 } 388 return dict 389 } (lldb)
|
@swift-ci please test |
|
@swift-ci please test and merge |
|
I think this is a candidate for backporting to the |
|
Absolutely! Can I leave that to you? |
|
Never mind, I’ve cherry picked a PR to 4.1-branch. |
|
Hopefully we can get CI to move on this one. |
|
@swift-ci please test and merge |
|
Thanks @millenomi, we got there in the end! |
Hi Apple,
Using a String as a key when setting a value in an NSDictionary causes a crash on Linux. This minor fix brings it into line with Darwin Foundation where this is possible.