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

impl Fn/FnMut/FnOnce for Arc/Rc #49224

Closed
wants to merge 5 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 20, 2018

This PR introduces the ability to have Rc/Arc<F>: Fn(A) -> B where F: Fn(A) -> B and Rc/Arc<Fn(A) -> B.

This was previously tried (and failed) in #34118 (comment) due to breakage with crater. But now that the new edition is approaching we might want to try this in edition 2018. We should probably do a new crater run to see how much the breakage has changed since the previous attempt.

cc @aturon

@Centril Centril added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. WG-epoch Working group: Epoch (2018) management labels Mar 20, 2018
@rust-highfive
Copy link
Collaborator

r? @sfackler

(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 Mar 20, 2018
@Centril
Copy link
Contributor Author

Centril commented Mar 21, 2018

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned sfackler Mar 21, 2018
@kennytm
Copy link
Member

kennytm commented Mar 24, 2018

Could you clarify how Rust 2018 is going to make this work? Both Rust 2015 and 2018 editions are going to share the same standard library.

@Centril
Copy link
Contributor Author

Centril commented Mar 24, 2018

@kennytm That's true. Honestly not sure at all here; but @aturon seemed to agree that we should try something, so I defer to them ;) And if we can't, we'll have to skip it.

@shepmaster
Copy link
Member

Ping from triage, @aturon — we eagerly await your review!

@bors
Copy link
Contributor

bors commented Apr 4, 2018

☔ The latest upstream changes (presumably #48333) made this pull request unmergeable. Please resolve the merge conflicts.

@aturon
Copy link
Member

aturon commented Apr 4, 2018

@rust-lang/infra Requesting crater run please!

@kennytm
Copy link
Member

kennytm commented Apr 4, 2018

@bors try

@bors
Copy link
Contributor

bors commented Apr 4, 2018

⌛ Trying commit 51b2ca5 with merge f131001...

@kennytm kennytm added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2018
bors added a commit that referenced this pull request Apr 4, 2018
impl Fn/FnMut/FnOnce for Arc/Rc

This PR introduces the ability to have `Rc/Arc<F>: Fn(A) -> B where F: Fn(A) -> B` and `Rc/Arc<Fn(A) -> B`.

This was previously tried (and failed) in #34118 (comment) due to breakage with crater. But now that the new edition is approaching we might want to try this in edition 2018. We should probably do a new crater run to see how much the breakage has changed since the previous attempt.

cc @aturon
@bors
Copy link
Contributor

bors commented Apr 4, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Mark-Simulacrum
Copy link
Member

Crater run started. Expected ETA is in 5-6 days.

@bors
Copy link
Contributor

bors commented Apr 18, 2018

☔ The latest upstream changes (presumably #50017) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member

This probably cannot land as-is: the impls introduced appear to conflict with tokio-tls which is quite widely used: Crater results are at: http://cargobomb-reports.s3.amazonaws.com/pr-49224/index.html. 'Blacklisted' crates (spurious failures etc) can be found here. If you see any spurious failures not on the list, please make a PR against that file.

(interested observers: Crater is a tool for testing the impact of changes on the crates.io ecosystem. You can find out more at the repo if you're curious)

@pietroalbini pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Apr 23, 2018
@pietroalbini
Copy link
Member

Ping from triage @aturon! What should we do with this PR?

@alexcrichton
Copy link
Member

Unfortunately that's a pretty high number of regressions, so I'm going to close this.

@Centril
Copy link
Contributor Author

Centril commented May 1, 2018

@alexcrichton Bummer on the regressions :(

I'll have to look through the regressions to see how specialization affects those, but do you think perhaps specialization could be our saving grace here?

@alexcrichton
Copy link
Member

Unsure, I just know we can't land this with so many known regressions.

@Centril
Copy link
Contributor Author

Centril commented May 1, 2018

@alexcrichton Agreed on that :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. 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. WG-epoch Working group: Epoch (2018) management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants