Updates for Xcode 8.1, Swift 3.0.1 #425
Conversation
With Swift 3.0.1, Swift changed some things about the way it casts to NSNumber[1]. As a result (I think?) using `as` to cast `UInt` and `Float` values causes an EXC_BAD_ACCESS crash when decoding Swift dictionaries. There are two solutions to this issue: 1. Explicitly cast these values to `NSNumber` in the test data 2. Use `.uintValue` and `.floatValue` to convert from `NSNumber` to the expected type. Option 1 seemed like a poor choice because it meant users of Argo would potentially need to make source changes. This would have been unfortunate. Therefore, we choose to go with option 2, which should fix the issue for the users relying on this functionality, be backwards compatible with Swift 3.0, and be invisible to the users that have never used Argo to parse `UInt` values from a Swift dictionary (most likely the entirety of the Argo codebase). [1]: https://github.com/apple/swift-evolution/blob/master/proposals/0139-bridge-nsnumber-and-nsvalue.md
Relevant changes: - Disable code signing for frameworks. Apparently this is no longer required/suggested at compile time. This is a good change. - Enable `CLANG_WARN_INFINITE_RECURSION`. From the documentation: "Warn if all paths through a function call itself." - Enable `CLANG_WARN_SUSPICIOUS_MOVE`. From the documentation: "Warn about suspicious uses of std::move." This seems like it's unlikely for us to hit, but hey.
|
So I thought this crash seemed a bit weird, and I had a play around. I found that the two literals in the tests weren't actually resolving to numbers of the expected type: 277d491. Double literals will be doubles without any other type annotations, and likewise integer literals prefer Int over UInt. This snippet reproduces the crash: import Foundation
let a: Any = 1.1
let b = a as! NSNumber
let c = b as Float // crashBut then, this also crashes: import Foundation
let a: Any = 1000 as UInt
let b = a as! NSNumber
let c = b as Int // crashThis seems like a bug to me, and it's definitely a regression. While using Before this is merged we should change all the other places where we're using |
The casts with `as` have worked up until now, but really this feels like it's more explicit, more future proof, and possibly more efficient.
|
I asked Joe Groff about this: <script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script> <script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script> <script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script> <script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script> |
|
Seems legit 👍 What do you think about including 277d491? |
|
I don't think we actually need that (these tests are now green), and I don't think we want to promote that as an acceptable solution for end users. I'm actually happy that the tests caught this specific crash here, and would like to have similar crashes caught in the future. |
|
Sure, catching crashes is good. I was mostly thinking it would make sense to have the |
|
I don't think so? IMO, this test is testing exactly the behavior we want, which is that a native swift dictionary can be parsed via Argo alongside JSON and plists. I wouldn't want to need to add that type info to my swift dictionary in order to get it to work with Argo, so I don't think we should be adding the type info in our tests. |
gfontenot
force-pushed
the
gf-xcode-8-1
branch
from
ea849d1 to
b29a86b
Compare
I totally agree with you on this when it comes to literals. 👍 I suppose the behaviour I'm describing is more likely if a |
|
👍 we should push this asap with a version bump |
|
There's now a bug tracking this on Jira |
See individual commits for more info.