fix auto-print detection on R 3.4/3.5 where sys.calls() returns a promise#7727
Open
skitsy24 wants to merge 17 commits into
Open
fix auto-print detection on R 3.4/3.5 where sys.calls() returns a promise#7727skitsy24 wants to merge 17 commits into
skitsy24 wants to merge 17 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7727 +/- ##
=======================================
Coverage 99.04% 99.04%
=======================================
Files 87 87
Lines 17123 17043 -80
=======================================
- Hits 16959 16880 -79
+ Misses 164 163 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aitap
reviewed
Apr 26, 2026
Comment on lines
+40
to
+47
| } else if (typeof(SYS[[1L]][[1L]]) == "promise") { | ||
| # in R 3.4 and R 3.5, auto-print uses a promise to reference base::print due to lazy loading | ||
| # safely evaluate promise to get the actual function | ||
| evaluated = tryCatch(eval(SYS[[1L]][[1L]]), error = function(e) NULL) | ||
| if (identical(evaluated, print)) { | ||
| is_print_call = TRUE | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
This disappeared in R-3.6.0, right? Let's wrap this branch in # nocov start ... # nocov end and add a # TODO(R>=3.6) comment to remove it once we raise the minimal supported R version.
aitap
requested changes
May 10, 2026
Member
aitap
left a comment
aitap
left a comment
There was a problem hiding this comment.
The fix is good, tested manually on R-3.5.0. Could you please fix the whitespace issues and add a NEWS.md item describing the bug fix?
* Restore files to state at 6d0dd35 * fix typo * fix coding style * fix coding style * unify approaches * small docs tweak --------- Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com>
* use more base R for github actions * reinstate MacOS build dependencies
* merge-commit * merge-commit * pr5427 * fread and 0.5 * shallow expr down * memrecycle * comma * expr dowm * merge * isoweek * doc commit source * fread * expr last * regression PR instead of parent of fix Co-authored-by: aitap <krylov.r00t@gmail.com> * merge commit PR6393 instead of close to last Co-authored-by: aitap <krylov.r00t@gmail.com> --------- Co-authored-by: Toby Dylan Hocking <toby.dylan.hocking@usherbrooke.ca> Co-authored-by: aitap <krylov.r00t@gmail.com>
When using the .0 release of R with binary packages from CRAN, library() will complain about the binary package having been built under a newer (but still compatible in this case) version of R. This was already suppressed in most places, but not in test 2265.
…rollfun (Rdatatable#7733) * Adding updated files * Updated NEWS.md * Updated test file * Fix test warnings and update output messages * Resolve merge conflict in NEWS.md * Update NEWS.md with new features and fixes * Change to ternary if operator * Change to ternary if operator in froll.c * Removing white space in froll.Rraw * Removing whitespace from NEWS.md * Refactoring rfunNames for copile time definition (in data.table.h) * Removing extra brackets * Declare extern array for roll function names * Reverting changes for rfunStr & adjusting for direct use of rfunNames (defined in froll.c & extern in data.table.h) * Update comments for clarity in frolladaptive.c * Removing spaces * Remove unnecessary newline in frolladaptive.c * Changing NEWS.md message The message now includes our group members as suggested by @ben-schwen * remove trailing comma * Moving message from NEW FEATURES -> Notes moved text stating new change from the NEW FEATURES section to Notes. --------- Co-authored-by: Aiden Seay <aidengseay@gmail.com> Co-authored-by: kkarissa <128569633+kkarissa@users.noreply.github.com> Co-authored-by: Benjamin Schwendinger <52290390+ben-schwen@users.noreply.github.com>
* Added a check to detect when blank lines should have been skipped, but it doesn't solve the problem yet. * Fixed check added to only check if 'topSkip' is greater than 0. 'topSkip' is greater than 0 when blank lines are present, so I also check if blank lines should be skipped so I can throw a warning to let the user know. * Updated check for blank lines to ask if 'topSkip' isgreater than 1 to accomodate situation where the header and data are separated by a blank line. * Used the 'prevStart' variable to detect when each line is separated by a blank line. In the case of each line separated by a blank line, 'prevStart' is always NULL because each line could be a possible header. * Added test '1578.10' for initial case which issue Rdatatable#3339 pointed out. Causes an error in test 1578.1? * Changed the number of the test written to verify fix for Rdatatable#3339. * Removed newline from end of expected warning message. * Updated NEWS.md with news of the new warning. * Updated sub-test numbers for test '1578' to match the format established where 0s are prepended when there is more than one significant digit following the decimal.
…ected warnings (Rdatatable#7706) * Improved verbosity of data.table:::test() function for mismatched expected warnings * Apply suggestion from @aitap Fixed NEWS.md punctuation and spacing. --------- Co-authored-by: aitap <krylov.r00t@gmail.com>
skitsy24
requested review from
Anirban166,
ben-schwen,
jangorecki and
tdhock
as code owners
Author
|
Changes made, and added item to NEWS |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7099.
On R 3.4 and 3.5, the top-level call captured via sys.calls() during auto-print is not a direct reference to base::print, but a promise object that evaluates to it. As a result, the existing check:
identical(SYS[[1L]][[1L]], print)
fails, and print.data.table() incorrectly treats auto-print as an explicit print() call. This leads to unintended output in cases where printing should be suppressed (e.g., after := assignments).
Thanks to @aitap for identifying the root cause.
My fix adds a check on whether print() is wrapped in a "promise" to differentiate between the older R versions. If it is a promise, the promise is evaluated, and then checked against print.