From 3cfb6f6051344de218f6cac87b0f0cd2b199c1cc Mon Sep 17 00:00:00 2001 From: Hiroaki Yutani Date: Sun, 20 Oct 2024 20:56:06 +0900 Subject: [PATCH] Remove `TryFrom for usize` and add `NumericScalar::as_usize()`, `NumericSexp::iter_usize()` (#311) --- CHANGELOG.md | 46 ++++++++++++ R-package/R/000-wrappers.R | 15 ++-- R-package/src/init.c | 18 +++-- R-package/src/rust/api.h | 3 +- R-package/src/rust/src/complex.rs | 4 +- .../src/rust/src/convert_from_rust_types.rs | 6 -- R-package/src/rust/src/init_vectors.rs | 12 ++-- R-package/src/rust/src/numeric.rs | 17 ++++- .../tests/testthat/test-from-rust-types.R | 3 - R-package/tests/testthat/test-numeric.R | 44 ++++++++++++ book/src/atomic_types.md | 7 +- book/src/scalar.md | 19 ++++- src/sexp/numeric.rs | 72 +++++++++++++++++-- src/sexp/scalar.rs | 9 --- 14 files changed, 229 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3353628..5aa90290 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,52 @@ ## [Unreleased] (ReleaseDate) +### Breaking Change + +Removed `TryFrom for usize`, so the following code no longer compiles. + +```rust +#[savvy] +fn foo(x: usize) -> savvy::Result<()> { + ... +} +``` + +Instead, you can use `i32` and convert it to `usize` by yourself. If you are +sure the input number is never negative, you can just use the `as` conversion. +If you are not sure, you should use `::try_from()` and handle the error +by yourself. Also, please be aware you need to handle NA as well. + +```rust +#[savvy] +fn foo(x: i32) -> savvy::Result<()> { + if x.is_na() { + return Err("cannot convert NA to usize".into())?; + } + + let x = ::try_from(x).map_err(|e| e.to_string().into()); + + ... +} +``` + +Alternatively, you can use newly-added methods, `NumericScalar::as_usize()` and +`NumericSexp::iter_usize()`. What's good is that this can handle integer-ish +numeric, which means you can allow users to input a larger number than the +integer max (2147483647)! + +```rust +fn usize_to_string_scalar(x: NumericScalar) -> savvy::Result { + let x_usize = x.as_usize()?; + x_usize.to_string().try_into() +} +``` + +```r +usize_to_string_scalar(2147483648) +#> [1] "2147483648" +``` + ## [v0.6.8] (2024-09-17) ### Minor Improvements diff --git a/R-package/R/000-wrappers.R b/R-package/R/000-wrappers.R index 1f5d6a5b..dfaa52b1 100644 --- a/R-package/R/000-wrappers.R +++ b/R-package/R/000-wrappers.R @@ -337,11 +337,6 @@ NULL } -`scalar_input_usize` <- function(`x`) { - invisible(.Call(savvy_scalar_input_usize__impl, `x`)) -} - - `scalar_input_real` <- function(`x`) { invisible(.Call(savvy_scalar_input_real__impl, `x`)) } @@ -554,6 +549,11 @@ NULL } +`usize_to_string` <- function(`x`) { + .Call(savvy_usize_to_string__impl, `x`) +} + + `times_two_numeric_f64_scalar` <- function(`x`) { .Call(savvy_times_two_numeric_f64_scalar__impl, `x`) } @@ -564,6 +564,11 @@ NULL } +`usize_to_string_scalar` <- function(`x`) { + .Call(savvy_usize_to_string_scalar__impl, `x`) +} + + `print_numeric` <- function(`x`) { invisible(.Call(savvy_print_numeric__impl, `x`)) } diff --git a/R-package/src/init.c b/R-package/src/init.c index b84cdd27..e83de904 100644 --- a/R-package/src/init.c +++ b/R-package/src/init.c @@ -304,11 +304,6 @@ SEXP savvy_scalar_input_int__impl(SEXP c_arg__x) { return handle_result(res); } -SEXP savvy_scalar_input_usize__impl(SEXP c_arg__x) { - SEXP res = savvy_scalar_input_usize__ffi(c_arg__x); - return handle_result(res); -} - SEXP savvy_scalar_input_real__impl(SEXP c_arg__x) { SEXP res = savvy_scalar_input_real__ffi(c_arg__x); return handle_result(res); @@ -524,6 +519,11 @@ SEXP savvy_times_two_numeric_i32__impl(SEXP c_arg__x) { return handle_result(res); } +SEXP savvy_usize_to_string__impl(SEXP c_arg__x) { + SEXP res = savvy_usize_to_string__ffi(c_arg__x); + return handle_result(res); +} + SEXP savvy_times_two_numeric_f64_scalar__impl(SEXP c_arg__x) { SEXP res = savvy_times_two_numeric_f64_scalar__ffi(c_arg__x); return handle_result(res); @@ -534,6 +534,11 @@ SEXP savvy_times_two_numeric_i32_scalar__impl(SEXP c_arg__x) { return handle_result(res); } +SEXP savvy_usize_to_string_scalar__impl(SEXP c_arg__x) { + SEXP res = savvy_usize_to_string_scalar__ffi(c_arg__x); + return handle_result(res); +} + SEXP savvy_print_numeric__impl(SEXP c_arg__x) { SEXP res = savvy_print_numeric__ffi(c_arg__x); return handle_result(res); @@ -770,7 +775,6 @@ static const R_CallMethodDef CallEntries[] = { {"savvy_abs_complex__impl", (DL_FUNC) &savvy_abs_complex__impl, 1}, {"savvy_new_value_pair__impl", (DL_FUNC) &savvy_new_value_pair__impl, 2}, {"savvy_scalar_input_int__impl", (DL_FUNC) &savvy_scalar_input_int__impl, 1}, - {"savvy_scalar_input_usize__impl", (DL_FUNC) &savvy_scalar_input_usize__impl, 1}, {"savvy_scalar_input_real__impl", (DL_FUNC) &savvy_scalar_input_real__impl, 1}, {"savvy_scalar_input_logical__impl", (DL_FUNC) &savvy_scalar_input_logical__impl, 1}, {"savvy_scalar_input_string__impl", (DL_FUNC) &savvy_scalar_input_string__impl, 1}, @@ -813,8 +817,10 @@ static const R_CallMethodDef CallEntries[] = { {"savvy_new_bool__impl", (DL_FUNC) &savvy_new_bool__impl, 1}, {"savvy_times_two_numeric_f64__impl", (DL_FUNC) &savvy_times_two_numeric_f64__impl, 1}, {"savvy_times_two_numeric_i32__impl", (DL_FUNC) &savvy_times_two_numeric_i32__impl, 1}, + {"savvy_usize_to_string__impl", (DL_FUNC) &savvy_usize_to_string__impl, 1}, {"savvy_times_two_numeric_f64_scalar__impl", (DL_FUNC) &savvy_times_two_numeric_f64_scalar__impl, 1}, {"savvy_times_two_numeric_i32_scalar__impl", (DL_FUNC) &savvy_times_two_numeric_i32_scalar__impl, 1}, + {"savvy_usize_to_string_scalar__impl", (DL_FUNC) &savvy_usize_to_string_scalar__impl, 1}, {"savvy_print_numeric__impl", (DL_FUNC) &savvy_print_numeric__impl, 1}, {"savvy_default_value_scalar__impl", (DL_FUNC) &savvy_default_value_scalar__impl, 1}, {"savvy_default_value_vec__impl", (DL_FUNC) &savvy_default_value_vec__impl, 1}, diff --git a/R-package/src/rust/api.h b/R-package/src/rust/api.h index bd7c40c1..86aaa471 100644 --- a/R-package/src/rust/api.h +++ b/R-package/src/rust/api.h @@ -52,7 +52,6 @@ SEXP savvy_first_complex__ffi(SEXP c_arg__x); SEXP savvy_abs_complex__ffi(SEXP c_arg__x); SEXP savvy_new_value_pair__ffi(SEXP c_arg__a, SEXP c_arg__b); SEXP savvy_scalar_input_int__ffi(SEXP c_arg__x); -SEXP savvy_scalar_input_usize__ffi(SEXP c_arg__x); SEXP savvy_scalar_input_real__ffi(SEXP c_arg__x); SEXP savvy_scalar_input_logical__ffi(SEXP c_arg__x); SEXP savvy_scalar_input_string__ffi(SEXP c_arg__x); @@ -96,8 +95,10 @@ SEXP savvy_new_real__ffi(SEXP c_arg__size); SEXP savvy_new_bool__ffi(SEXP c_arg__size); SEXP savvy_times_two_numeric_f64__ffi(SEXP c_arg__x); SEXP savvy_times_two_numeric_i32__ffi(SEXP c_arg__x); +SEXP savvy_usize_to_string__ffi(SEXP c_arg__x); SEXP savvy_times_two_numeric_f64_scalar__ffi(SEXP c_arg__x); SEXP savvy_times_two_numeric_i32_scalar__ffi(SEXP c_arg__x); +SEXP savvy_usize_to_string_scalar__ffi(SEXP c_arg__x); SEXP savvy_print_numeric__ffi(SEXP c_arg__x); SEXP savvy_default_value_scalar__ffi(SEXP c_arg__x); SEXP savvy_default_value_vec__ffi(SEXP c_arg__x); diff --git a/R-package/src/rust/src/complex.rs b/R-package/src/rust/src/complex.rs index 4ed26018..85ba9a71 100644 --- a/R-package/src/rust/src/complex.rs +++ b/R-package/src/rust/src/complex.rs @@ -2,8 +2,8 @@ use savvy::savvy; use savvy::NotAvailableValue; #[savvy] -fn new_complex(size: usize) -> savvy::Result { - savvy::OwnedComplexSexp::new(size)?.into() +fn new_complex(size: i32) -> savvy::Result { + savvy::OwnedComplexSexp::new(size as usize)?.into() } #[savvy] diff --git a/R-package/src/rust/src/convert_from_rust_types.rs b/R-package/src/rust/src/convert_from_rust_types.rs index 67acbba9..afabf6ae 100644 --- a/R-package/src/rust/src/convert_from_rust_types.rs +++ b/R-package/src/rust/src/convert_from_rust_types.rs @@ -11,12 +11,6 @@ fn scalar_input_int(x: i32) -> savvy::Result<()> { Ok(()) } -#[savvy] -fn scalar_input_usize(x: usize) -> savvy::Result<()> { - savvy::r_println!("{x}"); - Ok(()) -} - #[savvy] fn scalar_input_real(x: f64) -> savvy::Result<()> { savvy::r_println!("{x}"); diff --git a/R-package/src/rust/src/init_vectors.rs b/R-package/src/rust/src/init_vectors.rs index cb1ec054..5e2e4e7e 100644 --- a/R-package/src/rust/src/init_vectors.rs +++ b/R-package/src/rust/src/init_vectors.rs @@ -1,16 +1,16 @@ use savvy::savvy; #[savvy] -fn new_int(size: usize) -> savvy::Result { - savvy::OwnedIntegerSexp::new(size)?.into() +fn new_int(size: i32) -> savvy::Result { + savvy::OwnedIntegerSexp::new(size as usize)?.into() } #[savvy] -fn new_real(size: usize) -> savvy::Result { - savvy::OwnedRealSexp::new(size)?.into() +fn new_real(size: i32) -> savvy::Result { + savvy::OwnedRealSexp::new(size as usize)?.into() } #[savvy] -fn new_bool(size: usize) -> savvy::Result { - savvy::OwnedLogicalSexp::new(size)?.into() +fn new_bool(size: i32) -> savvy::Result { + savvy::OwnedLogicalSexp::new(size as usize)?.into() } diff --git a/R-package/src/rust/src/numeric.rs b/R-package/src/rust/src/numeric.rs index 91f0c2ee..7928c7ac 100644 --- a/R-package/src/rust/src/numeric.rs +++ b/R-package/src/rust/src/numeric.rs @@ -1,6 +1,6 @@ use savvy::{ r_println, savvy, NotAvailableValue, NumericScalar, NumericSexp, NumericTypedSexp, - OwnedIntegerSexp, OwnedRealSexp, Sexp, + OwnedIntegerSexp, OwnedRealSexp, OwnedStringSexp, Sexp, }; #[savvy] @@ -34,6 +34,16 @@ fn times_two_numeric_i32(x: NumericSexp) -> savvy::Result { out.into() } +#[savvy] +fn usize_to_string(x: NumericSexp) -> savvy::Result { + let mut out = OwnedStringSexp::new(x.len())?; + for (i, v) in x.iter_usize().enumerate() { + out.set_elt(i, &v?.to_string())?; + } + + out.into() +} + #[savvy] fn times_two_numeric_f64_scalar(x: NumericScalar) -> savvy::Result { let v = x.as_f64(); @@ -54,6 +64,11 @@ fn times_two_numeric_i32_scalar(x: NumericScalar) -> savvy::Result { } } +#[savvy] +fn usize_to_string_scalar(x: NumericScalar) -> savvy::Result { + x.as_usize()?.to_string().try_into() +} + #[savvy] fn print_numeric(x: NumericSexp) -> savvy::Result<()> { match x.into_typed() { diff --git a/R-package/tests/testthat/test-from-rust-types.R b/R-package/tests/testthat/test-from-rust-types.R index 6ac7c0e5..e44c3047 100644 --- a/R-package/tests/testthat/test-from-rust-types.R +++ b/R-package/tests/testthat/test-from-rust-types.R @@ -1,15 +1,12 @@ test_that("scalar functions reject non-scalar values and missing values", { # no error expect_output(scalar_input_int(1L), "1") - expect_output(scalar_input_usize(1L), "1") - expect_output(scalar_input_usize(0L), "0") expect_output(scalar_input_real(1.3), "1.3") expect_output(scalar_input_logical(FALSE), "false") expect_output(scalar_input_string("foo"), "foo") # error expect_error(scalar_input_int(1:10)) - expect_error(scalar_input_usize(-1L)) expect_error(scalar_input_real(c(1, 2))) expect_error(scalar_input_logical(c(TRUE, FALSE))) expect_error(scalar_input_str(c("foo", "bar"))) diff --git a/R-package/tests/testthat/test-numeric.R b/R-package/tests/testthat/test-numeric.R index 943455ba..441aa712 100644 --- a/R-package/tests/testthat/test-numeric.R +++ b/R-package/tests/testthat/test-numeric.R @@ -29,6 +29,30 @@ test_that("NumericSexp works", { expect_error(times_two_numeric_i32(c(1.1, -1.1))) # not integer-ish }) +test_that("NumericSexp works for usize conversions", { + # i32 to usize + expect_equal( + usize_to_string(c(0L, 10L)), + c("0", "10") + ) + + # f64 to usize + expect_equal( + # 2147483647 = .Machine$integer.max + usize_to_string(c(0.0, 10.0, 2147483648.0)), + c("0", "10", "2147483648") + ) + + # error cases + expect_error(usize_to_string(NA_integer_)) + expect_error(usize_to_string(NA_real_)) + expect_error(usize_to_string(Inf)) + expect_error(usize_to_string(NaN)) + expect_error(usize_to_string(-1L)) + expect_error(usize_to_string(-1.0)) +}) + + test_that("NumericScalar works", { expect_equal(times_two_numeric_f64_scalar(1L), 2) expect_equal(times_two_numeric_f64_scalar(1), 2) @@ -46,3 +70,23 @@ test_that("NumericScalar works", { expect_error(times_two_numeric_i32_scalar(2147483648)) # out of i32's range expect_error(times_two_numeric_i32_scalar(1.1)) # not integer-ish }) + +test_that("NumericScalar works for usize conversions", { + # i32 to usize + expect_equal(usize_to_string_scalar(0L), "0") + expect_equal(usize_to_string_scalar(10L), "10") + + # f64 to usize + expect_equal(usize_to_string_scalar(0.0), "0") + expect_equal(usize_to_string_scalar(10.0), "10") + # 2147483647 = .Machine$integer.max + expect_equal(usize_to_string_scalar(2147483648.0), "2147483648") + + # error cases + expect_error(usize_to_string_scalar(NA_integer_)) + expect_error(usize_to_string_scalar(NA_real_)) + expect_error(usize_to_string_scalar(Inf)) + expect_error(usize_to_string_scalar(NaN)) + expect_error(usize_to_string_scalar(-1L)) + expect_error(usize_to_string_scalar(-1.0)) +}) diff --git a/book/src/atomic_types.md b/book/src/atomic_types.md index 4ab8779e..7f71ec7c 100644 --- a/book/src/atomic_types.md +++ b/book/src/atomic_types.md @@ -84,7 +84,10 @@ it fails when any of the values is - out of range for `i32` - not integer-ish (e.g. `1.1`) -For example, you can rewrite the above function like this: +For convenience, `NumericSexp` also provides `iter_usize()`, which returns an +iterator of `Result`. + +With `NumericSexp`, you can rewrite the above `times_two` function like this: ```rust #[savvy] @@ -104,7 +107,7 @@ fn times_two(x: NumericSexp) -> savvy::Result { } ``` -Another way is to use `.into_typed()` and `match` the result to apply an +Alternatively, you can use `.into_typed()` and `match` the result to apply an appropriate function depneding on the type. In this case, you need to define two different functions, but this might be useful when the logic is very different for integer values and real values. diff --git a/book/src/scalar.md b/book/src/scalar.md index 83f57720..8dde7b47 100644 --- a/book/src/scalar.md +++ b/book/src/scalar.md @@ -43,13 +43,30 @@ fn times_two_numeric_i32_scalar(x: NumericScalar) -> savvy::Result { } ``` -Note that, while `as_f64()` is infallible, `as_i32()` can fail when the +Note that, while `as_f64()` is infallible, `as_i32()` can fail when the conversion is from `f64` to `i32` and - the value is `Inf` or `-Inf` - the value is out of range for `i32` - the value is not integer-ish (e.g. `1.1`) +For convenience, `NumericScalar` also provides a conversion to usize by +`as_usize()`. What's good is that this can handle integer-ish numeric, which +means you can allow users to input a larger number than the integer max +(2147483647)! + +```rust +fn usize_to_string_scalar(x: NumericScalar) -> savvy::Result { + let x_usize = x.as_usize()?; + x_usize.to_string().try_into() +} +``` + +```r +usize_to_string_scalar(2147483648) +#> [1] "2147483648" +``` + ## Output Just like a Rust vector, a Rust scalar value can be converted into `Sexp` by diff --git a/src/sexp/numeric.rs b/src/sexp/numeric.rs index bc4a85e9..957a168f 100644 --- a/src/sexp/numeric.rs +++ b/src/sexp/numeric.rs @@ -6,6 +6,7 @@ use crate::{IntegerSexp, NotAvailableValue, RealSexp, Sexp}; const I32MAX: f64 = i32::MAX as f64; const I32MIN: f64 = i32::MIN as f64; +const USIZEMAX: f64 = usize::MAX as f64; const TOLERANCE: f64 = 0.01; // This is super-tolerant than vctrs, but this should be sufficient. fn try_cast_f64_to_i32(f: f64) -> crate::Result { @@ -28,6 +29,26 @@ fn cast_i32_to_f64(i: i32) -> f64 { } } +fn try_cast_i32_to_usize(i: i32) -> crate::error::Result { + if i.is_na() { + Err("cannot convert NA to usize".into()) + } else { + ::try_from(i).map_err(|e| e.to_string().into()) + } +} + +fn try_cast_f64_to_usize(f: f64) -> crate::Result { + if f.is_na() || f.is_nan() { + Err("cannot convert NA or NaN to usize".into()) + } else if f.is_infinite() || !(0f64..=USIZEMAX).contains(&f) { + Err(format!("{f:?} is out of range for usize").into()) + } else if (f - f.round()).abs() > TOLERANCE { + Err(format!("{f:?} is not integer-ish").into()) + } else { + Ok(f as usize) + } +} + // --- Vector ------------------------- /// A enum to hold both the original data and the converted version. Since it @@ -280,10 +301,19 @@ impl NumericSexp { } } + /// Returns an iterator over the underlying data of the SEXP. + pub fn iter_usize(&self) -> NumericIteratorUsize { + NumericIteratorUsize { + sexp: self, + i: 0, + len: self.len(), + } + } + // Note: If the conversion is needed, to_vec_*() would copy the values twice // because it creates a `Vec` from to_slice(). This is inefficient, but I'm - // not sure which is worse to alwaays creates a `Vec` from scratch or use - // the cached one. So, I chose not to implement the method. + // not sure which is worse to always creates a `Vec` from scratch or use the + // cached one. So, I chose not to implement the method. } impl TryFrom for NumericSexp { @@ -336,7 +366,7 @@ impl TryFrom for NumericSexp { // --- Iterator ----------------------- -/// An iterator that retuns `i32` wrapped with `Result`. +/// An iterator that returns `i32` wrapped with `Result`. /// /// - If the underlying data is integer, use the value as it is. /// - If the underlying data is real, but there's already the `i32` values @@ -373,7 +403,7 @@ impl<'a> Iterator for NumericIteratorI32<'a> { } } -/// An iterator that retuns `f64`. +/// An iterator that returns `f64`. /// /// - If the underlying data is real, use the value as it is. /// - If the underlying data is integer, but there's already the `f64` values @@ -410,6 +440,33 @@ impl<'a> Iterator for NumericIteratorF64<'a> { } } +/// An iterator that returns `usize` wrapped with `Result`. +pub struct NumericIteratorUsize<'a> { + sexp: &'a NumericSexp, + i: usize, + len: usize, +} + +impl<'a> Iterator for NumericIteratorUsize<'a> { + type Item = crate::error::Result; + + fn next(&mut self) -> Option { + let i = self.i; + self.i += 1; + + if i >= self.len { + return None; + } + + let elem = match &self.sexp.0 { + PrivateNumericSexp::Integer { orig, .. } => try_cast_i32_to_usize(orig.as_slice()[i]), + PrivateNumericSexp::Real { orig, .. } => try_cast_f64_to_usize(orig.as_slice()[i]), + }; + + Some(elem) + } +} + // --- Scalar ------------------------- /// A struct that holds either an integer or a real scalar. @@ -442,6 +499,13 @@ impl NumericScalar { NumericScalar::Real(r) => *r, } } + + pub fn as_usize(&self) -> crate::error::Result { + match &self { + NumericScalar::Integer(i) => try_cast_i32_to_usize(*i), + NumericScalar::Real(r) => try_cast_f64_to_usize(*r), + } + } } impl TryFrom for NumericScalar { diff --git a/src/sexp/scalar.rs b/src/sexp/scalar.rs index 6a98c1da..c7a8fae6 100644 --- a/src/sexp/scalar.rs +++ b/src/sexp/scalar.rs @@ -63,12 +63,3 @@ impl TryFrom for u8 { Ok(unsafe { RAW_ELT(value.0, 0) }) } } - -impl TryFrom for usize { - type Error = crate::error::Error; - - fn try_from(value: Sexp) -> crate::error::Result { - let value = ::try_from(value)?; - ::try_from(value).map_err(|e| crate::Error::new(&e.to_string())) - } -}