-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(frontend): make path_to
breadth-first
#730
Conversation
2cb1f89
to
a996280
Compare
A first fix to this implementation (or to a bug revealed by it) is in #797 There is another problem remaining (reaching https://github.com/hacspec/hax/blob/main/frontend/exporter/src/traits.rs#L349) |
a996280
to
0d8198e
Compare
0d8198e
to
274bab2
Compare
This bug introduced by the initial version of this PR is now fixed. It was just that switching from a recursive depth-first search to a breadth-first search with a queue meant the parents and associated traits had to be taken from the candidate path and not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you @maximebuyse! That looks good to me, and @franziskuskiefer is currently trying to break it by throwing it on bigger crates 😅 🤞
The frontend panics now in most cases where it looped before.
|
This is #683, which is not related. IMO we should merge that PR, that's fixing the looping issue |
How do we know that it is fixing the issue? |
You're right, it doesn't work on #692 . The looping behavior was gone because the breadth-first search had a bug. |
Oh, you mean that it's still looping? :/ |
I pushed |
72dcea4
to
f552d50
Compare
f552d50
to
59f1190
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a few nits!
Thanks for the tips, I made the corrections. |
Nice, thanks! Have you tried the patch on both #686 and #692? (I updated Also, you can resolve conversations on the comments I added, otherwise the PR will be blocked from merging :) |
This seems to solve #692, I added a test with this example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, then let's get this in and keep looking for the issue in #686.
Mmh, ok. Added a comment there: #686 (comment). |
Fixes #692
Status: seems like this is introducing a bug, I have to debug.
Some implementations cannot be found after the change