Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a utility system for recursively extracting chk_() validation calls from function bodies to enable upfront validation of scenario data frames. The system analyzes functions (including S3 generics and their methods), identifies all checker function calls, and traces indirect validation through helper functions.
Key Changes:
- New recursive checker extraction system with S3 generic support
- Comprehensive test suite with snapshot testing for the new functionality
- Helper infrastructure for memoizing chk package exports
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
R/chk-infer.R |
Core implementation for recursively extracting chk validation calls from function bodies, including indirect tracing through helper functions |
R/chk-infer-s3.R |
S3 generic handling logic that collects and combines validation calls from all methods of a generic function |
R/chk.R |
Helper function to retrieve and cache chk package exports for identifying validation calls |
R/zzz.R |
Package load hook to memoize the chk exports function |
tests/testthat/test-chk-infer.R |
Comprehensive tests for the core inference logic with various scenarios (direct calls, indirect calls, nested functions) |
tests/testthat/test-chk-infer-s3.R |
Tests specifically for S3 generic handling including method collection, argument mapping, and result combination |
tests/testthat/_snaps/chk-infer.md |
Snapshot tests capturing expected output for various extraction scenarios |
tests/testthat/_snaps/chk-infer-s3.md |
Snapshot tests for S3-specific extraction scenarios |
tests/testthat/test-fit-dists-sims.R |
Added pillar option configuration to ensure consistent test output across environments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,3 @@ | |||
| .onLoad <- function(libname, pkgname) { | |||
| get_chk_exports <<- memoise::memoise(get_chk_exports) | |||
There was a problem hiding this comment.
The package memoise is used via memoise::memoise() but is not declared in the DESCRIPTION file under Imports or Suggests. This will cause the package to fail when loading if memoise is not installed.
Please add memoise to the Imports field in the DESCRIPTION file.
|
|
||
| if (is.call(expr)) { | ||
| # First, recursively search in nested calls to find innermost usage | ||
| innermost_result <- NULL |
There was a problem hiding this comment.
The variable innermost_result is assigned but never used. It can be safely removed.
Consider removing this line as it serves no purpose in the current implementation.
| innermost_result <- NULL |
|
|
||
| # Find the first usage of an argument in the function body | ||
| # Returns the innermost call that contains the argument | ||
| find_first_usage <- function(expr, arg_name, depth = 0) { |
There was a problem hiding this comment.
[nitpick] The parameter depth is passed through recursive calls but never actually used in the function body. If it's not needed for future functionality, consider removing it to simplify the function signature.
| for (expr in all_exprs) { | ||
| # Check if this expression is already in unique_exprs | ||
| is_duplicate <- FALSE | ||
| for (unique_expr in unique_exprs) { | ||
| if (identical(expr, unique_expr)) { | ||
| is_duplicate <- TRUE | ||
| break | ||
| } | ||
| } | ||
| if (!is_duplicate) { | ||
| unique_exprs[[length(unique_exprs) + 1]] <- expr |
There was a problem hiding this comment.
[nitpick] The nested loop structure for deduplication has O(n²) time complexity. For each expression in all_exprs, it iterates through all expressions in unique_exprs to check for duplicates. This could be inefficient when dealing with many expressions.
Consider using a more efficient approach, such as:
- Converting expressions to strings and using a set/hash for O(1) lookups
- Using
unique()with a custom comparison function if possible - Or accepting the current approach if the number of expressions is expected to be small
| for (expr in all_exprs) { | |
| # Check if this expression is already in unique_exprs | |
| is_duplicate <- FALSE | |
| for (unique_expr in unique_exprs) { | |
| if (identical(expr, unique_expr)) { | |
| is_duplicate <- TRUE | |
| break | |
| } | |
| } | |
| if (!is_duplicate) { | |
| unique_exprs[[length(unique_exprs) + 1]] <- expr | |
| seen_exprs <- character() | |
| for (expr in all_exprs) { | |
| expr_str <- if (exists("deparse1", mode = "function")) deparse1(expr) else paste(deparse(expr), collapse = "") | |
| if (!(expr_str %in% seen_exprs)) { | |
| unique_exprs[[length(unique_exprs) + 1]] <- expr | |
| seen_exprs <- c(seen_exprs, expr_str) |
| ) | ||
| arg_results | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The function combine_chk_calls() appears to be a main entry point for this feature (based on the PR description's example usage) but lacks roxygen2 documentation. Even for internal/build-time functions, basic documentation would help users understand:
- What the function does
- What parameters it accepts
- What it returns
- Example usage
Consider adding roxygen2 documentation with at minimum @param, @return, and @examples tags.
| #' Combine chk validation calls for all arguments of a function | |
| #' | |
| #' Extracts chk validation calls for each argument of the given function and combines them | |
| #' into a single expression. If an argument does not have a chk call, a placeholder is inserted. | |
| #' | |
| #' @param fun A function object to analyze for chk validation calls. | |
| #' @return An expression combining all chk calls (and placeholders) for the function's arguments. | |
| #' @examples | |
| #' combine_chk_calls(function(x, y) { chk::chk_numeric(x); chk::chk_integer(y) }) |
| arg_results | ||
| } | ||
|
|
||
| # Main function to extract chk function calls by argument |
There was a problem hiding this comment.
[nitpick] The function extract_chk_calls() is a key function in this feature but lacks roxygen2 documentation. Even for internal functions, basic documentation would improve maintainability. Consider adding documentation that explains:
- The purpose of the function
- The recursive extraction strategy
- Parameters (especially
.depthwhich controls recursion) - Return value structure
- How it handles S3 generics vs regular functions
| # Main function to extract chk function calls by argument | |
| #' Extract chk validation calls by argument from a function | |
| #' | |
| #' Recursively analyzes a function to extract calls to chk validation functions | |
| #' for each argument. If the function is an S3 generic, all methods are analyzed | |
| #' recursively and results are combined. For regular functions, the body is | |
| #' traversed to find chk calls associated with each argument. | |
| #' | |
| #' @param fun The function object to analyze. | |
| #' @param arg Optional. The name of the argument to focus on. If NULL, all arguments are considered. | |
| #' @param fun_name Optional. The name of the function (for messaging). If NULL, defaults to "<function>". | |
| #' @param .depth Internal. Tracks recursion depth for indentation and messaging. Should not be set by users. | |
| #' | |
| #' @return A named list where each element corresponds to a function argument and contains | |
| #' the extracted chk validation calls (as language objects). If no chk calls are found for | |
| #' an argument, the element is an empty list. | |
| #' | |
| #' @details | |
| #' - For S3 generics, all methods are analyzed recursively and results are merged. | |
| #' - For regular functions, the body is traversed to find chk calls for each argument. | |
| #' - The function uses `.depth` to control recursion and indentation for verbose messaging. | |
| #' - Handles both S3 generics and regular functions appropriately. | |
| #' | |
| #' @keywords internal |
|
Joe Fix |
|
This is to automatically generate efficient argument validity checks to apply to each row of a scenario data frame up front. Imagining a situation where we let the user prepare a large scenario and want to check up front if all arguments are good. Works by recursively looking for
chk_()functions. Intended to be run during package build time.Current scope of
ssd_hc_sims()Future scope of individual components of this function
Reason for splitting
ssd_hc_sims(): to be able to add custom behaviors without adding too much complexity tossd_hc_sims().Next steps
FIXME_ADD_CHECK_CALL(...)in the output.ssd_hc_sims()based on this utility.Example
Details
Details
Created on 2025-11-18 with reprex v2.1.1