Skip to content

Conversation

@marcospb19
Copy link
Contributor

While using Acyclic I got this panic:

thread 'tests::panic_on_short_cycle' panicked at /home/marcospb19/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/petgraph-0.7.1/src/graphmap.rs:1203:38:
called `Option::unwrap()` on a `None` value

While expected, it'd be more helpful if it pointed to the caller in my code, not the upstream code.

I started adding #[track_caller] for Acyclic and ended up going through all functions that contain "panics" in their docs, except for std::ops::Index[Mut] cause those have #[track_caller] in the trait definition.

@marcospb19 marcospb19 marked this pull request as draft March 16, 2025 04:54
@marcospb19 marcospb19 marked this pull request as ready for review March 16, 2025 04:58
@marcospb19 marcospb19 marked this pull request as draft March 16, 2025 05:01
@marcospb19
Copy link
Contributor Author

Oops, I think I found an undocumented panic, turning into draft for now while I figure out what's going on.

@marcospb19
Copy link
Contributor Author

marcospb19 commented Mar 16, 2025

Done, I documented more panics.

Now I'm get this panic message:

thread 'tests::panic_on_short_cycle' panicked at src/lib.rs:25:28:
node not found

I also changed wording around out of bounds for graphs like Acyclic and Stable where indices are non-contiguous.

@marcospb19 marcospb19 marked this pull request as ready for review March 16, 2025 05:27
@ABorgna ABorgna changed the title add #[track_caller] to functions that panic feat: add #[track_caller] to functions that panic Mar 19, 2025
@ABorgna ABorgna changed the title feat: add #[track_caller] to functions that panic feat: add #[track_caller] to functions that panic Mar 19, 2025
@marcospb19
Copy link
Contributor Author

@ABorgna ping, I solved the conflicts.

@starovoid
Copy link
Collaborator

Hi, nice feature! Plan to include it in the next minor release 0.8.1.

@starovoid starovoid added this to the 0.8.1 milestone Apr 3, 2025
@starovoid starovoid self-requested a review April 6, 2025 15:40
@starovoid starovoid modified the milestones: 0.8.2, 0.8.1 Apr 7, 2025
Copy link
Collaborator

@starovoid starovoid left a comment

Choose a reason for hiding this comment

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

It might also be worth using except instead of unwrap in Csr methods, but with the recent addition of a typed error, the messages look quite clear.
So let's leave it up to #724

@starovoid starovoid added this pull request to the merge queue Apr 7, 2025
Merged via the queue into petgraph:master with commit ba115d8 Apr 7, 2025
10 checks passed
@github-actions github-actions bot mentioned this pull request Apr 7, 2025
github-merge-queue bot pushed a commit that referenced this pull request Apr 7, 2025
## 🤖 New release

* `petgraph`: 0.8.0 -> 0.8.1 (✓ API compatible changes)

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

##
[0.8.1](https://github.com/petgraph/petgraph/compare/petgraph@v0.8.0...petgraph@v0.8.1)
- 2025-04-07

### Bug Fixes

- Bring back `VisitMap` impl for std `HashSet`
([#764](#764))

### New Features

- Add `UnionFind` capacity management methods
([#736](#736))
- add `#[track_caller]` to functions that panic
([#748](#748))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Agustín Borgna <agustinborgna@gmail.com>
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