Merge pull request #895 from lorenzwalthert/issue-892

Stronger guarantees for output

Author: lorenzwalthert

NEWS.md

@@ -38,6 +38,7 @@
 * ensure a trailing blank line also if the input is cached (#867).
 * Preserve trailing blank line in roxygen examples to simplify concatenation of
   examples (#880).
+* added guarantee that styled code is parsable (#892).
 * An error is now thrown on styling if input unicode characters can't be 
   correctly parsed for Windows and R < 4.2 (#883).
 * Fix argument name `filetype` in Example for `style_dir()` (#855).

R/communicate.R

@@ -4,11 +4,11 @@
 #' changes carefully.
 #' @param changed Boolean with indicating for each file whether or not it has
 #'   been changed.
-#' @inheritParams can_verify_roundtrip
+#' @inheritParams parse_tree_must_be_identical
 #' @keywords internal
 communicate_warning <- function(changed, transformers) {
   if (any(changed, na.rm = TRUE) &&
-    !can_verify_roundtrip(transformers) &&
+    !parse_tree_must_be_identical(transformers) &&
     !getOption("styler.quiet", FALSE)
   ) {
     cat("Please review the changes carefully!", fill = TRUE)

R/transform-files.R

@@ -246,9 +246,11 @@ parse_transform_serialize_r <- function(text,
     ) %>%
     unlist()
 
-  if (can_verify_roundtrip(transformers)) {
-    verify_roundtrip(text, text_out)
-  }
+  verify_roundtrip(
+    text, text_out,
+    parsable_only = !parse_tree_must_be_identical(transformers)
+  )
+
   text_out <- convert_newlines_to_linebreaks(text_out)
   if (cache_is_activated()) {
     cache_by_expression(text_out, transformers, more_specs = more_specs)
@@ -349,18 +351,23 @@ apply_transformers <- function(pd_nested, transformers) {
 #' @param transformers The list of transformer functions used for styling.
 #'   Needed for reverse engineering the scope.
 #' @keywords internal
-can_verify_roundtrip <- function(transformers) {
+parse_tree_must_be_identical <- function(transformers) {
   length(transformers$token) == 0
 }
 
 #' Verify the styling
 #'
 #' If scope was set to "line_breaks" or lower (compare [tidyverse_style()]),
 #' we can compare the expression before and after styling and return an error if
-#' it is not the same. Note that this method ignores roxygen code examples and
+#' it is not the same.
+#' If that's not possible, a weaker guarantee that we want to give is that the
+#' resulting code is parsable.
+#' @param parsable_only If we should only check for the code to be parsable.
+#' @inheritParams expressions_are_identical
+#' @section Limitation:
+#' Note that this method ignores roxygen code examples and
 #' comments and no verification can be conducted if tokens are in the styling
 #' scope.
-#' @inheritParams expressions_are_identical
 #' @importFrom rlang abort
 #' @examples
 #' styler:::verify_roundtrip("a+1", "a + 1")
@@ -369,8 +376,19 @@ can_verify_roundtrip <- function(transformers) {
 #' styler:::verify_roundtrip("a+1", "b - 3")
 #' }
 #' @keywords internal
-verify_roundtrip <- function(old_text, new_text) {
-  if (!expressions_are_identical(old_text, new_text)) {
+verify_roundtrip <- function(old_text, new_text, parsable_only = FALSE) {
+  if (parsable_only) {
+    rlang::with_handlers(
+      parse_safely(new_text),
+      error = function(e) {
+        rlang::abort(paste0(
+          "Styling resulted in code that isn't parsable. This should not ",
+          "happen. Please file a bug report on GitHub ",
+          "(https://github.com/r-lib/styler/issues) using a reprex."
+        ))
+      }
+    )
+  } else if (!expressions_are_identical(old_text, new_text)) {
     msg <- paste(
       "The expression evaluated before the styling is not the same as the",
       "expression after styling. This should not happen. Please file a",

man/can_verify_roundtrip.Rd renamed to man/parse_tree_must_be_identical.Rd

@@ -34,8 +34,8 @@ x <- '	2' # there is a tab emebbed (created with writeLines("x <- '\t2'"))
 x <- '\001'
 '\x01'
 
-"\01"
-'\01'
+"\001"
+'\001'
 
 #' things
 #'

man/verify_roundtrip.Rd

@@ -34,8 +34,8 @@ x <- "	2" # there is a tab emebbed (created with writeLines("x <- '\t2'"))
 x <- "\001"
 "\x01"
 
-"\0"
-"\0"
+"\001"
+"\001"
 
 #' things
 #'

tests/testthat/escaping/basic-escape-in.R

@@ -1,11 +1,15 @@
 context("roundtrip works")
 
 
-test_that("can_verify_roundtrip works", {
-  expect_true(can_verify_roundtrip(tidyverse_style(scope = "line_breaks")))
-  expect_true(can_verify_roundtrip(tidyverse_style(scope = "spaces")))
-  expect_true(can_verify_roundtrip(tidyverse_style(scope = "indention")))
-  expect_false(can_verify_roundtrip(tidyverse_style(scope = "tokens")))
+test_that("parse_tree_must_be_identical works", {
+  expect_true(
+    parse_tree_must_be_identical(tidyverse_style(scope = "line_breaks"))
+  )
+  expect_true(parse_tree_must_be_identical(tidyverse_style(scope = "spaces")))
+  expect_true(
+    parse_tree_must_be_identical(tidyverse_style(scope = "indention"))
+  )
+  expect_false(parse_tree_must_be_identical(tidyverse_style(scope = "tokens")))
 })
 
 test_that("correct styling does not give an error", {
@@ -15,3 +19,15 @@ test_that("correct styling does not give an error", {
 test_that("corrupt styling does give an error", {
   expect_error(verify_roundtrip("1-1", "1 + 1"), "bug")
 })
+
+
+test_that("the output is asserted to be parsable", {
+  expect_error(
+    verify_roundtrip("1+1", "1 +) 1", parsable_only = TRUE),
+    "Styling resulted in code that isn't parsable."
+  )
+
+  expect_silent(
+    verify_roundtrip("1+1", "1 + 1", parsable_only = TRUE)
+  )
+})