Refactor method-dispatch.c

[t-kalinowski]

This PR has fixes for current rchk warning, as well as non-API usage warnings.

rchk warnings:
https://raw.githubusercontent.com/kalibera/cran-checks/master/rchk/results/S7.out

Package S7 version 0.1.1
Package built using 86189/R 4.4.0; x86_64-pc-linux-gnu; 2024-03-25 22:22:05 UTC; unix   
Checked with rchk version fdc068715daa3a256062cc20e0d4a5157dacc9a4 LLVM version 14.0.6
More information at https://github.com/kalibera/cran-checks/blob/master/rchk/PROTECT.md
For rchk in docker image see https://github.com/kalibera/rchk/blob/master/doc/DOCKER.md

Function S7_obj_dispatch
  [UP] allocating function Rf_findVarInFrame(?,S:obj_dispatch) may destroy its unprotected argument (ns <arg 1>), which is later used. S7/src/method-dispatch.c:111
  [UP] calling allocating function Rf_findVarInFrame(?,S:obj_dispatch) with a fresh pointer (ns <arg 1>) S7/src/method-dispatch.c:111
  [UP] unprotected variable ns while calling allocating function Rf_install S7/src/method-dispatch.c:111
  [UP] unprotected variable ns while calling allocating function Rf_lang2 S7/src/method-dispatch.c:114
  [UP] calling allocating function Rf_eval with a fresh pointer (ns <arg 2>) S7/src/method-dispatch.c:115

Function generic_args
  [UP] calling allocating function Rf_eval with a fresh pointer (arg <arg 1>) S7/src/method-dispatch.c:57

Function method_
  [UP] unprotected variable m while calling allocating function Rf_asInteger S7/src/method-dispatch.c:98

Function method_call_
  [UP] allocating function Rf_eval may destroy its unprotected argument (arg <arg 1>), which is later used. S7/src/method-dispatch.c:158
  [UP] calling allocating function Rf_eval with a fresh pointer (arg <arg 1>) S7/src/method-dispatch.c:158

Function method_rec
  [UP] calling allocating function method_rec with a fresh pointer (val <arg 1>) S7/src/method-dispatch.c:20
  [UP] calling allocating function method_rec with a fresh pointer (val <arg 1>) S7/src/method-dispatch.c:30

non-API: #471

Result: NOTE
  File ‘S7/libs/S7.so’:
    Found non-API calls to R: ‘PRCODE’, ‘SET_PRVALUE’
  
  Compiled code should not call non-API entry points in R.
  
  See ‘Writing portable packages’ in the ‘Writing R Extensions’ manual,
  and section ‘Moving into C API compliance’ for issues with the use of
  non-API entry points.

[mmaechler]

I cannot really approve (as I did not study the check failures),
but congratulations on this refactoring, including no longer working with "promise-property"s which really are meant to be off-API

[t-kalinowski]

After exploring the removal of PRCODE(), I unfortunately had to keep it, as the tradeoffs are too costly until additional API entry points become available. I shared some findings and discussion in issue #471.

That said, I did update non-API calls where possible while I was at it. I also addressed all rchk flagged warnings (many of which are spurious or false positives, but adding a few extra PROTECT() calls seems fine).

rchk now flags no issues when run locally; rchk can now be run CI too: https://github.com/RConsortium/S7/actions/runs/11582187471/job/32244624698

[t-kalinowski]

I thought I had moved on, but woke up this morning with the solution for how to remove PRCODE and SET_PRVALUE, with minimal impact. Merging this should close #471 now.

[hadley]

Nice work!

Does this happen to fix this commented out test?

  # method(foo, class_character) <- function(x, ..., z = 1) substitute(z)
  # expect_equal(foo("x", z = letters), quote(letters))

I also double checked that we already had a test that ensures that the dispatch argument is only evaluated once, so there's no chance of a regression there.

[hadley]

Nice!!

[hadley]

Also would you mind doing a little benchmark of before and after just to verify that there's no performance regression?

[t-kalinowski]

I knitted the "performance" vignette locally and compared it before and after, and generally, the performance is similar or slightly faster. I'll verify one last time and post here before merging.

[t-kalinowski]

Does this happen to fix this commented out test?

I tweaked how we pass along the non-dispatch arguments and now substitute() works on those too! I also expanded the unit tests to cover ... args better.

[t-kalinowski]

I ran some quick benchmarks on dispatch by re-rendering the "performance" vignette on this branch and the main branch. At a glance, performance seems to have improved by about 20%.

For now, I’m including some screenshots of the vignette diffs. At some point, we may want to set up more consistent and rigorous benchmarks to track performance over time.

# Helper function to install and render vignette
render_vignette() {
  local branch=$(git branch --show-current)
  local output_file="performance_${branch}.md"
  local output_dir="~/github/RConsortium/S7/benchmarks/${branch}"

  # Install package and render vignette (warmup and actual run)
  Rscript -e 'remotes::install_local(force=TRUE)'
  Rscript -e "rmarkdown::render('~/github/RConsortium/S7/vignettes/performance.Rmd', encoding = 'UTF-8', output_format = 'github_document', output_file = '${output_file}', output_dir = '${output_dir}');"
  Rscript -e "rmarkdown::render('~/github/RConsortium/S7/vignettes/performance.Rmd', encoding = 'UTF-8', output_format = 'github_document', output_file = '${output_file}', output_dir = '${output_dir}');"
}

# Render vignette on main branch
git switch main
render_vignette "main"

# Render vignette on PR branch
export branch_name=rchk-and-non-API-fixes
git switch $branch_name
render_vignette $branch_name

# Diff the two generated Markdown files
code --diff ~/github/RConsortium/S7/benchmarks/$branch_name/performance_$branch_name.md ~/github/RConsortium/S7/benchmarks/main/performance_main.md

(this branch on the left, main branch on the right)

[hadley]

Great! Thanks so much for all your work on this.

[hadley]

Should we add a test for the visibility issue?

[t-kalinowski]

There’s already a unit test here:

S7/tests/testthat/test-method-dispatch.R

Lines 190 to 202 in 498cfad

	test_that("method dispatch preserves method return visibility", {
	foo <- new_generic("foo", "x")
	method(foo, class_integer) <- function(x) invisible("bar")
	expect_invisible(foo(1L))
	method(foo, class_character) <- function(x) {
	if (x == "nope") return(invisible("bar"))
	"bar"
	}
	expect_visible(foo("yep"))
	expect_invisible(foo("nope"))
	})

. Do you think this covers the visibility issue?

[hadley]

Oh yeah, that's perfect. I thought it was a new feature due to the use of .External.