Harden JSONEncoder.wrapEncodable + dictionary encoding regression test (skip-foundation#62) - #127
Open
vincentborko wants to merge 1 commit into
Open
Conversation
…on test On SKIP the generic `wrapEncodable` bound is weakened to `E: Any`, so its Encodable dispatch is an unchecked downcast. When a non-Encodable value reaches it the bare ClassCastException names nothing useful. Cast defensively and throw an EncodingError naming the concrete offending type instead, mirroring the EncodingError already thrown on the top-level encode path. Also add a JSONEncoder round-trip test for optional and nested dictionaries. Against skip-lib without the companion fix this reproduces skiptools#62 ("skip.lib.Tuple2 cannot be cast to skip.lib.Encodable") on Robolectric; with the fix it passes, producing the correct JSON object shape. Part of the fix for skiptools#62 (root fix: skiptools/skip-lib#46) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Companion to the root fix in skiptools/skip-lib#46. This PR is the skip-foundation half of #62.
Motivation
Encoding a
Codabletype with a populated dictionary property can crash with:The root cause is in skip-lib's type-erased Codable dispatch (a
Dictionaryis also aSequence<Tuple2>, so it was iterated as raw pairs) and is fixed in skiptools/skip-lib#46. This PR adds two things on the skip-foundation side.1. Defense-in-depth in
wrapEncodableOn SKIP the generic bound of
wrapEncodable<E: Encodable>is weakened toE: Any(see the// SKIP DECLAREabove it), so(encodable as Encodable).encode(...)is an unchecked downcast: any future value reaching this branch that is notEncodablefails with a bareClassCastExceptionnaming nothing. Cast defensively and throw anEncodingErrornaming the concrete offending type instead — mirroring theEncodingError.invalidValuealready thrown on the top-level encode path, and the "fail loud with an actionable message" pattern used elsewhere in the runtime. Behaviour is unchanged for genuineEncodablevalues.2. Regression test
Adds a
JSONEncoderround-trip/encode test (testDictionaryCodableTypeErased) covering an optional[String: String]?(the literal #62 shape, reached throughencodeIfPresent) and nested[String: [String: String]]dictionaries — shapes not previously exercised (the suite only had non-optional dictionaries, which already worked). Against unpatched skip-lib this reproduces theTuple2 cannot be cast to Encodablecrash on Robolectric; with skip-lib#46 it passes and produces the correct JSON object shape.The two nested fixtures are
Encodable-only, to isolate the #62 encode crash from an unrelated nested-dictionary decode codegen limitation (the transpiled decoder types the inner dictionary asDictionary<*, *>).Sequencing / CI note
The new test only passes once skip-lib#46 is released and this package's
skip-libdependency is bumped to include it. Until then it will fail against the currently pinnedskip-lib1.4.0 (reproducing the crash). ThewrapEncodablehardening in part 1 is independent and safe to land on its own. Sequencing is yours to decide — happy to split the test out into a follow-up if you'd prefer to merge the hardening first.Testing
swift test(native + transpiled/Robolectric), building against skip-lib#46 as a local dependency: the fullTestJSONsuite passes (5/5), including the new case; against unpatched skip-lib the new case reproduces the crash.swift testruns locally.