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.

Sign up for GitHub

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

Jump to bottom

Use &dyn Any rather than &(dyn Any + Send) for PanicInfo::payload() #110799

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Use &dyn Any rather than &(dyn Any + Send) for PanicInfo::payload() #110799

wants to merge 2 commits into from

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Apr 25, 2023 •
edited
Loading

The Send in &(dyn Any + Send) is basically useless, because it's a reference. Removing it from PanicInfo::payload() allows for some new possibilities in the future, if the type behind it no longer needs to be Send. (We could just safely transmute the + Send into the &dyn because it's meaningless for references, but it'd be nicer to just remove the needless + Send from the public API to clean things up.

Removing it will break some other places that (needlessly) have the same + Send though, luckily there don't seem to many cases of that in the Rust ecosystem.

@m-ou-se m-ou-se added the S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. label Apr 25, 2023
@m-ou-se m-ou-se self-assigned this Apr 25, 2023
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 25, 2023
@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 25, 2023

@bors try

@m-ou-se m-ou-se removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2023
@bors
Copy link
Contributor

bors commented Apr 25, 2023

⌛ Trying commit 80968e5a8740ec4cdf7ec89cb320e20fc1dbee37 with merge 721f09565b0c3089f05d0d05d88cb02947a00cd1...

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 25, 2023

☀️ Try build successful - checks-actions
Build commit: 721f09565b0c3089f05d0d05d88cb02947a00cd1 (721f09565b0c3089f05d0d05d88cb02947a00cd1)

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 25, 2023

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-110799 created and queued.
🤖 Automatically detected try build 721f09565b0c3089f05d0d05d88cb02947a00cd1
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Apr 25, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-110799 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-110799 is completed!
📊 31 regressed and 3 fixed (262520 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot 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 26, 2023
@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 26, 2023 •
edited
Loading

Looks like only six crates break when changing payload() to just &dyn Any without + Send:

  • jaffi_support-0.2.0
  • panic-message-0.3.0
  • procspawn-0.10.2
  • terminate-0.2.1
  • tester-0.9.0
  • tokio-unix-ipc-0.3.0

@m-ou-se m-ou-se mentioned this pull request Apr 28, 2023
Open
@m-ou-se m-ou-se changed the title [Experiment] Use &dyn Any rather than &(dyn Any + Send) for panic payload. [Experiment] Use &dyn Any rather than &(dyn Any + Send) for PanicInfo::payload() Apr 28, 2023
@m-ou-se m-ou-se changed the title [Experiment] Use &dyn Any rather than &(dyn Any + Send) for PanicInfo::payload() Use &dyn Any rather than &(dyn Any + Send) for PanicInfo::payload() Apr 28, 2023
This was referenced Apr 28, 2023
Merged
Merged
Merged
Merged
Open
Open
Closed
@est31
Copy link
Member

est31 commented Apr 28, 2023

If API breakages of this kind are okay, then maybe one could revisit #105643 as well. @m-ou-se what do you think?

@m-ou-se
Copy link
Member Author

m-ou-se commented May 1, 2023

If API breakages of this kind are okay,

I don't know if this is okay. That's up to the team, not just me. ^^ But we have made similar breaking changes in the past, so it might be acceptable.

then maybe one could revisit #105643 as well. @m-ou-se what do you think?

I've re-opened your issue and nominated it for discussion.

@m-ou-se
Use &dyn Any rather than &(dyn Any + Send) for panic payload.
825b5ae 
The `Send` is useless here, because it's a reference.
@m-ou-se m-ou-se force-pushed the panic-payload-dyn-any-no-send branch from bce9a83 to 51dcb5e Compare May 1, 2023 11:54
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@m-ou-se m-ou-se force-pushed the panic-payload-dyn-any-no-send branch from 51dcb5e to 70c69f0 Compare May 1, 2023 11:58
@m-ou-se m-ou-se added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2023
@bors
Copy link
Contributor

bors commented Jul 2, 2023

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

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 2, 2023
@Dylan-DPC
Copy link
Member

Closing this as it was an experiment

@Dylan-DPC Dylan-DPC closed this Oct 24, 2023
@Dylan-DPC Dylan-DPC removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Oct 24, 2023
@m-ou-se m-ou-se reopened this May 13, 2024
@m-ou-se m-ou-se added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@m-ou-se @bors @rust-log-analyzer @craterbot @est31 @Dylan-DPC @rustbot