diff --git a/NEWS.md b/NEWS.md index 32f21f36a..7b315df26 100644 --- a/NEWS.md +++ b/NEWS.md @@ -36,6 +36,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). diff --git a/R/communicate.R b/R/communicate.R index e4e0b58dd..3e50e272d 100644 --- a/R/communicate.R +++ b/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) diff --git a/R/transform-files.R b/R/transform-files.R index 839245bc5..764c90f91 100644 --- a/R/transform-files.R +++ b/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,7 +351,7 @@ 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 } @@ -357,10 +359,15 @@ can_verify_roundtrip <- function(transformers) { #' #' 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", diff --git a/man/can_verify_roundtrip.Rd b/man/parse_tree_must_be_identical.Rd similarity index 82% rename from man/can_verify_roundtrip.Rd rename to man/parse_tree_must_be_identical.Rd index 1bd993b1d..a9204cf03 100644 --- a/man/can_verify_roundtrip.Rd +++ b/man/parse_tree_must_be_identical.Rd @@ -1,10 +1,10 @@ % Generated by roxygen2: do not edit by hand % Please edit documentation in R/transform-files.R -\name{can_verify_roundtrip} -\alias{can_verify_roundtrip} +\name{parse_tree_must_be_identical} +\alias{parse_tree_must_be_identical} \title{Check whether a round trip verification can be carried out} \usage{ -can_verify_roundtrip(transformers) +parse_tree_must_be_identical(transformers) } \arguments{ \item{transformers}{The list of transformer functions used for styling. diff --git a/man/verify_roundtrip.Rd b/man/verify_roundtrip.Rd index c58e3ee0e..ecfcee948 100644 --- a/man/verify_roundtrip.Rd +++ b/man/verify_roundtrip.Rd @@ -4,20 +4,29 @@ \alias{verify_roundtrip} \title{Verify the styling} \usage{ -verify_roundtrip(old_text, new_text) +verify_roundtrip(old_text, new_text, parsable_only = FALSE) } \arguments{ \item{old_text}{The initial expression in its character representation.} \item{new_text}{The styled expression in its character representation.} + +\item{parsable_only}{If we should only check for the code to be parsable.} } \description{ If scope was set to "line_breaks" or lower (compare \code{\link[=tidyverse_style]{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. +} +\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. } + \examples{ styler:::verify_roundtrip("a+1", "a + 1") styler:::verify_roundtrip("a+1", "a + 1 # comments are dropped") diff --git a/tests/testthat/escaping/basic-escape-in.R b/tests/testthat/escaping/basic-escape-in.R index 0b62400bf..3ceeab053 100644 --- a/tests/testthat/escaping/basic-escape-in.R +++ b/tests/testthat/escaping/basic-escape-in.R @@ -34,8 +34,8 @@ x <- ' 2' # there is a tab emebbed (created with writeLines("x <- '\t2'")) x <- '\001' '\x01' -"\01" -'\01' +"\001" +'\001' #' things #' diff --git a/tests/testthat/escaping/basic-escape-in_tree b/tests/testthat/escaping/basic-escape-in_tree index 9b70d3ba2..8083077fa 100644 --- a/tests/testthat/escaping/basic-escape-in_tree +++ b/tests/testthat/escaping/basic-escape-in_tree @@ -42,10 +42,10 @@ ROOT (token: short_text [lag_newlines/spaces] {pos_id}) ¦ °--STR_CONST: '\001 [0/0] {40} ¦--expr: '\x01 [1/0] {43} ¦ °--STR_CONST: '\x01 [0/0] {42} - ¦--expr: "\01" [2/0] {45} - ¦ °--STR_CONST: "\0" [0/0] {44} - ¦--expr: '\01' [1/0] {47} - ¦ °--STR_CONST: '\0' [0/0] {46} + ¦--expr: "\001 [2/0] {45} + ¦ °--STR_CONST: "\001 [0/0] {44} + ¦--expr: '\001 [1/0] {47} + ¦ °--STR_CONST: '\001 [0/0] {46} ¦--COMMENT: #' th [2/0] {48} ¦--COMMENT: #' [1/0] {49} ¦--COMMENT: #' @e [1/0] {50} diff --git a/tests/testthat/escaping/basic-escape-out.R b/tests/testthat/escaping/basic-escape-out.R index 9b383911d..c35989e3f 100644 --- a/tests/testthat/escaping/basic-escape-out.R +++ b/tests/testthat/escaping/basic-escape-out.R @@ -34,8 +34,8 @@ x <- " 2" # there is a tab emebbed (created with writeLines("x <- '\t2'")) x <- "\001" "\x01" -"\0" -"\0" +"\001" +"\001" #' things #' diff --git a/tests/testthat/test-roundtrip.R b/tests/testthat/test-roundtrip.R index 406b111b7..52b172803 100644 --- a/tests/testthat/test-roundtrip.R +++ b/tests/testthat/test-roundtrip.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) + ) +})