diff --git a/NAMESPACE b/NAMESPACE index 46cfd3c3..fd9a7658 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -13,6 +13,7 @@ S3method(str,R7_union) export("@") export("method<-") export("prop<-") +export("props<-") export("r7_data<-") export(R7_object) export(as_class) @@ -37,6 +38,7 @@ export(r7_data) export(s3_class) export(valid_eventually) export(valid_implicitly) +export(validate) importFrom(stats,setNames) importFrom(utils,getFromNamespace) importFrom(utils,getS3method) diff --git a/NEWS.md b/NEWS.md index 64234cc1..cbb7080d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -2,6 +2,10 @@ ## Jan 2020 +* New `props<-` for setting multiple properties simultaneously and validating + afterwards (#149). +* Validation now happens recursively, and validates types before validating + the object (#149) * Classes (base types, S3, S4, and R7) are handled consistently wherever they are used. Strings now only refer to base types. New explicit `s3_class()` for referring to S3 classes (#134). diff --git a/R/class.R b/R/class.R index c3fb4765..61138e43 100644 --- a/R/class.R +++ b/R/class.R @@ -50,10 +50,20 @@ is_class <- function(x) inherits(x, "R7_class") #' #' @param constructor The constructor function. This is optional, unless #' you want to control which properties can be set on constructor. -#' @param validator A function used to determine whether or not an object -#' is valid. This is called automatically after construction, and -#' whenever any property is set. It should return `NULL` if the object is -#' valid, and otherwise return a character vector of problems. +#' @param validator A function taking a single argument, the object to validate. +#' +#' The job of a validator is to determine whether the object is valid, +#' i.e. if the current property values form an allowed combination. The +#' types of the properties are always automatically validator so the job of +#' the validator is to verify that the value of individual properties is +#' ok (i.e. maybe a property should have length 1, or should always be +#' positive), or that the combination of values of multiple properties is ok. +#' It is called after construction and whenever any property is set. +#' +#' The validator should return `NULL` if the object is valid. If not, it +#' should return a character vector where each element describes a single +#' problem. It's generally helpful to report as many problems at once +#' as possible. #' @param properties A list specifying the properties (data) that #' every object of the class will possess. Each property can either be #' a named string (specifying the class), or a call to [new_property()], diff --git a/R/data.R b/R/data.R index 2cec7513..e818fe3e 100644 --- a/R/data.R +++ b/R/data.R @@ -5,6 +5,7 @@ #' and all properties. #' #' @inheritParams prop +#' @param value Object used to replace the underlying data. #' @export #' @examples #' text <- new_class("text", parent = "character") diff --git a/R/object.R b/R/object.R index 4a27c3d0..f8043f10 100644 --- a/R/object.R +++ b/R/object.R @@ -26,38 +26,16 @@ new_object <- function(.data = NULL, ...) { } if (!is.null(.data)) { - # TODO eliminate this special case - if (obj_cls@name %in% names(base_types)) { - if (!inherits(.data, obj_cls@name)) { - stop(sprintf( - "`.data` must be %s not %s", - class_desc(as_class(obj_cls@name)), - obj_desc(.data) - )) - } - } else { - if (!class_inherits(.data, obj_cls@parent)) { - stop(sprintf( - "`.data` must be %s not %s", - class_desc(obj_cls@parent), - obj_desc(.data) - )) - } - } - object <- .data } else { object <- obj_cls@parent@constructor() } - attr(object, ".should_validate") <- FALSE class(object) <- "R7_object" object_class(object) <- obj_cls for (nme in nms) { - prop(object, nme) <- args[[nme]] + prop(object, nme, check = FALSE) <- args[[nme]] } - - attr(object, ".should_validate") <- NULL validate(object) object diff --git a/R/property.R b/R/property.R index 215a82ea..710aa1ae 100644 --- a/R/property.R +++ b/R/property.R @@ -80,14 +80,16 @@ str.R7_property <- function(object, ..., nest.lev = 0) { #' rather than throwing an error. #' - `prop<-` and `@<-` set a new value for the given property. #' - `props()` returns a list of all properties +#' - `props<-` sets multiple properties at once, validating once after all are set. #' - `prop_names()` returns the names of the properties #' - `prop_exists(x, "prop")` returns `TRUE` iif `x` has property `prop`. #' #' @param object An object from a R7 class #' @param name The name of the parameter as a character. Partial matching #' is not performed. -#' @param value A replacement value for the parameter. The object is -#' automatically checked for validity after the replacement is done. +#' @param value For `prop<-`, a replacement value for the property; +#' for `props<-`, a named list of values. The object is automatically +#' checked for validity after the replacement is done. #' @export #' @examples #' horse <- new_class("horse", properties = list( @@ -108,7 +110,7 @@ prop <- function(object, name) { if (!inherits(object, "R7_object")) { stop("`object` is not an ") } else if (!prop_exists(object, name)) { - stop(sprintf("Can't find property %s@%s", obj_desc(object), name)) + stop(prop_error_unknown(object, name)) } else { prop_val(object, name) } @@ -172,6 +174,19 @@ props <- function(object) { } } +#' @rdname prop +#' @export +`props<-` <- function(object, value) { + stopifnot(is.list(value)) + + for (name in names(value)) { + prop(object, name, check = FALSE) <- value[[name]] + } + validate(object) + + object +} + #' @rdname prop #' @export prop_exists <- function(object, name) { @@ -188,29 +203,42 @@ prop_exists <- function(object, name) { function(object, name, check = TRUE, value) { prop <- prop_obj(object, name) + if (is.null(prop)) { + stop(prop_error_unknown(object, name)) + } + if (!is.null(prop$setter) && !identical(setter_property, name)) { setter_property <<- name on.exit(setter_property <<- NULL, add = TRUE) object <- prop$setter(object, value) } else { if (isTRUE(check) && !class_inherits(value, prop$class)) { - stop(sprintf("%s@%s must be of class %s, not %s", - obj_desc(object), name, - class_desc(prop$class), - obj_desc(value) - ), call. = FALSE) + stop(prop_error_type(object, name, prop$class, value), call. = FALSE) } attr(object, name) <- value } if (isTRUE(check)) { - validate(object) + validate(object, properties = FALSE) } invisible(object) } }) +prop_error_unknown <- function(object, prop_name) { + sprintf("Can't find property %s@%s", obj_desc(object), prop_name) +} + +prop_error_type <- function(object, prop_name, expected, actual) { + sprintf("%s@%s must be of class %s, not %s", + obj_desc(object), + prop_name, + class_desc(expected), + obj_desc(actual) + ) +} + #' @rdname prop #' @usage object@name #' @export diff --git a/R/valid.R b/R/valid.R index 3d6f4561..9faae3cb 100644 --- a/R/valid.R +++ b/R/valid.R @@ -1,41 +1,119 @@ -#' Validation of R7 objects +#' Validate an R7 object #' -#' [validate()] calls the validation of an R7 object. This is done -#' automatically when creating new objects (at the end of [new_object]) and -#' when setting any property. +#' @description +#' `validate()` ensures that an R7 object is valid by calling the `validator` +#' provided in [new_class()]. This is done automatically when constructing new +#' objects and when modifying properties. #' -#' [valid_eventually()] disables validation of properties, runs a function on -#' the object, then validates the object. +#' `valid_eventually()` disables validation, modifies the object, then +#' revalidates. This is useful when a sequence of operations would otherwise +#' lead an object to be temporarily invalid, or when repeated property +#' modification causes a performance bottleneck because the validator is +#' relatively expensive. #' -#' [valid_implicitly()] does the same but does not validate the object at the end. -#' -#' [valid_implicitly()] should only be used rarely in performance critical code -#' where you are certain a sequence of operations cannot produce an invalid -#' object. +#' `valid_implicitly()` does the same but does not validate the object at the +#' end. It should only be used rarely, and in performance critical code where +#' you are certain a sequence of operations cannot produce an invalid object. #' @param object An R7 object #' @param fun A function to call on the object before validation. -validate <- function(object) { +#' @param properties If `TRUE`, the default, checks property types before +#' executing the validator. +#' @export +#' @examples +#' # A range class might validate that the start is less than the end +#' Range <- new_class("Range", +#' properties = list(start = "double", end = "double"), +#' validator = function(object) { +#' if (object@start >= object@end) "start must be smaller than end" +#' } +#' ) +#' # You can't construct an invalid object: +#' try(Range(1, 1)) +#' +#' # And you can't create an invalid object with @<- +#' r <- Range(1, 2) +#' try(r@end <- 1) +#' +#' # But what if you want to move a range to the right? +#' rightwards <- function(r, x) { +#' r@start <- r@start + x +#' r@end <- r@end + x +#' r +#' } +#' # This function doesn't work because it creates a temporarily invalid state +#' try(rightwards(r, 10)) +#' +#' # This is the perfect use case for valid_eventually(): +#' rightwards <- function(r, x) { +#' valid_eventually(r, function(object) { +#' object@start <- object@start + x +#' object@end <- object@end + x +#' object +#' }) +#' } +#' rightwards(r, 10) +#' +#' # Alternatively, you can set multiple properties at once using props<-, +#' # which validates once at the end +#' rightwards <- function(r, x) { +#' props(r) <- list(start = r@start + x, end = r@end + x) +#' r +#' } +#' rightwards(r, 20) +validate <- function(object, properties = TRUE) { if (!is.null(attr(object, ".should_validate"))) { return(invisible(object)) } - obj_class <- object_class(object) + class <- object_class(object) - validator <- prop_safely(obj_class, "validator") - if (is.null(validator)) { - return(invisible(object)) + # First, check property types - if these are incorrect, the validator + # is likely to return spurious errors + if (properties) { + errors <- validate_properties(object, class) + if (length(errors) > 0) { + bullets <- paste0("- ", errors, collapse = "\n") + msg <- sprintf("%s object properties are invalid:\n%s", obj_desc(object), bullets) + stop(msg, call. = FALSE) + } } - errors <- validator(object) + # Next, recursively validate the object + errors <- character() + while(!is.null(class) && is_class(class)) { + errors <- c(errors, class@validator(object)) + class <- prop_safely(class, "parent") + } + # If needed, report errors if (length(errors) > 0) { - msg <- sprintf("Invalid <%s> object:\n%s", obj_class@name, paste0("- ", errors, collapse = "\n")) + bullets <- paste0("- ", errors, collapse = "\n") + msg <- sprintf("%s object is invalid:\n%s", obj_desc(object), bullets) stop(msg, call. = FALSE) } invisible(object) } +validate_properties <- function(object, class) { + errors <- character() + + for (prop in class@properties) { + # Only validate static properties + if (!is.null(prop$getter) || !is.null(prop$setter)) { + next + } + + value <- prop(object, prop$name) + if (!class_inherits(value, prop$class)) { + errors <- c(errors, prop_error_type(object, prop$name, prop$class, value)) + } + prop + } + + errors +} + #' @rdname validate #' @export valid_eventually <- function(object, fun) { @@ -44,6 +122,8 @@ valid_eventually <- function(object, fun) { out <- fun(object) attr(out, ".should_validate") <- old validate(out) + + out } #' @rdname validate @@ -53,5 +133,6 @@ valid_implicitly <- function(object, fun) { attr(object, ".should_validate") <- FALSE out <- fun(object) attr(out, ".should_validate") <- old - invisible(out) + + out } diff --git a/R/zzz.R b/R/zzz.R index 1a03a4a3..b461e099 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -13,7 +13,16 @@ R7_object <- new_class( ) new_base_class <- function(name) { - R7_class(name = name, constructor = function(.data) new_object(.data)) + new_class( + name = name, + constructor = function(.data) new_object(.data), + validator = function(object) { + data <- unclass(object) + if (!name %in% .class2(data)) { + sprintf("Underlying data must be <%s> not %s", name, obj_desc(data)) + } + } + ) } # Define simple base types with constructors. See .onLoad() for more @@ -47,7 +56,7 @@ R7_generic <- new_class( R7_method <- new_class( name = "R7_method", - properties = list(generic = R7_generic, signature = "list", fun = "function"), + properties = list(generic = R7_generic, signature = "list"), parent = "function", constructor = function(generic, signature, fun) { if (is.character(signature)) { diff --git a/man/new_class.Rd b/man/new_class.Rd index dc2b8826..aeed867b 100644 --- a/man/new_class.Rd +++ b/man/new_class.Rd @@ -30,10 +30,20 @@ allowing greater flexibility.} \item{constructor}{The constructor function. This is optional, unless you want to control which properties can be set on constructor.} -\item{validator}{A function used to determine whether or not an object -is valid. This is called automatically after construction, and -whenever any property is set. It should return \code{NULL} if the object is -valid, and otherwise return a character vector of problems.} +\item{validator}{A function taking a single argument, the object to validate. + +The job of a validator is to determine whether the object is valid, +i.e. if the current property values form an allowed combination. The +types of the properties are always automatically validator so the job of +the validator is to verify that the value of individual properties is +ok (i.e. maybe a property should have length 1, or should always be +positive), or that the combination of values of multiple properties is ok. +It is called after construction and whenever any property is set. + +The validator should return \code{NULL} if the object is valid. If not, it +should return a character vector where each element describes a single +problem. It's generally helpful to report as many problems at once +as possible.} } \value{ A object constructor, a function that can be used to create objects diff --git a/man/prop.Rd b/man/prop.Rd index 98b0bd63..052434e2 100644 --- a/man/prop.Rd +++ b/man/prop.Rd @@ -5,6 +5,7 @@ \alias{prop_safely} \alias{prop_names} \alias{props} +\alias{props<-} \alias{prop_exists} \alias{prop<-} \alias{@} @@ -18,6 +19,8 @@ prop_names(object) props(object) +props(object) <- value + prop_exists(object, name) prop(object, name, check = TRUE) <- value @@ -30,11 +33,12 @@ object@name \item{name}{The name of the parameter as a character. Partial matching is not performed.} +\item{value}{For \verb{prop<-}, a replacement value for the property; +for \verb{props<-}, a named list of values. The object is automatically +checked for validity after the replacement is done.} + \item{check}{If \code{TRUE}, check that \code{value} is of the correct type and run \code{\link[=validate]{validate()}} on the object before returning.} - -\item{value}{A replacement value for the parameter. The object is -automatically checked for validity after the replacement is done.} } \description{ \itemize{ @@ -44,6 +48,7 @@ error if the property doesn't exist for that object. rather than throwing an error. \item \verb{prop<-} and \verb{@<-} set a new value for the given property. \item \code{props()} returns a list of all properties +\item \verb{props<-} sets multiple properties at once, validating once after all are set. \item \code{prop_names()} returns the names of the properties \item \code{prop_exists(x, "prop")} returns \code{TRUE} iif \code{x} has property \code{prop}. } diff --git a/man/r7_data.Rd b/man/r7_data.Rd index 040d9960..d85cfd07 100644 --- a/man/r7_data.Rd +++ b/man/r7_data.Rd @@ -15,8 +15,7 @@ r7_data(object, check = TRUE) <- value \item{check}{If \code{TRUE}, check that \code{value} is of the correct type and run \code{\link[=validate]{validate()}} on the object before returning.} -\item{value}{A replacement value for the parameter. The object is -automatically checked for validity after the replacement is done.} +\item{value}{Object used to replace the underlying data.} } \description{ When an R7 class inherits from an existing base type, it can be useful diff --git a/man/validate.Rd b/man/validate.Rd index dc804baa..8c6119fb 100644 --- a/man/validate.Rd +++ b/man/validate.Rd @@ -4,9 +4,9 @@ \alias{validate} \alias{valid_eventually} \alias{valid_implicitly} -\title{Validation of R7 objects} +\title{Validate an R7 object} \usage{ -validate(object) +validate(object, properties = TRUE) valid_eventually(object, fun) @@ -15,20 +15,65 @@ valid_implicitly(object, fun) \arguments{ \item{object}{An R7 object} +\item{properties}{If \code{TRUE}, the default, checks property types before +executing the validator.} + \item{fun}{A function to call on the object before validation.} } \description{ -\code{\link[=validate]{validate()}} calls the validation of an R7 object. This is done -automatically when creating new objects (at the end of \link{new_object}) and -when setting any property. +\code{validate()} ensures that an R7 object is valid by calling the \code{validator} +provided in \code{\link[=new_class]{new_class()}}. This is done automatically when constructing new +objects and when modifying properties. + +\code{valid_eventually()} disables validation, modifies the object, then +revalidates. This is useful when a sequence of operations would otherwise +lead an object to be temporarily invalid, or when repeated property +modification causes a performance bottleneck because the validator is +relatively expensive. + +\code{valid_implicitly()} does the same but does not validate the object at the +end. It should only be used rarely, and in performance critical code where +you are certain a sequence of operations cannot produce an invalid object. +} +\examples{ +# A range class might validate that the start is less than the end +Range <- new_class("Range", + properties = list(start = "double", end = "double"), + validator = function(object) { + if (object@start >= object@end) "start must be smaller than end" + } +) +# You can't construct an invalid object: +try(Range(1, 1)) + +# And you can't create an invalid object with @<- +r <- Range(1, 2) +try(r@end <- 1) + +# But what if you want to move a range to the right? +rightwards <- function(r, x) { + r@start <- r@start + x + r@end <- r@end + x + r } -\details{ -\code{\link[=valid_eventually]{valid_eventually()}} disables validation of properties, runs a function on -the object, then validates the object. +# This function doesn't work because it creates a temporarily invalid state +try(rightwards(r, 10)) -\code{\link[=valid_implicitly]{valid_implicitly()}} does the same but does not validate the object at the end. +# This is the perfect use case for valid_eventually(): +rightwards <- function(r, x) { + valid_eventually(r, function(object) { + object@start <- object@start + x + object@end <- object@end + x + object + }) +} +rightwards(r, 10) -\code{\link[=valid_implicitly]{valid_implicitly()}} should only be used rarely in performance critical code -where you are certain a sequence of operations cannot produce an invalid -object. +# Alternatively, you can set multiple properties at once using props<-, +# which validates once at the end +rightwards <- function(r, x) { + props(r) <- list(start = r@start + x, end = r@end + x) + r +} +rightwards(r, 20) } diff --git a/tests/testthat/_snaps/class.md b/tests/testthat/_snaps/class.md index b647d59e..64214dc9 100644 --- a/tests/testthat/_snaps/class.md +++ b/tests/testthat/_snaps/class.md @@ -37,7 +37,8 @@ Error `parent` must be an R7 class, S3 class, or base type, not a class union. -# constructor types check their values +# constructor types check their values - `.data` must be not + object is invalid: + - Underlying data must be not diff --git a/tests/testthat/_snaps/object.md b/tests/testthat/_snaps/object.md index 61dac500..922f9046 100644 --- a/tests/testthat/_snaps/object.md +++ b/tests/testthat/_snaps/object.md @@ -1,3 +1,12 @@ +# new_object: reports all property type errors + + Code + range(start = "x", end = "y") + Error + object properties are invalid: + - @start must be of class or , not + - @end must be of class or , not + # new_object: checks are arguments are properties Code diff --git a/tests/testthat/_snaps/property.md b/tests/testthat/_snaps/property.md index 6ad13aab..5d2376e2 100644 --- a/tests/testthat/_snaps/property.md +++ b/tests/testthat/_snaps/property.md @@ -2,6 +2,13 @@ Can't find property @st +# prop<-: errors if the property doesn't exist + + Code + x@foo <- 10 + Error + Can't find property @foo + # @: does not use partial matching Can't find property @st diff --git a/tests/testthat/_snaps/valid.md b/tests/testthat/_snaps/valid.md new file mode 100644 index 00000000..c27362c9 --- /dev/null +++ b/tests/testthat/_snaps/valid.md @@ -0,0 +1,42 @@ +# validate() validates object and type recursively + + Code + obj <- klass(1, -1) + attr(obj, "x") <- -1 + validate(obj) + Error + object is invalid: + - x must be positive + Code + attr(obj, "x") <- "y" + validate(obj) + Error + object properties are invalid: + - @x must be of class , not + +--- + + Code + obj <- klass2(1, -1, 1) + attr(obj, "x") <- -1 + validate(obj) + Error + object is invalid: + - x must be positive + Code + attr(obj, "x") <- "y" + attr(obj, "z") <- "y" + validate(obj) + Error + object properties are invalid: + - @x must be of class , not + - @z must be of class , not + +# validate checks base type + + Code + validate(x) + Error + object is invalid: + - Underlying data must be not + diff --git a/tests/testthat/test-class.R b/tests/testthat/test-class.R index a0b13dff..a416467e 100644 --- a/tests/testthat/test-class.R +++ b/tests/testthat/test-class.R @@ -73,6 +73,6 @@ test_that("default constructor works", { expect_s3_class(text2("abc", y = 1), "text2") }) -test_that("constructor types check their values", { +test_that("constructor types check their values", { expect_snapshot_error(new_class("foo", parent = integer)("abc")) }) diff --git a/tests/testthat/test-method.R b/tests/testthat/test-method.R index 674954b4..4a05b61f 100644 --- a/tests/testthat/test-method.R +++ b/tests/testthat/test-method.R @@ -112,7 +112,7 @@ test_that("next_method works for single dispatch", { foo <- new_generic("foo", dispatch_args = "x") new_method(foo, text, function(x, ...) { - x@.data <- paste0("foo-", r7_data(x)) + r7_data(x) <- paste0("foo-", r7_data(x)) }) new_method(foo, "character", function(x, ...) { as.character(x) diff --git a/tests/testthat/test-object.R b/tests/testthat/test-object.R index d13df528..48afad00 100644 --- a/tests/testthat/test-object.R +++ b/tests/testthat/test-object.R @@ -18,6 +18,10 @@ test_that("Additional attributes storing properties defined by the class, access }) describe("new_object", { + it("reports all property type errors", { + expect_snapshot(range(start = "x", end = "y"), error = TRUE) + }) + it("checks new objects for validity", { expect_error(range(start = 10, end = 1), "`end` must be greater than or equal to `start`") }) @@ -27,13 +31,6 @@ describe("new_object", { expect_equal(r7_data(y), as_class("character")("foo")) }) - it("errors if given an invalid property", { - expect_error( - range(1, "foo"), - "must be of class" - ) - }) - it("checks are arguments are properties", { expect_snapshot(error = TRUE, { foo <- new_class("foo") diff --git a/tests/testthat/test-property.R b/tests/testthat/test-property.R index 84b8a22e..e5fe8154 100644 --- a/tests/testthat/test-property.R +++ b/tests/testthat/test-property.R @@ -36,6 +36,10 @@ describe("prop<-", { prop(x, "start") <- 2 expect_equal(prop(x, "start"), 2) }) + it("errors if the property doesn't exist", { + x <- range(1, 10) + expect_snapshot(error = TRUE, x@foo <- 10) + }) it("errors if the value does not match the correct class", { x <- range(1, 10) expect_error( @@ -50,6 +54,15 @@ describe("prop<-", { }) }) +describe("props<-", { + it("validates after setting all properties", { + x <- range(1, 2) + props(x) <- list(start = 5, end = 10) + expect_equal(x@start, 5) + expect_equal(x@end, 10) + }) +}) + describe("@", { it("retrieves the property", { x <- range(1, 10) diff --git a/tests/testthat/test-valid.R b/tests/testthat/test-valid.R index 9f7f2f67..f0703301 100644 --- a/tests/testthat/test-valid.R +++ b/tests/testthat/test-valid.R @@ -1,12 +1,41 @@ -test_that("validate calls the validation function", { - obj <- range(1, 10) - # Use attr to set the property - attr(obj, "start") <- 11 - - expect_error( - validate(obj), - "must be greater than" +test_that("validate() validates object and type recursively", { + klass <- new_class("klass", + properties = list(x = "double", y = "double"), + validator = function(object) { + c( + if (object@x < 0) "x must be positive", + if (object@y > 0) "y must be negative" + ) + } ) + + expect_snapshot(error = TRUE, { + obj <- klass(1, -1) + attr(obj, "x") <- -1 + validate(obj) + + attr(obj, "x") <- "y" + validate(obj) + }) + + klass2 <- new_class("klass2", parent = klass, properties = list(z = "double")) + expect_snapshot(error = TRUE, { + obj <- klass2(1, -1, 1) + attr(obj, "x") <- -1 + validate(obj) + + attr(obj, "x") <- "y" + attr(obj, "z") <- "y" + validate(obj) + }) +}) + +test_that("validate checks base type", { + Double <- new_class("Double", parent = "double") + x <- Double(10) + mode(x) <- "character" + + expect_snapshot(error = TRUE, validate(x)) }) test_that("valid eventually calls the validation function only at the end", {