An Unfortunate Coercion

14 August 2015

When we started Rust-clippy’s eta-reduction lint, we did not have in mind that calling a closure involves deref coercions. This leads to false positives wherever a closure that takes a reference to X is called with another type that Derefs to X (as can be seen here – the over function is called via a closure to coerce the std::ptr::P smart pointers to &[PathSegment] slice refs.

Note that this aren’t exactly false positives – the closure is only calling the function with its arguments – but due to the deref coercions, our suggestion of removing the closure will actually lead to errors: Should we change over(left, right, |l, r| is_exp_equal(l, r)) to over(left, right, is_exp_equal), we get

src/eq_op.rs:71:5: 71:9 error: type mismatch: the type `fn(&syntax::ast::Expr,
&syntax::ast::Expr) -> bool {eq_op::is_exp_equal}` implements the trait `for<'r,
'r> core::ops::FnMut<(&'r syntax::ast::Expr, &'r syntax::ast::Expr)>`, but the
trait `for<'r, 'r> core::ops::FnMut<(&'r syntax::ptr::P<syntax::ast::Expr>, &'r
syntax::ptr::P<syntax::ast::Expr>)>` is required (expected struct
`syntax::ptr::P`, found struct `syntax::ast::Expr`) [E0281]
src/eq_op.rs:71     over(left, right, is_exp_equal)
                    ^~~~
src/eq_op.rs:71:5: 71:9 note: required by `eq_op::over`
src/eq_op.rs:71     over(left, right, is_exp_equal)
                    ^~~~

So, naturally I first tried to make the over(..) function more generic over Deref, but I couldn’t get it to work, because the function is sometimes called with a P<Vec<X>> (P is rustc’s owned smart pointer), and other times with a Vec<P<X>>, which both are silently coerced to &[X] for over. There is so much magic going on here that even if there was a generic version of over(..), I would not want to inflict our eta_reduction lint in its current state on anyone who fails to write that version.

(By the way, should you come up with a generic version that works, tell me to win internet points and bragging rights).

If the clippy code looks to complex, here is a simplified snippet with just a pointer-to-pointer to showcase the issue:

fn all<X, F>(x: &[X], y: &X, f: F) -> bool
where F: Fn(&X, &X) -> bool {
    x.iter().all(|e| f(e, y))
}

fn below(x: &u8, y: &u8) -> bool { x < y }

fn main() {
    let arr = [1u8, 2, 3];
    let arr2 = [&1u8];
    assert!(all(&arr, &5, |x, y| below(x, y)));
    assert!(all(&arr, &5, below)); // look ma, no closure!
    assert!(all(&arr2, &&2, |x, y| below(x, y))); //is coerced
  //assert!(all(&arr2, &&2, below)); // leads to ERROR
}

Now due to how the eta_reduction lint works, we have a problem: The lint matches on the closure (as it should), but to weed out the false positives, we need all expressions calling it. If any of them calls the closure with a different type than the function takes, we should balk.

The problem is exacerbated by the fact that the closure can be bound within a let-binding, loaned out to other functions, and taken, shaken, turned all around, and I think you get the picture. Even worse, closures could be public, in which case all bets are off (However, I suspect this to be a less common use case).

However, closures can be used as function parameters – a very common case. These functions usually take any of the Fn* types. So a possible approximation that would rid us of the false positives is to search for all arguments of all function/method calls where the expression type is the type of our closure and to omit the lint for those.

But even this crude approximation is a non-trivial undertaking. Also it would effectively disable the lint (because barring our very synthetic test cases, a lot of closures are actually used as an argument to some function, e.g. Iterator::map(…), Option::and_then etc. If we don’t find a better solution, we might as well get rid of the lint (or at least allow it by default and change the warning to something more cautious).

Alas, today I’m empty-handed – I’ve given you the gordian knot, but no sword to slice it. Perhaps one day there shall be a solution.

In the meantime, feel free to discuss this at /r/rust or users.rust-lang.

Bonus: Not a solution to the coercion conundrum, but at least I managed to make the lint somewhat work, albeit in a reduced fashion: It now only lints closures that are given as arguments in a method call – within this construct, we can check if type adjustments have been made to any argument of the closure within the call, and in that case omit the lint. The code to check for type adjustments is:

fn is_adjusted(cx: &Context, e: &Expr) -> bool {
    cx.tcx.tables.borrow().adjustments.get(&e.id).is_some()
}

... if args.iter().any(|arg| is_adjusted(cx, arg)) { return; } // omit lint