Add SE proposal for Codable error printing #2843
Conversation
ZevEisenberg
force-pushed
the
main
branch
5 times, most recently
from
758540b to
2e846c7
Compare
|
Hi @ZevEisenberg, I'm really excited about this. Can you begin a pitch thread on the forums, and then we can work on getting it ready for formal evolution review in a few weeks? |
|
|
||
| ## Implications on adoption | ||
|
|
||
| [Unsure what to add here. I see stuff in [SE-0445](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0445-string-index-printing.md#implications-on-adoption), but I'm not sure how much of that applies here. I don't know if I need to be doing the `@backDeployed` things that proposal mentions, and when I look at the code from the PR, I see `@_alwaysEmitIntoClient // FIXME: Use @backDeployed`.] |
There was a problem hiding this comment.
I suspect that in SE-0445 the back-deploying was important because the non-API public symbol _description existed on String.Index in prior Swift versions, and was reimplemented using debugDescription? I'm not certain that this is quite as relevant here, which suggests that there's nothing really useful to add to this section.
@lorentey can you provide any background on SE-0445 and/or advice here?
|
@xwu I saw that you self-assigned this. I just got this PR and my implementation PR rebased on main, and confirmed that the implementation still builds and passes tests when I run it locally. Is there anything I can or should be doing to help get this ready for proposal phase? Do you have a sense of whether it would land in Swift 6.2 or main? |
|
I'll do an editorial pass later this week (or weekend) and have suggestions to help you fill out all the parts of the proposal text. Once it's taken out of draft status, I'll get back to you after the language steering group is satisfied it's ready to run as a proposal and schedule the review period, which will run over two weeks. Implementation work proceeds separately; after a feature lands in main (which would be no earlier than July given the timeline above), then it can be evaluated if cherrypicking to a release branch is appropriate, but expect it's well out of the window for 6.2. |
|
|
||
| (Note: the output could be further improved by modifying `JSONDecoder` to write a better debug description. See [Future Directions](#future-directions) for more.) | ||
|
|
||
| ### Structure |
There was a problem hiding this comment.
In general, we don't guarantee the format of a description or debug description; giving a specific rundown of what information will be presented, under what conditions, and in which order reads like a guarantee that doesn't seem necessary to achieve your motivation (and we don't want users to be parsing this programmatically).
I would suggest either removing this information or phrasing this more as an example of how the implementation might improve usability—as well as explicitly noting, as we did in SE-0445, that it is not normative and is subject to future change without review or advance notice.
|
|
||
| ## Implications on adoption | ||
|
|
||
| [Unsure what to add here. I see stuff in [SE-0445](https://github.com/swiftlang/swift-evolution/blob/main/proposals/0445-string-index-printing.md#implications-on-adoption), but I'm not sure how much of that applies here. I don't know if I need to be doing the `@backDeployed` things that proposal mentions, and when I look at the code from the PR, I see `@_alwaysEmitIntoClient // FIXME: Use @backDeployed`.] |
There was a problem hiding this comment.
Here's what I might say: The conformance being proposed here is not backdeployable. As a result, code that runs on ABI-stable platforms with earlier versions of the standard library won't output these nicer debug descriptions.
It is up to you if you want to make debugDescription itself (the property, not the conformance) backdeployable—this would make it possible for newly compiled code to invoke debugDescription explicitly if they absolutely need the nicer description, but that is not something we would either recommend or expect users to reach for pervasively to work around the adoption issue outlined above.
There was a problem hiding this comment.
So, if we back-deployed debugDescription, and if someone knew about this change, they would be able to ship an update that would work even when running on OSes that don't have the change? But only if they explicitly called debugDescription instead of just logging the errors with default string interpolation or whatever method they are using now?
Since the information in question is primarily for use as a developer convenience while debugging, I'm inclined to go the simpler route. If you're digging around in error logs, and you think of looking into this and learn about the debugDescription hack, I figure you're probably savvy enough to just read the old-style error message yourself?
There was a problem hiding this comment.
Yup, that part is a design decision and not a proposal drafting decision, so I leave it to you :)
|
|
||
| The original version of this proposal suggested conforming `EncodingError` and `DecodingError` to `CustomStringConvertible`, not `CustomDebugStringConvertible`. The change to the debug-flavored protocol emphasizes that the new descriptions aren't intended to be used outside debugging contexts. This is in keeping with the precedent set by [SE-0445](0445-string-index-printing.md). | ||
|
|
||
| The original version also proposed changing `CodingKey.description` to return the bare string or int value, but changing the exsting implementation of an existing public method was deemed too potentially dangerous. |
There was a problem hiding this comment.
Can you specify what you mean here by "dangerous"?
There was a problem hiding this comment.
It was in reference to swiftlang/swift#80941 (comment). I'll update the proposal, plus I'll link to that thread if appropriate.
There was a problem hiding this comment.
So, if I understand the point correctly, "dangerous" is referring concretely to Foundation.JSONDecoder having a known dependency on non-guaranteed implementation details of CodingKey.description?
There was a problem hiding this comment.
yep! I'm thinking "dangerous" is not the right word, and @kperryua's original message or your summary is closer to what we should put here in the proposal.
SE proposal to accompany swiftlang/swift#80941