Fix line and column number wrongly reported by spaces_inside_linter()…

… in some cases (#203
r-lib · May 3, 2017 · 17a8ee8 · 17a8ee8
1 parent fd1ec51
commit 17a8ee8
Show file tree

Hide file tree

Showing 3 changed files with 55 additions and 49 deletions.
diff --git a/NEWS.md b/NEWS.md
@@ -1,4 +1,6 @@
 # lintr 1.0.0.9001 #
+* Fix line and column number sometimes wrongly reported by spaces_inside_linter()
+  (#203, @fangly)
 * Add `pipe_continuation_linter()` (#216).
 * Deprecated camel_case_linter(), snake_case_linter() and multiple_dots_linter()
   in favor of object_name_linter() which enforce the given style: snake_case,
@@ -24,7 +26,7 @@
   user-specified functions and operators (#48, #149, @fangly).
 * Relaxed the commented_code_linter(): do not lint comments within roxygen blocks
   and do not consider "-" an R operator to avoid too many false positives.
-* Fixed object linters to only lint objects declared in the current file 
+* Fixed object linters to only lint objects declared in the current file
   (#76, #108, #136, #191, #194, #201, @fangly).
 * Fixed expect_lint() issues (#180, #211, @fangly): markers were displayed when
   check was NULL, some error messages were malformed.

diff --git a/R/spaces_inside_linter.R b/R/spaces_inside_linter.R
@@ -10,47 +10,48 @@ spaces_inside_linter <- function(source_file) {
 
   # using a regex here as checking each token is horribly slow
   re <- rex(list(one_of("[("), " ") %or% list(" " %if_prev_isnt% ",", one_of("])")))
-
-  res <- re_matches(source_file$lines, re, global = TRUE, locations = TRUE)
-
-  lapply(seq_along(res), function(line_number) {
-
-    mapply(
-      FUN = function(start, end) {
-        if (is.na(start)) {
-          return()
-        }
-
-        line <- unname(source_file$lines[line_number])
-
-        # we need to make sure that the closing are not at the start of a new
-        # line (which is valid).
-        start_of_line <- re_matches(line, rex(start, spaces, one_of("])")), locations = TRUE)
-
-        if (is.na(start_of_line$start) || start_of_line$end != end) {
-          is_token <-
-            any(source_file$parsed_content$line1 == line_number &
-               (source_file$parsed_content$col1 == end |
-                source_file$parsed_content$col1 == start) &
-              source_file$parsed_content$token %in% tokens)
-
-          if (is_token) {
-
-            Lint(
-              filename = source_file$filename,
-              line_number = line_number,
-              column_number = start,
-              type = "style",
-              message = "Do not place spaces around code in parentheses or square brackets.",
-              line = line,
-              linter = "spaces_inside_linter"
+  all_matches <- re_matches(source_file$lines, re, global = TRUE, locations = TRUE)
+  line_numbers <- as.integer(names(source_file$lines))
+
+  Map(
+    function(line_matches, line_number) {
+      apply(
+        line_matches,
+        1L,
+        function(match) {
+          start <- match[["start"]]
+          if (is.na(start)) {
+            return()
+          }
+          end <- match[["end"]]
+          line <- source_file$lines[[as.character(line_number)]]
+
+          # make sure that the closing is not at the start of a new line (which is valid).
+          start_of_line <- re_matches(line, rex(start, spaces, one_of("])")), locations = TRUE)
+
+          if (is.na(start_of_line$start) || start_of_line$end != end) {
+            pc <- source_file$parsed_content
+            is_token <-
+              any(pc[["line1"]] == line_number &
+                  (pc[["col1"]] == end | pc[["col1"]] == start) &
+                  pc[["token"]] %in% tokens)
+
+            if (is_token) {
+              Lint(
+                filename = source_file$filename,
+                line_number = line_number,
+                column_number = if (substr(line, start, start) == " ") {start} else {start + 1L},
+                type = "style",
+                message = "Do not place spaces around code in parentheses or square brackets.",
+                line = line,
+                linter = "spaces_inside_linter"
               )
+            }
           }
         }
-      },
-      start = res[[line_number]]$start,
-      end = res[[line_number]]$end,
-      SIMPLIFY = FALSE
       )
-    })
+    },
+    all_matches,
+    line_numbers
+  )
 }
diff --git a/tests/testthat/test-spaces_inside_linter.R b/tests/testthat/test-spaces_inside_linter.R
@@ -1,5 +1,7 @@
 context("spaces_inside_linter")
+
 test_that("returns the correct linting", {
+  msg <- rex::rex("Do not place spaces around code in parentheses or square brackets.")
 
   expect_lint("blah", NULL, spaces_inside_linter)
 
@@ -16,16 +18,16 @@ test_that("returns the correct linting", {
   expect_lint("fun(\na[1]\n  )", NULL, spaces_inside_linter)
 
   expect_lint("a[1 ]",
-    rex("Do not place spaces around code in parentheses or square brackets."),
+    c(message = msg, line_number = 1, column_number = 4, type = "style"),
     spaces_inside_linter)
 
-  expect_lint("a[ 1]",
-    rex("Do not place spaces around code in parentheses or square brackets."),
+  expect_lint("\n\na[ 1]",
+    c(message = msg, line_number = 3, column_number = 3, type = "style"),
     spaces_inside_linter)
 
   expect_lint("a[ 1 ]",
-    list("Do not place spaces around code in parentheses or square brackets.",
-    rex("Do not place spaces around code in parentheses or square brackets.")),
+    list(c(message = msg, line_number = 1, column_number = 3, type = "style"),
+         c(message = msg, line_number = 1, column_number = 5, type = "style")),
     spaces_inside_linter)
 
   expect_lint("a(, )", NULL, spaces_inside_linter)
@@ -35,19 +37,20 @@ test_that("returns the correct linting", {
   expect_lint("a(1)", NULL, spaces_inside_linter)
 
   expect_lint("a(1 )",
-    rex("Do not place spaces around code in parentheses or square brackets."),
+    c(message = msg, line_number = 1, column_number = 4, type = "style"),
     spaces_inside_linter)
 
   expect_lint("a( 1)",
-    rex("Do not place spaces around code in parentheses or square brackets."),
+    c(message = msg, line_number = 1, column_number = 3, type = "style"),
     spaces_inside_linter)
 
   expect_lint("a( 1 )",
-    list("Do not place spaces around code in parentheses or square brackets.",
-    rex("Do not place spaces around code in parentheses or square brackets.")),
+    list(c(message = msg, line_number = 1, column_number = 3, type = "style"),
+         c(message = msg, line_number = 1, column_number = 5, type = "style")),
     spaces_inside_linter)
 
   expect_lint("\"a( 1 )\"",
     NULL,
     spaces_inside_linter)
+
 })