Add lint for debug_assert_with_mut_call

[hellow554]

closes #1526

What does not work:

• detecting a mut call in the format string itself, e.g. debug_assert!(false, "{}", vec![1].pop())
• detecting *mut T usage (pointer)

---

changelog: add new lint debug_assert_with_mut_call

[bors]

☔ The latest upstream changes (presumably #4657) made this pull request unmergeable. Please resolve the merge conflicts.

[hellow554]

I think I'm done with this? @flip1995 do you want to see anything else? If not, I would rebase and force push the commits and pretty up the first comment in this thread. Are you okay with that?

[hellow554]

Rebased and ready to ship 🎉

[flip1995]

I just got one more test idea:

let mut x = 42_usize;
debug_assert!({
    foo(&mut x);
    x > 10
});

[hellow554]

let mut x = 42_usize;
debug_assert!({
    foo(&mut x);
    x > 10
});

That's nasty! Who writes such code ^^
Ofc it isn't covered yet. Adding code for it :/

Btw: every block isn't covered yet, e.g. if, match, for, while. Do you think adding them is worth it? (I don't think so)

[hellow554]

error: do not call functions with mutable arguments inside of a `debug_assert!`
   --> tests/ui/debug_assert_with_mut_call.rs:104:19
    |
104 |       debug_assert!({
    |  ___________________^
105 | |         bool_mut(&mut x);
106 | |         x > 10
107 | |     });
    | |_____^

the span now doesn't point at the function call, but to the block itself. Meh :/

On the other hand:

error: do not call functions with mutable arguments inside of a `debug_assert!`
   --> tests/ui/debug_assert_with_mut_call.rs:105:9
    |
105 |         bool_mut(&mut x);
    |         ^^^^^^^^^^^^^^^^
    |

This doesn't look like if it's inside the debug_assert macro at all. What do you think about this @flip1995

[hellow554]

I've gone full retard mode now and covered every ExprKind. I think this coveres everything, except closures and inline assembly (No... just, no) ^^

[flip1995]

That's nasty! Who writes such code

This makes sense, when you want to write more complex verification code. Though the simplest example is asserting an enum variant:

debug_assert!(if let Variant(_) = expr {
    true
} else {
    false
});

I've gone full retard mode now and covered every ExprKind.

Uh, now you basically rewritten a Visitor. It may be better to just implement this, then you just have to do:

struct MutArgVisitor {
    mut_arg_found: bool,
}

impl intravisit::Visitor for MutArgVisitor {
    fn visit_expr(&mut self, expr: &'_ Expr) {
        match &expr.kind {
            ExprKind::Call(.., args) | ExprKind::MethodCall(.., args) => {
                // check args, 
                if /* any arg is mutable */ {
                     self.mut_arg_found = true;
                }
            }
        }

        walk_expr(expr);
    }
}

Just grep the Clippy Codebase for intravisit to find more examples on how to write and use Visitors.

[hellow554]

@Manishearth can you help me again please? I try to solve the *mut pointer issue, but I can't get the type from adjustments table:

[clippy_lints/src/mutable_debug_assertion.rs:131] &expr.kind = Call(
    expr(HirId { owner: DefIndex(36), local_id: 1089 }: bool_ptr_mut),
    [
        expr(HirId { owner: DefIndex(36), local_id: 1091 }: v),
    ],
)
[clippy_lints/src/mutable_debug_assertion.rs:131] &expr.kind = Path(
    Resolved(
        None,
        path(bool_ptr_mut),
    ),
)
[clippy_lints/src/mutable_debug_assertion.rs:138] self.cx.tables.adjustments().get(expr.hir_id) = None
[clippy_lints/src/mutable_debug_assertion.rs:131] &expr.kind = Path(
    Resolved(
        None,
        path(v),
    ),
)
[clippy_lints/src/mutable_debug_assertion.rs:138] self.cx.tables.adjustments().get(expr.hir_id) = None

This is the debug output of bool_ptr_mut(v) where v is a *mut u32 with fn bool_ptr_mut(_: *mut u32) -> bool { false }.
As you can see the adjustements table doesn't have the type of v in it's Vec. What am I missing here?

[Manishearth]

tbh I feel like it's fine if we don't detect that case. The lint is already quite advanced (and I'm hoping it's not too slow to apply)

[flip1995]

The usage of the visitor should be more like the one of other visitors in the code base. Also as @Manishearth wrote, you can just leave out the mut pointer case.

[hellow554]

If you give the okay fir this PR, I will rebase it again and then it can be merged (finally) 🎉

[flip1995]

Looks great now, thanks! Rebase and this is ready to go

[hellow554]

Done. Waiting for travis (which failed the last two times, is there something broken?)

[flip1995]

Unrelated to this PR. Rustup in #4715

[phansch]

@bors r=flip1995

[bors]

📌 Commit 5572476 has been approved by flip1995

[bors]

⌛ Testing commit 5572476 with merge 850dfda...

[bors]

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 850dfda to master...