fix(swift): correct AnyCodable NSNumber Int and Double encode fidelity#215
Open
joshmouch wants to merge 1 commit into
Open
fix(swift): correct AnyCodable NSNumber Int and Double encode fidelity#215joshmouch wants to merge 1 commit into
joshmouch wants to merge 1 commit into
Conversation
When a [String: Any] dictionary comes from JSONSerialization, numeric and boolean values are boxed as NSNumber. Swift pattern-matching bridges NSNumber to Bool, Int, and Double indiscriminately, so the existing switch hit `case let bool as Bool` for any NSNumber, encoding integer 1 as `true` and double 1.5 as integer 1. Add an NSNumber guard at the top of encode(to:) that inspects objCType and dispatches faithfully: 'c'/'B' to boolValue, 'f'/'d' to doubleValue, all others to int64Value. The `type(of:) != Bool.self` check keeps native Swift Bool values (which are not NSNumber) on the existing path. Apply the same fix to the generator's anyCodableContent() template and reconcile its pre-existing drift from the on-disk file (the @unchecked Sendable rationale and the [Any]/[String: Any] equality arms) so a fresh scaffold reproduces AnyCodable.swift byte-for-byte. Tests cover Int, Bool, and Double from JSONSerialization, plus a native Swift Bool and a Float-backed NSNumber. Fixes microsoft#123.
There was a problem hiding this comment.
Pull request overview
This PR fixes Swift AnyCodable.encode(to:) so that values originating from JSONSerialization (NSNumber-backed) re-encode with the correct JSON primitive type, preventing Int→Bool and Double→integer corruption. It also aligns the Swift code generator’s AnyCodable scaffold with the checked-in implementation and adds regression tests plus a changelog entry.
Changes:
- Add an
NSNumberfast-path inAnyCodable.encode(to:)that dispatches byobjCTypebefore Swift pattern matching. - Update
scripts/generate-swift.ts’sAnyCodablescaffold to match the on-disk implementation. - Add Swift tests covering NSNumber round-trips through
JSONSerialization, and document the fix in the Swift changelog.
Show a summary per file
| File | Description |
|---|---|
| scripts/generate-swift.ts | Updates the AnyCodable scaffold template to include the NSNumber objCType dispatch and reconcile drift. |
| clients/swift/CHANGELOG.md | Adds an Unreleased “Fixed” entry describing the NSNumber encode-fidelity fix. |
| clients/swift/AgentHostProtocol/Tests/AgentHostProtocolTests/AnyCodableTests.swift | Adds regression tests for NSNumber-backed Int/Bool/Double, plus native Bool and Float-backed NSNumber coverage. |
| clients/swift/AgentHostProtocol/Sources/AgentHostProtocol/AnyCodable.swift | Implements the NSNumber objCType dispatch in encode(to:) ahead of Swift type pattern matching. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 3
Comment on lines
+52
to
+62
| switch objCType { | ||
| case 0x63 /* 'c' */, 0x42 /* 'B' */: | ||
| try container.encode(number.boolValue) | ||
| return | ||
| case 0x66 /* 'f' */, 0x64 /* 'd' */: | ||
| try container.encode(number.doubleValue) | ||
| return | ||
| default: | ||
| try container.encode(number.int64Value) | ||
| return | ||
| } |
Comment on lines
+1646
to
+1656
| switch objCType { | ||
| case 0x63 /* 'c' */, 0x42 /* 'B' */: | ||
| try container.encode(number.boolValue) | ||
| return | ||
| case 0x66 /* 'f' */, 0x64 /* 'd' */: | ||
| try container.encode(number.doubleValue) | ||
| return | ||
| default: | ||
| try container.encode(number.int64Value) | ||
| return | ||
| } |
Comment on lines
+49
to
+56
| func testAnyCodableEncodePreservesFloatBackedNSNumber() throws { | ||
| // A Float-backed NSNumber (objCType 'f') must encode as a decimal, not an | ||
| // integer. This exercises the 'f' dispatch arm; the JSONSerialization path | ||
| // boxes JSON numbers as 'd'/'q' and never produces it. | ||
| let wrapped = AnyCodable(["x": NSNumber(value: Float(1.5))] as [String: Any]) | ||
| let bytes = try JSONEncoder().encode(wrapped) | ||
| XCTAssertEqual(String(data: bytes, encoding: .utf8), #"{"x":1.5}"#) | ||
| } |
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.
Fixes #123.
The bug
When wire JSON is parsed with
JSONSerialization(which boxes every number and boolean asNSNumber) and the result is wrapped inAnyCodable, re-encoding throughJSONEncodercorrupts the values:Intwrites astrue/falseandDoublewrites as an integer. Thecase let bool as Boolandcase let int as Intarms both match anyNSNumber, andBoolis matched first.The fix
An
NSNumberguard at the top ofencode(to:)inspectsobjCTypeand dispatches to the faithful primitive (boolValueforc/B,doubleValueforf/d,int64Valueotherwise) before the existing switch. Thetype(of:) != Bool.selfcheck keeps native SwiftBoolon the existing path. This is the dispatch from the issue's "Suggested fix".Two things worth flagging for review
scripts/generate-swift.tsis also edited, and it produces no change in the checked-in Swift.AnyCodable.swiftis a generator scaffold (generate-swift.tswrites it only if missing), so the same fix is applied toanyCodableContent(). That template had also drifted from the on-disk file (it was missing the@unchecked Sendablerationale and the[Any]/[String: Any]equality arms); those are reconciled here so a from-scratch scaffold reproducesAnyCodable.swiftbyte-for-byte. The generator diff is larger than the generated-output diff only because of that drift reconciliation.AHPClient.swiftJSONSerializationbypass is intentionally left in place. That path solves the inbound/decode side (numbers boxed before they reachAnyCodable), which an encode-side fix cannot reach, so the two are complementary layers, not duplicates. Removing the bypass is a separate, riskier change and is out of scope here.Tests
AnyCodableTestscoversInt,Bool, andDoubleround-tripped throughJSONSerialization, plus a native SwiftBooland aFloat-backedNSNumber.swift test: 104 passed, 0 failed.