Suggest Option::as_deref{_mut} when calling Option::as_{ref,mut} on Option<Box<T>>

[lopopolo]

What it does

Suggest replacing calls to Option::<Box<T>>::as_ref and Option::<Box<T>>::as_mut with calls to Option::<Box<T>>::as_deref and Option::<Box<T>>::as_deref_mut.

Calling as_ref and as_mut on an Option<Box<T>> results in &Box<T> or &mut Box<T>, which are references to a pointer, or a double indirection. Changing these Option::as_ref and Option::as_mut calls to Option::as_deref and Option::as_deref_mut forces resolving the underlying &T and &mut T once, which amortizes the cost of following the double pointer over multiple uses of the option field.

These suggestions could also be implemented for other smart pointers like Rc and Arc.

Categories (optional)

• Kind: performance?

What is the advantage of the recommended code over the original code

The inner Box is derefed once.

Drawbacks

None.

Example

struct StateNotPresentError;

impl std::error::Error for StateNotPresentError {}

impl std::fmt::Display for StateNotPresentError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.write_str("State not present for extraction")
    }
}

struct State;

struct Interpreter {
    state: Option<Box<State>>,
}

fn heap_size(interp: &Interpreter) -> Result<String, Box<dyn std::error::Error> {
    let state = interp.state.as_ref().ok_or(StateNotPresentError)?;
    unimplemented!("Heap size not implemented");
    // access `state` multiple times
}

fn eval(interp: &mut Interpreter) -> Result<String, Box<dyn std::error::Error> {
    let state = interp.state.as_mut().ok_or(StateNotPresentError)?;
    unimplemented!("Eval not implemented");
    // access `state` multiple times
}

Could be written as:

struct StateNotPresentError;

impl std::error::Error for StateNotPresentError {}

impl std::fmt::Display for StateNotPresentError {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.write_str("State not present for extraction")
    }
}

struct State;

struct Interpreter {
    state: Option<Box<State>>,
}

fn heap_size(interp: &Interpreter) -> Result<String, Box<dyn std::error::Error> {
    let state = interp.state.as_deref().ok_or(StateNotPresentError)?;
    unimplemented!("Heap size not implemented");
    // access `state` multiple times
}

fn eval(interp: &mut Interpreter) -> Result<String, Box<dyn std::error::Error> {
    let state = interp.state.as_deref_mut().ok_or(StateNotPresentError)?;
    unimplemented!("Eval not implemented");
    // access `state` multiple times
}

[camsteffen]

So basically you have a binding of type &Box<T> and the binding is dereferenced multiple times.

There are many ways to produce a &Box<T> so I don't think it would be worth while to detect the specific case with Option::as_ref. We could instead suggest to simply deref before assignment: let state = &*..;. And, as your comment indicates, It should only lint if the binding is actually used (and implicitly dereferenced) multiple times.

In principle, I don't see any reason this would not be generalized to include all Deref types like Vec. But then I worry that such a lint will be too noisy and is likely optimized by the compiler anyways.