Skip to content

mut_from_ref and Pin<&mut> #7749

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.

Sign up for GitHub

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

Jump to bottom
Closed
geeklint opened this issue Oct 2, 2021 · 4 comments · Fixed by #14471
Closed

mut_from_ref and Pin<&mut> #7749

geeklint opened this issue Oct 2, 2021 · 4 comments · Fixed by #14471
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@geeklint
Copy link

geeklint commented Oct 2, 2021

Lint name: mut_from_ref

I tried this code:

use std::pin::Pin;

pub struct IsUnpin {
    data: u32,
    _marker: std::marker::PhantomPinned,
}

impl IsUnpin {
    pub fn test<'a>(self: Pin<&'a mut Self>, _something_else: &'a ()) -> &'a mut u32 {
        unsafe {
            &mut self.get_unchecked_mut().data
        }
    }
}

I expected to see this happen: No errors

Instead, this happened: Clippy reported the mut_from_ref error level lint.

The lint documentation says

To be on the conservative side, if there’s at least one mutable reference with the output lifetime, this lint will not trigger.

and the lint does not trigger if it is just Pin<&mut Self> by itself, only when it is used in conjunction with another immutable borrow. If there is a reason this is not a false positive, can someone explain the details to me?

Meta

Rust version (rustc -Vv):

rustc 1.55.0 (c8dfcfe04 2021-09-06)
binary: rustc
commit-hash: c8dfcfe046a7680554bf4eb612bad840e7631c4b
commit-date: 2021-09-06
host: x86_64-unknown-linux-gnu
release: 1.55.0
LLVM version: 12.0.1

@ rustbot label +I-suggestion-causes-error
@geeklint geeklint added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Oct 2, 2021
@xFrednet
Copy link
Member

xFrednet commented Oct 2, 2021

This looks like a false positive. From the example, I would be guessing that the logic only checks if self is mut and not what the type of self is (I can be wrong though)

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Oct 7, 2021

Did some research, and it would appear that this lint isn't taking smart pointers into account. Some relevant code snippets

if let FnRetTy::Return(ty) = decl.output {
    if let Some((out, Mutability::Mut, _)) = get_rptr_lm(ty) {
        let mut immutables = vec![];
        for (_, ref mutbl, ref argspan) in decl
            .inputs
            .iter()
            .filter_map(get_rptr_lm)
            .filter(|&(lt, _, _)| lt.name == out.name)
        {
            if *mutbl == Mutability::Mut {
                return;
            }
            immutables.push(*argspan);
        }
        if immutables.is_empty() {
            return;
        }
        span_lint_and_then(
            cx,
            MUT_FROM_REF,
            ty.span,
            "mutable borrow from immutable input(s)",
            |diag| {
                let ms = MultiSpan::from_spans(immutables);
                diag.span_note(ms, "immutable borrow here");
            },
        );
    }
}
fn get_rptr_lm<'tcx>(ty: &'tcx Ty<'tcx>) -> Option<(&'tcx Lifetime, Mutability, Span)> {
    if let TyKind::Rptr(ref lt, ref m) = ty.kind {
        Some((lt, m.mutbl, ty.span))
    } else {
        None
    }
}

My first thought about fixing this is to scan for input types that implement DerefMut, and have a lifetime matching that of the output, however it would appear that we don't have that information in this context, The input to this part of the code is of the type FnDecl, and I can't determine what traits the input may implement based on that info alone. Any tips on how we might get that info?

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Oct 7, 2021

Also I just realized this lint isn't recursing into tuples, slices, or arrays.

@Xaeroxe
Copy link
Contributor

Xaeroxe commented Oct 7, 2021

I found this: https://github.com/rust-lang/rust-clippy/blob/master/clippy_utils/src/ty.rs#L118 which seems useful. I also learned that walking a type returns lifetime information, now I just need to determine a way to make that lifetime information useful in this context.

@arnaudgolfouse arnaudgolfouse mentioned this issue Mar 25, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Apr 14, 2025
@blyxyas
Consider nested lifetimes in mut_from_ref (#14471)
d3267e9 
fixes #7749.
This issue proposes searching for `DerefMut` impls, which is not done
here: every lifetime parameter (aka `<'a>`) is the input types is
considered to be potentially mutable, and thus deactivates the lint.

changelog: [`mut_from_ref`]: Fixes false positive, where lifetimes
nested in the type (e.g. `Box<&'a mut T>`) were not considered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Consider nested lifetimes in mut_from_ref arnaudgolfouse/rust-clippy
3 participants
@geeklint @Xaeroxe @xFrednet