Bug 17300

Summary: Patch: avoid deep recursion in methods:::cbind and methods:::rbind
Product: R Reporter: Suharto Anggono <suharto_anggono>
Component: WishlistAssignee: R-core <R-core>
Status: CLOSED FIXED    
Severity: enhancement CC: maechler, michafla
Priority: P5    
Version: R-devel (trunk)   
Hardware: All   
OS: All   
URL: https://stat.ethz.ch/pipermail/r-devel/2016-May/072682.html
Attachments: cbind.R and rbind.R in methods
cbind.R and rbind.R in package methods
cbind.R and rbind.R in package methods
Package methods: rbind.R: missed change
Package methods: cbind.R, rbind.R

Description Suharto Anggono 2017-06-30 12:14:02 UTC
Created attachment 2269 [details]
cbind.R and rbind.R in methods

From https://stat.ethz.ch/pipermail/r-devel/2016-May/072682.html :

rbind() and cbind() are able to deal with S4 "matrix-like" objects, via the
hidden methods:::rbind /  methods:::cbind functions where these recursively build on appropriate S4 methods for rbind2() / cbind2().

A simple MRE (minimal reproducible example) for the problem seen with Matrix:

  S <- sparseMatrix(i=1:4, j=9:6, x=pi*1:4)
  n <- 410 # it succeeds for 407 -- sometimes even with 408
  X <- replicate(n, S, simplify = FALSE)
  Xb <- do.call("rbind", X)
  ## -> Error in as(x, "CsparseMatrix") : node stack overflow

The source of deep recursion is that the code of methods:::cbind uses do.call(cbind, *) for 3 or more arguments. The code of methods:::rbind is similar. This patch avoid it. Effectively, this calls 'cbind2' iteratively.
Comment 1 Suharto Anggono 2017-06-30 12:31:18 UTC
Created attachment 2270 [details]
cbind.R and rbind.R in package methods
Comment 2 Suharto Anggono 2017-06-30 12:46:32 UTC
Created attachment 2271 [details]
cbind.R and rbind.R in package methods
Comment 3 Michael Lawrence 2017-07-03 02:41:40 UTC
Thanks. I will work on merging this.
Comment 4 Suharto Anggono 2017-07-27 16:02:34 UTC
Created attachment 2286 [details]
Package methods: rbind.R: missed change

Sorry, attachment 2271 [details] has this change to cbind.R, but not to rbind.R.
'iV' is not used afterwards.
Comment 5 Suharto Anggono 2017-08-04 16:41:25 UTC
Could the patch in attachment 2286 [details] be applied?
Comment 6 Michael Lawrence 2017-08-04 16:56:34 UTC
Applied, thanks for the contribution.
Comment 7 Suharto Anggono 2017-08-06 09:06:32 UTC
Created attachment 2287 [details]
Package methods: cbind.R, rbind.R
Comment 8 Suharto Anggono 2017-08-06 12:21:41 UTC
attachment 2287 [details]: fix 'N2' in 'rbind', fix commented code, cleaner 'ism2'

Embarassingly for me, attachment 2271 [details] has another change that is applied to cbind.R and not to rbind.R. Assignment to 'N2' should use Nms(na), not Nms(2). An example that gives wrong row names because of it, modified from the example in reg-tests-1d.R:
myM <- setClass("myMatrix", contains="matrix")
rbind(1:2, c=2, myM(4:1,2), "a+"=10)

For 'rbind', result like in R 3.4.1 can be obtained by uncommenting the following.
    ## if(deparse.level == 0)
    ##     if(i == 1L) return(r) else next

With changed 'ism2', it behaves like the comment "and since it's a 'matrix' now, cbind() below may not name it" and like in R 3.4.1. With the original 'ism2', when 'fix.na' is TRUE, the last name is set twice, inside and outside the 'for' loop.