Skip to content

Handle unparsed non-optional complex property types#554

Merged
natecook1000 merged 3 commits into
apple:mainfrom
gwynne:main
Feb 23, 2023
Merged

Handle unparsed non-optional complex property types#554
natecook1000 merged 3 commits into
apple:mainfrom
gwynne:main

Conversation

@gwynne
Copy link
Copy Markdown
Contributor

@gwynne gwynne commented Feb 20, 2023

As initially demonstrated in #553, the existing ArgumentDecoder logic fails when presented with a property which:

  • Is unparsed
  • Is not optional
  • Has a default value, and
  • Requests a keyed container during decoding

This occurs because while the actual property in question is properly assessed as having .defaultValue as an InputOrigin, the request for a keyed decoding container bypasses the logic which would otherwise return said default value, and since the ArgumentDefinition logic is (correctly) not recursive, the type fails to decode any keys it contains due to a "missing value".

The most straightforward solution I could find, one which covers all cases without negatively affecting handling of parsed properties (to the best of my ability to determine), was to apply the "default value" handling from the decodeIfPresent<T>(_:forKey:) method of the keyed decoding container (which ensured that optional properties of this sort already worked correctly) to the more fundamental "bottleneck" decode<T>(_:forKey:) method as well, modified to throw a wrongType error instead of returning nil on type mismatch.

I additionally added a functioning allKeys property accessor for additional robustness against custom Decodable.init(from:) implementations. (The choice not to do the same for nestedContainer(keyedBy:), nestedUnkeyedContainer(), superDecoder(), and superDecoder(forKey:) was simply pragmatic - it's not even close to worth the effort.)

Fixes #553

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@gwynne
Copy link
Copy Markdown
Contributor Author

gwynne commented Feb 20, 2023

cc @natecook1000 @hassila

Copy link
Copy Markdown
Member

@natecook1000 natecook1000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwynne Thanks so much for this fix! I believe your change to decode(_:forKey:) is sufficient to solve the problem. Unless we can create a test that exercises the new allKeys, let's revert that back to a fatal error.

Can you also switch to 2-space indenting? Thanks!

@gwynne
Copy link
Copy Markdown
Contributor Author

gwynne commented Feb 22, 2023

@natecook1000 Well, I tried to come up with a test that would exercise the allKeys implementation, but it turns out nothing I could think of would get that far with the change to decode(_:forKey:) in place; I've removed it and put back the fatalError() - and also fixed my indentation, don't worry! 😆

@natecook1000
Copy link
Copy Markdown
Member

@swift-ci Please test

@hassila
Copy link
Copy Markdown

hassila commented Feb 23, 2023

(thanks @gwynne for fixing this, much obliged!)

@natecook1000 natecook1000 merged commit c5050aa into apple:main Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash when having a dictionary property (ArgumentDecoder.swift:89: Fatal error)

3 participants