initiative-sh / initiative.sh

A web-based command line for game masters
https://initiative.sh/
GNU General Public License v3.0
44 stars 4 forks source link

Clippy: creating a shared reference to mutable static is discouraged #340

Open MikkelPaulson opened 3 months ago

MikkelPaulson commented 3 months ago

Clippy sez:

error: creating a shared reference to mutable static is discouraged
  --> web/src/lib.rs:91:40
   |
91 |     if let Some(element_id) = unsafe { &ROOT_ELEMENT_ID } {
   |                                        ^^^^^^^^^^^^^^^^ shared reference to mutable static
   |
   = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447>
   = note: this will be a hard error in the 2024 edition
   = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior
   = note: `-D static-mut-refs` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(static_mut_refs)]`
help: use `addr_of!` instead to create a raw pointer
   |
91 |     if let Some(element_id) = unsafe { addr_of!(ROOT_ELEMENT_ID) } {
   |                                        ~~~~~~~~~~~~~~~~~~~~~~~~~

This needs a bit of testing to verify that it has no side effects, but "hard error in 2024" sounds bad.

ThinkerDreamer commented 3 weeks ago

What kind of testing did you have in mind? I changed that to

fn get_root_element() -> Option<Element> {
    // #[allow(static_mut_refs)]
    if let Some(element_id) = unsafe { (*addr_of!(ROOT_ELEMENT_ID)).clone() } {
        window()
            .and_then(|w| w.document())
            .and_then(|d| d.get_element_by_id(&element_id))
    } else {
        None
    }
}

and I ran test.sh and all the existing tests passed.

MikkelPaulson commented 3 weeks ago

@ThinkerDreamer There are no end-to-end tests (#61), so this case probably wouldn't be covered. It will need to be tested manually. If the website loads at all, the change is valid.

ThinkerDreamer commented 3 weeks ago

Okay, I'll try it and see 👍🏻