Different conversion rule between Option<T> vs T

[yutannihilation]

Was this intentional? Sorry if I forget some discussion on this...

rextendr::rust_function(r"(
fn f1(x: i32) -> i32 {
    x
}
)", patch.crates_io = list(`extendr-api` = list(git = "https://github.com/extendr/extendr"))
)
#> ℹ build directory: '/tmp/RtmpVy1jIt/file4bcab43332e1e'
#> ✓ Writing '/tmp/RtmpVy1jIt/file4bcab43332e1e/target/extendr_wrappers.R'.

rextendr::rust_function(r"(
fn f2(x: Option<i32>) -> i32 {
    if let Some(x) = x {
        x
    } else {
        1
    }
}
)", patch.crates_io = list(`extendr-api` = list(git = "https://github.com/extendr/extendr"))
)
#> ℹ build directory: '/tmp/RtmpVy1jIt/file4bcab43332e1e'
#> ✓ Writing '/tmp/RtmpVy1jIt/file4bcab43332e1e/target/extendr_wrappers.R'.

# without option, tolerant to numeric
f1(1)
#> [1] 1

# with option, intolerant
f2(1)
#> Error in f2(1): expected an integer scalar

^{Created on 2021-06-14 by the reprex package (v2.0.0)}

[clauswilke]

Seems inconsistent to me. I've added this to the 0.3 milestone.

[andy-thomason]

I'll take a look when I'm back online.

On Mon, 14 Jun 2021, 15:02 Claus Wilke, @.***> wrote:

Seems inconsistent to me. I've added this to the 0.3 milestone.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/extendr/extendr/issues/219#issuecomment-860709478, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL36XB23NYYLXFCKSUKFLDTSYDVXANCNFSM46VFYR5A .

[Ilia-Kosenkov]

There is a certain degree of inconsistency in type conversion and error reporting. Here is an example

rextendr::rust_function("fn test_na(x : i32) -> i32 {x}")
#> i build directory: 'C:\Users\...\AppData\Local\Temp\RtmpaGV9dI\file1c845d623969'
#> v Writing 'C:/Users/.../AppData/Local/Temp/RtmpaGV9dI/file1c845d623969/target/extendr_wrappers.R'.
test_na(NA)
#> Error in test_na(NA): unable to convert R object to primitive
test_na(NA_integer_)
#> Error in test_na(NA_integer_): Input must not be NA.
test_na(NA_real_)
#> Error in test_na(NA_real_): Input must not be NA.

The rules for regular types are broad (I personally not a fan of). Yet for Option<T> it is much more strict. I am not sure we need to have broad type conversions at the R-Rust border. Here is how {vctrs} treats types and casts

vctrs::vec_is(1L, double())
#> [1] FALSE
vctrs::vec_is(1, integer())
#> [1] FALSE
vctrs::vec_cast(1L, double())
#> [1] 1
vctrs::vec_cast(1, integer())
#> [1] 1
vctrs::vec_cast(1.5, integer())
#> Error: Can't convert from <double> to <integer> due to loss of precision.
#> * Locations: 1

rextendr::rust_function("fn idx(x : i32) -> i32 {x}")
#> i build directory: 'C:\Users\...\AppData\Local\Temp\RtmpI9PXVO\file2134e5b5553'
#> v Writing 'C:/Users/.../AppData/Local/Temp/RtmpI9PXVO/file2134e5b5553/target/extendr_wrappers.R'.
idx(1.5)
#> [1] 1

I find this behavior unbelievably dangerous in real-life applications.

[andy-thomason]

I would like to shift all the fromrobj to tryfrom in a consistent way.

On Tue, 15 Jun 2021, 08:46 Ilia, @.***> wrote:

There is a certain degree of inconsistency in type conversion and error reporting. Here is an example

rextendr::rust_function("fn test_na(x : i32) -> i32 {x}")#> i build directory: 'C:\Users...\AppData\Local\Temp\RtmpaGV9dI\file1c845d623969'#> v Writing 'C:/Users/.../AppData/Local/Temp/RtmpaGV9dI/file1c845d623969/target/extendr_wrappers.R'. test_na(NA)#> Error in test_na(NA): unable to convert R object to primitive test_na(NAinteger)#> Error in test_na(NAinteger): Input must not be NA. test_na(NAreal)#> Error in test_na(NAreal): Input must not be NA.

The rules for regular types are broad (I personally not a fan of). Yet for Option it is much more strict. I am not sure we need to have broad type conversions at the R-Rust border. Here is how {vctrs} treats types and casts

vctrs::vec_is(1L, double())#> [1] FALSEvctrs::vec_is(1, integer())#> [1] FALSEvctrs::vec_cast(1L, double())#> [1] 1vctrs::vec_cast(1, integer())#> [1] 1vctrs::vec_cast(1.5, integer())#> Error: Can't convert from to due to loss of precision.#> * Locations: 1 rextendr::rust_function("fn idx(x : i32) -> i32 {x}")#> i build directory: 'C:\Users...\AppData\Local\Temp\RtmpI9PXVO\file2134e5b5553'#> v Writing 'C:/Users/.../AppData/Local/Temp/RtmpI9PXVO/file2134e5b5553/target/extendr_wrappers.R'. idx(1.5)#> [1] 1

I find this behavior unbelievably dangerous in real-life applications.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/extendr/extendr/issues/219#issuecomment-861267620, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL36XDRM7AEGZZLL6TFTTDTS4ANFANCNFSM46VFYR5A .

[yutannihilation]

shift all the fromrobj to tryfrom

Thanks, I just remember you commented this on #190. I'll try figuring out the direction...

Each wrapper implements:

A "from_xxx" impl which constructs the type from components.

A FromRobj impl for legacy parameters. A TryFrom impl for new style of parameters. A From impl for returning values.

We hope to migrate from FromRobj to TryFrom in due course. The TryFrom uses the official extendr Error type to return failure if the object type is unexpected.

All the as_* impls in Robj can be replaced by try_from versions we can write a macro to do this.

The names of the is_* impls should match the type.

[yutannihilation]

Here's the result of cargo expand. We are currently using <T>::from_robj(&_x_robj)) to convert Robj to a Rust type. So, are we replacing this with <T>::try_from(&_x_robj))?

fn f1(x: i32) -> i32 {
    x
}
#[no_mangle]
#[allow(non_snake_case)]
pub extern "C" fn wrap__f1(x: extendr_api::SEXP) -> extendr_api::SEXP {
    unsafe {
        use extendr_api::robj::*;
        let _x_robj = extendr_api::new_owned(x);
        extendr_api::handle_panic("f1 panicked.\u{0}", || {
            extendr_api::Robj::from(f1(extendr_api::unwrap_or_throw(<i32>::from_robj(&_x_robj))))
                .get()
        })
    }
}
#[allow(non_snake_case)]
fn meta__f1(metadata: &mut Vec<extendr_api::metadata::Func>) {
    let mut args = <[_]>::into_vec(box [extendr_api::metadata::Arg {
        name: "x",
        arg_type: "i32",
    }]);
    metadata.push(extendr_api::metadata::Func {
        doc: "",
        name: "f1",
        args: args,
        return_type: "i32",
        func_ptr: wrap__f1 as *const u8,
        hidden: false,
    })
}

[andy-thomason]

I would love to convert all the Robj::from (alias FromRobj) conversions to try_from.

We also have a new error type which can return details about failures, rather than just text messages.

[brendanrbrown]

I would love to convert all the Robj::from (alias FromRobj) conversions to try_from.

Hello all -- If there is room for and interest in getting some assistance with this I would be glad to do so. I am new (~3 mo.) to extendr, still somewhat new to rust, and I think what you've done so far is great. This particular issue has cropped up in the little toy utilities I've created with extendr and rextendr for personal projects, so I'm interested in it.

It seems like it would require time, thought and effort but perhaps would not be too technically challenging. If I'm mistaken and this should be handled by someone more experienced, no problem.

[clauswilke]

@brendanrbrown You're welcome to submit a PR. I would suggest to start with something small, then we can review and go from there.

[andy-thomason]

Very welcome.

There are quite a few shortcuts we can do such as generating macros to handle whole classes of types. A lot of the newer wrappers already have try_from conversions, but primitive types need an overhaul.

The Rust type matching is much stricter than C++, which makes it more challenging to write generalised cases as they must be unique. C++ will just make the "best guess" which often differs by implementation.

TryFrom returns a specific error type which we can customise to give better user experience when errors occur. There should be some examples, especially in newer code.

Feel free to chat about this.

[brendanrbrown]

great, thanks all. will fork etc., look at existing conversions, play a bit, introduce myself on discord, and go from there. probably would need to file a new issue since this one is not precisely about try_from, but we can discuss.

[andy-thomason]

I can add an option to the extendr macro to use try_from. This should form a bridge to enable testing of the new method.

I'll raise a PR for this.

[Ilia-Kosenkov]

@andy-thomason, If migration to try_form takes a lot of time & effort, would it be possible to create an issue listing what needs to be done? In that way, contributors can grab small chunks and implement changes, while you orchestrate everything? This will save time and effort for you (for more important extendr features) while occupying us, contributors.

[andy-thomason]

Good idea. I think we are most of the way there, but any feedback will be welcome. I'll try to make a list of everything we may need to convert and what the error conditions and messages should be.

I've made a new PR #222 to try out the new method without switching fully. I'll merge his one ASAP so you can create new PRs from this.

Expanding the new test would be very welcome for everyone. More tests are never a bad thing...

[andy-thomason]

@brendanrbrown @Ilia-Kosenkov Have a look at the new PR which I will merge shortly.

If we can all contribute some tests, even if they do not pass yet, it would be very useful for nailing down the conversion behaviour.

[andy-thomason]

The changes are merged, so I'll add some smaller issues based on your feedback.

[yutannihilation]

I'm closing this issue as the original problem is now solved by TryFrom implementation. Thanks @brendanrbrown!

rextendr::rust_source(code = r"(
#[extendr(use_try_from = true)]
fn f1(x: i32) -> i32 {
    x
}
)", patch.crates_io = list(`extendr-api` = list(git = "https://github.com/extendr/extendr"))
)
#> ℹ build directory: '/tmp/RtmpTjqi4J/file2265b2dec007d'
#> ✓ Writing '/tmp/RtmpTjqi4J/file2265b2dec007d/target/extendr_wrappers.R'.

rextendr::rust_source(code = r"(
#[extendr(use_try_from = true)]
fn f2(x: Option<i32>) -> i32 {
    if let Some(x) = x {
        x
    } else {
        1
    }
}
)", patch.crates_io = list(`extendr-api` = list(git = "https://github.com/extendr/extendr"))
)
#> ℹ build directory: '/tmp/RtmpTjqi4J/file2265b2dec007d'
#> ✓ Writing '/tmp/RtmpTjqi4J/file2265b2dec007d/target/extendr_wrappers.R'.

f1(1)
#> [1] 1

f2(1)
#> [1] 1

^{Created on 2021-07-24 by the reprex package (v2.0.0)}