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

Stricter input checking for extendr functions #64

Closed
clauswilke opened this issue Dec 6, 2020 · 7 comments
Closed

Stricter input checking for extendr functions #64

clauswilke opened this issue Dec 6, 2020 · 7 comments
Labels
good first issue Good for newcomers

Comments

@clauswilke
Copy link
Member

Related to #63, I think conversion from Robj to Rust native types should be stricter and only succeed when the coercion is unique and provably correct. As an example, vector input should not be silently converted into a single value. See reprex for an example. If input is f64, not &[f64], then only vectors of length 1 should be allowed as input.

library(rextendr)

rust_function(
  code = "fn one_float(input: f64) -> f64 {
    5.*input
  }",
  patch.crates_io = c(
    'extendr-api = { git = "https://github.com/extendr/extendr" }',
    'extendr-macros = { git = "https://github.com/extendr/extendr" }'
  )
)

# correct
one_float(numeric(0))
#> Error in one_float(numeric(0)): zero length vector

# correct
one_float(numeric(1))
#> [1] 0

# incorrect, should complain about vector of length > 1
one_float(numeric(2))
#> [1] 0

Created on 2020-12-06 by the reprex package (v0.3.0)

@andy-thomason
Copy link
Contributor

I'll try to take a look at this soon unless others want this as a starter task.

@andy-thomason andy-thomason added the good first issue Good for newcomers label Dec 6, 2020
@yutannihilation
Copy link
Contributor

yutannihilation commented Dec 8, 2020

Related. We should reject factors in the following example. cpp11 uses Rf_isInteger() (code) to check if the value can be converted to an integer, and Rf_isInteger() verifies if the value is not a factor (code).

library(rextendr)

rust_function(
  code = "fn one_int(input: i32) -> i32 {
    5*input
  }",
  patch.crates_io = c(
    'extendr-api = { git = "https://github.com/extendr/extendr" }',
    'extendr-macros = { git = "https://github.com/extendr/extendr" }'
  )
)
#> build directory: /tmp/RtmpP4IZYS/file513f5fb1b04e
#> Prebuild libR bindings are not available. Run `install_libR_bindings()` to improve future build times.

one_int(factor("10"))
#> [1] 5

Created on 2020-12-08 by the reprex package (v0.3.0)

library(cpp11)

cpp_function('int one_int(int input) {
  return 5*input;
}')

one_int(factor("10"))
#> Error: Expected single integer value

Created on 2020-12-08 by the reprex package (v0.3.0)

@yutannihilation
Copy link
Contributor

It seems I need to reconsider the comment above. Maybe the function should accept factors, as exptendr supports factors.

@clauswilke
Copy link
Member Author

I'm not sure. That's the problem with automatic type conversion. Do you want to be maximally strict or maximally lenient? My gut feeling is that automatic conversion of factors to either strings or integers is fine, but I could be wrong.

@yutannihilation
Copy link
Contributor

Ah, I see. I was a bit confused. I think conversion to strings is fine. For the one to integers, I'm not yet sure if I like it or not.

@lebensterben
Copy link
Contributor

there are different level of strictness for type checking:

  1. Don't allow any conversion
  2. Don't allow conversion of factor and integer, or Date and double. These pairs have same representation in C, but are treated differently in R.
  3. Don't allow conversion between numeric types.
  4. Don't allow conversion between atomic and vector/list types.

It's best to be configurable by users

@clauswilke
Copy link
Member Author

I feel this issue lacks focus, because it doesn't state what specific checks are currently missing. I think we should close it in favor of individual issues tackling specific conversions, such as #101 or #102.

@yutannihilation Does the factor issue you brought up still need to be addressed? If yes, could you file a separate issue about it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants