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

bevy_dynamic_plugin: fix unsafe_op_in_unsafe_fn lint #11622

Merged
merged 3 commits into from
Feb 12, 2024
Merged

bevy_dynamic_plugin: fix unsafe_op_in_unsafe_fn lint #11622

merged 3 commits into from
Feb 12, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Jan 30, 2024 •
edited
Loading

Objective

Solution

  • Fix unsafe_op_in_unsafe_fn for bevy_dynamic_plugin.

Changelog

  • Added further restrictions to the safety requirements of bevy_dynamic_plugin::dynamically_load_plugin.

I had a few issues, specifically with the safety comment on dynamically_load_plugin. There are three different unsafe functions called within the function body, and they all need their own justification / message.

Also, would it be unsound to call dynamically_load_plugin multiple times on the same file? I feel the documentation needs to be more clear.

@BD103
Copy link
Member Author

BD103 commented Jan 30, 2024

It seems there is a deeper issue with bevy_dynamic_plugin after asking on Discord. While a more permanent fix should eventually be developed, I think this PR is trivial enough that it should still be merged.

I do honestly need help writing the text for that one unsafe block though. :)

@alice-i-cecile alice-i-cecile added D-Complex Quite challenging from either a design or technical perspective. Ask for help! P-Unsound A bug that results in undefined compiler behavior labels Jan 30, 2024
@tguichaoua tguichaoua mentioned this pull request Jan 31, 2024
Open
3 tasks
@BD103 BD103 marked this pull request as ready for review January 31, 2024 19:39
@BD103 BD103 requested a review from tguichaoua January 31, 2024 19:43
tguichaoua
tguichaoua approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@tguichaoua tguichaoua left a comment

Choose a reason for hiding this comment

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

LGTM

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 8, 2024
BD103 added 3 commits February 9, 2024 17:00
@BD103
chore(dynamic_plugin): deny unsafe_op_in_unsafe_fn
5027f24
@BD103
feat(dynamic_plugin): add safety comments to dynamically_load_plugin
9d44dcb 
Additionally restrict the function further so that it follows the safety requirements of `libloading::Library::new`.
@BD103
refactor: fmt
46e573c
@BD103 BD103 force-pushed the dynamic_plugin/unsafe_fn branch from 71f4dab to 46e573c Compare February 9, 2024 22:00
@BD103
Copy link
Member Author

BD103 commented Feb 9, 2024

I just rebased this branch and resolved the conflicts. :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bevyengine:main with commit 078dd06 Feb 12, 2024
23 checks passed
@BD103 BD103 deleted the dynamic_plugin/unsafe_fn branch February 12, 2024 17:34
@BD103 BD103 mentioned this pull request Feb 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tguichaoua tguichaoua tguichaoua approved these changes

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
D-Complex Quite challenging from either a design or technical perspective. Ask for help! P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BD103 @james7132 @tguichaoua @alice-i-cecile