Add ability to return multiple values - #49
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for returning multiple values from quickr functions by allowing the last expression to be a list of symbols instead of just a single symbol. The implementation handles both direct list returns and assignment-based returns, extending the existing single return value functionality.
- Support for multiple return values through list syntax
- Enhanced preprocessing to handle assignment patterns
- Updated C bridge generation for multi-value returns
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-multi-return.R | Adds comprehensive tests for multiple return values functionality |
| R/subroutine.R | Extends return variable detection to handle list expressions |
| R/preprocess-lang.R | Adds preprocessing logic for assignment-based multi-returns |
| R/manifest.R | Updates scope processing to use plural return variable names |
| R/c-wrapper.R | Implements C bridge generation for multiple return values |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (is.symbol(last_expr)) { | ||
| n <- length(bdy) | ||
| if (n >= 3L) { | ||
| prev_expr <- bdy[[n - 1L]] | ||
| if ( | ||
| is_call(prev_expr, quote(`<-`)) && | ||
| identical(prev_expr[[2L]], last_expr) && | ||
| is_call(prev_expr[[3L]], quote(list)) | ||
| ) { | ||
| args <- as.list(prev_expr[[3L]])[-1L] | ||
| if (!all(map_lgl(args, is.symbol))) { | ||
| stop("all elements of return list must be symbols") | ||
| } | ||
| bdy_list <- as.list(bdy) | ||
| bdy_list <- bdy_list[-(n - 1L)] | ||
| bdy_list[[length(bdy_list)]] <- prev_expr[[3L]] | ||
| return(as.call(bdy_list)) | ||
| } | ||
| } | ||
| return(bdy) | ||
| } |
There was a problem hiding this comment.
The nested conditional logic (lines 25-28) has multiple conditions combined in a single if statement, making it difficult to read and understand. Consider extracting this logic into a helper function or breaking it into multiple conditional checks with descriptive variable names.
There was a problem hiding this comment.
copilot, propose a suggested change.
| append(c_body) <- c( | ||
| glue("SEXP _ans = PROTECT(Rf_allocVector(VECSXP, {length(return_var_names_un)}));"), | ||
| imap(return_var_names_un, function(nm, i) { | ||
| glue("SET_VECTOR_ELT(_ans, {i-1}, {nm});") | ||
| }), | ||
| glue("SEXP _names = PROTECT(Rf_allocVector(STRSXP, {length(return_var_names_un)}));"), |
There was a problem hiding this comment.
The expression length(return_var_names_un) is repeated twice. Consider extracting it to a variable like n_return_vars to improve readability and maintainability.
| append(c_body) <- c( | |
| glue("SEXP _ans = PROTECT(Rf_allocVector(VECSXP, {length(return_var_names_un)}));"), | |
| imap(return_var_names_un, function(nm, i) { | |
| glue("SET_VECTOR_ELT(_ans, {i-1}, {nm});") | |
| }), | |
| glue("SEXP _names = PROTECT(Rf_allocVector(STRSXP, {length(return_var_names_un)}));"), | |
| n_return_vars <- length(return_var_names_un) | |
| append(c_body) <- c( | |
| glue("SEXP _ans = PROTECT(Rf_allocVector(VECSXP, {n_return_vars}));"), | |
| imap(return_var_names_un, function(nm, i) { | |
| glue("SET_VECTOR_ELT(_ans, {i-1}, {nm});") | |
| }), | |
| glue("SEXP _names = PROTECT(Rf_allocVector(STRSXP, {n_return_vars}));"), |
|
@t-kalinowski, Thank you very much for the PR! It looks great at a glance. I'll review more thorougly and merge later in the week. Good work! |
|
I saw that you were preparing for a new release, so I kind of hastily created a pull request in hopes of it making in to this release. So it is a bit raw. Let me try to refactor the process-lang part and review some of the other parts before you review it again. I will create new commits in a day or two. |
test file and added a failure test. Removed some checkking in subroutine and c-wrapper Changes to be committed: modified: R/c-wrapper.R modified: R/preprocess-lang.R modified: R/subroutine.R deleted: tests/testthat/test-multi-return.R new file: tests/testthat/test-multiple-return.R
|
@t-kalinowski I tried to refactor the code a bit and add some comments. This pull request extends quickr by allowing it to return a list of vectors. This is achieved by:
I notice that we are checking that the last expression is symbol in both ensure_last_expr_sym, new_fortran_subroutine and closure_return_var_names. I implemented a validate list in preprocess-lang - but i am not sure if you also think this is the place to validate and error. |
t-kalinowski
left a comment
t-kalinowski
left a comment
There was a problem hiding this comment.
Very nice work! I think this is almost ready to merge. Just had some minor questions about how we handle named lists.
|
We are now much closer to R and we can return unnamed elements and custom names for elements. We still require that what is returned is a symbol like
R/c-wrapper.R:
tests/testthat/test-multiple-return.R:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| if (length(return_var_names) == 1L) { | ||
| if (n_protected > 0) { | ||
| append(c_body) <- glue("UNPROTECT({n_protected});") | ||
| } | ||
| append(c_body) <- glue("return {return_var_names};") |
There was a problem hiding this comment.
[P1] Preserve list semantics when only one value is returned
The bridge only wraps the return values into an R list when length(return_var_names) > 1. A function whose last expression is list(...) with a single element now falls through the single‑value branch and returns the bare SEXP instead of the list. For example, a quickr function ending with list(result = y) will compile to a function that returns y rather than list(result = y), so the compiled code no longer matches the R reference implementation. The branch should detect whether the user explicitly returned a list rather than relying solely on the number of elements.
Useful? React with 👍 / 👎.
| if (has_any_names) { | ||
| names_to_use <- provided_names | ||
| append(c_body) <- c( | ||
| glue("SEXP _names = PROTECT(Rf_allocVector(STRSXP, {length(return_var_values)}));"), | ||
| imap(names_to_use, function(nm, i) { | ||
| glue('SET_STRING_ELT(_names, {i-1}, Rf_mkChar("{nm}"));') |
There was a problem hiding this comment.
[P2] Escape list element names before embedding in generated C
When named multiple return values are handled, the code writes each provided name directly into a C string literal (Rf_mkChar("{nm}")). If a caller supplies a name containing quotes or backslashes (list("a\"b" = y, z = z)), the generated C code becomes syntactically invalid or can miscompile. The names should be run through a C string escaping helper before interpolation.
Useful? React with 👍 / 👎.
|
@codex add tests for the single list edge case, show the test failing (by pushing), then implement the fix (in another commit). |
For now, I can only help with PRs you've created. |
|
@codex open a new PR into this branch. |
For now, I can only help with PRs you've created. |
…ts; validate non-syntactic names in return lists
|
@codex review again |
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| return_var_names <- closure_return_var_names(closure) | ||
| return_vars <- mget(return_var_names, scope) | ||
| for (return_var in return_vars) { |
There was a problem hiding this comment.
[P1] De-duplicate return vars before emitting C allocations
The loop that prepares output buffers iterates over every entry returned by closure_return_var_names() without removing duplicates. If a function returns the same symbol twice (for example list(y, y)), the loop generates two identical declarations and PROTECT statements for y, which will fail compilation due to redeclared identifiers and mismatched protection counts. The code should allocate each return variable once and reuse it when packing the list.
Useful? React with 👍 / 👎.
| bad <- nzchar(nms) & make.names(nms) != nms | ||
| if (any(bad)) { | ||
| stop( | ||
| "only syntactic names are valid, encountered: ", |
There was a problem hiding this comment.
[P2] Replace invalid paste0 call in non‑syntactic name error
When validating names for list returns, the error message uses paste0(nms[bad], sep = ", "), but paste0 does not accept a sep argument. Hitting this branch (e.g. returning list(a b = y)) raises an unused argument (sep = ...) before the intended stop() message, so callers see a confusing error unrelated to the actual problem. Use paste() or paste0(..., collapse = ", ") to list the offending names.
Useful? React with 👍 / 👎.
|
@codex review again |
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| return_var_names <- closure_return_var_names(closure) | ||
| return_vars <- mget(return_var_names, scope) | ||
| for (return_var in return_vars) { | ||
| if (!return_var@name %in% closure_arg_names) { | ||
| return_var@modified <- TRUE | ||
| assign(return_var@name, return_var, scope) | ||
| append(c_body) <- return_var_c_defs(return_var, fsub@scope) |
There was a problem hiding this comment.
[P1] Deduplicate return variables before emitting C defs
The loop generates return-variable declarations for every element returned by closure_return_var_names, including duplicates. When a function returns the same symbol more than once (e.g. list(y, y)), this code emits two identical return_var_c_defs blocks and the generated wrapper fails to compile with redefinition of ‘y__len_’/y. The set of return variables should be uniqued before generating the C definitions and PROTECT counts.
Useful? React with 👍 / 👎.
|
@codex review again |
|
Codex Review: Didn't find any major issues. Hooray! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
This pull request adds the ability to return multiple values from a quickr function.
I'll admit that it was mostly written by AI, but in my testing it seem to work well. It clears all test and I have added a new test file with a few tests.
This closes #50