Activate 5 more linters, including return_linter (#6128)

* missing_argument_linter and return_linter

* add scalar_in_linter

* nzchar_linter too

* errant character

* errant character was masking more lints; fixed

* correct error message in test

* fix test, use nolint

* remove overly-cautious keepNA=TRUE

.ci/.lintr.R

@@ -54,19 +54,14 @@ linters = c(dt_linters, all_linters(
   todo_comment_linter = NULL,
   # TODO(michaelchirico): Enforce these and re-activate them one-by-one.
   brace_linter = NULL,
-  condition_call_linter = NULL,
   fixed_regex_linter = NULL,
   if_not_else_linter = NULL,
   implicit_assignment_linter = NULL,
   implicit_integer_linter = NULL,
   keyword_quote_linter = NULL,
-  missing_argument_linter = NULL,
-  nzchar_linter = NULL,
   object_overwrite_linter = NULL,
   paren_body_linter = NULL,
   redundant_equals_linter = NULL,
-  return_linter = NULL,
-  scalar_in_linter = NULL,
   undesirable_function_linter = NULL,
   unnecessary_concatenation_linter = NULL,
   unnecessary_nesting_linter = NULL,
@@ -101,8 +96,10 @@ exclusions = c(local({
       undesirable_operator_linter = Inf, # For ':::', possibly we could be more careful to only exclude ':::'.
       # TODO(michaelchirico): Enforce these and re-activate them one-by-one.
       comparison_negation_linter = Inf,
+      condition_call_linter = Inf,
       duplicate_argument_linter = Inf,
       equals_na_linter = Inf,
+      missing_argument_linter = Inf,
       paste_linter = Inf,
       rep_len_linter = Inf,
       sample_int_linter = Inf,

R/IDateTime.R

@@ -129,7 +129,7 @@ round.IDate = function(x, digits=c("weeks", "months", "quarters", "years"), ...)
   }
   ans = as.integer(unclass(e1) - unclass(e2))
   if (!inherits(e2, "Date")) setattr(ans, "class", c("IDate", "Date"))
-  return(ans)
+  ans
 }
 
 
@@ -177,7 +177,7 @@ as.ITime.character = function(x, format, ...) {
       w = w[!nna]
     }
   }
-  return(as.ITime(y, ...))
+  as.ITime(y, ...)
 }
 
 as.ITime.POSIXlt = function(x, ms = 'truncate', ...) {

R/as.data.table.R

@@ -180,7 +180,7 @@ as.data.table.list = function(x,
     if (eachnrow[i]>1L && nrow%%eachnrow[i]!=0L)   # in future: eachnrow[i]!=nrow
       warningf("Item %d has %d rows but longest item has %d; recycled with remainder.", i, eachnrow[i], nrow)
     if (is.data.table(xi)) {   # matrix and data.frame were coerced to data.table above
-      prefix = if (!isFALSE(.named[i]) && isTRUE(nchar(names(x)[i])>0L)) paste0(names(x)[i],".") else ""  # test 2058.12
+      prefix = if (!isFALSE(.named[i]) && isTRUE(nzchar(names(x)[i], keepNA=TRUE))) paste0(names(x)[i],".") else ""  # test 2058.12
       for (j in seq_along(xi)) {
         ans[[k]] = recycle(xi[[j]], nrow)
         vnames[k] = paste0(prefix, names(xi)[j])
@@ -251,5 +251,5 @@ as.data.table.data.table = function(x, ...) {
   x = copy(x) # #1681
   # fix for #1078 and #1128, see .resetclass() for explanation.
   setattr(x, 'class', .resetclass(x, "data.table"))
-  return(x)
+  x
 }

R/bmerge.R

@@ -185,5 +185,5 @@ bmerge = function(i, x, icols, xcols, roll, rollends, nomatch, mult, ops, verbos
   # TO DO: xo could be moved inside Cbmerge
 
   ans$xo = xo  # for further use by [.data.table
-  return(ans)
+  ans
 }

R/cedta.R

@@ -35,7 +35,7 @@ cedta.pkgEvalsUserCode = c("gWidgetsWWW","statET","FastRWeb","slidify","rmarkdow
     the_call <- calls[[ii]][[1L]]
     if (is.name(the_call) && (the_call %chin% c("eval", "evalq"))) return(TRUE)
   }
-  return(FALSE)
+  FALSE
 }
 # nocov end
 

R/data.table.R

@@ -618,7 +618,7 @@ replace_dot_alias = function(e) {
         else stopf("i evaluates to a logical vector length %d but there are %d rows. Recycling of logical i is no longer allowed as it hides more bugs than is worth the rare convenience. Explicitly use rep(...,length=.N) if you really need to recycle.", length(i), nrow(x))
       } else {
         irows = as.integer(i)  # e.g. DT[c(1,3)] and DT[c(-1,-3)] ok but not DT[c(1,-3)] (caught as error)
-        if (nomatch0) warning("Please use nomatch=NULL instead of nomatch=0; see news item 5 in v1.12.0 (Jan 2019)")
+        if (nomatch0) warningf("Please use nomatch=NULL instead of nomatch=0; see news item 5 in v1.12.0 (Jan 2019)")
                       # warning only for this case where nomatch was ignored before v1.14.2; #3109
         irows = .Call(CconvertNegAndZeroIdx, irows, nrow(x),
                       is.null(jsub) || root!=":=",   # allowOverMax (NA when selecting, error when assigning)
@@ -803,7 +803,7 @@ replace_dot_alias = function(e) {
           nzidx = nzchar(bysub)
           # by='' means by=NULL, tests 592&596
           if (!all(nzidx)) {
-            if (length(bysub) > 1L) stop("At least one entry of by is empty")
+            if (length(bysub) > 1L) stopf("At least one entry of by is empty")
             bysub = NULL
           } else {
             bysub = as.call(c(list(quote(list)), lapply(bysub, as.name)))
@@ -948,7 +948,7 @@ replace_dot_alias = function(e) {
             nm = names(q[-1L])   # check list(a=sum(v),v)
             if (is.null(nm)) nm = rep.int("", qlen-1L)
             # attempt to auto-name unnamed columns
-            for (jj in which(nm=="")) {
+            for (jj in which(!nzchar(nm))) {
               thisq = q[[jj + 1L]]
               if (missing(thisq)) stopf("Item %d of the .() or list() passed to j is missing", jj) #3507
               if (is.name(thisq)) nm[jj] = drop_dot(thisq)
@@ -976,7 +976,7 @@ replace_dot_alias = function(e) {
           if (length(q) == 4L && !is.null(q[[4L]])) q[[4L]] = do_j_names(q[[4L]])
           return(q)
         }
-        return(q)
+        q
       }
       if (is.name(jsub)) {
         # j is a single unquoted column name
@@ -1453,13 +1453,14 @@ replace_dot_alias = function(e) {
   #   independently by group & attr mismatch among groups is ignored. The latter
   #   is a more general issue but the former can be fixed by forcing units='secs'
   SDenv$`-.POSIXt` = function(e1, e2) {
-    if (inherits(e2, 'POSIXt')) {
-      if (verbose && !get0('done_units_report', parent.frame(), ifnotfound = FALSE)) {
-        catf('\nNote: forcing units="secs" on implicit difftime by group; call difftime explicitly to choose custom units\n')
-        assign('done_units_report', TRUE, parent.frame())
-      }
-      return(difftime(e1, e2, units='secs'))
-    } else return(base::`-.POSIXt`(e1, e2))
+    if (!inherits(e2, 'POSIXt')) {
+      return(base::`-.POSIXt`(e1, e2))
+    }
+    if (verbose && !get0('done_units_report', parent.frame(), ifnotfound = FALSE)) {
+      catf('\nNote: forcing units="secs" on implicit difftime by group; call difftime explicitly to choose custom units\n')
+      assign('done_units_report', TRUE, parent.frame())
+    }
+    difftime(e1, e2, units='secs')
   }
 
   if (byjoin) {
@@ -1948,7 +1949,7 @@ replace_dot_alias = function(e) {
     if (is.null(jvnames)) jvnames = character(length(ans)-length(bynames))
     if (length(bynames)+length(jvnames)!=length(ans))
       stopf("Internal error: jvnames is length %d but ans is %d and bynames is %d", length(jvnames), length(ans), length(bynames)) # nocov
-    ww = which(jvnames=="")
+    ww = which(!nzchar(jvnames))
     if (any(ww)) jvnames[ww] = paste0("V",ww)
     setattr(ans, "names", c(bynames, jvnames))
   } else {
@@ -2293,7 +2294,7 @@ transform.data.table = function(`_data`, ...)
   if (!cedta()) return(NextMethod()) # nocov
   `_data` = copy(`_data`)
   e = eval(substitute(list(...)), `_data`, parent.frame())
-  set(`_data`, ,names(e), e)
+  set(`_data`, NULL, names(e), e)
   `_data`
 }
 
@@ -2804,7 +2805,7 @@ setDF = function(x, rownames=NULL) {
     if (is.null(xn)) {
       setattr(x, "names", paste0("V",seq_len(length(x))))
     } else {
-      idx = xn %chin% ""
+      idx = !nzchar(xn) # NB: keepNA=FALSE intentional
       if (any(idx)) {
         xn[idx] = paste0("V", seq_along(which(idx)))
         setattr(x, "names", xn)
@@ -2877,7 +2878,7 @@ setDT = function(x, keep.rownames=FALSE, key=NULL, check.names=FALSE) {
     if (is.null(xn)) {
       setattr(x, "names", paste0("V", seq_along(x)))
     } else {
-      idx = xn %chin% "" # names can be NA - test 1006 caught that!
+      idx = !nzchar(xn) # NB: keepNA=FALSE intentionally, see test 1006
       if (any(idx)) {
         xn[idx] = paste0("V", seq_along(which(idx)))
         setattr(x, "names", xn)
@@ -3052,7 +3053,8 @@ is_constantish = function(q, check_singleton=FALSE) {
   if (is.symbol(q1)) return(if (q1 %chin% gfuns) q1)
   if (!q1 %iscall% "::") return(NULL)
   if (q1[[2L]] != "data.table") return(NULL)
-  return(if (q1[[3L]] %chin% gdtfuns) q1[[3L]])
+  if (q1[[3L]] %chin% gdtfuns) return(q1[[3L]])
+  NULL
 }
 .gforce_ok = function(q, x) {
   if (is.N(q)) return(TRUE) # For #334
@@ -3291,7 +3293,7 @@ is_constantish = function(q, check_singleton=FALSE) {
         ## search for column names
         thisCols = c(thisCols, trimws(strsplit(pieces[[i]][j], pat)[[1L]]))
         ## there can be empty string column names because of trimws, remove them
-        thisCols = thisCols[thisCols != ""]
+        thisCols = thisCols[nzchar(thisCols)]
         j = j+1L
       }
     }
@@ -3336,5 +3338,5 @@ is_constantish = function(q, check_singleton=FALSE) {
   ## the final on will contain the xCol as name, the iCol as value
   on = iCols
   names(on) = xCols
-  return(list(on = on, ops = idx_op))
+  list(on = on, ops = idx_op)
 }

R/fcast.R

@@ -5,7 +5,7 @@ guess = function(x) {
     return("(all)")
   var = names(x)[ncol(x)]
   messagef("Using '%s' as value column. Use 'value.var' to override", var)
-  return(var)
+  var
 }
 
 dcast <- function(

R/fmelt.R

@@ -39,7 +39,7 @@ measure = function(..., sep="_", pattern, cols, multiple.keyword="value.name") {
   formal.names = names(formals())
   formal.i.vec = which(names(L) %in% formal.names)
   fun.list = L[-formal.i.vec]
-  user.named = names(fun.list) != ""
+  user.named = nzchar(names(fun.list))
   is.symb = sapply(fun.list, is.symbol)
   bad.i = which((!user.named) & (!is.symb))
   if (length(bad.i)) {
@@ -73,7 +73,7 @@ measurev = function(fun.list, sep="_", pattern, cols, multiple.keyword="value.na
   if (!missing(sep) && !missing(pattern)) {
     stopf("both sep and pattern arguments used; must use either sep or pattern (not both)")
   }
-  if (!(is.character(multiple.keyword) && length(multiple.keyword)==1 && !is.na(multiple.keyword) && nchar(multiple.keyword)>0)) {
+  if (!(is.character(multiple.keyword) && length(multiple.keyword)==1 && !is.na(multiple.keyword) && nzchar(multiple.keyword))) {
     stopf("multiple.keyword must be a character string with nchar>0")
   }
   if (!is.character(cols)) {
@@ -82,7 +82,7 @@ measurev = function(fun.list, sep="_", pattern, cols, multiple.keyword="value.na
   prob.i <- if (is.null(names(fun.list))) {
     seq_along(fun.list)
   } else {
-    which(names(fun.list) == "")
+    which(!nzchar(names(fun.list)))
   }
   if (length(prob.i)) {
     stopf("in measurev, %s must be named, problems: %s", group.desc, brackify(prob.i))

R/fread.R

@@ -153,10 +153,10 @@ yaml=FALSE, autostart=NA, tmpdir=tempdir(), tz="UTC")
   if (length(tt)) {
     msg = gettextf('na.strings[%d]=="%s" consists only of whitespace, ignoring', tt[1L], na.strings[tt[1L]])
     if (strip.white) {
-      if (any(na.strings=="")) {
-        warningf('%s. strip.white==TRUE (default) and "" is present in na.strings, so any number of spaces in string columns will already be read as <NA>.', msg)
-      } else {
+      if (all(nzchar(na.strings))) {
         warningf('%s. Since strip.white=TRUE (default), use na.strings="" to specify that any number of spaces in a string column should be read as <NA>.', msg)
+      } else {
+        warningf('%s. strip.white==TRUE (default) and "" is present in na.strings, so any number of spaces in string columns will already be read as <NA>.', msg)
       }
       na.strings = na.strings[-tt]
     } else {
@@ -299,7 +299,7 @@ yaml=FALSE, autostart=NA, tmpdir=tempdir(), tz="UTC")
   }
 
   colClassesAs = attr(ans, "colClassesAs", exact=TRUE)   # should only be present if one or more are != ""
-  for (j in which(colClassesAs!="")) {       # # 1634
+  for (j in which(nzchar(colClassesAs))) {       # # 1634
     v = .subset2(ans, j)
     new_class = colClassesAs[j]
     new_v = tryCatch({    # different to read.csv; i.e. won't error if a column won't coerce (fallback with warning instead)
@@ -317,7 +317,7 @@ yaml=FALSE, autostart=NA, tmpdir=tempdir(), tz="UTC")
       },
       warning = fun <- function(e) {
         warningf("Column '%s' was requested to be '%s' but fread encountered the following %s:\n\t%s\nso the column has been left as type '%s'", names(ans)[j], new_class, if (inherits(e, "error")) "error" else "warning", e$message, typeof(v))
-        return(v)
+        v
       },
       error = fun)
     set(ans, j = j, value = new_v)  # aside: new_v == v if the coercion was aborted

R/fwrite.R

@@ -48,7 +48,7 @@ fwrite = function(x, file="", append=FALSE, quote="auto",
   }
   stopifnot(is.list(x),
     identical(quote,"auto") || isTRUEorFALSE(quote),
-    is.character(sep) && length(sep)==1L && (nchar(sep) == 1L || sep == ""),
+    is.character(sep) && length(sep)==1L && (nchar(sep) == 1L || identical(sep, "")),
     is.character(sep2) && length(sep2)==3L && nchar(sep2[2L])==1L,
     is.character(dec) && length(dec)==1L && nchar(dec) == 1L,
     dec != sep,  # sep2!=dec and sep2!=sep checked at C level when we know if list columns are present

R/merge.R

@@ -59,7 +59,7 @@ merge.data.table = function(x, y, by = NULL, by.x = NULL, by.y = NULL, all = FAL
   if (length(list(...))) {
     ell = as.list(substitute(list(...)))[-1L]
     for (n in setdiff(names(ell), "")) warningf("Unknown argument '%s' has been passed.", n)
-    unnamed_n = length(ell) - sum(names(ell) != "")
+    unnamed_n = length(ell) - sum(nzchar(names(ell)))
     if (unnamed_n)
       warningf("Passed %d unknown and unnamed arguments.", unnamed_n)
   }

R/print.data.table.R

@@ -79,7 +79,7 @@ print.data.table = function(x, topn=getOption("datatable.print.topn"),
 
   # FR #353 - add row.names = logical argument to print.data.table
   if (isTRUE(row.names)) rownames(toprint)=paste0(format(rn,right=TRUE,scientific=FALSE),":") else rownames(toprint)=rep.int("", nrow(toprint))
-  if (is.null(names(x)) || all(names(x) == ""))
+  if (is.null(names(x)) || !any(nzchar(names(x), keepNA=TRUE)))
     # fixes bug #97 and #545
     colnames(toprint)=rep("", ncol(toprint))
   if (isTRUE(class) && col.names != "none") {
@@ -148,7 +148,7 @@ mimicsAutoPrint = c("knit_print.default")
 # add maybe repr_text.default.  See https://github.com/Rdatatable/data.table/issues/933#issuecomment-220237965
 
 shouldPrint = function(x) {
-  ret = (.global$print=="" ||   # to save address() calls and adding lots of address strings to R's global cache
+  ret = (identical(.global$print, "") ||   # to save address() calls and adding lots of address strings to R's global cache
      address(x)!=.global$print)
   .global$print = ""
   ret

R/shift.R

@@ -1,6 +1,6 @@
 shift = function(x, n=1L, fill, type=c("lag", "lead", "shift", "cyclic"), give.names=FALSE) {
   type = match.arg(type)
-  if (type == "cyclic" && !missing(fill)) warning("Provided argument fill=", fill, " will be ignored since type='shift'.")
+  if (type == "cyclic" && !missing(fill)) warningf("Provided argument fill=%s will be ignored since type='cyclic'.", fill)
   if (missing(fill)) fill = NA
   stopifnot(is.numeric(n))
   ans = .Call(Cshift, x, as.integer(n), fill, type)

R/utils.R

@@ -25,7 +25,7 @@ if (!exists('startsWith', 'package:base', inherits=FALSE)) {  # R 3.3.0; Apr 201
   startsWith = function(x, stub) substr(x, 1L, nchar(stub))==stub
 }
 # endsWith no longer used from #5097 so no need to backport; prevent usage to avoid dev delay until GLCI's R 3.1.0 test
-endsWith = function(...) stop("Internal error: use endsWithAny instead of base::endsWith")
+endsWith = function(...) stop("Internal error: use endsWithAny instead of base::endsWith", call.=FALSE)
 
 startsWithAny = function(x,y) .Call(CstartsWithAny, x, y, TRUE)
 endsWithAny = function(x,y) .Call(CstartsWithAny, x, y, FALSE)
@@ -92,7 +92,7 @@ name_dots = function(...) {
   } else {
     vnames[is.na(vnames)] = ""
   }
-  notnamed = vnames==""
+  notnamed = !nzchar(vnames)
   if (any(notnamed)) {
     syms = vapply_1b(dot_sub, is.symbol)  # save the deparse() in most cases of plain symbol
     for (i in which(notnamed)) {
@@ -144,7 +144,7 @@ is_utc = function(tz) {
   # via grep('UTC|GMT', OlsonNames(), value = TRUE); ordered by "prior" frequency
   utc_tz = c("UTC", "GMT", "Etc/UTC", "Etc/GMT", "GMT-0", "GMT+0", "GMT0")
   if (is.null(tz)) tz = Sys.timezone()
-  return(tz %chin% utc_tz)
+  tz %chin% utc_tz
 }
 
 # very nice idea from Michael to avoid expression repetition (risk) in internal code, #4226

inst/tests/benchmark.Rraw

@@ -360,7 +360,7 @@ fff = function(aref) {
   ff = lapply(1:5, function(i) {
     DT[,list(sumA=sum(get(aref))),by=b][,c:=letters[i]]
   })
-  return(rbindlist(ff))
+  rbindlist(ff)
 }
 for(i in 1:100) {
   f = fff("a")
@@ -490,7 +490,7 @@ out = capture.output(print(DT))
 tt = out[grep("V",out)]
 tt = unlist(strsplit(gsub(" ","",tt), "V"))
 test(1982.1, tt[1L], "")
-tt = as.integer(tt[tt!=""])
+tt = as.integer(tt[nzchar(tt)])
 test(1982.2, tt, seq_along(tt))
 
 # fread leak, #3292

inst/tests/other.Rraw

@@ -408,7 +408,7 @@ if (loaded[["yaml"]]) {  # csvy; #1701. Was 2032-2033 in tests.Rraw, #5516
   fwrite(DT, f<-tempfile(), bom=TRUE, yaml=TRUE)
   fcon = file(f, encoding="UTF-8")  # Windows readLines needs to be told; see also test 1658.50 in tests.Rraw
   lines = readLines(fcon)
-  lines = lines[lines!=""]  # an extra "" after "eol: |2+" (line 16) on Linux but not Windows
+  lines = lines[nzchar(lines)]  # an extra "" after "eol: |2+" (line 16) on Linux but not Windows
                             # remove the blank here so we don't need to change this test if/when that changes in yaml package
   test(17.11, length(lines), 48L)
   close(fcon)
@@ -417,7 +417,7 @@ if (loaded[["yaml"]]) {  # csvy; #1701. Was 2032-2033 in tests.Rraw, #5516
   fwrite(DT, f<-tempfile(), bom=TRUE, yaml=TRUE)
   fcon = file(f, encoding="UTF-8")
   lines = readLines(fcon)
-  lines = lines[lines!=""]
+  lines = lines[nzchar(lines)]
   test(17.13, length(lines), 48L)
   close(fcon)
   test(17.14, fread(f), DT)