Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Check length of input in <&str>::try_from() #226

Conversation

yutannihilation
Copy link
Contributor

Fix #102

While other try_from() implementations use the following logic, &str is a bit tricky in that it fails with cannot return value referencing local variable error,

        if let Some(v) = robj.as_<type>_slice() {
            match v.len() {
                0 => Err(Error::ExpectedNonZeroLength(robj)),
                1 => Ok(v[0]),
                _ => Err(Error::ExpectedScalar(robj)),
            }
        } else {
            Err(Error::ExpectedLogical(robj))
        }

so I chose to check the length before trying to convert the input to the actual variable.

Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

However, shouldn't this require changes in the integration tests, here:

expect_error(char_scalar(c("hello", "world")), "not a string object") # why this error message and not "Input must be of length 1"?

@yutannihilation
Copy link
Contributor Author

In my understanding, try_from() isn't used yet by #[extendr] macro. It still uses from_robj() (c.f. #219 (comment)). I'm wondering if I should modify from_robj() as well at the moment, considering it is still used but will be replaced...

@clauswilke
Copy link
Member

Ah, Ok. We probably need a complete implementation of try_from() for all types before we can switch over. It's not quite clear to me whether we're there yet or not.

@yutannihilation
Copy link
Contributor Author

In my understanding, try_from() isn't used yet by #[extendr] macro.

Sorry, I was a bit confused. We can confirm the behavior with #[extendr(use_try_from = true)].

gen_fun <- function(fun_name, use_try_from) {
  use_try_from <- if(isTRUE(use_try_from)) "true" else "false"
  code <- r"(
#[extendr(use_try_from = {use_try_from})]
fn {fun_name}(x: &str) {{
    println!("{{}}", x)
}}
)"

  rextendr::rust_source(
    code = glue::glue(code),
    patch.crates_io = list(`extendr-api` = list(git = "https://github.com/yutannihilation/extendr", branch = "fix/issue-102-improve-string-scalar-error")),
    env = globalenv()
  )
}

# Use try_from impl
gen_fun("f1", TRUE)
#> ℹ build directory: '/tmp/Rtmp9mx0FQ/file2b2fb60e771c1'
#> ✓ Writing '/tmp/Rtmp9mx0FQ/file2b2fb60e771c1/target/extendr_wrappers.R'.
# Use from_robj impl
gen_fun("f2", FALSE)
#> ℹ build directory: '/tmp/Rtmp9mx0FQ/file2b2fb60e771c1'
#> ✓ Writing '/tmp/Rtmp9mx0FQ/file2b2fb60e771c1/target/extendr_wrappers.R'.

f1(letters)
#> Error in f1(letters): Expected Scalar, got String
f2(letters)
#> Error in f2(letters): Not a string object.

Created on 2021-07-18 by the reprex package (v2.0.0)

@andy-thomason andy-thomason merged commit f523333 into extendr:master Jul 19, 2021
@brendanrbrown brendanrbrown mentioned this pull request Jul 20, 2021
@yutannihilation yutannihilation deleted the fix/issue-102-improve-string-scalar-error branch July 22, 2021 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conversion of string vector to string scalar fails with "not a string object"
3 participants