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

Double dispatch doesn't seem to use "the first method" when the methods are different #1195

Closed
yutannihilation opened this issue Sep 16, 2018 · 2 comments

Comments

@yutannihilation
Copy link
Contributor

commented Sep 16, 2018

"14.7.5 Double dispatch" says:

The methods are different, and R calls the first method with a warning.

But the actual implementation seems different; C function DispatchGroup is

    if( lsxp != rsxp ) {
	if ( isFunction(lsxp) && isFunction(rsxp) ) {
	    /* special-case some methods involving difftime */

	    ...snip...

	    else {
		warning(_("Incompatible methods (\"%s\", \"%s\") for \"%s\""),
			lname, rname, generic);
		UNPROTECT(4);
		return 0;
	    }
	}

https://github.com/wch/r-source/blob/fabab8108783c8c48ae277120cfa0046b7b48f17/src/main/eval.c#L3623-L3641

This means do_arith skips this if branch and uses the internal method dispatch, which follows after:

	if (DispatchGroup("Ops", call, op, args, env, &ans))
	    return ans;

https://github.com/wch/r-source/blob/5a156a0865362bb8381dcd69ac335f5174a4f60c/src/main/arithmetic.c#L403-L404

So, I feel the line should be

The methods are different, and R uses the internal dispatch with a warning.

(FWIW, I noticed this when I investigated the error below, which I don't come up with the nice way to do (other than using S4) yet...)

library(ggplot2)

set.seed(100)
d1 <- data.frame(x = 1:100, y = cumsum(runif(100)))
d2 <- data.frame(x = 1:100, y = cumsum(runif(100)))

plot_all <- function(...) {
  l <- lapply(list(...), function(d) ggplot(d, aes(x, y)) + geom_line())
  l <- unname(l)
  class(l) <- "manyplot"
  l
}

print.manyplot <- function(x, ...) {
  do.call(gridExtra::grid.arrange, x)
}

p <- plot_all(d1, d2)
p

`+.manyplot` <- function(e1, e2) {
  l <- lapply(e1, function(x) x + e2)
  class(l) <- "manyplot"
  l
}

p + theme_bw()
#> Warning: Incompatible methods ("+.manyplot", "+.gg") for "+"
#> Error in p + theme_bw(): non-numeric argument to binary operator
@hadley

This comment has been minimized.

Copy link
Owner

commented Sep 16, 2018

Oh good point. Do you want to do a PR?

ggplot2 might eventually end up using the double-dispatch approach in vctrs, once I've worked it out)

@yutannihilation

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

Thanks! I'll create a PR.

ggplot2 might eventually end up using the double-dispatch approach in vctrs

Wow, cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.