Cleanup Conditional Bridge Behavior of NSNumber to Bool #1703
Conversation
|
Good catch. LGTM except the change that's not needed, which you've already noted. |
|
I've now added the commit to undo the unnecessary change. The Foundation tests look good on Darwin (except SR-8819), and my own unit tests that discovered this bug are passing again, so I don't have anything else I can think of to test on this. |
|
@swift-ci test |
There was a problem hiding this comment.
The base changes look good, though personally, I'd prefer to keep the changes required to run the tests on Darwin separate from this PR. I've filed Radars for those and @millenomi is tracking them (though I'm sure we'd be happy to get those fixes, just separately).
As for the test failures: if the new tests are going to fail due to SR-8819, can we please comment them out for now and leave a comment in there so we can try to keep CI green?
|
@itaiferber One thing to point out, testNSNumberToBool() should already be failing right now. How is it not? My change doesn't introduce any new failures, but rather XCTAssertNil() in that test is effectively always going to fail, at least on Darwin with Xcode 10.0 today. Is it possible that the Linux build of Swift/Foundation doesn't have SR-8819 affecting it? |
|
@Kaiede I'm not sure, to be honest — it might be related to architecture/OS differences between the version of Swift you tested on and what we've got in CI. It's worth seeing if this test run passes or not; if it does then we have some signal that SR-8819 affects your current config, but not what we have in CI (for better or worse). |
|
@itaiferber I guess I answered my own question, the Linux CI is passing testNSNumberToBool() properly, even with my changes. So I guess SR-8819 does only affect building Foundation on Darwin. I'll go ahead and add that information to the bug then, since the limited scope is probably good to know about (and reduces any priority on fixing it, I bet). |
|
@Kaiede Ah, I had misread your initial report — I mistakenly assumed that this was happening on Linux (as I had read In which case, the tests are fine to leave as-is. (Though I think that pulling out the unrelated Darwin changes is likely for the best. I'll let @millenomi speak to that though.) |
|
I'll go ahead and cherry-pick the Darwin build fixes into their own PR. They were mostly required so that I could at least use the unit tests to debug/validate the change on Darwin. I haven't setup an x86_64 Linux VM yet (my focus has been on armv7 and aarch64). Probably should. |
The original change for this did not check the behavior. A better implementation here is to use the one from Swift itself, which also matches the initializer that this extension also adds.
|
I've pulled them out, squashed the partial revert, and updated the primary comment to reflect the true state of the fix. |
|
@swift-ci Please test |
|
Once we're good here, let's put the fix on the 4.2 branch too. |
|
@swift-ci please test and merge |
-- Main Change:
Bring over the implementation of the conditional bridge from the Swift stdlib for NSNumber to Bool, which matches with the initializer of NSNumber to Bool. The reason for this is that the current implementation truncates Float/Double to perform the check. This diverges from the initializer's behavior which only matches 0.0 and 1.0, specifically.
It also adds a couple checks to the Foundation Tests to avoid regressing Double/Float to Bool behavior.
This issue is also responsible for a regression in http://github.com/IBM-Swift/SwiftyJSON/ which was depending a bit on 'as? Bool' returning nil which was true in 4.1.3.
-- Further Issue (Darwin Only):
I've opened https://bugs.swift.org/browse/SR-8819 to track an issue where Swift on Darwin is emitting unconditional casts for
NSNumber(value: 0.25) as? Bool. This appears to work correctly on Linux, though, and the tests pass.