Skip to content
This repository has been archived by the owner on Feb 8, 2021. It is now read-only.

Issue when using nested keypaths when compiling with optimization + Swift 2.2 #135

Closed
stuartervine opened this issue Mar 22, 2016 · 23 comments

Comments

@stuartervine
Copy link

With Swift 2.2, I've run into an issue when decoding deep json using the dot syntax. The decoding crashes with the error:

Gloss was compiled with optimization - stepping may behave oddly; variables may not be available.
(lldb) 

The following test case fails with the above error:

import XCTest
import Gloss

class GlossTestTests: XCTestCase {

    func testExample() {
        let json = [
            "foo":["bar":"woo"]
        ]
        let woo = FooBar(json: json)?.woo ?? "none"
        print(woo)
    }

}

class FooBar: Decodable {
    let woo: String?

    required init?(json: JSON) {
        self.woo = "foo.bar" <~~ json
    }
}

I'm not sure if I'm missing something, but everything seemed ok with Swift 2.1.

@stuartervine
Copy link
Author

This looks like a bug in the ExtensionDictionary valueForKeyPath method. I'll submit a pull request - unfortunately testing for it is pretty difficult as it only occurs when optimizations are switched on. This causes a crash when trying to remove an element from the keys array - but it doesn't crash when not optimized.

@hkellaway hkellaway added the bug label Mar 22, 2016
@hkellaway hkellaway reopened this Mar 27, 2016
@hkellaway hkellaway changed the title Swift 2.2 dot syntax decoding Issue when using nested keypaths when compiling with optimization + Swift 2.2 Mar 27, 2016
@hkellaway
Copy link
Owner

@tholo has provided some additional context:

FWIW I am seeing this issue with some of my stuff too — but moving the statement to remove element 0 does not fix it — and indeed, at the point where it is being removed we already know that there are at least one element in the set of keys.

Building the framework (I am using Carthage) for Debug instead of Release does fix it, and it seems to me that this must be an issue not with Gloss but with Swift / LLVM somewhere...

@tholo
Copy link

tholo commented Apr 1, 2016

I did come up with a (not very pretty) workaround...

---Sources/ExtensionDictionary.swift      2016-03-29 11:11:46.000000000 -0700
+++ Sources/ExtensionDictionary.swift   2016-04-01 11:54:02.000000000 -0700
@@ -49,11 +49,9 @@
         guard let value = self[first] as? AnyObject else {
             return nil
         }
