Skip to content
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

JSON: fix 0/1 integer parsing #786

Merged
merged 7 commits into from
Oct 6, 2020
Merged

JSON: fix 0/1 integer parsing #786

merged 7 commits into from
Oct 6, 2020

Conversation

djbe
Copy link
Member

@djbe djbe commented Oct 5, 2020

Fixes #781

With JSONSerialization, 0 and 1 are parsed as NSNumbers. The issue is that these values are considered valid boolean values, so the is Bool or is [Bool] tests succeed.

This PR addresses the issue using runtime introspection.

Implementation

There is no "simple" check to see what's actually inside an NSNumber instance. We first have to check if it's a boolean, and if not, get the internal type of the number. We then map these to the info we actually want.

The fix for Arrays is a bit more involved, because [NSNumber] is tool free bridged to [Int], [Bool], etc... And the same applies to the inverse (a [Bool] is bridged to [NSNumber]). When we just check if something is [NSNumber] (and sometimes bridge it), we lose the easy check to see if all the elements in an array have the same type. We'd have to go through each element and check their type.

Without this extra array inspection (see commit b12aaa3), things break. In the old method, [0.1, 2, true] is considered a [Double] array (see mixed2 in the tests). This generates wrong Swift code, which causes compilation to fail:

static let mixed2: [Double] = [0.1, 2, true]

Note

This issue does not appear in the YAML or Plist parsers. I've added a bunch of tests for all 3 parsers to verify some scenarios.

@djbe djbe added this to the 6.4.0 milestone Oct 5, 2020
@SwiftGen-Eve
Copy link

SwiftGen-Eve commented Oct 5, 2020

1 Warning
⚠️ Big PR

Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!


Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

@djbe djbe force-pushed the bugfix/json-integer-parsing branch from 2485184 to ee5e534 Compare October 5, 2020 18:09
@djbe djbe requested a review from AliSoftware October 5, 2020 20:04
@djbe djbe force-pushed the bugfix/json-integer-parsing branch from 810ee0f to d4d26d2 Compare October 5, 2020 20:05
@djbe djbe force-pushed the bugfix/json-integer-parsing branch from d4d26d2 to b12aaa3 Compare October 5, 2020 20:06
@AliSoftware
Copy link
Collaborator

What about using Codable (and our AnyCodable helper recently introduced to fix a similar issue in Plist) instead?

@djbe
Copy link
Member Author

djbe commented Oct 5, 2020

@AliSoftware see #781 where I did some tests with AnyCodable, the performance isn't that good unfortunately.

Copy link
Collaborator

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we're adding more and more special cases as we encounter differences between JSONSerialization/Codable/YAMS interpreting things differently each; that also doesn't inspire confidence regarding unit tests (which always go through the intermediate state of YAML representation while real run doesn't)…

I really wish we could find a satisfying unified solution for those behavior diffs between test vs run one day…

Apart from that, the fix looks ok to me and should do (until we hopefully solve the bigger problem one day?)

Sources/SwiftGenKit/Utils/Metadata.swift Show resolved Hide resolved
@djbe
Copy link
Member Author

djbe commented Oct 6, 2020

There are actually quite a few things going here (and all the other issues):

  • Wild world of unstructured input data, anything goes (unfortunately?)
  • Using JSONSerialization for performance reasons, which uses ObjC types internally
  • Our metadata runtime inspection
  • Swift implicit toll-free bridging, which we can't turn off even if we wanted to (for some cases)

I can always check if there are other (faster) JSON parsers directly to Swift types, although I'm not 100% sure it will fix everything. I think the other parsers are working correctly, it's only JSONSerialization + ObjC types that are throwing a wrench in things.

Note: let's also not forget that Stencil sometimes does weird things too, especially when outputting/writing the content of a variable.

@AliSoftware
Copy link
Collaborator

AliSoftware commented Oct 6, 2020

Yeah which is why even if I think the ideal solution would be to find a fast parser that would parse directly into Swift types, I'm not sure such parser combining the two (Swift types + top performance) would be easy to find. So for now keeping NSJSONserialization + having those fixes to handle those "wild world of unstructured data" indeed, should be enough 👍

@djbe djbe merged commit 81507fc into develop Oct 6, 2020
@djbe djbe deleted the bugfix/json-integer-parsing branch October 6, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants