Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 26, 2025

The auto-generated biconnected_components_impl() was creating duplicate empty fields with dot notation (tree.edges, component.edges, articulation.points) alongside the correct underscore fields from the C layer.

Root Cause

The impl function received underscore field names from C but incorrectly accessed them using dot notation:

res$tree.edges <- lapply(res$tree.edges, ...)  # res$tree.edges doesn't exist yet

This created new empty dot fields while leaving the original underscore fields unchanged, resulting in:

> names(res)
[1] "no" "tree_edges" "component_edges" "components" "articulation_points"
[6] "tree.edges" "component.edges" "articulation.points"  # Empty duplicates

Changes

  • R/aaa-auto.R: Fixed field access to use underscore notation (res$tree_edges, res$component_edges, res$articulation_points)
  • R/components.R: Removed redundant workaround that was recreating underscore fields and deleting empty dot fields
  • tests/testthat/_snaps/aaa-auto.md: Updated snapshot to reflect correct output format

Result

> res <- biconnected_components(make_graph(c(1,2, 2,3, 3,1, 1,4), directed = FALSE))
> names(res)
[1] "no" "tree_edges" "component_edges" "components" "articulation_points"

All fields now contain properly typed igraph.vs/igraph.es objects.

Original prompt

This section details on the original issue you should resolve

<issue_title>biconnected_components() returns invalid result fields</issue_title>
<issue_description>biconnected_components() returns a result list with named components tree_edges, component_edges, articulation_points. These list components have valid values, However, the result also has components tree.edges, component.edges and articulation.points. These are empty.

Note that biconnected_components() is fully auto-generated, but somewhere during auto-generation the _ is changed to a ., but only in some places, not everywhere.

> res<-biconnected_components(make_graph(c(1,2, 2,3, 3,1, 1,4), directed = F))
> res
$no
[1] 2

$tree_edges
$tree_edges[[1]]
[1] 2 1

$tree_edges[[2]]
[1] 4


$component_edges
$component_edges[[1]]
[1] 3 2 1

$component_edges[[2]]
[1] 4


$components
$components[[1]]
+ 3/4 vertices, from c488bbb:
[1] 3 2 1

$components[[2]]
+ 2/4 vertices, from c488bbb:
[1] 4 1


$articulation_points
[1] 1

$articulation.points
+ 0/4 vertices, from c488bbb:

Also notice the function definition:

biconnected_components_impl <- function(graph) {
  # Argument checks
  ensure_igraph(graph)

  on.exit( .Call(R_igraph_finalizer) )
  # Function call
  res <- .Call(R_igraph_biconnected_components, graph)
  if (igraph_opt("return.vs.es")) {
    res$tree.edges <- lapply(res$tree.edges, unsafe_create_es, graph = graph, es = E(graph))
  }
  if (igraph_opt("return.vs.es")) {
    res$component.edges <- lapply(res$component.edges, unsafe_create_es, graph = graph, es = E(graph))
  }
  if (igraph_opt("return.vs.es")) {
    res$components <- lapply(res$components, unsafe_create_vs, graph = graph, verts = V(graph))
  }
  if (igraph_opt("return.vs.es")) {
    res$articulation.points <- create_vs(graph, res$articulation.points)
  }
  res
}

</issue_description>

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

@ntamas The replacement is done in Stimulus in `src/stimulus/generators/r.py` in the function named `get_r_parameter_name` -- that part of the code is actually replacing underscores with dots. I don't have more time to debug this, but start looking there. @szhorvat This broke between 1.2.11 and 1.3.0. Isn't that when stimulus was separated into a library?

This specific function used underscore names in 1.2.11, res$articulation_points

The autogenerated code in 1.3.0 uses a buggy mixture of underscore and dot, and ends up with the mess I showed above. Furthermore, the outputs are no longer a proper vertex/edges sequences.

As Tamás said in chat, we need to decide whether we go with dots or underscores for output parameter names. Input parameters and attributes already use dots, so that's a natural choice, but that would involve a breaking change for biconnected_components().</comment_new>
<comment_new>@szhorvat
I would have expected this to be fixable using

igraph_biconnected_components:
    PARAM_NAMES:
        articulation_points: articulation_points
        tree_edges: tree_edges
        component_edges: component_edges

but this doesn't disable the _ -> . replacement. If the parameters are renamed to anything but the original name, then the replacement is disabled. It would be good to fix this in Stimulus, to allow for special cases when we DON'T want the .-renaming.</comment_new>
<comment_new>@ntamas
The _ -> . replacement is specific to the R code generator and it was there to enforce R's then-preferred dotted notation without having to spell out the alternative names one by one for each argument. Also, this came before we've had PARAM_NAMES. I wouldn't try to fix the treatment of PARAM_NAMES vs the replacement, I'd rather prefer first to decide once and for all which of the two formats (dotted or underscored) is preferred in the R interface. If it is the dotted notation, then everything is fine and we don't need any special treatment in PARAM_NAMES. If it is the underscored notation, we need to remove the replacement code and that's all.</comment_new>
<comment_new>@szhorvat

I'd rather prefer first to decide once and for all which of the two formats (dotted or underscored) is preferred in the R interface. If it is the dotted notation, then everything is fine and we don't need any special treatment in PARAM_NAMES. If it is the underscored notation, we need to remove the replacement code and that's all.

@krlmlr, the ball's in your court on this one.</comment_new>
<comment_new>@krlmlr
Changing the component names in a back-compatible way seems difficult, I wonder if it's worth the effort.

Just breaking the functionality seems to require a...


✨ 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 AI and others added 2 commits October 26, 2025 18:21
Fixed the auto-generated biconnected_components_impl() to use underscore field names (tree_edges, component_edges, articulation_points) instead of incorrectly creating dot versions. Simplified the wrapper function as it no longer needs to fix the field names.

Co-authored-by: krlmlr <[email protected]>
Updated test snapshot to reflect the fix - now correctly shows only underscore field names without the empty dot versions.

Co-authored-by: krlmlr <[email protected]>
Copilot AI changed the title [WIP] Fix invalid result fields in biconnected_components() fix: biconnected_components() returns invalid result fields Oct 26, 2025
Copilot AI requested a review from krlmlr October 26, 2025 18:27
Copilot finished work on behalf of krlmlr October 26, 2025 18:27
Copy link
Contributor

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot: The changes won't survive regeneration of aaa-auto.R . Ensure stability.

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.

biconnected_components() returns invalid result fields

2 participants