roaring-rs icon indicating copy to clipboard operation
roaring-rs copied to clipboard

`advance_to` not implemented for `roaring::treemap::Iter`

Open sdd opened this issue 8 months ago • 9 comments

I have a use case where I need the advance_to method, which is present on RoaringBitmap's Iter, but I want to use it from a RoaringTreemap. This doesn't seem to exist - I'm happy to contribute an implementation, unless there a reason why this is not implemented already that I'm unaware of. Would you be open to this?

Additionally, I looked into implementing a struct outside of this library that would provide this functionality, but it's not possible to implement this easily because roaring::treemap::iter::BitmapIter is not publicly exported. Is this just an oversight? It's not currently possible to store the result of the public RoaringTreemap::bitmaps method in a struct because of this.

sdd avatar Mar 19 '25 07:03 sdd

This doesn't seem to exist - I'm happy to contribute an implementation, unless there a reason why this is not implemented already that I'm unaware of. Would you be open to this?

Ho, yes, please contribute. The only reason it doesn't exist is that we didn't spend time on it.

[..] it's not possible to implement this easily because roaring::treemap::iter::BitmapIter is not publicly exported. Is this just an oversight?

I think it is an oversight indeed. All publicly accessible struct/types should be available. Do you know about a certain edition, compiler parameter or CI setup we can use to make sure the CI fails in case we miss that again?

Kerollmops avatar Mar 19 '25 09:03 Kerollmops

Ho, yes, please contribute. The only reason it doesn't exist is that we didn't spend time on it.

With pleasure, I'm only too happy to give something back to this great library! :-)

I think it is an oversight indeed. All publicly accessible struct/types should be available. Do you know about a certain edition, compiler parameter or CI setup we can use to make sure the CI fails in case we miss that again?

I don't, off the top of my head - but I'll see what I can find and incorporate it into a PR with the fix, if I find something.

sdd avatar Mar 19 '25 18:03 sdd

Looks like there are no Clippy lints that do this. But dylint could do it. I've not written a dylint linter before - I'll dig a bit deeper to see how hard it would be for this case.

sdd avatar Mar 19 '25 19:03 sdd

It should warn about private items' exposure from public functions... so. Or maybe we don't run the correct cargo doc command to see it?

Kerollmops avatar Mar 19 '25 20:03 Kerollmops

It works only if I add a reference to the returned type to the doc comment, and also only if I run cargo rustdoc -p roaring. It seems that the cargo clippy -p roaring --all-targets --no-default-features -- -D warnings command that runs in the GitHub action does not check rustdoc lints:

❯ cargo rustdoc -p roaring
 Documenting roaring v0.10.10 (/Users/scott/projects/roaring-rs/roaring)
warning: public documentation for `bitmaps` links to private item `BitmapIter`
   --> roaring/src/treemap/iter.rs:247:21
    |
247 |     /// Returns a [`BitmapIter`].
    |                     ^^^^^^^^^^ this item is private
    |
    = note: this link will resolve properly if you pass `--document-private-items`
    = note: `#[warn(rustdoc::private_intra_doc_links)]` on by default

warning: `roaring` (lib doc) generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.66s
   Generated /Users/scott/projects/roaring-rs/target/doc/roaring/index.html

sdd avatar Mar 20 '25 07:03 sdd

Hum... ok, so can we have it detect private items returned from public functions? Or we have to add it to a doc comment?

Kerollmops avatar Mar 20 '25 08:03 Kerollmops

That lint only picks up docs that contain links to private types - it will only emit a warning if you explicitly put a link in the doc comment to a type that is not public. Useful, but not really for this use case - you'd have to remember to add in a link to catch the return type not being pub.

sdd avatar Mar 20 '25 19:03 sdd

I think this is the unnameable_types lint built into rust: https://doc.rust-lang.org/beta/rustc/lints/listing/allowed-by-default.html#unnameable-types

Dr-Emann avatar Mar 21 '25 01:03 Dr-Emann

Thanks @Dr-Emann ! Looks like that lint is allow by default. I'll raise a PR to set it to deny and fix any violations that this uncovers

sdd avatar Mar 21 '25 13:03 sdd