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: remove shared ref to static mut #73

Merged
merged 3 commits into from
Apr 15, 2024
Merged

Conversation

JyJyJcr
Copy link
Contributor

@JyJyJcr JyJyJcr commented Apr 6, 2024

Proposed changes

This PR removes shared references to mutable statics, which is announced to be treated as hard error in 2024 edition.
Link to the issue: rust-lang/rust#114447
While the compiler suggests to use addr_of!() or addr_of_mut!(), In most cases we use borrow() because we need not raw pointers but references.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have written my commit messages in the Conventional Commits format.
  • I have read the CONTRIBUTING doc
  • I have added tests (when possible) that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@JyJyJcr JyJyJcr changed the title Rem warn exam fix: remove shared ref to static mut Apr 6, 2024
@dekobon dekobon requested a review from bavshin-f5 April 9, 2024 20:46
@bavshin-f5
Copy link
Member

Thanks for the PR!

I don't believe std::borrow::Borrow is the right fix here. It silences the compiler diagnostic, but does not forbid creating shared references or mutating the global (I'm actually convinced it is a flaw in the diagnostic).

The proper way to address the UB could be a static with interior mutability (e.g. SyncUnsafeCell). This would require waiting for the stabilization of sync_unsafe_cell and implementing Sync for the ngx_module_t (which is safe, as the module declarations are only ever used from the main thread).
And while we're waiting for the API necessary for a proper fix, it's better to use a suggested mitigation method (&*addr_of!()) or recommend to disable the warning. Either way would keep the problematic constructions visible.

BTW, there's a proposal to remove static mut in the edition after 2024, so SyncUnsafeCell is also the most future-proof solution.

@JyJyJcr
Copy link
Contributor Author

JyJyJcr commented Apr 13, 2024

Thank you for the review!

Now I replaced borrow() to &*addr_of!() following the recommended migration. I agree that using borrow() is defective.

I had tried to use addr_of!() to deal with the warning, but I couldn't find the proper way. Thank you for teaching me the pattern working correctly.

Copy link
Member

@bavshin-f5 bavshin-f5 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@dekobon dekobon merged commit b8377d0 into nginx:master Apr 15, 2024
5 of 6 checks passed
bavshin-f5 added a commit to bavshin-f5/ngx-rust that referenced this pull request Apr 23, 2024
The example is disabled by default and was missed in nginx#73
ivanitskiy pushed a commit that referenced this pull request Apr 25, 2024
The example is disabled by default and was missed in #73
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants