Some tests fail with upcoming release of R6

[wch]

I'm about to submit a new version of R6 to CRAN, but am getting errors with this package when I run checks on reverse dependencies.

Some background: In previous versions of R6, when cloning an object with active bindings, it will extract the function for the active binding at cloning time and put it in the new object. This relied on behavior from as.list.environment which apparently was a bug, and will be changed in upcoming versions of R. The bug was that as.list.environment would give the function behind an active binding, rather than the value that the function returns when invoked. In upcoming versions of R, it will call the function and return that value.

I have been asked by a Luke Tierney (who is a member of R-Core) to release a new version of R6 which does not rely on the buggy behavior of as.list.environment, so that he can fix the bug in R. When the bug is fixed, it will no longer be possible to get the function for an active binding.

In the development version of R6, it separately stores the functions for active bindings when an R6 object is created, and uses those stored functions to create new active bindings when the object is cloned.

Currently, tests for the CRAN version of eplusr fail with the development version of R6.

eplusr check log

Package: eplusr
Check: tests
New result: ERROR
    Running ‘testthat.R’ [87s/37s]
  Running the tests in ‘tests/testthat.R’ failed.
  Complete output:
    > library(testthat)
    > library(eplusr)
    > eplusr_option(verbose_info = FALSE)
    $verbose_info
    [1] FALSE
    
    > 
    > test_check("eplusr")
    trying URL 'https://raw.githubusercontent.com/NREL/EnergyPlus/v9.1.0/idd/V8-8-0-Energy%2B.idd'
    Content type 'text/plain; charset=utf-8' length 4055399 bytes (3.9 MB)
    ==================================================
    downloaded 3.9 MB
    
    ── 1. Failure: Idf class (@test_idf.R#478)  ────────────────────────────────────
    idf2$Zone[[1]]$Name not equal to "ZONE ONE".
    1/1 mismatches
    x[1]: "zone"
    y[1]: "ZONE ONE"
    
    ── 2. Failure: Insert (@test_impl-idf.R#805)  ──────────────────────────────────
    ins$object not equivalent to data.table(...).
    Different number of rows
    
    ── 3. Failure: Insert (@test_impl-idf.R#810)  ──────────────────────────────────
    ins$value$value_id not equivalent to 349:357.
    Lengths differ: 0 is not 9
    
    ── 4. Error: Insert (@test_impl-idf.R#811)  ────────────────────────────────────
    subscript out of bounds
    1: expect_equivalent(ins$value$value_chr[[1L]], "new_mat") at testthat/test_impl-idf.R:811
    2: quasi_label(enquo(object), label, arg = "object")
    3: eval_bare(get_expr(quo), get_env(quo))
    
    �� testthat results  �����������������������������������������������������������
    [ OK: 1096 | SKIPPED: 7 | WARNINGS: 0 | FAILED: 4 ]
    1. Failure: Idf class (@test_idf.R#478) 
    2. Failure: Insert (@test_impl-idf.R#805) 
    3. Failure: Insert (@test_impl-idf.R#810) 
    4. Error: Insert (@test_impl-idf.R#811) 
    
    Error: testthat unit tests failed
    Execution halted

Please install the development version of R6 and release a new version of eplusr which has passing tests.

devtools::install_github('r-lib/R6@rc-v2.4.1')

In some cases, errors occur only when the package is loaded the normal way, but not when load_all() is used.

Since I have been asked by a member of R-core to submit this fixed version of R6, I want to release it as soon as possible, but this requires that an updated version of eplusr is on CRAN first. Thank you.

[hongyuanjia]

Thanks @wch! I will look into it and submit a new version as soon as possible.

[hongyuanjia]

@wch, I can locate where the problem comes from. However, I did not know how to handle it correctly. Here is the case:

• In my R6 class Idf, there is no active fields defined in the definitionusing R6::R6Class.

• After an Idf object is created, I manually create active bindings based on the Idf object contents using base::makeActiveBinding() in function add_idf_class_bindings(), in order to provide dynamic content query. The results of those queries are a list of R6 objects of another class called IdfObject and IdfObject does not have its own deep_clone() method.
  

  eplusr/R/idf.R

  Lines 2658 to 2666 in 272a2a3

  	# unique classes
  	for (i in setdiff(cls[flg], ls(idf))) {
  	makeActiveBinding(i, get_object_unique(env, self, private, i, value), idf)
  	}
  	# other classes
  	for (i in setdiff(cls[!flg], ls(idf))) {
  	makeActiveBinding(i, get_objects_in_class(env, self, private, i, value), idf)
  	}

• I defined a custom deep_clone() method in Idf private environment, which is pretty simple. At that time, the purpose of this function is to create a copy of private fields which are environments containing multiple data.tables. I did not know that I have already depended on the buggy behavior of as.list.environment() to get the active binding function content...
  

  eplusr/R/idf.R

  Lines 2477 to 2488 in 272a2a3

  	idf_deep_clone <- function (self, private, name, value) {
  	if (is_idd(value)) {
  	value
  	} else if (is.environment(value)) {
  	l <- as.list.environment(value)
  	# copy data.table is necessary here
  	l <- lapply(l, function (x) if (inherits(x, "data.table")) copy(x) else x)
  	list2env(l)
  	} else {
  	value
  	}
  	}

• Now with the development version of R6, as these active bindings are not active fields defined in R6::R6Class and .__enclos_env__$.__active__ being NULL, binding_names here will catch them and treat them as normal methods. After cloning, the binding environment of those active bindings will still be the old Idf, not the new one.

There are two ways coming to my mind:

1. Define a custom public clone() method that does the same as the default clone() method defined in R6 package, except that it excludes or directly removes all those active bindings that are not defined in this R6 class. The problem is that I did not know how to call the default clone() method in R6::R6Class().

2. In the custom deep_clone() method, catch that situation and assign new enclosing environment of the new cloned object. However I did not know how to get access to the new enclosing environment in the original private environment when deep_clone() is called, except the hacking parent.frame(3)...

The first one is preferable. Could you please give me some advice on how I can provide a new public clone() method and at the same time call the default clone() method in that function? Thanks a lot!

[wch]

The clone() method is quite complicated and captures a lot of edge cases (most of the difficulty is related to inheritance). I would not recommend trying to copy and modify it.

There is a base R function called bindingIsActive() which can tell you whether a binding is an active one. I think you can use that in your deep_clone() method to avoid copying active bindings.

[hongyuanjia]

Thanks @wch.

I tried to adopt your suggestion and return NULL when it is an active binding. However after cloning, they are still in the new cloned object. As this issue seems urgent, I do not want to hold you back too long for submitting new R6 because of eplusr package. So I used the first dirty approach #165. The new version of eplusr has passed tests with R6@rc-v2.4.1 and successfully landed on CRAN.

I will adopt to the 2nd approach in the next release.

[wch]

Thank you for the fast turnaround!