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

Unshadow device re-exports #54

Merged
merged 1 commit into from
Sep 20, 2022
Merged

Conversation

vmx
Copy link
Contributor

@vmx vmx commented Sep 19, 2022

In the device module, all public items from cl3::device are re-exported. They even show up as public in the documentation. Nonetheless they aren't actually public.

To reproduce this issue, try to import opencl3::device::CL_UUID_SIZE_KHR, you will get an error like:

error[E0603]: constant `CL_UUID_SIZE_KHR` is private
    --> src/device.rs:2209:30
     |
2209 |             opencl3::device::CL_UUID_SIZE_KHR,
     |                              ^^^^^^^^^^^^^^^^ private constant
     |
note: the constant `CL_UUID_SIZE_KHR` is defined here
    --> /home/vmx/src/pl/filecoin/upstream/opencl3/src/device.rs:45:23
     |
45   |     CL_LUID_SIZE_KHR, CL_UUID_SIZE_KHR,
     |                       ^^^^^^^^^^^^^^^^

The private imports at

use cl3::ext::{
shadow the previous public re-exports
pub use cl3::device::*;

As most of the private imports are a subset of the public re-exports, we can just import the missing ones, which in this case is only cl_device_feature_capabilities_intel.

In order verify the subset, you can copy the list of imports from cl3::ext and the cl3::device re-exports from opencl-sys into two fiels and run tr ',' '\n|awk '{$1=$1;print}'|sort on each of them and finally diff the results.


I've never been a fan of glob imports, but I wasn't aware of that limitation in Rust. rust-lang/rust#31337 and https://rust-lang.github.io/rfcs/1560-name-resolution.html touches those topics in case someone is interested in all the details.


This is a regression, in earlier versions of opencl3 opencl3::device::CL_UUID_SIZE_KHR. used to be public.

In the `device` module, all public items from `cl3::device` are re-exported.
They even [show up as public in the documentation]. Nonetheless they aren't
actually public.

To reproduce this issue, try to import `opencl3::device::CL_UUID_SIZE_KHR`,
you will get an error like:

    error[E0603]: constant `CL_UUID_SIZE_KHR` is private
        --> src/device.rs:2209:30
         |
    2209 |             opencl3::device::CL_UUID_SIZE_KHR,
         |                              ^^^^^^^^^^^^^^^^ private constant
         |
    note: the constant `CL_UUID_SIZE_KHR` is defined here
        --> /home/vmx/src/pl/filecoin/upstream/opencl3/src/device.rs:45:23
         |
    45   |     CL_LUID_SIZE_KHR, CL_UUID_SIZE_KHR,
         |                       ^^^^^^^^^^^^^^^^

The private imports at
https://github.com/kenba/opencl3/blob/d004f1cf5f6ad78d158c1f67bb96ce1fed0190dc/src/device.rs#L18
shadow the previous public re-exports
https://github.com/kenba/opencl3/blob/d004f1cf5f6ad78d158c1f67bb96ce1fed0190dc/src/device.rs#L15

As most of the private imports are a subset of the public re-exports, we can
just import the missing ones, which in this case is only
`cl_device_feature_capabilities_intel`.

In order verify the subset, you can copy the list of [imports from `cl3::ext`]
and the [`cl3::device` re-exports from `opencl-sys`] into two fiels and run
` tr ',' '\n|awk '{$1=$1;print}'|sort` on each of them and finally diff the
results.

[show up as public in the documentation]: https://docs.rs/opencl3/0.9.0/opencl3/device/constant.CL_UUID_SIZE_KHR.html
[imports from `cl3::ext`]: https://github.com/kenba/opencl3/blob/d004f1cf5f6ad78d158c1f67bb96ce1fed0190dc/src/device.rs#L19-L45
[`cl3::device` re-exports from `opencl-sys`]: https://github.com/kenba/cl3/blob/9c97521ab27dd709e2ec9482855023ef0c5d4838/src/device.rs#L21-L106
@kenba
Copy link
Owner

kenba commented Sep 20, 2022

I've never been a fan of glob imports, but I wasn't aware of that limitation in Rust.

Normally I wouldn't use glob imports, but it made sense to me to import all the definitions from the cl3 device package,
I'm surprised that the cl3::ext imports hid them. Thank you for finding and fixing this issue.

@kenba kenba merged commit 94eac05 into kenba:develop Sep 20, 2022
@kenba
Copy link
Owner

kenba commented Sep 20, 2022

@vmx this change has now been published in version 0.9.1 of the crate.

@vmx vmx deleted the unshadow-public-re-exports branch September 21, 2022 07:17
@vmx
Copy link
Contributor Author

vmx commented Sep 21, 2022

Thanks for the quick turn-around time, it fixes the issues I was seeing!

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