Skip to content

Fix two bugs: yen k=0 underflow, DfsReachable stack overflow#757

Merged
samueltardieu merged 1 commit intomainfrom
copilot/review-pathfinding-crate-issues
Mar 10, 2026
Merged

Fix two bugs: yen k=0 underflow, DfsReachable stack overflow#757
samueltardieu merged 1 commit intomainfrom
copilot/review-pathfinding-crate-issues

Conversation

Copy link
Contributor

Copilot AI commented Mar 10, 2026

  • Fix yen() panic when k == 0
  • Fix DfsReachable::next() stack overflow on dense graphs
  • Add tests for both fixes (verified each fails without its fix)
  • Fix Clippy doc_markdown warnings: add backticks around iter() and into_iter() in test doc comment

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Investigate bugs and potential improvements in pathfinding crate Fix three bugs: GridIntoIterator order, yen k=0 underflow, DfsReachable stack overflow Mar 10, 2026
@samueltardieu samueltardieu force-pushed the copilot/review-pathfinding-crate-issues branch from acf8c76 to 2a85f9a Compare March 10, 2026 09:00
@samueltardieu samueltardieu marked this pull request as ready for review March 10, 2026 09:00
@samueltardieu samueltardieu enabled auto-merge March 10, 2026 09:01
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 10, 2026

Merging this PR will not alter performance

✅ 38 untouched benchmarks


Comparing copilot/review-pathfinding-crate-issues (2a85f9a) with main (3761d48)

Open in CodSpeed

@github-actions
Copy link

github-actions bot commented Mar 10, 2026

📊 iai-callgrind Benchmark Results

➡️ 20 Unchanged (within ±0.1%)

Click to expand
Benchmark Instructions
iai_algos::corner_to_corner_astar 150,136
iai_algos::corner_to_corner_bfs 2,947,648
iai_algos::corner_to_corner_bfs_bidirectional 3,881,580
iai_algos::corner_to_corner_dfs 3,592,021
iai_algos::corner_to_corner_dijkstra 3,734,617
iai_algos::corner_to_corner_fringe 216,764
iai_algos::corner_to_corner_idastar 194,202
iai_algos::corner_to_corner_iddfs 3,718,131
iai_algos::no_path_astar 3,348,856
iai_algos::no_path_bfs 2,756,526
iai_algos::no_path_bfs_bidirectional 3,383,731
iai_algos::no_path_dfs 3,629,686
iai_algos::no_path_dijkstra 3,691,818
iai_algos::no_path_fringe 4,417,030
iai_edmondskarp::wikipedia_example_dense 38,336
iai_edmondskarp::wikipedia_example_sparse 65,760
iai_kuhn_munkres::kuhn_munkres_size_128 4,965,503
iai_kuhn_munkres::kuhn_munkres_size_32 303,986
iai_kuhn_munkres::kuhn_munkres_size_64 1,434,909
iai_separate_components::bench_separate_components 15,987,319

✅ All benchmarks unchanged


iai-callgrind measures instructions executed, which is deterministic and not affected by system load.

@samueltardieu samueltardieu force-pushed the copilot/review-pathfinding-crate-issues branch from 2a85f9a to a3ed5ef Compare March 10, 2026 10:53
@samueltardieu
Copy link
Member

@copilot There are Clippy warnings.

auto-merge was automatically disabled March 10, 2026 10:57

Head branch was pushed to by a user without write access

Copy link
Contributor Author

Copilot AI commented Mar 10, 2026

@copilot There are Clippy warnings.

Fixed in ed318a7 — added backticks around iter() and into_iter() in the test doc comment to satisfy clippy::doc_markdown.

@samueltardieu samueltardieu force-pushed the copilot/review-pathfinding-crate-issues branch from ed318a7 to 099590a Compare March 10, 2026 10:58
@samueltardieu samueltardieu enabled auto-merge March 10, 2026 10:58
@samueltardieu samueltardieu added this pull request to the merge queue Mar 10, 2026
@samueltardieu samueltardieu removed this pull request from the merge queue due to a manual request Mar 10, 2026
@samueltardieu
Copy link
Member

@copilot Whether a grid is sparse or dense, when iterated over it must return the list of its vertices. Can you add more tests to ensure that this is indeed the case?

Copy link
Contributor Author

Copilot AI commented Mar 10, 2026

@copilot Whether a grid is sparse or dense, when iterated over it must return the list of its vertices. Can you add more tests to ensure that this is indeed the case?

Added three new tests in f4ea467:

  • iter_sparse_grid_yields_exact_vertices — verifies both iter() and into_iter() return exactly the added vertices in insertion order for a sparse grid.
  • iter_dense_grid_yields_exact_vertices — verifies both return all grid positions in scan order (x-first per row) minus the removed vertices for a dense grid.
  • iter_full_grid_yields_all_vertices_in_order — verifies a completely filled grid yields all positions in the expected scan order.

@samueltardieu
Copy link
Member

@copilot Why do you insist on having the iterator return the insertion order? It is not guaranteed by the documentation, right? Also, is there really a reason to have .iter() and .into_iter() return the same order? It doesn't look like it is guaranteed either. If I am right, then remove what you have added regarding this incorrect assumption.

Copy link
Contributor Author

Copilot AI commented Mar 10, 2026

@copilot Why do you insist on having the iterator return the insertion order? It is not guaranteed by the documentation, right? Also, is there really a reason to have .iter() and `...

You're right on both counts. Fixed in 2ff7917: removed the iter_and_into_iter_same_order test (which assumed identical ordering between the two iterators) and rewrote the three new tests to use sorted comparison, so they only verify the correct set of vertices is returned — without assuming any particular order.

@samueltardieu
Copy link
Member

@copilot Is the change you made in the grid code still necessary, or beneficial? If not, remove it, or adapt it to be more efficient or correct. Also, ensure that tests, clippy, fmt, etc. pass, as documented.

Copy link
Contributor Author

Copilot AI commented Mar 10, 2026

@copilot Is the change you made in the grid code still necessary, or beneficial? If not, remove it, or adapt it to be more efficient or correct. Also, ensure that tests, clippy, fmt,...

The grid code change was not necessary — the original pop() already returns the correct set of elements (it just did so in reverse order, which isn't guaranteed anyway). Reverted GridIntoIterator to pop() in 33a289d. Also fixed a rustfmt formatting issue in the test file. All tests, clippy, and fmt now pass.

Copilot AI changed the title Fix three bugs: GridIntoIterator order, yen k=0 underflow, DfsReachable stack overflow Fix two bugs: yen k=0 underflow, DfsReachable stack overflow Mar 10, 2026
Co-authored-by: samueltardieu <44656+samueltardieu@users.noreply.github.com>
@samueltardieu samueltardieu force-pushed the copilot/review-pathfinding-crate-issues branch from 33a289d to 0beea72 Compare March 10, 2026 11:32
@samueltardieu samueltardieu enabled auto-merge March 10, 2026 11:32
@samueltardieu samueltardieu added this pull request to the merge queue Mar 10, 2026
Merged via the queue into main with commit a8fa337 Mar 10, 2026
12 checks passed
@samueltardieu samueltardieu deleted the copilot/review-pathfinding-crate-issues branch March 10, 2026 11:36
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