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

Rework type hierarchies #260

Merged
merged 30 commits into from Jun 18, 2019

Conversation

Projects
None yet
5 participants
@SimonSapin
Copy link
Contributor

commented May 20, 2019

This is a series of breaking API changes that implement the proposals from #258.

The following methods are added on Surface, although they are only relevant to surfaces created with {Pdf,Ps,Svg}Surface::for_{raw_,}stream. (They return None or Ok(()) in other cases.)

pub fn take_output_stream(&self) -> Option<Box<dyn Any>> {…}
pub fn take_io_error(&self) -> Result<(), io::Error> {…}
pub fn borrow_output_stream<'surface>(&'surface self) -> Option<RefMut<'surface, dyn Any>> {…}
pub fn borrow_io_error<'surface>(&'surface self) -> Result<(), RefMut<'surface, io::Error>> {…}
/// to ensure that all writes to the stream are done.
///
/// Use [`Box::downcast`] to recover the concrete type.
pub fn take_output_stream(&self) -> Option<Box<dyn Any>> {

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 20, 2019

Author Contributor

I’ve put these public methods on Surface, but they could instead be on PdfSurface, PsSurface, and SvgSurface.

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

It would seem nicer to not pollute the normal Surface API with that but have it only on the types where it can actually exist, just like the data related functions on ImageSurface.

What was your reasoning for having it on the main Surface type? Maybe I'm missing an advantage of doing so :)

src/pdf.rs Outdated
surface.set_size(100., 100.);
draw(&surface);
let custom_writer = surface.finish();
surface.finish();
let custom_writer = surface.take_output_stream().unwrap().downcast::<CustomWriter>().unwrap();

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez May 20, 2019

Member

Minor: Please use expect instead of unwrap, makes errors easier to spot when they occur (even more in tests!).

Show resolved Hide resolved src/ps.rs Outdated
@GuillaumeGomez
Copy link
Member

left a comment

Looks good to me, thanks!

}.into()
}

struct WriteEnv<'a, W: 'a + Write> {
writer: &'a mut W,
error: Option<Error>,
io_error: Option<Error>,
unwind_payload: Option<Box<dyn Any + Send + 'static>>,

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Can we have unwind and error? Maybe makes sense to use Either here to make it more explicit that only one of the cases can exist, but I'm fine with this here too :)

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

With the current behavior it’s possible to have both. However we may want to change the behavior to not attempt another write/read if a previous one returned an error or panicked.

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Seems reasonable to have all future read/writes fail

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

Done.

}

// Safety: unwinding into C is undefined behavior (https://github.com/rust-lang/rust/issues/58794)
// so code outside of the `catch_unwind` call must never panic.

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Isn't this actually defined behaviour now (panic) since one of the last few releases? But your solution is nicer of course.

We depend on this being defined behaviour in the other crates. Started to do so in 1.24, then it was backed out again for 1.25, and we were told that it was going to be stabilized again in some 1.3x release I can't remember right now. All this back and forth about this behaviour is really annoying.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

As the linked issue rust-lang/rust#58794 explains, abort-on-unwind-through-extern fn was landed then reverted. Twice. But even if it lands again, isn’t propagating the panic preferable to aborting the process?

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

In this specific case here we can do that, in general we unfortunately can't (there might be no well known place from which we would propagate the panic further) so aborting the process is the best we can do.

Not sure if we want to, yet another time, revert back to adding guards to the other crates to exit the process at the FFI boundary. It's getting boring and causes a lot of churn, and the undefined behaviour right now seems to be to abort the process...

Not complaining to you personally here btw :) Thanks for providing the link to the latest version of the adventure.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

there might be no well known place from which we would propagate the panic further

Indeed, #262 is such a case.

The PDF / PS / SVG output stream case is sort of in-between. And we’re not alone with these questions, the end of https://www.cairographics.org/manual/bindings-streams.html discusses it as well.

use Status;
use std::fmt;

#[derive(Debug)]
pub struct ImageSurface(Surface);

