convert() should be idempotent when from is an instance of to

[lawremi]

If from is an instance of to, i.e., identical(S7_class(from), to) is TRUE (assuming from is an S7 object), then no dispatch should occur.

This is not (just) about efficiency. If there are convert() methods for downcasting to to, one of them will be called even if there is no need to downcast, potentially resulting in data being overwritten, or other weird things happening.

I'll also note that methods::as() is idempotent when to is class_any, but class_any is not allowed with convert(). Somewhat pedantic, but I thought it was worth mentioning.

[lawremi]

Just realized the problem is worse: if inherits(from, to) is TRUE, then the developer probably doesn't want to call any method higher on the class hierarchy than to. That is, an inherited upcasting method (for which there is a default) is desired, but not an inherited downcasting method.

The methods package seems to get this right, but the implementation of the logic probably rivals the entire S7 package in terms of lines of code.

Perhaps something like this would work inside convert() (untested):

  from_dispatch <- obj_dispatch(from)
  to_class <- class_register(to)
  if (inherits(from, to))
    from_dispatch <- head(from_dispatch, match(to_class, from_dispatch) - 1L)
  if (length(from_dispatch) == 0L)
    return(from)
  dispatch <- list(from_dispatch, to_class)
  convert <- .Call(method_, convert, dispatch, environment(), FALSE)

Or a bit more efficiently:

  from_dispatch <- obj_dispatch(from)
  to_class <- class_register(to)
  m <- match(to_class, from_dispatch)
  if (!is.na(m)) # upcasting case: avoid downcasting methods
    from_dispatch <- head(from_dispatch, m - 1L)
  if (length(from_dispatch) == 0L)
    return(from)
  dispatch <- list(from_dispatch, to_class)
  convert <- .Call(method_, convert, dispatch, environment(), FALSE)

[lawremi]

Just a note that the above solution does not preclude dispatch to method(convert, list(class_any, Foo)), except in the case of the early return. That would require changes at the dispatch level.

Another note: this change would also prevent the current loophole of:

method(convert, list(Foo, Foo)) <- function(from) from

which a devious person, like me, might define to make convert() behave in a non-strict manner, like the proposal in #428. It appears to satisfy the contract of convert(), but it actually breaks it.

[t-kalinowski]

Was this resolved by #444?

[lawremi]

No, I don't think so. This relates to selecting a convert() method. Let's say you have three classes, A <- B <- C. By default, calling convert(c, B) should upcast to B. However, let's say there is a user-defined method(convert, list(A, B)). Calling convert(c, B) would select the A => B method, which is probably not desirable. It might, for example, replace some slots in c with default values when forming b. Instead, we want it to fallback to upcasting. The suggestions above, which you can find in the convert-tweaks branch, avoid selecting a method that is further up the hierarchy from to, when from already inherits from to.

Btw, I think the convert-tweaks branch also added default downcasting, like #444.

[t-kalinowski]

Link to convert-tweaks: main...convert-tweaks