Skip to content

[SR-5206] [Codable] The encoded results of Optional<URL> are different between Container.encode (_: ...) and Container.encodeIfPresent (_: ...) . #3846

Closed
Closed
[SR-5206] [Codable] The encoded results of Optional<URL> are different between Container.encode (_: ...) and Container.encodeIfPresent (_: ...) .#3846
Assignees
itaiferber
Labels
FoundationStandard Librarybug

Description

@norio-nomura
norio-nomura
opened on Jun 13, 2017
Previous ID SR-5206
Radar rdar://problem/32535991
Original Reporter @norio-nomura
Type Bug
Status Closed
Resolution Done
Environment

Xcode 9 beta 1
swift-4.0-DEVELOPMENT-SNAPSHOT-2017-06-11-a

Additional Detail from JIRA
Votes 0
Component/s Foundation, Standard Library
Labels Bug
Assignee @itaiferber
Priority Medium

md5: 0c154bae2e74b21f73dca582213bd87d

is duplicated by:

  • SR-6112 arrays and dictionaries do not call container encode for the elements

Issue Description:

This issue was founded at https://gist.github.com/ken0nek/4a84f5fb1d13e4201907ddcdcccc3cf1

Simplified version:

import Foundation

struct S {
    let url: URL?
}

extension S: Codable {
    enum CodingKeys: String, CodingKey { case url }

    public init(from decoder: Decoder) throws {
        let container = try decoder.container(keyedBy: CodingKeys.self)
        url = try container.decode(Optional<URL>.self, forKey: .url)
    }

    public func encode(to encoder: Encoder) throws {
        var container = encoder.container(keyedBy: CodingKeys.self)
        try container.encode(url, forKey: .url)
    }
}

do {
    let s = S(url: URL(string: "https://apple.com"))
    let payload = try JSONEncoder().encode(s)
    // {"url":{"relative":"https:\/\/apple.com"}}
    print(String(data: payload, encoding: .utf8)!)
    print(try JSONDecoder().decode(S.self, from: payload))
} catch {
    // typeMismatch(Swift.SingleValueDecodingContainer, Swift.DecodingError.Context(codingPath: [Optional(__lldb_expr_57.S.CodingKeys.url)], debugDescription: "Cannot get single value decoding container -- found keyed container instead."))
    print(error)
}

On implementing Codable conformance, if it uses container.encode(_:…) instead of container.encodeIfPresent(_:…), container.encode(url, forKey: .url) produces dictionary. But, `JSONDecoder` tries to decode Optional<URL> from string.

This behavior is caused by https://github.com/itaiferber/swift/blob/a0f5c102aeef0c9812b5e7424ef5c3a880824b56/stdlib/public/core/Codable.swift#L3249
It should call container.encode(_:) instead of (wrapped as! Encodable).encode(to: encoder).

