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

Use R_ParseEvalString in test #188

Merged
merged 2 commits into from
Oct 7, 2023
Merged

Use R_ParseEvalString in test #188

merged 2 commits into from
Oct 7, 2023

Conversation

CGMossa
Copy link
Member

@CGMossa CGMossa commented Oct 5, 2023

In a comment, it is stated that we'd ideally use R_ParseEvalString if we could.
Years has passed, and now all the supported versions of R contains R_ParseEvalString.
Replaced the test with it.

kevin@pop-os:~/GitHub/libR-sys$ rg -l R_ParseEvalString ./bindings/
./bindings/bindings-windows-x86_64-R4.3.rs
./bindings/bindings-macos-aarch64-R4.3.rs
./bindings/bindings-macos-aarch64-R4.2.rs
./bindings/bindings-linux-x86_64-R4.3.rs
./bindings/bindings-macos-x86_64-R4.3.rs
./bindings/bindings-linux-aarch64-R4.3.rs
./bindings/bindings-macos-x86_64-R4.2.rs
./bindings/bindings-linux-x86_64-R4.4-devel.rs
./bindings/bindings-linux-x86_64-R4.2.rs
./bindings/bindings-linux-aarch64-R4.2.rs
./bindings/bindings-linux-aarch64-R4.4-devel.rs
./bindings/bindings-windows-x86_64-R4.4-devel.rs
./bindings/bindings-macos-x86_64-R4.4-devel.rs
./bindings/bindings-windows-x86_64-R4.2.rs

@CGMossa
Copy link
Member Author

CGMossa commented Oct 5, 2023

I don't know why this is passing tests.

If I run this

#[test]
fn test_parse_eval_string() {
    with_r(|| unsafe {
        let bad_code = cstr!("c(10,42,20)");
        let bad = Rf_protect(R_ParseEvalString(bad_code.as_ptr(), R_GlobalEnv));
        Rf_PrintValue(bad);
        println!("Did we get here?");
        Rf_unprotect(1);
    });
}
[1] 10 42 20
Did we get here?
test test_parse_eval_string ... ok

But then

        let bad_code = cstr!("c(10,,42,20)");
        let bad = Rf_protect(R_ParseEvalString(bad_code.as_ptr(), R_NilValue));
        Rf_PrintValue(bad);
        println!("Did we get here?");
        Rf_unprotect(1);
running 1 test
Error: 'rho' must be an environment not NULL: detected in C-level eval
Fatal error: unable to initialize the JIT

error: test failed, to rerun pass `-p extendr-api --test parse_tests`

So the second argument to R_ParseEvalString must be R_GlobalEnv or something.
But that's not what is in this PR right now.

@Ilia-Kosenkov
Copy link
Member

It could be that c() is a call to a function, and you need an environment to look it up. But you do not need one if you just evaluate a constant 1.

@CGMossa CGMossa merged commit a452bff into master Oct 7, 2023
18 checks passed
@CGMossa CGMossa deleted the use_r_parseevalstring branch October 7, 2023 22:03
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.

2 participants