Detecting mutation in debug_assert!

[alex-ozdemir]

Problem

Hey there. I stumbled upon a possible use case for clippy: detecting mutation inside debug_assert! (and its two sister macros).

Why might this be a problem? Consider this program:

fn main() {
    let mut l = vec![0];
    debug_assert!(l.pop().is_some());
    println!("{:?}", l.len());
}

And notice the difference when it is compiled with an without optimization

$ rustc -O debug.rs && ./debug 
1
$ rustc debug.rs && ./debug 
0

Generally this is undesirable.

Solution

I wanted to ask how solvable this is. As I understand it, by the time the compiler has type information (which would be needed to approximately detect mutation), macros have long-since been expanded. Would it be possible to find out whether a potentially-mutating statement

1. Was in the user's original source?
2. Was originally inside one of the debug_assert* macros?

Any advice would be much appreciated!

Other Things

Perhaps this is not worth linting -- is there a significant class of false positive?

Also, the embarrassing evidence that this can be a problem.

[Manishearth]

Heh, this is a pretty neat bug.

[killercup]

I'd lint this in any kind of assert macro.

(Also works nicely as underhanded code btw)

[hellow554]

Maybe one could generalize that: If a method call with &mut self as argument is made (or anything else thatn &self or self (&T / T)), then issue a potential warning?

[flip1995]

This shouldn't be that hard to implement anymore. With

rust-clippy/clippy_lints/src/utils/mod.rs

Lines 747 to 756 in df80193

	/// Returns the pre-expansion span if the span directly comes from an expansion
	/// of the macro `name`.
	/// The difference with `is_expn_of` is that in
	/// ```rust,ignore
	/// foo!(bar!(42));
	/// ```
	/// `42` is considered expanded from `foo!` and `bar!` by `is_expn_of` but only
	/// `bar!` by
	/// `is_direct_expn_of`.
	pub fn is_direct_expn_of(span: Span, name: &str) -> Option<Span> {

we can find expressions that come from direct expansions of assert macros. Then we need to check if a ExprKind::MethodCall takes it first argument as mutable.

[hellow554]

Then we need to check if a ExprKind::MethodCall takes it first argument as mutable.

How to exactly do that? I tried it with a LateLintPass, but it seems, that I can't get any variable information regarding mutablity from it, or am I missing something? All I can get is a MethodCall ExprKind which gives me the the arguments as Expr.
What I got so far is:

[clippy_lints/src/mutable_debug_assertion.rs:44] (&seg, &args) = (
    PathSegment {
        ident: remove#0,
        hir_id: Some(
            HirId {
                owner: DefIndex(12),
                local_id: 151,
            },
        ),
        res: Some(
            Err,
        ),
        args: None,
        infer_args: true,
    },
    [
        expr(HirId { owner: DefIndex(12), local_id: 153 }: v),
        expr(HirId { owner: DefIndex(12), local_id: 154 }: 0),
    ],
)
[clippy_lints/src/mutable_debug_assertion.rs:30] &e.kind = Path(
    Resolved(
        None,
        path(v),
    ),
)
[clippy_lints/src/mutable_debug_assertion.rs:30] &e.kind = Lit(
    Spanned {
        node: Int(
            0,
            Unsuffixed,
        ),
        span: foobar.rs:5:31: 5:32,
    },
)

where you can see the result of assert_eq!(v.remove(0), 1).

How can I receive the mutability of the arguments passed to the function? Is it even possible in HIR?

[hellow554]

@flip1995 can you give me a hint to my question, please? :)

[flip1995]

Oh sorry, I missed that one.

I'm not really sure myself on how to do this. What I would try is to get the cx.tables.expr_ty of the expression args of the method call. If one of them is a mutable Ref, the lint should trigger.

I don't think this is a problem if something mutable is passed by move, since either

1. (non-Copy-case): the value won't be usable after the debug_assert!, since it was used, or
2. (Copy-case): Not the value in the argument gets modified in the debug_assert!, but the copied value: Playground