rigraph icon indicating copy to clipboard operation
rigraph copied to clipboard

[WIP] Remove `.Call()` usage from user code

Open Copilot opened this issue 2 months ago • 1 comments

  • [x] Explored repository structure and identified .Call() usage
  • [x] Found 394 unique C functions called directly outside aaa-auto.R
  • [x] Identified that functions-R.yaml contains IGNORE directives preventing autogeneration
  • [x] Enabled autogeneration for 10 functions across 4 categories:
    • Independent vertex set functions (4)
    • Cocitation functions (2)
    • Minimum spanning tree functions (2)
    • Graphlet functions (2)
  • [x] Regenerated aaa-auto.R and rinterface.c using make
  • [x] Updated R code in cliques.R, cocitation.R, minimum.spanning.tree.R, glet.R
  • [x] Removed manual C implementations from rinterface_extra.c
  • [x] Fixed graphlet functions to match original behavior
  • [x] Addressed code review feedback - extracted helper function
  • [x] Run full test suite - all tests pass (6952 passing, 2 skipped)
  • [ ] Address remaining .Call() usage (approximately 384 functions in ~36 files remaining)

Summary

Successfully migrated 10 functions from direct .Call() usage to autogenerated bindings, demonstrating the pattern for the remaining ~384 functions. Key improvements:

  • Type safety through autogenerated code
  • Consistency in C interface
  • Proper parameter conversion and validation
  • Better maintainability

All tests pass. The pattern established here can be applied to remaining functions.

Original prompt

This section details on the original issue you should resolve

<issue_title>Avoid .Call() in code other than aaa-auto.R</issue_title> <issue_description>All calls into C code should be autogenerated. This ensures that we either mess up big time (and notice it), or get it right.

In practice, this means hunting down all instances of .Call in user code and using autogenerated bindings instead. Related: igraph/rigraph#1044.

https://github.com/igraph/rigraph/pull/1057#issuecomment-1872612836</issue_description>

Comments on the Issue (you are @copilot in this section)

@szhorvat One issue that will need to be solved (also for `transitivity()`) is what happened here: https://github.com/igraph/rigraph/pull/1028#issuecomment-1871607308 This problem went away for `k_shortest_paths()` after I figured out how to use the generated function directly (instead of calling it). But we'll need a solution when we call it. Do you have any suggestions @krlmlr ?

In short, suppose the function has a mode = c("out", "in", "all") parameter. Multiple values are allowed as a default, but not as a manually passed value. This poses an issue when passing the default mode to the _impl function.</comment_new> <comment_new>@krlmlr

We can call rlang::arg_match() in the manual wrapper in such cases, before forwarding to _impl().</comment_new> <comment_new>@krlmlr Scratch that, we probably need igraph.match.arg() also in the manual wrapper.</comment_new> <comment_new>@krlmlr It's a case-insensitive version of match.arg(), and rlang::arg_match() is a version of that with better behavior and error messages.</comment_new>
  • Fixes igraph/rigraph#1058

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot avatar Oct 25 '25 16:10 Copilot

The .yaml changes are good, need to rethink most changes from there.

krlmlr avatar Oct 25 '25 19:10 krlmlr