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

Rust API: Expose RecordingStreamError #3735

Closed
kpreid opened this issue Oct 9, 2023 · 7 comments · Fixed by #3777
Closed

Rust API: Expose RecordingStreamError #3735

kpreid opened this issue Oct 9, 2023 · 7 comments · Fixed by #3777
Assignees
Labels
😤 annoying Something in the UI / SDK is annoying to use 🏎️ Quick Issue Can be fixed in a few hours or less 🦀 Rust API Rust logging API
Milestone

Comments

@kpreid
Copy link
Collaborator

kpreid commented Oct 9, 2023

Describe the annoyance

My preferred coding style is to never use type aliases for std::result::Result (such as rerun::RecordingStreamResult), but the type RecordingStreamError is private. Rust libraries typically make their error types public even if they also offer a Result.

This lack of public type might also make it impossible to pass the error through some generic error-handling code that required the error type to be named, but I don't have a practical example of that.

Please make RecordingStreamError public (possibly as a struct wrapper if you wish to avoid revealing the enum variants).

Additional context

  • rerun 0.9.0
@kpreid kpreid added 👀 needs triage This issue needs to be triaged by the Rerun team 😤 annoying Something in the UI / SDK is annoying to use labels Oct 9, 2023
@emilk emilk added this to the 0.9.1 milestone Oct 9, 2023
@emilk
Copy link
Member

emilk commented Oct 9, 2023

Good catch!

There really should be some compiler setting to catch this mistake 🤔

@abey79 abey79 added 🦀 Rust API Rust logging API and removed 👀 needs triage This issue needs to be triaged by the Rerun team labels Oct 9, 2023
@emilk emilk added the 🏎️ Quick Issue Can be fixed in a few hours or less label Oct 9, 2023
@kpreid
Copy link
Collaborator Author

kpreid commented Oct 9, 2023

There really should be some compiler setting to catch this mistake

https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unreachable-pub

@kpreid
Copy link
Collaborator Author

kpreid commented Oct 9, 2023

Oh, and I could write a PR for this if you like — just let me know whether the enum should be wrapped to be hidden or not as I mentioned above.

@emilk
Copy link
Member

emilk commented Oct 9, 2023

unreachable-pub does something else - it checks that everything marked pub is exported from the crate. What I want is that anything marked pub only uses types that are also marked pub

@emilk
Copy link
Member

emilk commented Oct 9, 2023

Let's just make RecordingStreamError public. Adding wrappers around everything is very annoying at this early stage in the project. A PR would be very welcome!

@emilk emilk self-assigned this Oct 10, 2023
@emilk emilk mentioned this issue Oct 10, 2023
4 tasks
@emilk
Copy link
Member

emilk commented Oct 10, 2023

I found the bug that prevents rustc from warning about things like this:

@kpreid
Copy link
Collaborator Author

kpreid commented Oct 10, 2023

unreachable-pub does something else - it checks that everything marked pub is exported from the crate.

unreachable-pub should trigger on this case, because RecordingStreamError is pub but not exported, but it seems that unreachable-pub is counting the type alias as a kind of reachability. (Which I suppose it is if you try hard enough — you can write a trait that lets you extract the error type of a Result as an associated type. But that doesn't seem in the spirit of the thing.) I've filed a Rust issue: rust-lang/rust#116604

What I want is that anything marked pub only uses types that are also marked pub

That rule already exists and is mandatory. If you remove pub from enum RecordingStreamError you get compilation errors.

Let's just make RecordingStreamError public. Adding wrappers around everything is very annoying at this early stage in the project. A PR would be very welcome!

Done: #3786

emilk added a commit that referenced this issue Oct 10, 2023
* Closes #3735

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested [demo.rerun.io](https://demo.rerun.io/pr/3777) (if
applicable)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/3777)
- [Docs
preview](https://rerun.io/preview/a7dd4dce56f3fec82e3ab5e38c1f3f38606e249c/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/a7dd4dce56f3fec82e3ab5e38c1f3f38606e249c/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://ref.rerun.io/dev/bench/)
- [Wasm size tracking](https://ref.rerun.io/dev/sizes/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
😤 annoying Something in the UI / SDK is annoying to use 🏎️ Quick Issue Can be fixed in a few hours or less 🦀 Rust API Rust logging API
Projects
None yet
3 participants