rust-lang/rust

`FnOnce` doesn't seem to support type-based dispatch (as `Add`, etc. do)

Closed this issue · 11 comments

UPDATE: Mentoring instructions below.


Here's a minimal example program showing the issue:

#![feature(fn_traits)] 
#![feature(unboxed_closures)]

struct Ishmael;
struct Maybe;
struct CallMe;

impl FnOnce<(Ishmael,)> for CallMe {
    type Output = ();
    extern "rust-call" fn call_once(self, _args: (Ishmael,)) -> () {
        println!("Split your lungs with blood and thunder!");
    }
}

impl FnOnce<(Maybe,)> for CallMe {
    type Output = ();
    extern "rust-call" fn call_once(self, _args: (Maybe,)) -> () {
        println!("So we just met, and this is crazy");
    }
}

fn main() {
    CallMe(Ishmael);
    CallMe(Maybe);
}

This works perfectly if you comment out either the Ishmael or Maybe implementations, and the corresponding call. I've wanted this behavior at various times - if nothing else, it'd be useful for binding to certain parts of C++ - and at least in theory it's the same exact mechanism Add::add uses.

In addition, the error message is (to steal lovingly borrow a term from the Perl 6 community) "less than awesome" - it informs the user error[E0059]: cannot use call notation; the first type parameter for the function trait is neither a tuple nor unit when it clearly is (as it succeeds in the single-impl case).

It also claims error[E0619]: the type of this value must be known in this context, but only for the argument of the first call - reversing the order also exchanges the subject of the error message.

Should this be supported? If so, what needs done? If not, how can we make the error messages more helpful? Not supporting it now and adding support later is forwards-compatible, but at very least the error message should probably be improved before stabilization (cc #29625)

EDIT: Ah, seems this may be covered by #18952

Incidentally, disallowing it doesn't really reduce people's ability to write multi-dispatch - it just makes it uglier. Consider the atrocity I just perpetrated (to see if I could):

use std::ops::Add;

struct Ishmael;
struct Maybe;
struct CallMe;

impl Add<Ishmael> for CallMe {
    type Output = ();
    fn add(self, _args: (Ishmael)) -> () {
        println!("Split your lungs with blood and thunder!");
    }
}

impl Add<Maybe> for CallMe {
    type Output = ();
    fn add(self, _args: (Maybe)) -> () {
        println!("So we just met, and this is crazy");
    }
}

impl Add<(Ishmael, Maybe)> for CallMe {
    type Output = ();
    fn add(self, _args: (Ishmael, Maybe)) -> () {
        println!("But I didn't know salty captains liked pop");
    }
}

fn main() {
    CallMe+(Ishmael);
    CallMe+(Maybe);
    CallMe+(Ishmael, Maybe);
}
eddyb commented

cc @rust-lang/lang Given that, in the future, Variadic Generics would almost certainly allow arity dispatch (if we ever get them), and that Fn impls are unstable for now, can't we just fix this/#18952?

AIUI, the fix involves generating an N-tuple of inference variables for an N-argument call, i.e. instead of searching for FnOnce<_>, we'd be searching for FnOnce<(_, _, _)> for a 3-argument call.

eddyb commented

As @nikomatsakis is no longer opposed to the fix, here's some context for it:
The problem lies with the last argument here being None:

match self.lookup_method_in_trait(call_expr.span,
method_name,
trait_def_id,
adjusted_ty,
None) {

It's equivalent to this:

Some(&[self.next_ty_var(TypeVariableOrigin::TypeInference(call_expr.span))])

Instead, arg_exprs from check_call

pub fn check_call(&self,
call_expr: &'gcx hir::Expr,
callee_expr: &'gcx hir::Expr,
arg_exprs: &'gcx [hir::Expr],

should be passed down through try_overloaded_call_step to try_overloaded_call_traits and its length be used to determine the arity.
Then, tcx.mk_tup can be used to create a tuple, with a different inference variable (created by self.next_ty_var calls as above) for each argument, to pass to the lookup_method_in_trait call.

@eddyb I figured I would give this a try. I've got the above example compiling, but I'm just looking into a few failures that probably have something to do with resolving closure checks in DeferredCallResolution::resolve.

eddyb commented

@sifkarov Can you open a pull request? Could be just a pre-existing bug that gets exposed by this.

@eddyb I think the issue was more or less just me being silly... I'll open a pull request shortly.

@sifkarov Did you ever make a PR for this?

jhod0 commented

I've run into a problem that I think is the converse of this. I am trying to implement wrappers for an integration library, and want to automatically handle functions of any number of dimensions, so something like this:

trait Integrand {
    ...
}

impl<F> Integrand for Fn(f64) -> f64 {
    ...
}

impl<F> Integrand for Fn(f64, f64) -> f64 {
    ...
}

But the compiler gives me a "conflicting implementation" error, since it sees both cases as impl<A> Integrand for Fn<A>. Is there any way for me to fix this, or will I need to wait for the same fixes as this and #29625 ?

eddyb commented

@jhod0 Did you mean impl<F: Fn(f64) -> f64> Integrand for F?
Because then the error is legitimate and I don't see a way for us to support it, other than having a form of "impl specialization" which can support overlaps, by requiring another impl, e.g.:

impl<F: Fn(f64) -> f64 + Fn(f64, f64) -> f64> Integrand for F {
    // here you'd have to choose which "overload" to use if both exist
}

This seems like it has been fixed (by #55986) and can be closed, no?

This does appear to be fixed by #55986. Closing as fixed.