impl ImageSurface {
pub fn from(surface: Surface) -> Result<ImageSurface, Surface> {
impl TryFrom<Surface> for ImageSurface {

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Should make sure in the release notes that we communicate that we only support 1.34+ now.

Mesh(Mesh),
#[derive(Debug)]
pub struct Pattern {
pointer: *mut cairo_pattern_t,

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

ptr::NonNull?

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Should also create an issue to transform any other types to that instead of plain pointers, unless you want to do this as part of this PR too.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

I think that would work, here and for many (every?) other structs that wrap cairo pointers. But this PR is already large and using NonNull internally doesn’t affect the public API, so I’m happy to leave for later or someone else.

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Works for me :)

This comment has been minimized.

Copy link
@sdroege
/// The concrete type behind the `Box<dyn Any>` value returned by `take_output_stream`
/// is private, so you won’t be able to downcast it.
/// But removing it anyway ensures that later writes do no go through a dangling pointer.
pub unsafe fn for_raw_stream<W: io::Write + 'static>(width: f64, height: f64, stream: *mut W) -> Self {

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Should we also remove the 'static here to make it more generic and unsafe? :)

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

We can’t easily. That bound is required to keep even *mut W in a Box<dyn Any>.

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Makes sense, thanks


trait AnyExt {
/// Any::downcast_mut, but YOLO
unsafe fn downcast_mut_unchecked<T>(&mut self) -> &mut T {

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Does it make any measurable performance difference compared to the safe version?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin May 21, 2019

Author Contributor

I haven’t measured it, but I’m relatively confident about this particular aspect of the unsafety of the implementation. See the comment at the call site: https://github.com/gtk-rs/cairo/pull/260/files/cf2f6925ffe09f45429afd1940731b57454c6d49#diff-2763e33724cc9b239c750ae973480953R168

I did encounter segfauts initially when this method was a free function taking a &mut dyn Any parameter. I would pass it &mut Box<dyn Any> expecting auto-deref, but the unsizing coercion of Box<dyn Any> to dyn Any (adding another layer of trait object indirection) took precedence. So I would end up with a reference at the wrong address, even though debug_assert!(stream.is::<W>()); passed! Making this a method in an extension trait fixes this because unsizing coercion desn’t happen for method receivers like it does for call arguments.

This comment has been minimized.

Copy link
@sdroege

sdroege May 21, 2019

Member

Ok, it seems fine to me too so let's keep it then :) Thanks for the details

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

I’ve added some tests.

As the last commit (48ee818) shows, it’s easy to forget to check for IO errors and accidentally ignore them.

Calling finish in take_output_stream would exacerbate this, since finish can (and typically does) cause more writes to happen and these writes can return an IO error. So it might be better to have a single method that combines finish, checking for error, and returning the stream. However the signature can quickly become unwieldy.

fn finish_output_stream(&self) -> (Option<Box<dyn Any>>, Result<(), io::Error>)

However a tuple doesn’t benefit from #[must_use], so a top-level Result might be preferable. But a user might want recover the stream even in the error case.

fn finish_output_stream(&self) -> Result<Option<Box<dyn Any>>, (io::Error, Option<Box<dyn Any>>)>

Maybe we culd simply a bit by removing the Option layer and returning Box<dyn Any> of a private zero-size type instead of None (when the output stream was already taken, or there wasn’t one in the first place). This would make downcast always return an error in those cases (since users do not have access to this private type). This makes user code less noisy (with potentially fewer unwrap calls) but maybe it becomes less easy to follow everything that goes on.

fn finish_output_stream(&self) -> Result<Box<dyn Any>, (io::Error, Box<dyn Any>)>

A Result value can be used with the ? operator, but perhaps not easily if the error type is a tuple that doesn’t implement any conversion. So we could add a dedicated error type:

impl Surface {
    fn finish_output_stream(&self) -> Result<Box<dyn Any>, ErroredStream> {…}
}
pub struct ErroredStream {
    pub error: io::Error,
    pub stream: Box<dyn Any>,
}
impl From<ErroredStream> for io::Error {
    fn from(e: ErroredStream) -> Self {
        e.error
    }
}

… but yet another type adds “weight” to the API and doesn’t feel great.

Any opinion?

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented May 31, 2019

@SimonSapin: I have a preference for the last one. Yet another type but so much easier to read than the previously suggested...

@sdroege

This comment has been minimized.

Copy link
Member

commented May 31, 2019

I agree, the last one seems cleanest. And it probably makes sense to not provide a non-finishing take function for the stream but to ensure that the surface is first finished (i.e. only have the new function you propose here). It does not seem nice or useful to take away the stream before the surface is finished.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@SimonSapin Thanks for thinking through the various options here. FWIW I'd also say the last one is preferable.

Nit: Perhaps the error conversion impl could be generalized like:

Edit: Nvm, bad idea...

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Eh, sorry I wasn’t clear. I didn’t mean this as a closed set of signatures to choose from. Rather, there’s a series of trade-offs that don’t have an obvious answer to me. For example:

  • Is an Option useful to separately signal cases where the surface doesn’t have an output stream (or where it was already taken), or is Box<dyn Any> of a dummy private concrete type a good idea and not too hard to understand?
    • Or even should these APIs panic it those cases?
    • Should the answer be the same for the two cases?
  • Is it worth having the Result at the "top-level" even if this makes the signature more complex? (With the stream type in two places)
    • If so, does a “stream + IO error” struct carry its weight compare to a tuple?
    • Or should we have neither because no-one even care about recovering the stream in the error case?

@vojtechkral I think that impl coherence rules would not allow this, because neither T nor From is crate-local. And even if it did, this kind of overly-generic API can become hard to use as more turbofishes are required because type inference doesn’t have enough context to determine an "intermediate" type parameter.

@sdroege sdroege referenced this pull request Jun 4, 2019

Closed

New release #73

10 of 10 tasks complete
@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Is an Option useful to separately signal cases where the surface doesn’t have an output stream (or where it was already taken), or is Box<dyn Any> of a dummy private concrete type a good idea and not too hard to understand?

* Or even should these APIs panic it those cases?

* Should the answer be the same for the two cases?

My vote here would be to panic. If it's called on a surface that the user did not create with a stream, then there's something very wrong in the user code.

Is it worth having the Result at the "top-level" even if this makes the signature more complex? (With the stream type in two places)

* If so, does a “stream + IO error” struct carry its weight compare to a tuple?

* Or should we have neither because no-one even care about recovering the stream in the error case?

IMHO the struct with stream+error in a Result gives the nicest API here

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

My vote here would be to panic. If it's called on a surface that the user did not create with a stream, then there's something very wrong in the user code.

If it potentially comes from user code, then I'd prefer avoid panics if possible. But not sure if this is better in here...

@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

If it potentially comes from user code, then I'd prefer avoid panics if possible. But not sure if this is better in here...

What do you expect the user to do, other than panicking, with the None that they expect to be Some(stream) in their code? :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Basically yes. 😆

SimonSapin added some commits Jun 4, 2019

Remove borrow_output_stream and borrow_io_error
Using them correctly pretty much requires calling `Surface::finish`,
at which point one might as well use the `take_*` methods.
@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Thanks, lgtm :)

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Alright. The signature is now:

pub fn finish_output_stream(&self) -> Result<Box<dyn Any>, StreamWithError>

It panics when called more than once, or if there was no output stream to begin with.

I’ve removed the borrow_* methods. While it’s cool that they can safely be implemented, they’re not useful without also calling Surface::finish at which point the surface pretty much can’t be used anymore so one might as well call finish_output_stream instead.

@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@vojtechkral @GuillaumeGomez @gekola @meh Does it look good to all of you too? IMHO we're done here now :)

@EPashkin

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

LGFM, but doc-check better fixed by updating lgpl-docs or adding src/stream.rs etc. to IGNORES in build.rs

@vojtechkral

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

lgtm, @SimonSapin thanks for doing all this

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

As promised to @SimonSapin, he did his part so I'll check what's missing on the other repositories then merge this PR. Thanks a lot @SimonSapin !

@sdroege

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Indeed, thanks a lot for cleaning this up @SimonSapin :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

It'll require to update gdk. I'll do it shortly. Thanks a lot @SimonSapin !

@GuillaumeGomez GuillaumeGomez merged commit 59363aa into gtk-rs:master Jun 18, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.