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

Fix a typo in an unsafe impl Send/Sync #6692

Merged

Conversation

alexcrichton
Copy link
Member

This commit fixes a mistake in the PrePatchedFuncRef type where it has an unsafe impl to make it send/sync but the target of the impl was mistakenly InstancePre<T>

Note that this doesn't actually have any impact on the send/sync-ness of InstancePre<T> since it's not storing an instance of T, so it's always Send/Sync anyway. Nevertheless this reduces the scope of unsafety slightly as was originally intended.

This commit fixes a mistake in the `PrePatchedFuncRef` type where it has
an `unsafe impl` to make it send/sync but the target of the impl was
mistakenly `InstancePre<T>`

Note that this doesn't actually have any impact on the send/sync-ness of
`InstancePre<T>` since it's not storing an instance of `T`, so it's
always `Send`/`Sync` anyway. Nevertheless this reduces the scope of
unsafety slightly as was originally intended.
@alexcrichton alexcrichton requested a review from fitzgen July 5, 2023 19:41
@alexcrichton alexcrichton requested a review from a team as a code owner July 5, 2023 19:41
@github-actions github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Jul 5, 2023
@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

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

This makes sense. But since this was passing CI before, I guess that nothing was depending on PrePatchedFuncRef being Send or Sync, so do we need those at all? Could these unsafe impls just be deleted instead?

@alexcrichton
Copy link
Member Author

Oh it was InstancePre<T> that depends on that impl. The previous impl though was unsafe impl<T> Send for InstancePre<T>, which led me to believe that these impls were still necessary.

That being said if I remove the impls InstancePre<T> is indeed Send/Sync as expected. My guess is that while this was historically necessary refactorings ended up making it not necessary!

I'll update this to remove the wrapper entirely

No longer necessary for `Send`/`Sync`-ness any more
@alexcrichton alexcrichton added this pull request to the merge queue Jul 5, 2023
Merged via the queue into bytecodealliance:main with commit 75a66b5 Jul 5, 2023
19 checks passed
@alexcrichton alexcrichton deleted the smaller-unsafe-scope branch July 5, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants