Merge branch 'master' into issue_6720

Author: Mukulyadav2004

.ci/README.md

@@ -13,7 +13,8 @@ Test jobs:
 - `test-lin-dev-gcc-strict-cran` - `--as-cran` on Linux, `r-devel` built with `-enable-strict-barrier --disable-long-double`, test for compilation warnings, test for new NOTEs/WARNINGs from `R CMD check`.
 - `test-lin-dev-clang-cran` - same as `gcc-strict` job but R built with `clang` and  no `--enable-strict-barrier --disable-long-double` flags.
 - `test-lin-ancient-cran` - Stated R dependency version (currently 3.4.0) on Linux.
-- `test-lin-dev-san` - `r-devel` on Linux built with `clang -fsanitize=address,undefined` (including LeakSanitizer), test for sanitizer output in tests and examples.
+- `test-lin-dev-clang-san` - `r-devel` on Linux built with `clang -fsanitize=address,undefined` (including LeakSanitizer), test for sanitizer output in tests and examples.
+- `test-lin-dev-gcc-san` - `r-devel` on Linux built with `gcc -fsanitize=address,undefined` (including LeakSanitizer), test for sanitizer output in tests and examples.
 - `test-win-rel` - `r-release` on Windows.
 - `test-win-dev` - `r-devel` on Windows.
 - `test-win-old` - `r-oldrel` on Windows.
@@ -49,6 +50,23 @@ Base R implemented helper script, [originally proposed to base R](https://svn.r-
 
 Base R implemented helper script to orchestrate generation of most artifacts and to arrange them nicely. It is being used only in [_integration_ stage in GitLab CI pipeline](./../.gitlab-ci.yml).
 
+### [`lint.R`](./lint.R)
+
+Base R runner for the manual (non-`lintr`) lint checks to be run from GitHub Actions during the code quality check. The command line arguments are as follows:
+1. Path to the directory containing files defining the linters. A linter is a function that accepts one argument (typically the path to the file) and signals an error if it fails the lint check.
+2. Path to the directory containing files to check.
+3. A regular expression matching the files to check.
+
+One of the files in the linter directory may define the `.preprocess` function, which must accept one file path and return a value that other linter functions will understand. The function may also return `NULL` to indicate that the file must be skipped.
+
+Example command lines:
+
+```sh
+Rscript .ci/lint.R .ci/linters/c src '[.][ch]$'
+Rscript .ci/lint.R .ci/linters/po po '[.]po$'
+Rscript .ci/lint.R .ci/linters/md . '[.]R?md$'
+```
+
 ## GitLab Open Source Program
 
 We are currently part of the [GitLab for Open Source Program](https://about.gitlab.com/solutions/open-source/). This gives us 50,000 compute minutes per month for our GitLab CI. Our license needs to be renewed yearly (around July) and is currently managed by @ben-schwen.

.ci/lint.R

@@ -0,0 +1,36 @@
+#!/usr/bin/Rscript
+# Runner for the manual lint checks in .ci/linters
+args = commandArgs(TRUE)
+if (identical(args, '--help')) {
+  writeLines(c(
+    'Usage: Rscript .ci/lint.R .ci/linters/<KIND> <WHERE> <WHAT> [PREPROCESS]',
+    'KIND must name the directory containing the *.R files defining the linter functions.',
+    'WHERE must name the directory containing the files to lint, e.g. "po", or "src".',
+    "WHAT must contain the regular expression matching the files to lint, e.g., '[.]po$', or '[.][ch]$'.",
+  ))
+  q('no')
+}
+stopifnot(`Invalid arguments, see .ci/lint.R --help` = length(args) == 3)
+
+linter_env = list2env(list(.preprocess = identity))
+for (f in list.files(args[[1]], full.names=TRUE)) sys.source(f, linter_env)
+if (!length(ls(linter_env))) stop(
+  "No linters found after sourcing files in ", dQuote(args[[1]])
+)
+
+sources = list.files(args[[2]], pattern = args[[3]], full.names = TRUE, recursive = TRUE)
+if (!length(sources)) stop(
+  "No files to lint found in directory ", dQuote(args[[2]]), " for mask ", dQuote(args[[3]])
+)
+sources = Filter(Negate(is.null), lapply(setNames(nm = sources), linter_env$.preprocess))
+
+okay = TRUE
+for (src in names(sources))
+for (linter in ls(linter_env)) tryCatch(
+  linter_env[[linter]](sources[[src]]),
+  error = function(e) {
+    message('Source file ', dQuote(src), ' failed lint check ', dQuote(linter), ': ', conditionMessage(e))
+    okay <<- FALSE
+  }
+)
+stopifnot(`Please fix the issues above.` = okay)

.ci/linters/c/00preprocess.R

@@ -0,0 +1,7 @@
+.preprocess = function (f) list(
+  c_obj = f, lines = readLines(f),
+  preprocessed = system2(
+    "gcc", shQuote(c("-fpreprocessed", "-E", f)),
+    stdout = TRUE, stderr = FALSE
+  )
+)

.ci/linters/md/heading_id_linter.R

@@ -1,8 +1,8 @@
-any_mismatch = FALSE
-
 # ensure that ids are limited to alphanumerics and dashes
 # (in particular, dots and underscores break the links)
 check_header_ids = function(md) {
+  if (!grepl('[.]Rmd$', md)) return(invisible())
+  md = readLines(md)
   # A bit surprisingly, some headings don't start with a letter.
   # We're interested in those that set an id to link to, i.e., end with {#id}.
   heading_captures = regmatches(md, regexec("^#+ \\S.*[{]#([^}]*)[}]$", md))
@@ -14,13 +14,5 @@ check_header_ids = function(md) {
     "On line %d, bad heading id '%s':\n%s\n",
     line, heading_captures[[line]][2], heading_captures[[line]][1]
   ))
-  !all(good_ids)
+  stopifnot('Please fix the vignette issues above' = all(good_ids))
 }
-
-any_error = FALSE
-for (vignette in list.files('vignettes', pattern = "[.]Rmd$", recursive = TRUE, full.name = TRUE)) {
-  cat(sprintf("Checking vignette file %s...\n", vignette))
-  rmd_lines = readLines(vignette)
-  any_error = check_header_ids(rmd_lines) || any_error
-}
-if (any_error) stop("Please fix the vignette issues above.")

.ci/linters/md/news_linter.R

@@ -1,13 +1,13 @@
-any_mismatch = FALSE
-
 # ensure that numbered list in each section is in sequence
 check_section_numbering = function(news) {
+  if (!grepl("NEWS", news)) return(invisible())
+  news = readLines(news)
   # plain '#' catches some examples; 'd' for 'data.table'
   sections = grep("^#+ [A-Zd]", news)
   entries = grep("^[0-9]+[.]", news)
   entry_value = as.integer(gsub("^([0-9]+)[.].*", "\\1", news[entries]))
   section_id = findInterval(entries, sections)
-  
+
   any_mismatch = FALSE
   for (id in unique(section_id)) {
     section_entries = entry_value[section_id == id]
@@ -22,11 +22,12 @@ check_section_numbering = function(news) {
       paste0("  [", section_entries[!matched], " --> ", intended_value[!matched], "]", collapse="\n")
     ))
   }
-  return(any_mismatch)
+  stopifnot("Please fix the NEWS issues above" = !any_mismatch)
 }
 
 # ensure that GitHub link text & URL actually agree
 check_gh_links = function(news) {
+  news = readLines(news)
   gh_links_info = gregexpr(
     "\\[#(?<md_number>[0-9]+)\\]\\(https://github.com/Rdatatable/data.table/(?<link_type>[^/]+)/(?<link_number>[0-9]+)\\)",
     news,
@@ -48,14 +49,5 @@ check_gh_links = function(news) {
     "In line %d, link pointing to %s %s is written #%s\n",
     line_number, link_type, link_number, md_number
   )))
-  return(TRUE)
-}
-
-any_error = FALSE
-for (news in list.files(pattern = "NEWS")) {
-  cat(sprintf("Checking NEWS file %s...\n", news))
-  news_lines = readLines(news)
-  any_error = check_section_numbering(news_lines) || any_error
-  any_error = check_gh_links(news_lines) || any_error
+  stop("Please fix the NEWS issues above.")
 }
-if (any_error) stop("Please fix the NEWS issues above.")

.ci/linters/po/00preprocess.R

@@ -0,0 +1,4 @@
+.preprocess = function (f) {
+  diff_v_master = system2('git', c('diff', 'master', f), stdout=TRUE)
+  if (length(diff_v_master)) f
+}

.github/workflows/code-quality.yaml

@@ -40,35 +40,20 @@ jobs:
         run: /usr/bin/sudo apt-get -y install coccinelle
       - name: Lint
         run: |
-          linter_env = new.env()
-          for (f in list.files('.ci/linters/c', full.names=TRUE)) sys.source(f, linter_env)
-          for (f in list.files('src', pattern='[.][ch]$', full.names=TRUE)) {
-            c_obj = list(path = f, lines = readLines(f))
-            c_obj$preprocessed = system2("gcc", c("-fpreprocessed", "-E", f), stdout=TRUE, stderr=FALSE)
-            for (linter in ls(linter_env)) linter_env[[linter]](c_obj)
-            # TODO(#6272): Incorporate more checks from CRAN_Release
-          }
-        shell: Rscript {0}
+          Rscript .ci/lint.R .ci/linters/c src '[.][ch]$'
   lint-po:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
       - uses: r-lib/actions/setup-r@v2
       - name: Check translations
+        # only pay attention to files edited in the current PR, otherwise we can get
+        # a situation like after #6424 where some untranslated messages were added
+        # as part of non-translation maintenance, but this GHA would go red repeatedly
+        # until a translation is added or the blank/fuzzy translations removed. We'd
+        # rather only have the failure on one PR, then ignore these files later.
         run: |
-          linter_env = new.env()
-          for (f in list.files('.ci/linters/po', full.names=TRUE)) sys.source(f, linter_env)
-          for (po_file in list.files(pattern = "[.]po$", full.names=TRUE)) {
-            # only pay attention to files edited in the current PR, otherwise we can get
-            #   a situation like after #6424 where some untranslated messages were added
-            #   as part of non-translation maintenance, but this GHA would go red repeatedly
-            #   until a translation is added or the blank/fuzzy translations removed. We'd
-            #   rather only have the failure on one PR, then ignore these files later.
-            diff_v_master = system2("git", c("diff", "master", po_file), stdout=TRUE)
-            if (!length(diff_v_master)) next
-            for (linter in ls(linter_env)) linter_env[[linter]](po_file)
-          }
-        shell: Rscript {0}
+          Rscript .ci/lint.R .ci/linters/po po '[.]po$'
   lint-md:
     runs-on: ubuntu-latest
     steps:
@@ -77,17 +62,3 @@ jobs:
       - name: Lint
         run: for (f in list.files('.ci/linters/md', full.names=TRUE)) source(f)
         shell: Rscript {0}
-  build:
-    runs-on: ubuntu-latest
-    steps:
-      - uses: actions/checkout@v2
-      - uses: r-lib/actions/setup-r@v2
-      - name: Install remotes
-        run: Rscript -e "install.packages('remotes')"
-      - name: Install dependencies
-        run: Rscript -e 'remotes::install_deps(dependencies = TRUE)'
-      - name: Check documentation and options consistency
-        run: Rscript .ci/check-options.R
-      - name: Run R CMD check
-        run: Rscript -e 'devtools::check()'
-      

.gitlab-ci.yml

@@ -218,8 +218,8 @@ test-lin-ancient-cran:
     # Restore checking vignettes if upgrading our R dependency means knitr can be installed, or when we switch to litedown.
     - R CMD check --no-manual --no-build-vignettes --ignore-vignettes $(ls -1t data.table_*.tar.gz | head -n 1)
 
-# run the main checks with Address(+Leak),UBSanitizer enabled
-test-lin-dev-san:
+# run the main checks with Address(+Leak),UBSanitizer enabled, GCC _and_ Clang
+test-lin-dev-clang-san:
   <<: *test-lin
   image: registry.gitlab.com/rdatatable/dockerfiles/r-devel-clang-san
   variables:
@@ -233,8 +233,27 @@ test-lin-dev-san:
     - |
         res1=0; ASAN_OPTIONS=detect_leaks=1 R CMD check --no-manual $(ls -1t data.table_*.tar.gz | head -n 1) || res1=$?
         res2=0; perl -nle '(print, $a=1) if /: runtime error: |ERROR: LeakSanitizer/../SUMMARY.*Sanitizer/ }{ exit $a' data.table.Rcheck/**/*.Rout* || res2=$?
+        res3=0; tail -n 1 data.table.Rcheck/00check.log | grep -q -e '^Status: [0-9]* NOTEs*$' -e '^Status: OK$' || res3=$?
         # fail if R CMD check had failed or if sanitizer output found
-        if [ $res1 -ne 0 ] || [ $res2 -ne 0 ]; then exit 1; fi
+        if [ $res1 -ne 0 ] || [ $res2 -ne 0 ] || [ $res3 -ne 0 ]; then exit 1; fi
+
+test-lin-dev-gcc-san:
+  <<: *test-lin
+  image: registry.gitlab.com/rdatatable/dockerfiles/r-devel-gcc-san
+  variables:
+    # must be set for most of the process because there are pseudo-leaks everywhere
+    ASAN_OPTIONS: "detect_leaks=0"
+    # fontconfig is known to leak; add more suppressions as discovered
+    LSAN_OPTIONS: "suppressions=$CI_PROJECT_DIR/.dev/lsan.supp"
+    UBSAN_OPTIONS: "print_stacktrace=1"
+  script:
+    - *install-deps
+    - |
+        res1=0; ASAN_OPTIONS=detect_leaks=1 R CMD check --no-manual $(ls -1t data.table_*.tar.gz | head -n 1) || res1=$?
+        res2=0; perl -nle '(print, $a=1) if /: runtime error: |ERROR: LeakSanitizer/../SUMMARY.*Sanitizer/ }{ exit $a' data.table.Rcheck/**/*.Rout* || res2=$?
+        res3=0; tail -n 1 data.table.Rcheck/00check.log | grep -q -e '^Status: [0-9]* NOTEs*$' -e '^Status: OK$' || res3=$?
+        # fail if R CMD check had failed or if sanitizer output found
+        if [ $res1 -ne 0 ] || [ $res2 -ne 0 ] || [ $res3 -ne 0 ]; then exit 1; fi
 
 .test-win-template: &test-win
   <<: *test
@@ -338,7 +357,7 @@ integration:
     - saas-linux-medium-amd64
   only:
     - master
-  needs: ["mirror-packages","build","test-lin-rel","test-lin-rel-cran","test-lin-dev-gcc-strict-cran","test-lin-dev-clang-cran","test-lin-rel-vanilla","test-lin-ancient-cran","test-lin-dev-san","test-win-rel","test-win-dev" ,"test-win-old","test-mac-rel","test-mac-old"]
+  needs: ["mirror-packages","build","test-lin-rel","test-lin-rel-cran","test-lin-dev-gcc-strict-cran","test-lin-dev-clang-cran","test-lin-rel-vanilla","test-lin-ancient-cran","test-lin-dev-clang-san","test-lin-dev-gcc-san","test-win-rel","test-win-dev" ,"test-win-old","test-mac-rel","test-mac-old"]
   script:
     - R --version
     - *install-deps ## markdown pkg not present in r-pkgdown image

DESCRIPTION

@@ -55,7 +55,7 @@ Authors@R: c(
   person("James","Sams",           role="ctb"),
   person("Martin","Morgan",        role="ctb"),
   person("Michael","Quinn",        role="ctb"),
-  person("@javrucebo","",          role="ctb"),
+  person(given="@javrucebo",       role="ctb", comment="GitHub user"),
   person("Marc","Halperin",        role="ctb"),
   person("Roy","Storey",           role="ctb"),
   person("Manish","Saraswat",      role="ctb"),
@@ -102,6 +102,6 @@ Authors@R: c(
   person("Aljaž", "Sluga",         role="ctb"),
   person("Bill", "Evans",          role="ctb"),
   person("Reino", "Bruner",        role="ctb"),
-  person(comment=c(github="@badasahog"), role="ctb"),
+  person(given="@badasahog",       role="ctb", comment="GitHub user"),
   person("Vinit", "Thakur",        role="ctb")
   )

NEWS.md

@@ -42,6 +42,8 @@
 
 11. Out of sample type bumps now respect `integer64=` selection, [#7032](https://github.com/Rdatatable/data.table/pull/7032).
 
+12. Internal functions used to signal errors are now marked as non-returning, silencing a compiler warning about potentially unchecked allocation failure. Thanks to Prof. Brian D. Ripley for the report and @aitap for the fix, [#7070](https://github.com/Rdatatable/data.table/pull/7070).
+
 ### NOTES
 
 1. Continued work to remove non-API C functions, [#6180](https://github.com/Rdatatable/data.table/issues/6180). Thanks Ivan Krylov for the PRs and for writing a clear and concise guide about the R API: https://aitap.codeberg.page/R-api/.