nginxinc / ngx-rust

Rust binding for NGINX
Apache License 2.0
731 stars 60 forks source link

fix: remove shared ref to static mut #73

Closed JyJyJcr closed 5 months ago

JyJyJcr commented 6 months ago

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: https://github.com/rust-lang/rust/issues/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.

bavshin-f5 commented 5 months ago

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 commented 5 months ago

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.