Skip to content

fixes a crash when skipping array fields during decoding#1813

Merged
kperryua merged 3 commits intoswiftlang:experimental/new-codablefrom
t089:fix/skip-value-coding-path-node
Mar 25, 2026
Merged

fixes a crash when skipping array fields during decoding#1813
kperryua merged 3 commits intoswiftlang:experimental/new-codablefrom
t089:fix/skip-value-coding-path-node

Conversation

@t089
Copy link
Copy Markdown

@t089 t089 commented Mar 12, 2026

Fixes a crash in skipValue() when skipping array/dictionary values during decoding.

Motivation:

When implementing JSONDecodable conformance with decodeEachKeyAndValue and a default: break to skip unknown keys, decoding crashes with preconditionFailure("Wrong node type") if any of the unknown fields values is an array (or a nested object containing arrays). This is because skipValue() delegates to BlackHoleVisitor via ArrayDecoder/StructDecoder, but these decoders inherit the parent's coding path node (e.g. .dictionary) instead of pushing their own. When decodeEachElement calls incrementArrayIndex() on the inherited .dictionary node, it hits the precondition failure.

Modifications:

  • In ParserState.skipValue(), push the appropriate CodingPathNode before creating the sub-decoder for both ._openbrace and ._openbracket cases.
  • Point currentTopCodingPathNode at the new node before constructing the StructDecoder/ArrayDecoder
  • After the sub-decoder finishes, unwind back to the parent node (using withExtendedLifetime to ensure the node stays alive).

Result:

Skipping keys in JSON whose values are objects or array no longer crashes.

Testing:

Validated with a reproducer that no longer crashes after the patch is applied.

@kperryua kperryua added the new-codable Related to new Swift (de)serialization APIs label Mar 12, 2026
@kperryua
Copy link
Copy Markdown
Contributor

@t089 is there a reproducer test case you could add to the suite with this fix?

@t089
Copy link
Copy Markdown
Author

t089 commented Mar 16, 2026

Sure, before I check myself, does the Suite Support "exit tests" to observe a crash?

@kperryua
Copy link
Copy Markdown
Contributor

Looks like it's supported on a number of platforms. We'd have to crib a conditional compiler setting from Foundation's testOnlySwiftSettings in the Package.swift to enable it safely.

But we're not testing an expected assertion here right? Just a crash fix? The test running to completion should be verification enough. Using the process exit infrastructure might even make it harder for us to get backtrace diagnostics if it were to start crashing again later.

@t089
Copy link
Copy Markdown
Author

t089 commented Mar 16, 2026

Ok got it. Yes then I can add a test case for this.

@kperryua
Copy link
Copy Markdown
Contributor

Thanks a ton! You probably want to rebase the branch on top of the latest version that has the various test fixes.

@t089 t089 force-pushed the fix/skip-value-coding-path-node branch from d5a13ab to 2b038cd Compare March 21, 2026 09:17
@t089 t089 force-pushed the fix/skip-value-coding-path-node branch from 2b038cd to 4dc4aea Compare March 21, 2026 09:35
@t089
Copy link
Copy Markdown
Author

t089 commented Mar 23, 2026

@kperryua added a couple of tests

@kperryua kperryua merged commit 876cae4 into swiftlang:experimental/new-codable Mar 25, 2026
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-codable Related to new Swift (de)serialization APIs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants