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

Make decoding result enum public #562

Merged
merged 1 commit into from Sep 2, 2016
Merged

Make decoding result enum public #562

merged 1 commit into from Sep 2, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jul 24, 2016

I was trying to compile a simple example using a jpeg decoder and noticed the DecodingResult enum is not public.

Example code I mentioned above: https://github.com/srdqty/rust-practice/blob/57cbc9e72d2429d6600c7adbdfc73bc6d027e1b5/image-hello-world/src/main.rs

@nwin
Copy link
Contributor

nwin commented Jul 24, 2016

How could that have happened?

Can you please remove the test? That's a bug in rustc it should have given an error. Thanks. 🙂

@ghost
Copy link
Author

ghost commented Jul 24, 2016

It seems like the problem is that there existed no tests trying to use that type outside of the private scope of the library. Before the change, the test does fail via compiler error.

I'm pretty new to rust so maybe I'm missing something related to the precise rules of visibility in rust. I'm not sure I understand why this would indicate a bug in rustc? Should the library itself (without the test) have failed to compile because the return type of a method on a public type was not also declared public?

@nwin
Copy link
Contributor

nwin commented Jul 24, 2016

Exactly, rustc usually disallows to expose a private type via a public interface. As such, before your change, the library should have failed to compile. This is why we don't need to test for it separately. I'm currently trying to reproduce this behavior in form of a minimal example.

@nwin
Copy link
Contributor

nwin commented Jul 24, 2016

@Apparently I'm mistaken currently it should only emit a warning (which it didn’t) instead of failing. Maybe this is also a feature of rustc, but then I don’t know how we could possibly keep track whether all types where properly re-exported.

@ghost
Copy link
Author

ghost commented Jul 24, 2016

Yeah, that does seem strange. Even if you want the type to be abstract and not export the definition, you would at least want the name to be public.

@ghost
Copy link
Author

ghost commented Aug 30, 2016

@nwin This PR should be good to merge?

@nwin
Copy link
Contributor

nwin commented Sep 2, 2016

Yes, don’t have much time lately.

@nwin nwin merged commit 0d3b652 into image-rs:master Sep 2, 2016
@ghost
Copy link
Author

ghost commented Sep 2, 2016

Awesome, thanks.

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