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

object-safety and static methods #428

Closed
nrc opened this Issue Nov 3, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@nrc
Member

nrc commented Nov 3, 2014

RFC #255 glossed over the issue of static methods. With the current implementation (plus rust-lang/rust#18527) we ignore static methods when deciding on object-safety (pre rust-lang/rust#18527, we ICE'ed). However, this does not allow us to use object-safety to guide the use of trait objects with traits (the 'auto-impl's for trait objects). E.g.,

trait T {
    fn foo();
}

fn bar<X: T>(x: Box<T>) {
    X::foo();
}

fn main() {
    let y: Box<T> = ...;
    bar(y);
}

Here, the call to bar should be disallowed because the call X::foo() cannot be resolved because foo is static.

I see three options:

  • Require no static methods in object-safe traits (this is restrictive, due to the constructor issue)
  • Check that traits are 'static-safe' when doing the 'auto-impl' trick when binding a type variable to a trait (this is slightly unfortunate since the object-safety principle was supposed to mean no extra checks here, and instead only restricting the traits which could be used as objects)
  • Abandon object-safety at object creation time and instead check object-safety and 'static-safety' when doing the 'auto-impl' (this would be a shame, I think object-safety is a good principle)

We could also relax the check on static methods where the method is provided rather than required, since there is then an implementation for a method when making a static method call. However, this would lose the usual, expected overriding behaviour (the provided method would always be called, even if the dynamic concrete type overrides it).

I think my favoured solution is option 1 with the provided method relaxation. I believe (though might be wrong here) that there is only benefit to having static constructor methods in a trait, rather than a concrete type, if there is a provided default method. So, the only cases which become inconvenient are where there is a provided constructor and this is overridden in a concrete type which is used to make an object.

See also discussion in rust-lang/rust#18527

cc @aturon @nikomatsakis

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Nov 3, 2014

Contributor

I don't think having a default implementation in a constructor would be too common. At least, I have 2 use cases in hyper.

Here's one: I had to break up NetworkStream into NetworkStream and NetworkConnector. The Connector has one method: connect() that returns a NetworkStream. I originally had it as one trait.

// ideally in the future
trait NetworkStream {
    // 'constructor'
    fn connect(host, port, scheme) -> Self;

    // instance methods
    fn peer_name(&self) -> SocketAddr;
    fn set_timeout(&self);
}
Contributor

seanmonstar commented Nov 3, 2014

I don't think having a default implementation in a constructor would be too common. At least, I have 2 use cases in hyper.

Here's one: I had to break up NetworkStream into NetworkStream and NetworkConnector. The Connector has one method: connect() that returns a NetworkStream. I originally had it as one trait.

// ideally in the future
trait NetworkStream {
    // 'constructor'
    fn connect(host, port, scheme) -> Self;

    // instance methods
    fn peer_name(&self) -> SocketAddr;
    fn set_timeout(&self);
}
@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Nov 3, 2014

Member

@seanmonstar why do you have connect on the trait, rather than the concrete type? I'm trying to understand this corner of the design space a bit better. It seems in this case that connect might be better considered part of a NetworkStreamFactory abstraction, rather than a NetworkStream - is there a reason why not?

Member

nrc commented Nov 3, 2014

@seanmonstar why do you have connect on the trait, rather than the concrete type? I'm trying to understand this corner of the design space a bit better. It seems in this case that connect might be better considered part of a NetworkStreamFactory abstraction, rather than a NetworkStream - is there a reason why not?

@mitchmindtree

This comment has been minimized.

Show comment
Hide comment
@mitchmindtree

mitchmindtree Nov 3, 2014

Not sure if it's of as much interest to you guys, but I also have a use-case currently within my procedural music engine.

/// Generate a fill for the rhythm and append it to the end.                          
fn gen_fill(&mut self,                                                                
            attr: &SongAttributes,                                                    
            part_dur: &Duration,                                                      
            phrase: &Phrase,                                                          
            generated: &Vec<Vec<&Rhythm>>,                                            
            r: RhythmPack) {                                                          
    let mut temp: Self = Rhythm::empty(TimePack::from_attr(attr));                    
    let fill_duration = self.gen_fill_duration(attr);                                 
    temp.gen_section(attr, part_dur, fill_duration, phrase, generated, r);            
    let perc = (self.get_ticks() - temp.get_ticks()) as f32 / self.get_ticks() as f32;
    self.truncate(perc);                                                              
    self.append(&temp);                                                               
}                                                                                     

In saying this, I think I've managed to work out a way of moving the Rhythm trait into an enum instead, however it'll be a lot of work.

mitchmindtree commented Nov 3, 2014

Not sure if it's of as much interest to you guys, but I also have a use-case currently within my procedural music engine.

/// Generate a fill for the rhythm and append it to the end.                          
fn gen_fill(&mut self,                                                                
            attr: &SongAttributes,                                                    
            part_dur: &Duration,                                                      
            phrase: &Phrase,                                                          
            generated: &Vec<Vec<&Rhythm>>,                                            
            r: RhythmPack) {                                                          
    let mut temp: Self = Rhythm::empty(TimePack::from_attr(attr));                    
    let fill_duration = self.gen_fill_duration(attr);                                 
    temp.gen_section(attr, part_dur, fill_duration, phrase, generated, r);            
    let perc = (self.get_ticks() - temp.get_ticks()) as f32 / self.get_ticks() as f32;
    self.truncate(perc);                                                              
    self.append(&temp);                                                               
}                                                                                     

