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

Jump to bottom

[SR-10596] Decodable does not fail to decode Float as Int for whole numbers #3423

Open
swift-ci opened this issue May 1, 2019 · 8 comments
Open

[SR-10596] Decodable does not fail to decode Float as Int for whole numbers #3423

swift-ci opened this issue May 1, 2019 · 8 comments
Labels
bug Codable Foundation

Comments

@swift-ci
Copy link
Contributor

swift-ci commented May 1, 2019

Previous ID SR-10596
Radar rdar://problem/50371749
Original Reporter jozen (JIRA User)
Type Bug
Status In Progress
Resolution

Attachment: Download

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug, Codable
Assignee hungtruong (JIRA)
Priority Medium

md5: bc6d6a7e328908f8a562ab76fbb1e965

Issue Description:

Decodable should cause run time errors when attempting to parse Floats as Integers. However, this does not occur when the Float value is a whole number. Because Decodable will only fail at runtime, it is prudent that it correctly fails on these simpler cases as it can mask issues

import Foundation

struct Swifter: Decodable {
  let floatParsedAsInt: Int
}

let json = """
{
 "floatParsedAsInt": 1.0,
}
""".data(using: .utf8)!
let myStruct = try JSONDecoder().decode(Swifter.self, from: json)
print(myStruct.floatParsedAsInt) //output 1

By comparison the swift compiler would reject the follow code:

var intValue: Int = 1.0 // Error: cannot convert value of type 'Double' to specified type 'Int'

or

struct JSONStructureWithInt: Decodable {
  let intAsString: Int
}

let json = """
{
 "intAsString": "1",
}
""".data(using: .utf8)!

let myStruct1 = try JSONDecoder().decode(JSONStructureWithString.self, from: json) // Error: Expected to decode Int but found a string/data instead
print(myStruct1.intAsString)
@theblixguy
Copy link

cc @itaiferber

@millenomi
Copy link
Contributor

I don't have the JSON spec in front of me, but IIRC in JavaScript all numbers are Double and they are just treated as integer in specific instances. I would not be surprised if JSON mandates this be the case.

@swift-ci
Copy link
Contributor Author

Comment by Hung Truong (JIRA)

@millenomi ah interesting, perhaps I should write some more test cases with an "Int" in the JSON to see how it behaves. Though I think there are already a few tests in that suite that serialize and deserialize some Ints.

@itaiferber
Copy link
Contributor

Just to copy my comments from hungtruong (JIRA User)'s PR:

Fundamentally, JSON doesn't have multiple data types beyond "number", and the value 1 is no different from 1.0 as far as JSON is concerned. If a number can be exactly represented as an integer, I don't see a reason to prevent it from decoding as such.

We have to consider backwards compatibility, too — it's entirely possible there are folks relying on being able to decode 1.0 as an Int, and we'd need a very compelling reason to break the behavior on them.

@swift-ci
Copy link
Contributor Author

Comment by Julian (JIRA)

So some background here. I filed this bug after I shipped a pretty big bug into production. That was totally my fault for screwing up the data parsing, but I was pretty surprised to find that the API didn’t fail on runtime for an incorrect parse when I had specified the parse incorrectly

@swift-ci
Copy link
Contributor Author

Comment by Julian (JIRA)

Obviously I’ve fixed my code and updated my tests to include some non-whole numbers since that incident

@swift-ci
Copy link
Contributor Author

Comment by Hung Truong (JIRA)

jozen (JIRA User) I typically prefer to fail early on cases like these where an edge case in the implementation could cause unexpected behavior like the one you describe. I think the challenge here is to take a really weakly typed language like Javascript and convert it to a strongly typed language like Swift. And I think @itaiferber does bring up a good point about backwards compatibility, though hopefully devs would be able to test the new version of Swift and make updates accordingly before any bugs made it into production. I'm still sort of on the fence on this but I can see why we'd want a really compelling reason to make a change like this.

@swift-ci
Copy link
Contributor Author

Comment by Julian (JIRA)

Just wanted to follow up here and thank you guys for taking a look at my ticket!

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Codable Foundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@itaiferber @millenomi @theblixguy @swift-ci