-
-        keys.removeAtIndex(0)
-
-        if !keys.isEmpty, let subDict = value as? JSON {
-            let rejoined = keys.joinWithSeparator(delimiter)
+
+        if keys.count > 1, let subDict = value as? JSON {
+            let rejoined = keys[1..<keys.endIndex].joinWithSeparator(delimiter)

             return subDict.valueForKeyPath(rejoined, withDelimiter: delimiter)
         }

Hm, if this is done then keys should be let and not var too, I guess...

@hkellaway
Copy link
Owner

thanks @tholo

@stuartervine if you have the time, can you try seeing if your problems are resolved when using branch bugfix/crash_valueforkeypath_when_optimized

@darkFunction
Copy link

I was having the same problem. bugfix/crash_valueforkeypath_when_optimized resolves it. Thanks!

@hkellaway
Copy link
Owner

thanks for that feedback (and @tholo for the solution), i'll do more testing and likely merge it

@hkellaway hkellaway added this to the 0.7.2 milestone Apr 15, 2016
@hkellaway
Copy link
Owner

now present in 0.7.2 🎉

@sfaxon
Copy link

sfaxon commented May 6, 2016

I think I've run into this issue running 0.7.2 and 0.7.3 in a different scenario.

func testNestedEncoding() {
    var result: [JSON?] = []
    result.append(Encoder.encode("stripe.account.legal_entity.last_name")("User"))
    result.append(Encoder.encode("stripe.account.legal_entity.dob.year")(2014))
    let x = jsonify(result)
    print(x)
}

Running this in a test with optimization turned on will stop on line 135 of ExtensionDictionary.swift with an EXC_BAD_INSTRUCTION error and print the same Gloss was compiled with optimization - stepping may behave oddly; variables may not be available message. Running without optimizations does not produce the error.

I'm getting this error with Xcode 7.3.1. and the newly released Gloss 0.7.3. I first noticed the problem when the error happened in an application distributed via Test Flight.

Am I missing something? Thanks!

@hkellaway hkellaway reopened this May 6, 2016
@hkellaway
Copy link
Owner

hkellaway commented May 6, 2016

hey @sfaxon - thanks for reporting. nope, you're not missing anything; i'll have to do more investigation

the temporary solution, of course, would be to not use nested keypaths for the time being

@hkellaway hkellaway removed this from the 0.7.2 milestone May 6, 2016
@ferusinfo
Copy link
Contributor

ferusinfo commented Jun 7, 2016

Reporting the same in my project, both on 0.7.2 and 0.7.3

Changing the Swift Optimization to None (on our custom build configuration called AdHoc) is resolving the problem, but that should be temporary.
Not using nested keypaths is not something we want to follow - changing that in our models would cause a lot of work..

@AndrewSB
Copy link
Contributor

Apologies for the unrelated interjection:

@tholo how did you embed that diff?

@tholo
Copy link

tholo commented Jun 21, 2016

On Jun 20, 2016, at 21:37, Andrew Breckenridge notifications@github.com wrote:

Apologies for the unrelated interjection:

@tholo https://github.com/tholo how did you embed that diff?

I … am not sure I understand the question. Can you rephrase it?

Thorsten

@AndrewSB
Copy link
Contributor

I was trying to ask how you embedded this code snippet

screen shot 2016-06-21 at 9 05 04 am

@tholo
Copy link

tholo commented Jun 21, 2016

Oh. Used the standard markdown for code, in this case specifying that it was in "diff" format...

So, pasted in some code (diff output, in this case), and wrapped it in backtick sections with the opening section saying

```diff
... lines ...
```

This is documented in Creating and highlighting code blocks

@AndrewSB
Copy link
Contributor

Ahh, thats a really cool feature.

Thanks @tholo

On Tue, Jun 21, 2016 at 9:42 AM Thorsten Lockert notifications@github.com
wrote:

Oh. Used the standard markdown for code, in this case specifying that it
was in "diff" format...

So, pasted in some code (diff output, in this case), and wrapped it in
backtick sections with the opening section saying

... lines ...

This is documented in Creating and highlighting code blocks
https://help.github.com/articles/creating-and-highlighting-code-blocks/


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#135 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ADo1dFwp0mfrJMRmY9TJSRYjwEQD5a3gks5qOBR7gaJpZM4H2j4U
.

@valeIT
Copy link

valeIT commented Jun 28, 2016

Reporting in as well and same:

Changing the Swift Optimization to None (on our custom build configuration called AdHoc) is resolving the problem, but that should be temporary.
Not using nested keypaths is not something we want to follow - changing that in our models would cause a lot of work..

Found it as well in an application distributed via Test Flight. I've disabled optimisations and now it works.

@ferusinfo
Copy link
Contributor

ferusinfo commented Jun 28, 2016

Here is a quick snippet for those of you who use CocoaPods and wants to disable the code optimization on the pods targets (put it at the end of your Podfile):

post_install do |installer_representation|
    installer_representation.pods_project.targets.each do |target|
            target.build_configurations.each do |config|
                if config.name == 'AdHoc'
                    config.build_settings['SWIFT_OPTIMIZATION_LEVEL'] ||= "-Onone"
                end
            end
    end
end

We are temporarily using this solution to build adhoc builds for our CI.
Hope that will help someone.

@hkellaway
Copy link
Owner

i just released 1.0.0, which is compatible with Swift 3.0 🎉 however, i was not comfortable including a feature that caused crashes at runtime - and had yet to figure out an implementation. therefore, nested keypaths have been removed.

apologies to those who have a lot of nested keypaths! models should be updated before moving to 1.0.0 (or 0.8.0 if you're using Swift 2.3). See more here https://github.com/hkellaway/Gloss#nested-keypaths-deprecation

@ferusinfo
Copy link
Contributor

Hey @hkellaway - after I read that - the best JSON library written in Swift, that we've been using in both commercial and in-house apps is just making us sad.

Could you work on the feature a little bit more?
We will stick with 0.7.x version for now, but we are moving to the Swift 3.0 in our newest project and moving away from Gloss that we use heavily will impact our development (we need to find a good alternative as we have a lot of nested keypaths models).

Does the optimization fix does not work for you at this time?
I am sure you can add a Pod post-install hookup to the documentation and it will make a lot of developers happy.

@hkellaway
Copy link
Owner

yes, it's likely possible to turn off optimization for the Release confguration of the library - but that's not what's expected of the configuration. so the choice was turning off the proper optimization level or keep this convenience feature. It's a nice-to-have, not a need-to-have, for any JSON parsing library in my mind - so I chose to stick with the proper settings. I totally get if that's a dealbreaker; I'd recommend you another library that does that if I knew of one.

I currently don't have the bandwidth to work on reincorporating it, though may eventually - so I can't give a guarantee. I'm open to others submitting PRs to that effect.

another possibility is creating your own fork, reapplying the implementation, and adding what can be done to adjust the optimization level.

@ed-mejia
Copy link

I agree with @hkellaway about keeping the right values for optimisation and dropping the feature.. but must to say as well like many others, that dot notation was a killer feature for me and it is really sad to go back to the messy nested handling ☹️

@hkellaway
Copy link
Owner

nested keypaths have been reintroduced as of the latest version, 1.1.0 🎉

you can thank @ferusinfo for that

@ed-mejia
Copy link

Yayyyyy!!!!! thanks @ferusinfo ... AWESOME!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants