Skip to content
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.

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

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

Closed
eternaleye opened this issue Oct 24, 2017 · 11 comments
Closed

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

eternaleye opened this issue Oct 24, 2017 · 11 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@eternaleye
Copy link
Contributor

eternaleye commented Oct 24, 2017

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

@eternaleye
Copy link
Contributor Author

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
Copy link
Member

eddyb commented Oct 25, 2017

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 eddyb added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 25, 2017
@eddyb
Copy link
Member

eddyb commented Jan 18, 2018

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.

@nikomatsakis nikomatsakis added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed I-nominated labels Jan 19, 2018
@XAMPPRocky XAMPPRocky added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Feb 12, 2018
@sifkarov
Copy link

sifkarov commented Mar 5, 2018

@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
Copy link
Member

eddyb commented Mar 6, 2018

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

@sifkarov
Copy link

sifkarov commented Mar 6, 2018

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

@alexreg
Copy link
Contributor

alexreg commented Aug 20, 2018

@sifkarov Did you ever make a PR for this?

@jhod0
Copy link

jhod0 commented Aug 23, 2018

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
Copy link
Member

eddyb commented Sep 6, 2018

@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
}

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Jan 3, 2019
Allow to dispatch fn traits depending on number of parameters

Hello,

By following @eddyb's advise on issue rust-lang#45510, I managed to have the snippets of code in rust-lang#45510 and rust-lang#18952 passing without breaking older diagnostics.

EDIT: the codegen tests breakage I experienced is due to the poor quality of my laptop.

If any kind reviewer has any advice, you are very welcome.
bors added a commit that referenced this issue Jan 4, 2019
Allow to dispatch fn traits depending on number of parameters

Hello,

By following @eddyb's advise on issue #45510, I managed to have the snippets of code in #45510 and #18952 passing without breaking older diagnostics.

EDIT: the codegen tests breakage I experienced is due to the poor quality of my laptop.

If any kind reviewer has any advice, you are very welcome.
@jplatte
Copy link
Contributor

jplatte commented Jun 6, 2019

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

@pnkfelix
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants