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

Updates for Xcode 8.1, Swift 3.0.1 #425

Merged
merged 3 commits into from Nov 2, 2016

Conversation

Projects
None yet
3 participants
@gfontenot
Collaborator

gfontenot commented Nov 2, 2016

See individual commits for more info.

gfontenot added some commits Nov 2, 2016

Update cast method for Float/UInt
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
Update to recommended settings for Xcode 8.1
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.
@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Nov 2, 2016

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 // crash

But then, this also crashes:

import Foundation
let a: Any = 1000 as UInt
let b = a as! NSNumber
let c = b as Int // crash

This seems like a bug to me, and it's definitely a regression. While using NSNumber's *Value properties cast the value safely, I don't think we should have to do it that way. Having said that, I think it makes sense to fix this crash ASAP.

Before this is merged we should change all the other places where we're using as to cast an NSNumber, because they're all potentially broken.

sharplet commented Nov 2, 2016

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 // crash

But then, this also crashes:

import Foundation
let a: Any = 1000 as UInt
let b = a as! NSNumber
let c = b as Int // crash

This seems like a bug to me, and it's definitely a regression. While using NSNumber's *Value properties cast the value safely, I don't think we should have to do it that way. Having said that, I think it makes sense to fix this crash ASAP.

Before this is merged we should change all the other places where we're using as to cast an NSNumber, because they're all potentially broken.

Use properties for all NSNumber transformations
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.
@gfontenot

This comment has been minimized.

Show comment
Hide comment
@gfontenot

gfontenot Nov 2, 2016

Collaborator

I asked Joe Groff about this:

It seems like this: https://t.co/j20ee1uN9W

might have been an accidental side effect of this? https://t.co/7lZr4c9NZO

/cc @jckarter

— Gordon Fontenot (@gfontenot) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>

@GFontenot The intended behavior is that a bridged NSNumber only bridges back to its original Swift type. We probably shouldn't allow `as`

— Joe Groff (@jckarter) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>

@jckarter would you recommend that we remove all of those casts and use the methods instead? Seems like that might be safer long term?

— Gordon Fontenot (@gfontenot) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>

@GFontenot Yeah, the properties would be safer, and probably more efficient.

— Joe Groff (@jckarter) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>
Collaborator

gfontenot commented Nov 2, 2016

I asked Joe Groff about this:

It seems like this: https://t.co/j20ee1uN9W

might have been an accidental side effect of this? https://t.co/7lZr4c9NZO

/cc @jckarter

— Gordon Fontenot (@gfontenot) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>

@GFontenot The intended behavior is that a bridged NSNumber only bridges back to its original Swift type. We probably shouldn't allow `as`

— Joe Groff (@jckarter) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>

@jckarter would you recommend that we remove all of those casts and use the methods instead? Seems like that might be safer long term?

— Gordon Fontenot (@gfontenot) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>

@GFontenot Yeah, the properties would be safer, and probably more efficient.

— Joe Groff (@jckarter) November 2, 2016
<script async src="//platform.twitter.com/widgets.js" charset="utf-8"></script>
@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Nov 2, 2016

Seems legit 👍 What do you think about including 277d491?

sharplet commented Nov 2, 2016

Seems legit 👍 What do you think about including 277d491?

@gfontenot

This comment has been minimized.

Show comment
Hide comment
@gfontenot

gfontenot Nov 2, 2016

Collaborator

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.

Collaborator

gfontenot commented Nov 2, 2016

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.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Nov 2, 2016

Sure, catching crashes is good. I was mostly thinking it would make sense to have the "float" key in the tests actually be bridging a Float, rather than a Double (for example). Testing that a JSON number can be safely decoded as any Swift numeric type feels like a slightly different test?

sharplet commented Nov 2, 2016

Sure, catching crashes is good. I was mostly thinking it would make sense to have the "float" key in the tests actually be bridging a Float, rather than a Double (for example). Testing that a JSON number can be safely decoded as any Swift numeric type feels like a slightly different test?

@gfontenot

This comment has been minimized.

Show comment
Hide comment
@gfontenot

gfontenot Nov 2, 2016

Collaborator

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.

Collaborator

gfontenot commented Nov 2, 2016

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.

@sharplet

This comment has been minimized.

Show comment
Hide comment
@sharplet

sharplet Nov 2, 2016

I wouldn't want to need to add that type info to my swift dictionary in order to get it to work with Argo

I totally agree with you on this when it comes to literals. 👍 I suppose the behaviour I'm describing is more likely if a Float, CGFloat, UInt, etc. is passed in as a parameter or some other place where the type annotation is explicit.

sharplet commented Nov 2, 2016

I wouldn't want to need to add that type info to my swift dictionary in order to get it to work with Argo

I totally agree with you on this when it comes to literals. 👍 I suppose the behaviour I'm describing is more likely if a Float, CGFloat, UInt, etc. is passed in as a parameter or some other place where the type annotation is explicit.

@tonyd256

This comment has been minimized.

Show comment
Hide comment
@tonyd256

tonyd256 Nov 2, 2016

Contributor

👍 we should push this asap with a version bump

Contributor

tonyd256 commented Nov 2, 2016

👍 we should push this asap with a version bump

@gfontenot

This comment has been minimized.

Show comment
Hide comment
@gfontenot
Collaborator

gfontenot commented Nov 2, 2016

@gfontenot gfontenot merged commit b29a86b into master Nov 2, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!

@gfontenot gfontenot deleted the gf-xcode-8-1 branch Nov 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment