borrow_interior_mutable_const should ignore UnsafeCells behind pointers

[daboross]

+Hi!

I recently made a pull request to TockOS which switches to using UnsafeCell for memory-mapped registers. This triggered a clippy lint, clippy::borrow_interior_mutable_const, which I believe is incorrect and a false positive.

Specifically, when there's a const item that contains a nesting like:

*const UnsafeCell<T>

Borrowing said const item should not invoke borrow_interior_mutable_const. As I understand it, this lint is warning against accidentally copying a constant and then mutating it, so that the mutation goes nowhere. However, when behind a ptr, the UnsafeCell itself is not copied, and the mutation is effective. This should not trigger the lint.

Here's a self-contained example reproducing the error. It's a bit long, but most of it is required to reproduce the error, and I want to present a full picture of why code like this exists:

use std::cell::UnsafeCell;
use std::ops::Deref;

// StaticRef from https://github.com/tock/tock/blob/37a9ac4c15a7b2c1065645cb28bd9a4da5732315/kernel/src/common/static_ref.rs#L14

/// A pointer to statically allocated mutable data such as memory mapped I/O
/// registers.
///
/// This is a simple wrapper around a raw pointer that encapsulates an unsafe
/// dereference in a safe manner. It serve the role of creating a `&'static T`
/// given a raw address and acts similarly to `extern` definitions, except
/// `StaticRef` is subject to module and crate boundaries, while `extern`
/// definitions can be imported anywhere.
pub struct StaticRef<T> {
    ptr: *const T,
}

impl<T> StaticRef<T> {
    /// Create a new `StaticRef` from a raw pointer
    ///
    /// ## Safety
    ///
    /// Callers must pass in a reference to statically allocated memory which
    /// does not overlap with other values.
    pub const unsafe fn new(ptr: *const T) -> StaticRef<T> {
        StaticRef { ptr }
    }
}

impl<T> Deref for StaticRef<T> {
    type Target = T;

    fn deref(&self) -> &'static T {
        unsafe { &*self.ptr }
    }
}

/// Approximate of `WriteOnly` from tock.
#[repr(transparent)]
pub struct WriteOnly<T: Copy> {
    value: UnsafeCell<T>,
}

impl<T: Copy> WriteOnly<T> {
    #[inline]
    /// Set the raw register value
    pub fn set(&self, value: T) {
        unsafe { ::core::ptr::write_volatile(self.value.get(), value) }
    }
}

struct RegisterSet {
    thing_1: WriteOnly<u32>,
}

// The cast is trivially incorrect on desktop operating systems, but code
// similar to it is correct on any embedded systems with memory-mapped
// peripherals.
//
// This is a common pattern in TockOS.
const STATIC_REF: StaticRef<RegisterSet> =
    unsafe { StaticRef::new(0x0200_0000 as *const RegisterSet) };

pub fn uses_static_ref() {
    // This should be OK. But instead, we get:
    STATIC_REF.thing_1.set(3);
    // ~^error: a `const` item with interior mutability should not be borrowed
}

(playground)

I expect this to lint completely correctly, with no warnings.

Instead, I get an error:

error: a `const` item with interior mutability should not be borrowed
  --> src/lib.rs:66:5
   |
66 |     STATIC_REF.thing_1.set(3);
   |     ^^^^^^^^^^
   |
   = note: `#[deny(clippy::borrow_interior_mutable_const)]` on by default
   = help: assign this const to a local or static variable, and use the variable here
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#borrow_interior_mutable_const

For completeness, here's a fully minimal example:

use std::cell::UnsafeCell;
use std::ops::Deref;

pub struct StaticRef<T> {
    ptr: *const T,
}

impl<T> StaticRef<T> {
    /// ## Safety
    ///
    /// Callers must pass in a reference to statically allocated memory which
    /// does not overlap with other values.
    pub const unsafe fn new(ptr: *const T) -> StaticRef<T> {
        StaticRef { ptr }
    }
}

impl<T> Deref for StaticRef<T> {
    type Target = T;

    fn deref(&self) -> &'static T {
        unsafe { &*self.ptr }
    }
}

struct RegisterSet(UnsafeCell<u32>);

const STATIC_REF: StaticRef<RegisterSet> = unsafe { StaticRef::new(std::ptr::null()) };

pub fn uses_static_ref() {
    let _x = &STATIC_REF.0;
}

(playground)

Meta

• cargo clippy -V: clippy 0.0.212 (bb37a0f 2020-06-16)
• rustc -Vv:

  rustc 1.44.1 (c7087fe00 2020-06-17)
  binary: rustc
  commit-hash: c7087fe00d2ba919df1d813c040a5d47e43b0fe7
  commit-date: 2020-06-17
  host: x86_64-apple-darwin
  release: 1.44.1
  LLVM version: 9.0