Better debugDescription for EncodingError and DecodingError
#80941
Conversation
ZevEisenberg
force-pushed
the
main
branch
2 times, most recently
from
4cd346f to
1ae5683
Compare
|
CC @kperryua |
ZevEisenberg
changed the title
description for DecodingErrordescription for DecodingError and CodingKey
kperryua
left a comment
kperryua
left a comment
There was a problem hiding this comment.
Thank you for this awesome contribution. The coding path description look so much more readable than present. A fair number of comments and suggestions, but hopefully easily resolved.
With regards to your suggestions about improving the error types in general: feel free to discuss this more in depth on the existing serialization thread, or a new one in the forums. DecodingError and EncodingError have to be discarded anyway in the new designs, because of their reliance on existentials, so we'll have a clean slate with which to make any fundamental improvements.
stdlib/public/core/Codable.swift
Outdated
| } else { | ||
| stringValue | ||
| } | ||
| } |
There was a problem hiding this comment.
It seems like a lone CodingKey ought to have a more complete description than just "[0]" or "keyName". Could this instead be a separate property (probably named similarly to the [any CodingKey] property) that gets called specifically in this context?
There was a problem hiding this comment.
Perhaps, but an issue is that the description is also called by JSONDecoder in a few cases when providing debug descriptions for decoding error contexts, so it may clobber those too. Unless a) we're fine with that, or b) I'm misunderstanding what you're asking?
There was a problem hiding this comment.
It's not a great idea for any external entity (JSONDecoder here) to expect a certain format out of another value's description, even for the purposes of string interpolation unless it's explicitly defined, like one can expect from Int or Double or whatever.
If JSONDecoder wants a certain format when printing CodingKeys in its descriptions, then it should do the work to generate that format itself. At present, that might involve some duplication of code between CodingKey and JSONDecoder / PropertyListDecoder / etc. unless/until that format can be wrapped in some API that does explicitly define the string format.
So, I still think CodingKey's description should remain untouched, and we perhaps follow up with a swift-foundation PR to improve the description construction for the cases you're talking about.
There was a problem hiding this comment.
I can make that change if you'd like: removing the changes to the description, and exposing the more concise version via a different variable. Shall I?
Also, just to make sure I understand: is this specifically because you don't want to change the existing implementation in case someone is relying on it? Or do you prefer the implementation of Reading comprehension - your intent is clearer than I realized.description regardless of concerns around breaking compatibility?
|
This would be such a great addition 👏🏼. We console-output errors in a CLI assuming the readability of the output, and users end up getting something that's impossible to debug. |
|
I love it! What do you think about using JSONPath for the paths to the troublesome values? It can speed up looking up the actual value, especially in larger documents. |
I'm not familiar with JSONPath. Can you say more? Also, the changes I'm making here in the stdlib are agnostic to the kind of decoder being used. I'm wondering if this suggestion would find a better home in either a PR to swift-foundation's JSONDecoder, or else as part of the nascent successor to Codable being discussed here. |
JSONPath is a query expression syntax for JSON, like XPath for XML or perhaps CSS selectors. The key paths your PR constructs for However, after reading over the diff, I realize there's nothing in |
|
@kperryua I've addressed all feedback, and update the description to give an easy view of the new before/after test cases. I left a custom description of I tried to keep the commits atomic, but I figure I'll squash them all down once we're ready to merge. |
|
@swift-ci test |
|
This is my first time trying to read Swift CI failure messages. I'm also on my phone at the moment and Safari is struggling to render the CI failure page. I think I'm seeing something about a mangled name changing? Is that because of the added conformances? Do we need to do some kind of retroactive conformance or availability annotation or something? |
Availability is required on the new conformance at a minimum. There might be some other files to adjust that acknowledge the addition to ABI. I'll try find to out what those are after we rebuild with availability and see what the tests say. |
|
What should I do for availability on the conformances? Is it gated to a Swift version (like 6.2 or something)? I'd appreciate some guidance here, and then I can make the necessary changes, or feel free to make them yourself if that's easier. |
|
|
kperryua
left a comment
kperryua
left a comment
There was a problem hiding this comment.
Also: seems like similar improvements could be made to EncodingError. Happy to treat that as a separate PR though.
We've already branched, so if the intention is for this to go into 6.2 it will need a cherry-pick for the release/6.2 branch after landing on main. Otherwise, it would get 6.3 availability (which I think someone still has to add) |
|
(ah, here's the PR for 6.3: #80870) |
|
I'm happy to target whichever release seems appropriate. This has been 4+ years in the making; there's no particular urgency to get it into 6.2. That said, if it's not too painful to get it in, it would be nice to ship it sooner rather than later! And as for |
|
I rebased on main, squashed commits, and added the availability annotation for 6.2 as requested. But that may not be needed, because I'm going to tweak the implementation per #80941 (comment). |
ZevEisenberg
force-pushed
the
main
branch
2 times, most recently
from
67f6878 to
6d5c8fc
Compare
|
Oh, also, I don't know how important PR tags are, but I just noticed that this still has "swift evolution proposal needed" on it, and I can't change it. |
|
@swift-ci Please test |
rintaro
added
swift evolution approved
|
I'll have to look into it later, but these failed on macOS in this build. |
|
For the above tests, I installed the latest main snapshot with swiftly and I rebased my branch on main. Then I built the compiler using:
When I run the first test like this:
I get: When I run the second test, using:
I get: I've rebased and force-pushed my branch in case it's just fixed up updating to main, but otherwise, does anyone know how I might go about reproducing these failures so I can fix them locally? |
|
(But, if anyone thinks a rebase alone is likely to fix it, could you please try a macOS or full build to check?) |
|
@swift-ci please test |
|
@swift-ci Please clean smoke test macOS platform |
|
Thanks for the build help, @jamieQ! I'm wondering if this needs anything else to be ready to merge? |
|
bump 🙏🏻 |
|
I had to rebase to address a conflict with a recent addition to the ABI tests. Could I please have a rebuild of all the required things? And @lorentey is there anything else you'd like to see before merging? |
|
@swift-ci please test |
|
Windows failed, but I didn't see anything that looked related to this PR, and I'm not currently equipped to test Windows locally. Anyone know what's up? |
…bugStringConvertible and return a tidy debugDescription to make errors easier to read in debug output.
… from before int keys.
…s, backslashes, or backticks, and backslash-escape backslashes or backticks therein.
# Conflicts: # test/abi/macOS/arm64/stdlib.swift
…dingError to CustomDebugStringConvertible.
…xplicitly note that the returned format is not stable. This tends to be generally true for most `debugDescription` properties, but explicitly calling it out should help preventing people from trying to parse the result in this case. Borrowed from swiftlang#75433 and SE-0445.
# Conflicts: # test/api-digester/stability-stdlib-abi-without-asserts.test
|
Rebased to resolve conflict in test/api-digester/stability-stdlib-abi-without-asserts.test. Could I have another build please? (I requested member access to be able to start my own builds.) |
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
|
All builds are now passing 👀🙏🏻🤞🏻 |
11 participants
Now accepted as SE-0489, but we still need to refine the implementation.
To Do
Context
Re: Swift forum post, where a discussion about future serialization tools in Swift prompted Kevin Perry to suggest that some proposed changes could actually be made in today's stdlib.
Summary of Changes
Conforms
EncodingErrorandDecodingErrortoCustomDebugStringConvertibleand adds a more-readabledebugDescription.Future Directions
This is a pared-down version of some experiments I did in UsefulDecode. The changes in this PR are the best I could do without changing the public interface of
DecodingErrorandEncodingError, and without modifying the way theJSON/PropertyListEncoder/Decoderin Foundation generate their errors' debug descriptions.In the above-linked UsefulDecode repo, when JSON decoding fails, I go back and re-decode the JSON using
JSONSerializationin order to provide more context about what failed, and why. I didn't attempt to make such a change here, but I'd like to discuss what may be possible.Examples
To illustrate the effect of the changes in this PR, I removed my changes to stdlib/public/core/Codable.swift and ran my new test cases again. Here are the resulting diffs.
test_encodingError_invalidValue_nonEmptyCodingPath_nilUnderlyingErrorBefore
invalidValue(234, Swift.EncodingError.Context(codingPath: [GenericCodingKey(stringValue: "first", intValue: nil), GenericCodingKey(stringValue: "second", intValue: nil), GenericCodingKey(stringValue: "2", intValue: 2)], debugDescription: "You cannot do that!", underlyingError: nil))After
EncodingError.invalidValue: 234 (Int). Path: first.second[2]. Debug description: You cannot do that!test_decodingError_valueNotFound_nilUnderlyingErrorBefore
valueNotFound(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "firstName", intValue: nil)], debugDescription: "Description for debugging purposes", underlyingError: nil))After
DecodingError.valueNotFound: Expected value of type String but found null instead. Path: [0].firstName. Debug description: Description for debugging purposestest_decodingError_keyNotFound_nonNilUnderlyingErrorBefore
keyNotFound(GenericCodingKey(stringValue: "name", intValue: nil), Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "city", intValue: nil)], debugDescription: "Just some info to help you out", underlyingError: Optional(main.GenericError(name: "hey, who turned out the lights?"))))After
DecodingError.keyNotFound: Key \'name\' not found in keyed decoding container. Path: [0].address.city. Debug description: Just some info to help you out. Underlying error: GenericError(name: "hey, who turned out the lights?")test_decodingError_typeMismatch_nilUnderlyingErrorBefore
typeMismatch(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "city", intValue: nil), GenericCodingKey(stringValue: "birds", intValue: nil), GenericCodingKey(stringValue: "1", intValue: 1), GenericCodingKey(stringValue: "name", intValue: nil)], debugDescription: "This is where the debug description goes", underlyingError: nil))After
DecodingError.typeMismatch: expected value of type String. Path: [0].address.city.birds[1].name. Debug description: This is where the debug description goestest_decodingError_dataCorrupted_nonEmptyCodingPathBefore
dataCorrupted(Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "first", intValue: nil), GenericCodingKey(stringValue: "second", intValue: nil), GenericCodingKey(stringValue: "2", intValue: 2)], debugDescription: "There was apparently some data corruption!", underlyingError: Optional(main.GenericError(name: "This data corruption is getting out of hand"))))After
DecodingError.dataCorrupted: Data was corrupted. Path: first.second[2]. Debug description: There was apparently some data corruption!. Underlying error: GenericError(name: "This data corruption is getting out of hand")test_decodingError_valueNotFound_nonNilUnderlyingErrorBefore
valueNotFound(Swift.Int, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "population", intValue: nil)], debugDescription: "Here is the debug description for value-not-found", underlyingError: Optional(main.GenericError(name: "these aren\\\'t the droids you\\\'re looking for"))))After
DecodingError.valueNotFound: Expected value of type Int but found null instead. Path: [0].population. Debug description: Here is the debug description for value-not-found. Underlying error: GenericError(name: "these aren\\\'t the droids you\\\'re looking for")test_encodingError_invalidValue_emptyCodingPath_nonNilUnderlyingErrorBefore
invalidValue(345, Swift.EncodingError.Context(codingPath: [], debugDescription: "You cannot do that!", underlyingError: Optional(main.GenericError(name: "You really cannot do that"))))After
EncodingError.invalidValue: 345 (Int). Debug description: You cannot do that!. Underlying error: GenericError(name: "You really cannot do that")test_decodingError_typeMismatch_nonNilUnderlyingErrorBefore
typeMismatch(Swift.String, Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "1", intValue: 1), GenericCodingKey(stringValue: "street", intValue: nil)], debugDescription: "Some debug description", underlyingError: Optional(main.GenericError(name: "some generic error goes here"))))After
DecodingError.typeMismatch: expected value of type String. Path: [0].address[1].street. Debug description: Some debug description. Underlying error: GenericError(name: "some generic error goes here")test_encodingError_invalidValue_emptyCodingPath_nilUnderlyingErrorBefore
invalidValue(123, Swift.EncodingError.Context(codingPath: [], debugDescription: "You cannot do that!", underlyingError: nil))After
EncodingError.invalidValue: 123 (Int). Debug description: You cannot do that!test_decodingError_keyNotFound_nilUnderlyingErrorBefore
keyNotFound(GenericCodingKey(stringValue: "name", intValue: nil), Swift.DecodingError.Context(codingPath: [GenericCodingKey(stringValue: "0", intValue: 0), GenericCodingKey(stringValue: "address", intValue: nil), GenericCodingKey(stringValue: "city", intValue: nil)], debugDescription: "How would you describe your relationship with your debugger?", underlyingError: nil))After
DecodingError.keyNotFound: Key \'name\' not found in keyed decoding container. Path: [0]address.city. Debug description: How would you describe your relationship with your debugger?test_decodingError_dataCorrupted_emptyCodingPathBefore
dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "The given data was not valid JSON", underlyingError: Optional(main.GenericError(name: "just some data corruption"))))After
DecodingError.dataCorrupted: Data was corrupted. Debug description: The given data was not valid JSON. Underlying error: GenericError(name: "just some data corruption")test_encodingError_invalidValue_nonEmptyCodingPath_nonNilUnderlyingErrorBefore
invalidValue(456, Swift.EncodingError.Context(codingPath: [GenericCodingKey(stringValue: "first", intValue: nil), GenericCodingKey(stringValue: "second", intValue: nil), GenericCodingKey(stringValue: "2", intValue: 2)], debugDescription: "You cannot do that!", underlyingError: Optional(main.GenericError(name: "You really cannot do that"))))After
EncodingError.invalidValue: 456 (Int). Path: first.second[2]. Debug description: You cannot do that!. Underlying error: GenericError(name: "You really cannot do that")