Skip to content
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

Incorrect comparison id.0 == 0 #9

Open
aDotInTheVoid opened this issue Dec 17, 2024 · 1 comment
Open

Incorrect comparison id.0 == 0 #9

aDotInTheVoid opened this issue Dec 17, 2024 · 1 comment

Comments

@aDotInTheVoid
Copy link

The id.0 == 0 here is meaningless, and will only be true for 1 random item.

BuFFI/buffi/src/lib.rs

Lines 173 to 184 in c3a1c61

// we might get several candidates. In that case check that:
//
// * We resolve against the local crate (indicated by '0' in the beginning)
// * There is a candidate coming from this crate (indicated by the parent_crate)
// argument
let matches_parent_crate = items
.iter()
.find(|i| extract_crate_from_span(i) == parent_crate);
match matches_parent_crate {
Some(t) if id.0 == 0 => {
return rustdoc_types::Item::clone(t);
}

To quote the docs:

Rustdoc makes no guarantees about the inner value of Id’s. Applications should treat them as opaque keys to lookup items, and avoid attempting to parse them, or otherwise depend on any implementation details.

Even if you want to use implementation details that may change, it's still meaningless, as id 0 is will be some arbitrary item (and not the root module, like 0:0 in the old scheme)

I suspect the right thing to do here is to instead look at crate_id

This was changed in c3a1c61#diff-02bde36a4d29c3e4ab7b11c4a2d6acf78f0f83605ad40bfed50aee352488653cL182. The prior code did id.0.starts_with('0') which is questionable (as it inspects the opaque id) but did work correctly on specific rustdoc versions.

@SwishSwushPow
Copy link
Member

Hi and thank you for your contribution! (also sorry that I've missed it over the holidays, I thought I'd receive emails for issues, but apparently I don't 😅 ) I intend to have a closer look at this soon again, but you are absolutely spot on from what I remember.

Truth be told the matching code is a bit of a mess right now (historically grown to be that way, to choose one of the more common excuses) and I think the id matching at this point always fails and goes to the "backup" part of the function, where we then (hopefully!) find the correct item. At the moment we rely on BuFFIs tests and our internal binding generation to make sure our "approximations" work out correctly. So far they work surprisingly well (fingers crossed), but it would be so worth to clean up the matching code again. Thanks for pointing that out and providing some valuable information on top of that! :)

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

No branches or pull requests

2 participants