In saying this, I think I've managed to work out a way of moving the Rhythm trait into an enum instead, however it'll be a lot of work.

@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Nov 3, 2014

Member

@mitchmindtree could you link to the traits and function definitions please? And explain how this fits with the static method issue? I don't understand what is going on, sorry.

Member

nrc commented Nov 3, 2014

@mitchmindtree could you link to the traits and function definitions please? And explain how this fits with the static method issue? I don't understand what is going on, sorry.

@mitchmindtree

This comment has been minimized.

Show comment
Hide comment
@mitchmindtree

mitchmindtree Nov 3, 2014

@nick29581 my apologies, maybe I have the wrong idea of what's going on! I thought we were discussing use cases of static trait methods (that are constructor-like) in regards to your first option for dealing with object safety by disallowing them? Sorry if I've totally missed the mark here :)

edit: To clarify, I wouldn't be opposed to the decision, as I feel it would simply encourage me to use algebraic data types more often instead.

mitchmindtree commented Nov 3, 2014

@nick29581 my apologies, maybe I have the wrong idea of what's going on! I thought we were discussing use cases of static trait methods (that are constructor-like) in regards to your first option for dealing with object safety by disallowing them? Sorry if I've totally missed the mark here :)

edit: To clarify, I wouldn't be opposed to the decision, as I feel it would simply encourage me to use algebraic data types more often instead.

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Nov 3, 2014

Contributor

@nick29581 because each implementation of a NetworkStream may have some different way it connects, but I need the guarantee that the method will be there (compiler enforces this).

impl Request {
    fn new(method: Method, url: Url) -> Request {
        Request::with_stream::<HttpStream>(method, url)
    }

    fn with_stream<N: NetworkStream>(method: Method, url: Url) -> Request {
        let stream: N = NetworkStream::connect(...);
        Request { body: box stream as Box<NetworkStream>, ... }
    }
}

This gave me a few things:

  1. The possibility to create new types of streams, such as a MockStream to facilitate testing, and in the future, a Http2Stream. Others can make their own as well if desired.
  2. Boxing it lets the Request type be more ergonomic: all methods can just accept Request, instead of Request<HttpStream>, which is more to type and means other streams can't work there, even though there shouldn't be any reason a higher level function would care.

Essentially NetworkConnector did become a factory trait, but besides making the compiler happy about object-safety, there was no desire to do so. It means implementors need to import a second trait. And having connect() on the NetworkStream seemed just fine. It can be thought of as new().

Contributor

seanmonstar commented Nov 3, 2014

@nick29581 because each implementation of a NetworkStream may have some different way it connects, but I need the guarantee that the method will be there (compiler enforces this).

impl Request {
    fn new(method: Method, url: Url) -> Request {
        Request::with_stream::<HttpStream>(method, url)
    }

    fn with_stream<N: NetworkStream>(method: Method, url: Url) -> Request {
        let stream: N = NetworkStream::connect(...);
        Request { body: box stream as Box<NetworkStream>, ... }
    }
}

This gave me a few things:

  1. The possibility to create new types of streams, such as a MockStream to facilitate testing, and in the future, a Http2Stream. Others can make their own as well if desired.
  2. Boxing it lets the Request type be more ergonomic: all methods can just accept Request, instead of Request<HttpStream>, which is more to type and means other streams can't work there, even though there shouldn't be any reason a higher level function would care.

Essentially NetworkConnector did become a factory trait, but besides making the compiler happy about object-safety, there was no desire to do so. It means implementors need to import a second trait. And having connect() on the NetworkStream seemed just fine. It can be thought of as new().

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Nov 3, 2014

Contributor

I think that if we stick with object safety (which at the moment I still prefer, though it's a fine line), then we should be consistent and hence consider "static fns" as making a trait not "object safe".

Contributor

nikomatsakis commented Nov 3, 2014

I think that if we stick with object safety (which at the moment I still prefer, though it's a fine line), then we should be consistent and hence consider "static fns" as making a trait not "object safe".

nrc added a commit to nrc/rust that referenced this issue Dec 30, 2014

Forbid static methods in object safe traits
Closes rust-lang#19949 and rust-lang/rfcs#428

[breaking change]

If you have traits used with objects with static methods, you'll need to move
the static methods to a different trait.
@nrc

This comment has been minimized.

Show comment
Hide comment
@nrc

nrc Dec 30, 2014

Member

I see no other way than to forbid static methods and it turned out not to have much fallout, so we'll give it a go. Closing - tracking issue is rust-lang/rust#19949.

Member

nrc commented Dec 30, 2014

I see no other way than to forbid static methods and it turned out not to have much fallout, so we'll give it a go. Closing - tracking issue is rust-lang/rust#19949.

@nrc nrc closed this Dec 30, 2014

alexcrichton added a commit to alexcrichton/rust that referenced this issue Dec 31, 2014

rollup merge of rust-lang#20325: nick29581/obj-safety-static
Closes rust-lang#19949 and rust-lang/rfcs#428

[breaking change]

If you have traits used with objects with static methods, you'll need to move
the static methods to a different trait.

r? @cmr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment