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

stabilize std::error::<dyn Error>::chain() and std::error::Chain #66448

Closed
wants to merge 1 commit into from

Conversation

haraldh
Copy link
Contributor

@haraldh haraldh commented Nov 15, 2019

remove Copy from Iterator as per comment
#58520 (comment)

and change #[unstable] to #[stable]

Tracker: #58520

remove Copy from Iterator as per comment
rust-lang#58520 (comment)

and change #[unstable] to #[stable]
@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2019
@haraldh
Copy link
Contributor Author

haraldh commented Nov 15, 2019

r? @tmandry

@jonas-schievink jonas-schievink added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 15, 2019
@jonas-schievink jonas-schievink added this to the 1.41 milestone Nov 15, 2019
@jonas-schievink jonas-schievink added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 15, 2019
@tmandry
Copy link
Member

tmandry commented Nov 15, 2019

r? @withoutboats

@withoutboats
Copy link
Contributor

It seems a bit far-fetched to make a breaking change to an unstable API and then stabilize it as soon as that's merged, especially since it was merged by @sfackler on the grounds that its just a change to an unstable API.

In terms of semantics, this is the API surface I want and I think the libs team has consensus on that. But I still am not a fan of the choice of name you've given to this API, and I think the libs team as a whole needs to reach a consensus here.

I would r+ the copy change immediately, however.

@withoutboats
Copy link
Contributor

The other issue I see blocking stability (aside from the name) is the T: Error vs dyn Error issue. For reasons that are beyond annoying, Box<dyn Error> does not implement Error. This makes whether methods are implemented for dyn Error or T: Error matter in really annoying ways.

Can this method be implemented on both dyn Error and T: Error? If so, is it? If it can't be, why not? Have we chosen the right choice? I had to make these choices for failure once, and I'm not sure I made them right then (not that I even remember clearly what I did without checking). I would like to see some write up explaining our policy around where we add these convenience methods to convince me we've done it in the right way and not just introduced yet another papercut to the error API.

@haraldh
Copy link
Contributor Author

haraldh commented Nov 18, 2019

I would r+ the copy change immediately, however.

PR #66511

@haraldh haraldh closed this Nov 18, 2019
@haraldh
Copy link
Contributor Author

haraldh commented Nov 18, 2019

The other issue I see blocking stability (aside from the name) is the T: Error vs dyn Error issue. For reasons that are beyond annoying, Box<dyn Error> does not implement Error. This makes whether methods are implemented for dyn Error or T: Error matter in really annoying ways.

Can this method be implemented on both dyn Error and T: Error? If so, is it? If it can't be, why not? Have we chosen the right choice? I had to make these choices for failure once, and I'm not sure I made them right then (not that I even remember clearly what I did without checking). I would like to see some write up explaining our policy around where we add these convenience methods to convince me we've done it in the right way and not just introduced yet another papercut to the error API.

Please add your comment to the tracker #58520 so we can discuss this

@haraldh haraldh deleted the error_chain_stabilize branch November 18, 2019 08:04
@Centril Centril removed the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 20, 2019
@Centril Centril removed this from the 1.41 milestone Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants