Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add second version of new error handling blog post #840

Merged
merged 8 commits into from
Jul 1, 2021

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented May 13, 2021

This is a follow up from #799

cc @bstrie @kw-fn @therealprof @rust-lang/libs @nikomatsakis @flip1995 who all gave feedback on the previous version

@yaahc yaahc requested a review from a team as a code owner May 13, 2021 20:49
@nikomatsakis
Copy link
Contributor

This is great 🎉

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

This looks great! I think the only feedback I have is about the "Guidelines for implementing Display::fmt and Error::source" section. The recommendation itself sounds great, but I wonder if it's worth acknowledging that this will require some kind of migration in the ecosystem. e.g., If I switched my crates to match this recommendation, then I believe it's likely that folks just printing the error without handling the causal chain will end up losing information.

Now maybe this isn't a problem that's terribly common in practice, but it seems good to have on our radar and acknowledge it maybe? To be honest, I don't know if it belongs in this specific blog post, but thought I would throw it out there.

@yaahc
Copy link
Member Author

yaahc commented May 20, 2021

This looks great! I think the only feedback I have is about the "Guidelines for implementing Display::fmt and Error::source" section. The recommendation itself sounds great, but I wonder if it's worth acknowledging that this will require some kind of migration in the ecosystem. e.g., If I switched my crates to match this recommendation, then I believe it's likely that folks just printing the error without handling the causal chain will end up losing information.

Now maybe this isn't a problem that's terribly common in practice, but it seems good to have on our radar and acknowledge it maybe? To be honest, I don't know if it belongs in this specific blog post, but thought I would throw it out there.

@BurntSushi I feel like this is a really good point and belongs in this blog post, and more specifically I think we should also include guidance on how libraries should manage the migration of their error types to follow the guidance.

As for the nature of the guidance, my guess is that we're going to have to state that (if?) we feel that the change we're recommending constitutes a breaking change. Then we should point out that the break is purely on runtime behavior so it won't be caught by the compiler, and that just incrementing the version number is likely insufficient for helping dependencies upgrade smoothly without breaking any code that used to rely on Display or downcast behavior. My feeling is that we should push people towards removing the source error message from their Display rather than not returning their error via source. Between only printing the outermost error message and making it so an error that was previously reacted to via downcast now sneaks through, I'd much rather break the display output.

Also, thinking about it more I think it will be easier to add lints to clean up the old Display based printing across a codebase than it will be to handle changes in downcast behavior. I'm thinking we might want to add a restriction lint against printing Errors directly via their Display or Debug traits, which we can then include in our guidance as a tool to be used for the migration.

@BurntSushi
Copy link
Member

@yaahc I think we're on the same page. Might be good to have an issue for this on the error handling repo where can dig into it a bit more. Maybe for this blog post all we need to say is that 1) it will require some kind of migration and 2) we are working out the details for that HERE, where "HERE" is a link to an issue seeded with your most recent comment. How does that sound?

@yaahc
Copy link
Member Author

yaahc commented May 20, 2021

@yaahc I think we're on the same page. Might be good to have an issue for this on the error handling repo where can dig into it a bit more. Maybe for this blog post all we need to say is that 1) it will require some kind of migration and 2) we are working out the details for that HERE, where "HERE" is a link to an issue seeded with your most recent comment. How does that sound?

That sounds like a great idea. I'll create that issue right now.

nikomatsakis
nikomatsakis previously approved these changes May 21, 2021
@yaahc
Copy link
Member Author

yaahc commented May 21, 2021

@nikomatsakis fwiw I don't think this is ready to merge yet. I still want to decide on and fill out a suggested migration plan in rust-lang/project-error-handling#44, then reference it in this post, before we post this.

@yaahc
Copy link
Member Author

yaahc commented Jun 10, 2021

Okay, I think this may be ready now.

@yaahc
Copy link
Member Author

yaahc commented Jul 1, 2021

Bumping, still ready for approval / publication @rust-lang/core

@Manishearth Manishearth merged commit ff4e05f into rust-lang:master Jul 1, 2021
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.

6 participants