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

Revert #123203 #125377

Closed
wants to merge 1 commit into from
Closed

Revert #123203 #125377

wants to merge 1 commit into from

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented May 21, 2024

This reverts #123203 which caused a regression by making Context no longer UnwindSafe.

It may be possible to re-merge #123203 once we move towards deprecating UnwindSafe entirely.

Fixes #125193

…manieu"

This reverts commit 9c1c0bf, reversing
changes made to e41d7e7.
@rustbot
Copy link
Collaborator

rustbot commented May 21, 2024

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 21, 2024
@matthiaskrgr
Copy link
Member

Could we add #125193 (comment) as a test?

@inquisitivecrystal
Copy link
Contributor

I do want to point out that there's a pretty simple alternative here: you could simply implement UnwindSafe and RefUnwindSafe for Context (or use AssertUnwindSafe). Whether or not this alternative is preferred is a judgement call, and it may be reasonable to revert pending discussion.

@workingjubilee
Copy link
Member

I think AssertUnwindSafe makes the most sense, pending a decision about what "unwind safety" even means, since it's just this one field.

@apiraino apiraino removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 22, 2024
@Amanieu
Copy link
Member Author

Amanieu commented May 22, 2024

Closing in favor of #125392.

@Amanieu Amanieu closed this May 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 23, 2024
…-context, r=Amanieu

Wrap Context.ext in AssertUnwindSafe

Fixes rust-lang#125193

Alternative to rust-lang#125377

Relevant to rust-lang#123392

I believe this approach is justifiable due to the fact that this function is unstable API and we have been considering trying to dispose of the notion of "unwind safety". Making a more long-term decision should be considered carefully as part of stabilizing `fn ext`, if ever.

r? `@Amanieu`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 23, 2024
Rollup merge of rust-lang#125392 - workingjubilee:unwind-a-problem-in-context, r=Amanieu

Wrap Context.ext in AssertUnwindSafe

Fixes rust-lang#125193

Alternative to rust-lang#125377

Relevant to rust-lang#123392

I believe this approach is justifiable due to the fact that this function is unstable API and we have been considering trying to dispose of the notion of "unwind safety". Making a more long-term decision should be considered carefully as part of stabilizing `fn ext`, if ever.

r? `@Amanieu`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request May 24, 2024
… r=Amanieu

Wrap Context.ext in AssertUnwindSafe

Fixes rust-lang/rust#125193

Alternative to rust-lang/rust#125377

Relevant to rust-lang/rust#123392

I believe this approach is justifiable due to the fact that this function is unstable API and we have been considering trying to dispose of the notion of "unwind safety". Making a more long-term decision should be considered carefully as part of stabilizing `fn ext`, if ever.

r? `@Amanieu`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regression: the type (dyn Any + 'static) may contain interior mutability
8 participants