servo / core-foundation-rs

Rust bindings to Core Foundation and other low level libraries on Mac OS X and iOS
Other
1.02k stars 223 forks source link

Improve type safety in cocoa crate #200

Open jrmuizel opened 6 years ago

jrmuizel commented 6 years ago

currently all of the interfaces are implemented as traits on Id. This means there's no type safety between objects. We should probably use something like the foreign_obj_type! macro from https://github.com/gfx-rs/metal-rs/blob/master/src/lib.rs to improve things here.

jrmuizel commented 6 years ago

https://github.com/SSheldon/rust-objc-foundation also contains some better infrastructure than we are currently using.

vbo commented 6 years ago

Could we just start by using newtype pattern around id for selected interfaces and incrementally move towards full type safety?

Also in this case I am wondering if we could make some methods safe. It seems if we can guarantee id can only be allocated through a typed constructor for the client there should be nothing unsafe about calling at least a subset of methods on it. Am I missing something?

E.g.

let mut window: NSWindow = NSWindow::init()
                                    .with_content_rect(rect)
                                    .with_style_mask(mask)
                                    .with_backing(backing)
                                    .with_defer(false)
                                    .build().unwrap();
let title: NSString = NSString::from_str("Hello world");
window.set_title(title)

should be safe.

jrmuizel commented 6 years ago

Yes, that sounds reasonable to me.

jrmuizel commented 6 years ago

https://github.com/gfx-rs/metal-rs/ is another project that has better type safety.

hardiesoft commented 6 years ago

Now that impl Trait has landed in stable rust, could using it for return types in place of -> id be an acceptable option?

This would be a breaking change. Does this crate have constraints that require it to work on earlier versions of rust?

I'd be happy to submit a PR if this seems like a good way forward. It seems simpler than newtyping all the things, but maybe there are some other type-safety benefits of going the newtype route that I am missing?

jrmuizel commented 6 years ago

Can you write up an example of what that code would look like?

hardiesoft commented 6 years ago

I think it's a pretty straightforward transformation of ids into impl {Object} really, which has the nice fringe benefit of giving proper completion in IntelliJ Rust/rls.

In this example, note that CALayer and NSLayoutDimension are not currently defined, these could be left as opaque ids for now, and replaced if/when those types get defined.

pub trait NSView: Sized {
    unsafe fn alloc(_: Self) -> impl NSView {
        msg_send![class("NSView"), alloc]
    }

    unsafe fn init(self) -> impl NSView;
    unsafe fn initWithFrame_(self, frameRect: NSRect) -> impl NSView;
    unsafe fn bounds(self) -> NSRect;
    unsafe fn frame(self) -> NSRect;
    unsafe fn display_(self);
    unsafe fn setWantsBestResolutionOpenGLSurface_(self, flag: BOOL);
    unsafe fn convertPoint_fromView_(self, point: NSPoint, view: impl NSView) -> NSPoint;
    unsafe fn addSubview_(self, view: impl NSView);
    unsafe fn superview(self) -> impl NSView;
    unsafe fn removeFromSuperview(self);
    unsafe fn setAutoresizingMask_(self, autoresizingMask: NSAutoresizingMaskOptions);

    unsafe fn wantsLayer(self) -> BOOL;
    unsafe fn setWantsLayer(self, wantsLayer: BOOL);
    unsafe fn layer(self) -> impl CALayer;
    unsafe fn setLayer(self, layer: impl CALayer);

    unsafe fn widthAnchor(self) -> impl NSLayoutDimension;
    unsafe fn heightAnchor(self) -> impl NSLayoutDimension;
    unsafe fn convertRectToBacking(self, rect: NSRect) -> NSRect;
}
hardiesoft commented 6 years ago

Ugh, never mind, this totally doesn't work the way I thought it did, and is in fact invalid syntax.

It's possible this, or something like it might work in the future: https://github.com/rust-lang/rfcs/blob/master/text/1522-conservative-impl-trait.md#limitation-to-freeinherent-functions

hardiesoft commented 6 years ago

This does work however:

pub trait NSView: Sized {
    unsafe fn alloc(_: Self) -> id where id:NSView {
        msg_send![class("NSView"), alloc]
    }

    unsafe fn init(self) -> id where id:NSView;
    unsafe fn initWithFrame_(self, frameRect: NSRect) -> id where id:NSView;
    unsafe fn bounds(self) -> NSRect;
    unsafe fn frame(self) -> NSRect;
    unsafe fn display_(self);
    unsafe fn setWantsBestResolutionOpenGLSurface_(self, flag: BOOL);
    unsafe fn convertPoint_fromView_(self, point: NSPoint, view: impl NSView) -> NSPoint;
    unsafe fn addSubview_(self, view: impl NSView);
    unsafe fn superview(self) -> id where id:NSView;
    unsafe fn removeFromSuperview(self);
    unsafe fn setAutoresizingMask_(self, autoresizingMask: NSAutoresizingMaskOptions);

    unsafe fn wantsLayer(self) -> BOOL;
    unsafe fn setWantsLayer(self, wantsLayer: BOOL);
    unsafe fn layer(self) -> id where id:CALayer;
    unsafe fn setLayer(self, layer: impl CALayer);

    unsafe fn widthAnchor(self) -> id where id:NSLayoutDimension;
    unsafe fn heightAnchor(self) -> id where id:NSLayoutDimension;
    unsafe fn convertRectToBacking(self, rect: NSRect) -> NSRect;
}
NeoLegends commented 6 years ago

What's the state on this? Newtypes seem like a suitable solution, no?