Activity

  1. norio-nomura commented on Jun 13, 2017

    @norio-nomura
    norio-nomura
    on Jun 13, 2017
    ContributorAuthor

    @itaiferber Can you see this issue?

  2. norio-nomura commented on Jun 13, 2017

    @norio-nomura
    norio-nomura
    on Jun 13, 2017
    ContributorAuthor

    Perhaps I think that this issue is caused by wrong usage of the API.
    If Xcode can generate the same code as the Codable implementation generated by the compiler, such a mistake will decrease.

  3. itaiferber commented on Jun 13, 2017

    @itaiferber
    itaiferber
    on Jun 13, 2017
    Contributor

    Unfortunately, this is expected behavior that we can't do much about. This is a consequence of not yet having conditional conformance in Swift — the compiler has no static guarantee that Wrapped is Encodable. Even though we can do a dynamic check, there is no way to force it statically so we can make the generic encode<T : Encodable>(_🙂 call. At best, we can cast the value to Encodable, but at that point, we've lost type information that it was Wrapped; the only thing we can then call is encode(to🙂 and pass the encoder in, but that never gives the container a chance to intercept and replace the value or change its format.

    This will be resolved when we have conditional conformance and can make that static assertion.

  4. norio-nomura commented on Jun 14, 2017

    @norio-nomura
    norio-nomura
    on Jun 14, 2017
    ContributorAuthor

    This will be resolved when we have conditional conformance and can make that static assertion.

    Hmm.
    If type checking at runtime is required within the container implementation, it would be better to use encode(_: Encodable) rather than the generic encode<T: Encodable>(_:) I think.

  5. itaiferber commented on Jun 14, 2017

    @itaiferber
    itaiferber
    on Jun 14, 2017
    Contributor

    @norio-nomura That would work on encode(), but not on decode(), which has to accept the type as an argument. It's better to be consistent so you can actually round-trip data, instead of being correct in only one direction.

  6. norio-nomura commented on Jun 15, 2017

    @norio-nomura
    norio-nomura
    on Jun 15, 2017
    ContributorAuthor

    Ah, I see. I didn't think about decode().
    We will wait for the conditional conformance to come. 🙂

    By the way, I filed a rdar://problem/32752900 as suggesting:

    If Xcode can generate the same code as the Codable implementation generated by the compiler, such a mistake will decrease.

  7. itaiferber commented on Jun 15, 2017

    @itaiferber
    itaiferber
    on Jun 15, 2017
    Contributor

    @norio-nomura Thanks! We've got a Radar tracking that internally — hopefully we'll be able to get that in an upcoming version of Xcode.

  8. itaiferber commented on Jul 5, 2017

    @itaiferber
    itaiferber
    on Jul 5, 2017
    Contributor

    Looks like there is actually a (small, non-scalable) workaround for this in the meantime, at least for JSONEncoder and JSONDecoder: PR-10766

  9. norio-nomura commented on Aug 2, 2017

    @norio-nomura
    norio-nomura
    on Aug 2, 2017
    ContributorAuthor

    @itaiferber How about following workaround?

    extension Decodable {
    fileprivate init(from container: SingleValueDecodingContainer) throws {
        self = try container.decode(Self.self)
    }
}

extension Optional : Decodable /* where Wrapped : Decodable */ {
    public init(from decoder: Decoder) throws {
        // Initialize self here so we can get type(of: self).
        self = .none
        assertTypeIsDecodable(Wrapped.self, in: type(of: self))

        let container = try decoder.singleValueContainer()
        if !container.decodeNil() {
            let metaType = (Wrapped.self as! Decodable.Type)
            let element = try metaType.init(from: container)
            self = .some(element as! Wrapped)
        }
    }
}

    This would work with third party Decoders.

  10. itaiferber commented on Aug 2, 2017

    @itaiferber
    itaiferber
    on Aug 2, 2017
    Contributor

    @norio-nomura That looks... surprisingly reasonable. 🙂 I hadn't considered that, so thanks for pointing it out! I'll need to see how this plays with method dispatch (i.e. which overload of `SingleValueDecodingContainer.decode(_🙂` this will call for different types, or whether they'll always fall into the generic case). Unfortunately, we're past the deadline for the Swift 4.0 release for something like this, but this could be beneficial in an upcoming update.

  11. itaiferber commented on Aug 2, 2017

    @itaiferber
    itaiferber
    on Aug 2, 2017
    Contributor

    @norio-nomura This worked out surprisingly nicely, actually: PR-11315

  12. norio-nomura commented on Aug 4, 2017

    @norio-nomura
    norio-nomura
    on Aug 4, 2017
    ContributorAuthor

    @itaiferber I confirmed that this issue has been resolved on swift-DEVELOPMENT-SNAPSHOT-2017-08-03-a. 😃
    Checking commits: jpsim/Yams@nn-SE-0166...nn-SR-5206
    Build log on travis: https://travis-ci.org/jpsim/Yams/jobs/260850551
    Thanks!
    After all, is there no possibility that this fix will enter swift 4.0?

  13. itaiferber commented on Aug 4, 2017

    @itaiferber
    itaiferber
    on Aug 4, 2017
    Contributor

    @norio-nomura Glad this resolved the issue, and thank you for the suggestion! Unfortunately, though, we're past the deadline on fixes for Swift 4.0 except for the most critical of bug fixes... This will make it to the next dot release, though.

  14. norio-nomura commented on Aug 6, 2017

    @norio-nomura
    norio-nomura
    on Aug 6, 2017
    ContributorAuthor

    @itaiferber This issue seems to be critical for third party Encoders since it is hard to writing workaround code for third party Encoders.
    If third party encoder does not write workaround, users of third party encoder will have to write migration code which will absorb the difference of behaviors of third party encoder before and after this issue correction in future releases. (Swift 4.0.1? 4.1?)
    And I can not imagine how to migrate when deserializing serialized data before this problem is fixed in the fixed release. 🙁

  15. norio-nomura commented on Aug 9, 2017

    @norio-nomura
    norio-nomura
    on Aug 9, 2017
    ContributorAuthor

    @itaiferber I would like to guard my tests by using {{#if swift()}} until this fix will be released. What version will this fix include? 4.0.1 or 4.1?

  16. norio-nomura commented on Nov 1, 2017

    @norio-nomura
    norio-nomura
    on Nov 1, 2017
    ContributorAuthor

    I confirmed that this issue has been fixed on Xcode 9.2 beta (9C32c). 👍

  17. itaiferber commented on Nov 2, 2017

    @itaiferber
    itaiferber
    on Nov 2, 2017
    Contributor

    @norio-nomura Thanks for confirming! 🙂

  18. swift-ci
    transferred this issue fromapple/swift-issueson Apr 25, 2022
  19. shahmishal
    transferred this issue fromswiftlang/swifton May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

FoundationStandard Librarybug

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions