Fix bug with raw identifiers in function arguments (#528)

[bicarlsen]

Added functionality to remove the raw identifier prefix (r#) from function arguments in the R bindings.

[Ilia-Kosenkov]

Hey, thanks for the contribution. There a couple of edge cases we need to cover, but right now I do not have time to comment on this. Can you hold on to this PR for a little bit longer?

[bicarlsen]

Of course, just let me know.

[Ilia-Kosenkov]

Ok so I was thinking about touching this thing:

extendr/extendr-api/src/metadata.rs

Lines 153 to 159 in 8dc13d8

	fn sanitize_identifier(name: &str) -> String {
	if name.starts_with('_') {
	format!("`{}`", name)
	} else {
	name.to_string()
	}
	}

We have a method that sanitizes rust identifiers on the R side. E.g., if you have a fn my_fn(_x :i32) {}, R cannot handle the _x argument, but it instead can handle `_x`, which we effectively pass around when we detect an incompatible name. I propose to add another check, like .contains('#'), instead of changing generated code. We just sanitize wrappers properly.

There is another thing, we should also consider a case of functions that have r# in the name, I suggest we also add a r#true() function to the test package. Now if sanitize_warppers() is updated, r#true() will be wrapped accordingly, but not the argument passed to the .Call() (here it is something like wrap__mypkg_r#true, which is invalid). This part I have not yet solved, but we need to also sanitize it the same way we sanitize arguments -- `wrap__mapkg_r#true` is a valid R identifier.

This way we preserve exact 1-to-1 mapping between rust names and R names.

[bicarlsen]

I updated the implementation to incorporate the sanitize_identifiers method mentioned by @Ilia-Kosenkov above. This did not address the wrapped function names in the extendr-wrappers.R file though, so still needed sanitization in the extendr_macros/wrappers.rs module.

[Ilia-Kosenkov]

Oh shoot, completely forgot about this, my apologies. I will check it today.

[Ilia-Kosenkov]

I think you need to merge master in your branch to update CI

[bicarlsen]

I don't understand why the Markdown links are causing errors in the tests. This issue was discussed in #534 . I fixed the typos with the parentheses, so not sure what is causing the issue now.

[CGMossa]

There are more places where the markdown-links are done incorrectly. But here you go!
Just accept these suggestions.

[bicarlsen]

Geeze! Thanks @CGMossa.

[CGMossa]

@bicarlsen Right, you need to run the document() function again, as to regenerate Rd-files with the right markdown link notation in them.

[bicarlsen]

Okay, I was having trouble running the tests on my local machine, but noticed that I had to change the tests/extendrtests/src/rust/Cargo.toml > extendr-api to have an absolute path. I am still receiving an error

── Error ('test-graphic-device.R:2:5'): `my_device()` works as expected ────────
Error in `my_device("foo")`: Graphics API version mismatch
Backtrace:
     ▆
  1. ├─testthat::expect_output(...) at test-graphic-device.R:2:4
  2. │ └─testthat:::quasi_capture(...)
  3. │   ├─testthat (local) .capture(...)
  4. │   │ └─testthat::capture_output_lines(code, print, width = width)
  5. │   │   └─testthat:::eval_with_output(code, print = print, width = width)
  6. │   │     ├─withr::with_output_sink(path, withVisible(code))
  7. │   │     │ └─base::force(code)
  8. │   │     └─base::withVisible(code)
  9. │   └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
 10. └─extendrtests::my_device("foo")

but am not sure why as I didn't touch that code and it seems unrelated to this PR.

[CGMossa]

This happened to me the other day as well... Run an rextendr colon colon clean() and try again.

…

On Thu, 27 Apr 2023, 13.32 bicarlsen, ***@***.***> wrote: Okay, I was having trouble running the tests on my local machine, but noticed that I had to change the tests/extendrtests/src/rust/Cargo.toml > extendr-api <https://github.com/extendr/extendr/blob/fc677380fa03c638e7913fdd96140da9ff1a5ad4/tests/extendrtests/src/rust/Cargo.toml#L23> to have an absolute path. I am still receiving an error ── Error ('test-graphic-device.R:2:5'): `my_device()` works as expected ──────── Error in `my_device("foo")`: Graphics API version mismatch Backtrace: ▆ 1. ├─testthat::expect_output(...) at test-graphic-device.R:2:4 2. │ └─testthat:::quasi_capture(...) 3. │ ├─testthat (local) .capture(...) 4. │ │ └─testthat::capture_output_lines(code, print, width = width) 5. │ │ └─testthat:::eval_with_output(code, print = print, width = width) 6. │ │ ├─withr::with_output_sink(path, withVisible(code)) 7. │ │ │ └─base::force(code) 8. │ │ └─base::withVisible(code) 9. │ └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo)) 10. └─extendrtests::my_device("foo") but am not sure why as I didn't touch that code and it seems unrelated to this PR. — Reply to this email directly, view it on GitHub <#531 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIDVSC7GJ63SZOCFA6WYGDXDJKMXANCNFSM6AAAAAAW7KIGT4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[CGMossa]

Alright, that part is resolved. Did you have success with running tests after running rextendr::clean().

[Ilia-Kosenkov]

I would say looks good. We have tests to cover these edge cases, everything seems to be in place. Please finalize the change request discussion with @CGMossa and we can proceed.

Oh and BTW, please write a note in the changelog.

[bicarlsen]

After a rextendr::clean() I still receive the error

Error (test-graphic-device.R:2:5): `my_device()` works as expected
Error in `my_device("foo")`: Graphics API version mismatch
Backtrace:
  1. testthat::expect_output(...)
       at test-graphic-device.R:2:4
 10. extendrtests::my_device("foo")

[bicarlsen]

@Ilia-Kosenkov I browsed the CHANGELOG but had a difficult time understanding how exactly to add an entry in the correct format. Could you give some insight?

[CGMossa]

After a rextendr::clean() I still receive the error

Error (test-graphic-device.R:2:5): `my_device()` works as expected
Error in `my_device("foo")`: Graphics API version mismatch
Backtrace:
  1. testthat::expect_output(...)
       at test-graphic-device.R:2:4
 10. extendrtests::my_device("foo")

When I had this error, I couldn't even do plot.new() in R. Are you able to do that? Please try to restart your session as well.

[bicarlsen]

Restarted the session a few times, cleaned and redocumented a few times, still the same error. Plotting seems to work for me though.

[Ilia-Kosenkov]

Summoning @yutannihilation . You have experience with graphic devices, why are we getting version mismatch here (when executed locally)? Can it be that our bindings are for R 4.2.3 and are labeled 4.2, so if you now use R 4.2.x, the bindings are resolved to 4.2, but there still is a mismatch (i.e. if R people bumped graphics version in patch)/

[Ilia-Kosenkov]

We (are trying to) maintain CHANGELOG according to these rules. In your case, you need to go to the top of the file and add under unreleased/fixed section something that describes your changes. Make it human readable, so it goes something like:

[Fixed]
- <existing entry>
- A bug preventing using raw rust literals in exported extendr functions [PR link]

Use present tense and simple wording. Give an example of a raw literal (like r#type) in and how it is converted to type now. I would say 1-2 sentences are enough.

[yutannihilation]

R people bumped graphics version in patch

It can happen.

https://github.com/wch/r-source/blob/ed2b4252a103c2f8494b906c404fbc9a8cd2d5ef/src/include/R_ext/GraphicsEngine.h#L81

But, I guess it's not the case (unless you are using R 4.1). Let me try reproducing it on my local.

[bicarlsen]

Shall I push wish the new CHANGELOG or wait to resolve the graphics issue?

[CGMossa]

Push the changes. Gives us time to review that.

[yutannihilation]

Okay, I reproduced it. I think you can just remove your tests/extendrtests/src/rust/Cargo.lock to update the dependency crates.

[bicarlsen]

Okay, I reproduced it. I think you can just remove your tests/extendrtests/src/rust/Cargo.lock to update the dependency crates.

Yup, that did the trick :)

[bicarlsen]

Is there anything else to be done here before we can merge?

[CGMossa]

LGTM

[CGMossa]

I had already seen this and thought @Ilia-Kosenkov wanted to give it a last look and then submission. Looks great from my end.