Traits not implemented for all boxed trait objects (e.g. Box<dyn WordSplitter + Send>)

[sdroege]

error[E0277]: the trait bound `Box<dyn WordSplitter + Send>: WordSplitter` is not satisfied
    --> text/wrap/src/gsttextwrap/imp.rs:224:59
     |
224  |                 let lines = textwrap::wrap(&current_text, options);
     |                                                           ^^^^^^^ the trait `WordSplitter` is not implemented for `Box<dyn WordSplitter + Send>`

I sent a PR for fixing that in 0.13 some months ago, but recent refactoring broke that once again.

[sdroege]

And this time it can't even be worked around without unsafe code because of the WordSplitterClone hack. This is the best I can come up with

#[derive(Debug)]
struct WrappedWordSplitter(Box<dyn textwrap::word_splitters::WordSplitter + Send>);

impl textwrap::word_splitters::WordSplitter for WrappedWordSplitter {
    fn split_points(&self, word: &str) -> Vec<usize> {
        self.0.split_points(word)
    }
}

impl Clone for WrappedWordSplitter {
    fn clone(&self) -> Self {
        WrappedWordSplitter(unsafe {
            std::mem::transmute::<
                Box<dyn textwrap::word_splitters::WordSplitter + 'static>,
                Box<dyn textwrap::word_splitters::WordSplitter + Send + 'static>,
            >(self.0.clone_box())
        })
    }
}

If WordSplitterClone would be implemented for Box<dyn WordSplitterClone + Send> then this would probably also work less annoyingly.

[mgeisler]

Hi Sebastian!

Uh oh, I'm starting to get the feeling I've managed to paint myself into a corner with my attempts at providing maximum compile-time flexibility 😄

I think I should run some benchmarks and see if this is actually faster or better (smaller binary?) than an approach with a bunch of enums using dynamic dispatch. This would mean reverting #357 and then do the same for the other two traits.

To make things extensible, the enums could looks like this, perhaps:

pub enum WrapSplitter {
    NoHyphenation,
    HyphenSplitter,
    #[cfg(feature = "hyphenation")]
    Hyphenation,
    Custom(Box<dyn Fn(&str) -> Vec<usize>>),
}

pub enum WordSeparator {
    #[cfg(feature = "unicode-linebreak")]
    UnicodeBreakProperties,
    AsciiSpace,
    Custom(Box<dyn Fn(&str) -> Box<dyn Iterator<Item = Word>>>),
}

pub enum WrapAlgorithm<'a, 'b> {
    #[cfg(feature = "smawk")]
    OptimalFit,
    FirstFit,
    Custom(Box<dyn Fn(&'a [Word<'b>], &'b [usize]) -> Vec<&'b [Word<'a>]>>),
}

This seems to compile on the playground, but I did not try to port the current code to these kinds of enums.

Do you think this would improve things from your perspective?

[sdroege]

This also wouldn't work for me because e.g. Box<dyn Fn(&str) -> Vec<usize>> is not Send and there's no way to make it so :)

I think your current implementation would work fine (apart from being quite heavy on the API side) if you implemented impl<T: YourTrait + ?Sized> YourTrait for Box<T> { ... } instead of impl YourTrait for Box<dyn YourTrait> { ... } everywhere.

But also adding a Send (and 'static) bound to the different traits in general seems like a good idea to make usage easier. You probably can't imagine how one could have a non-Send / non-'static implementation of these either.

---

Personally I think instead of having generic parameters for every possible configuration in your public API, I'd rather have this internal to your implementation unless it's really performance-critical and actually makes a difference to let the compiler unroll everything :) User-facing API with lots of generic parameters, lifetime parameters, custom traits, etc. is hard to use and learn.

Also keep in mind that a) Box<dyn YourTrait> only means an actual heap allocation if the type implementing the trait is non-zero-size, b) LLVM can de-virtualize calls to some degree during optimization so that all this can be inlined again instead of being a function call.

[mgeisler]

Hey @sdroege, just wanted to say that I haven't had time to look more at this since last week...

Personally I think instead of having generic parameters for every possible configuration in your public API, I'd rather have this internal to your implementation

Yes, very much this. Looking at the current API, it dies feel overly complex and that's why I wanted to see how a simpler alternative could work.

[mgeisler]

Hi Sebastian,

I took a second look at all the type parameters and the clone hack... and realized that it's time to clean it up. So the traits are now replaced by enums as suggested above. That happened in #438. The enums all implement Clone, so I believe they should be easy to work with for you and other advanced crates which wrap/extend Textwrap in this way.

I'll close this issue for now since it looks obsolete.

[sdroege]

Thanks, that simplifies things considerably :)

See https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/675

[mgeisler]

Nice, that's very encouraging to see! Thanks a lot for already validating the new API. I'll make a new release now.