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.
|
@ZevEisenberg When you're satisfied with your edits in relation to the above, feel free to ping me and take this out of draft status. |
|
Will do! Sorry, I've been sick. I'll get back to it ASAP. |
|
No worries—take your time! |
ZevEisenberg
force-pushed
the
main
branch
3 times, most recently
from
c6b2023 to
034df66
Compare
|
@xwu I've pushed some changes! I don't know if the section about ABI compatibility is still too hand-wavy, but I wanted to see what you thought before I dug in any deeper. |
|
|
||
| ### `debugDescription` Property | ||
|
|
||
| The change to the debug output is purely cosmetic: it improves readability, but does not change the information contained in the message. For this reason, we think it is not worthwhile to backport the `debugDescription` property itself. Since the conformance to `CustomDebugStringConvertible` is not back-deployable, a backport of property itself would be of limited utility anyway. |
There was a problem hiding this comment.
Whether something can be backdeployed is an implication on adoption that is appropriately part of the proposal, but it's outside of the scope of Swift Evolution whether something actually will be backdeployed on any particular platform since it's ultimately up to the platform owner. So you could make this even shorter and say something like, the implementation of debugDescription itself is possible to backdeploy but would be on its own of limited utility.
On the contrary, I think it's more than sufficient :) |
|
|
||
| The proposal conforms two previously existing stdlib types to a previously existing stdlib protocol. This is technically an ABI breaking change: on ABI-stable platforms, we may have preexisting Swift binaries that implement a retroactive `CustomDebugStringConvertible` conformance, or binaries that assume that the existing error types do _not_ conform to the protocol. | ||
|
|
||
| We do not expect this to be an issue in practice. The above checks represent edge cases, since checking an arbitrary error for conformance to `CustomDebugStringConvertible` at run-time seems unlikely. And if someone has added a conformance, when they re-build with a stdlib that includes this conformance, they will get a build-time warning. |
There was a problem hiding this comment.
And if someone has added a conformance, when they re-build with a stdlib that includes this conformance, they will get a build-time warning.
What happens when you recompile doesn't make ABI breakage on ABI-stable platforms any more or less problematic, so I don't think it needs comment here... The preceding statement that you think a certain usage is unlikely is pretty much a restatement of the first sentence in the paragraph that you don't think it's an issue in practice, so I think that one sentence pretty much captures the whole paragraph.
There was a problem hiding this comment.
I felt weird about just stating that we don't expect it to be an issue but not explaining why, but I've tightened up the wording. I figure, this section will be most relevant if someone in the future is negatively affected by the change, and in that case, I don't want them finding a years-old document telling them that their problem is unlikely 😆
|
@xwu ready for another look |
SE proposal to accompany swiftlang/swift#80941