search

r-lib / covr

Test coverage reports for R
https://covr.r-lib.org
Other
336 stars 117 forks source link

Fix check NOTE: non-API calls to SET_BODY, SET_CLOENV, SET_FORMALS #587

Closed t-kalinowski closed 6 days ago

t-kalinowski commented 2 weeks ago

This PR is an unsavory but pragmatic and expedient solution to the current R CMD check NOTE https://cran.r-project.org/web/checks/check_results_covr.html

Result: NOTE 
  File ‘covr/libs/covr.so’:
    Found non-API calls to R: ‘SET_BODY’, ‘SET_CLOENV’, ‘SET_FORMALS’

  Compiled code should not call non-API entry points in R.

This can be a “temporary” solution until the internal layout of a CLOSXP changes, closure modifiers become part of R’s C API, or we decide to undertake a larger refactor of covr to avoid needing this workaround.

jimhester commented 2 weeks ago

I wonder if we could explain this use case to Luke Tierney and see if we could get a API function for this. There is Rf_mkClosure which almost does what we need, but always allocates a new closure rather than swapping in place.

Making us dependent on the internal CLOSEXP memory layout seems like moving in the wrong direction from what R core intended with the extra checks.

Though if they are not going to change we may have to do this for now.

If we also captured the environment that the original function was stored in I guess we could just replace the function with the new version in that environment and avoid the in place swapping, but it would take some refactoring of covr to accomplish.

t-kalinowski commented 2 weeks ago

seems like moving in the wrong direction from what R core intended

That may be true. However, the usage in covr is unique, and I don’t anticipate an API endpoint to make this straightforward anytime soon. I understand and sympathize with R Core's desire to proceed cautiously and intentionally here. With this approach, the risks and assumptions are clearly defined and managed within covr, with the understanding that we’ll stay responsive to any upstream changes. Perhaps that aligns somewhat with R Core’s goal of distinguishing between what's officially supported and what's not?

If we also captured the environment that the original function was stored in

This would be a relatively involved refactoring, since not all functions are resolved via a binding in an environment. E.g., an S7 property setter is resolved from a list, and an S7 class validator from an attribute.

jimhester commented 1 week ago

Can we add a note to news about this change, then I can merge it. I appreciate you looking into it and coming up with a solution, even if it is not ideal, we have to work with what CRAN allows us to do.

jimhester commented 6 days ago

Thanks again for working on this!