Reintroducing the type parameter on Options (previously known as Wrapper).

[Cryptjar]

This PR reverts the internal changes of #206, but keeps the public API compatible to what it introduced. Essentially by adding a default for the type parameter.

However, now in addition to the dynamic dispatch by default, one may also explicitly use static dispatch by specifying a concrete type parameter. This now allows to construct an Options instance in const/static context (or introduces, it depending on the point of view, as it was possible before #206). Which is further facilitated by adding the const fns new_const and with_splitter.

Open Questions

• The name of Options::new_const could be changed (maybe to new_static). Or, for instance, the Options::new_const could replace the current Options::new entirely, reverting to a static dispatch by default. Then instead, a Options::new_boxed (or new_dyn or something) could be added which then creates a boxed Options with dynamic dispatch. Yet another alternative, would be to just remove Options::new_const, since the fundamental use-case (const fn) is also covered by Options::with_splitter.
• Whether to implement WordSplitter only for Box<dyn WordSplitter> or for all Box<S>
• Whether to implement splitter setter method only on the dynamic dispatch variant (aka Options<Box<dyn WordSplitter>>). Then, it might also take an arbitrary impl WordSplitter by value and put it into a Box.
• Now, it is also possible to make Options Clone again. Well, at least where the type parameter is Clone, which it isn't for the default Box<dyn WordSplitter>.

comments & other suggestions are welcome

[mgeisler]

Hi @Cryptjar,

Thanks for catching this! I had not thought about how changing to Box<dyn WordSplitter> would take away functionality, so thanks for restoring it!

As for your questions,

• The name of Options::new_const could be changed (maybe to new_static). [...] Yet another alternative, would be to just remove Options::new_const, since the fundamental use-case (const fn) is also covered by Options::with_splitter.

The situation that I to avoid is code like this

enum Wrapper {
    H(textwrap::Wrapper<'static, Standard>),
    N(textwrap::Wrapper<'static, textwrap::NoHyphenation>),
}

from gst-plugins-rs, discussed in #178.

I think I would lean towards let Options::new return a Options<Box<dyn WordSplitter>> by default so that all options can be changed in a uniform way afterwards. The Options::with_splitter constructor would then be the way to go if you want a hard-coded type.

• Whether to implement WordSplitter only for Box<dyn WordSplitter> or for all Box<S>

Are you suggesting adding an implementation for Box<S> where S: WordSplitter? I'm not quite sure what the implication of thi is, but if it makes it easier for people to use the library, then I'm all for it :-)

• Whether to implement splitter setter method only on the dynamic dispatch variant (aka Options<Box<dyn WordSplitter>>). Then, it might also take an arbitrary impl WordSplitter by value and put it into a Box.

Yeah, good question... I've so far though of the setter method as a builder method — something to use when contructing the Options. I was then thinking people would simply assign to the struct fields afterwards if something should be changed. I figured this would be the simplest API overall.

However, I'm very much curious to hear of better ways to design this.

• Now, it is also possible to make Options Clone again. Well, at least where the type parameter is Clone, which it isn't for the default Box<dyn WordSplitter>.

That would be nice 👍

[mgeisler]

Ah, this was the example you mentioned in your PR description. I'm not sure exactly what the pros/cons are of these two options.

[mgeisler]

I'll be happy to simply leave this out for now and then merge the rest — then we can make forward progress.

[mgeisler]

Thanks for writing a great commit message in your first commit — if you could squash the formatting commit into the first, then I'll be happy to merge this.

No big hurry, though, since I also want to redo the internal wrapping algorithm before I make a new release. I hope to have this out in a week or so.

[Cryptjar]

Sorry, for late response.

• Whether to implement splitter setter method only on the dynamic dispatch variant (aka Options<Box<dyn WordSplitter>>). Then, it might also take an arbitrary impl WordSplitter by value and put it into a Box.

Yeah, good question... I've so far though of the setter method as a builder method — something to use when contructing the Options. I was then thinking people would simply assign to the struct fields afterwards if something should be changed. I figured this would be the simplest API overall.

However, I'm very much curious to hear of better ways to design this.

Well, I don't know which one is good design, but there seems to be three reasonable ways to design the spiltter (builder) setter:

1. How it is right now:

impl<'a, S> Options<'a, S> {
    /// No type changes allowed, works for all `S`, requires a `Box<dyn _>`
    /// when coming from `new`
    pub fn splitter(self, splitter: S) -> Self {
        Options {
            splitter: splitter,
            ..self
        }
    }
}

2. Making it more general

impl<'a, S> Options<'a, S> {
    /// Allows type changes, works for all `S` & `T`, requires a `Box<dyn _>`
    /// when it should become `Options<Box<dyn _>>`
    pub fn splitter<T>(self, splitter: T) -> Options<T> {
        Options {
            width: self.width,
            initial_indent: self.initial_indent,
            subsequent_indent: self.subsequent_indent,
            break_words: self.break_words,
            splitter: splitter,
        }
    }
}

3. Restricting it to only the default type parameter (aka Options<Box<dyn WordSplitter>>)

impl<'a> Options<'a> {
    /// No type changes allowed, works only on `Options<Box<dyn _>>` (such as
    /// get from `new`), but allows all `T`, which is automatically boxed
    pub fn splitter<T: WordSplitter + 'static>(self, splitter: T) -> Self {
        Options {
            splitter: Box::new(splitter),
            ..self
        }
    }
}

While the 1st version is simpler to implement it is the less useful one, since it does not allow to change the type (so has the same effect as reassigning the field). The 2nd seems more useful (as it does more than just reassigning the field, i.e. the type change), but might be easier done by directly using the with_splitter constructor, unless the with_termwidth constructor is preferred. However, for the same reason, it is the most consistent one. The last and 3rd version, has a fundamental incompatibility with the with_splitter as it does an automatic boxing (which makes it in my opinion the ugliest), but maybe one of the more uniquely useful one, as it serves a purpose not directly covered by and other means (as it has auto-boxing unlike with_splitter and assigning the field).

Otherwise, it is pretty much a design question, implementable either way, or even, since there is now a with_splitter constructor, one might leave it out. I don't have a strong preference either, but I got a slight liking for option 2.

[Cryptjar]

Ok, I tried the 2nd version, and got a problem with the tests. Take for instance this one:

fn no_hyphenation() {
    let options = Options::new(8).splitter(Box::new(NoHyphenation));
    assert_iter_eq!(wrap("foo bar-baz", &options), vec!["foo", "bar-baz"]);
}

It would compile just fine, as did up until now, with the 1st version, since new explicitly creates a correctly boxed Options (that is with Box<dyn WordSplitter>), and in version 1, the type is preserved by splitter.

Now, with version 2, the splitter method, just converts it into an Options<Box<NoHyphenation>>, where right now Box<NoHyphenation> does not implement WordSplitter. This could be fixed by a few ways, first, one could expand the impl for WordSplitter on Box to a more general generic impl (as my before mentioned alternative does). Or, second, by coercing the Box<NoHyphenation> into a Box<dyn WordSplitter>, e.g. by placing a type annotation: let options: Options = ... . Alternatively, instead of dynamic dispatch, now and with version 2, also static dispatch could be used simply, by removing the Box making it a simple Options<NoHyphenation>.

As a note, with version 3, the tests also need to be rewritten to remove the Box, but then it would use dynamic dispatch instead of static.

I think, this kinda showcases the look&feel of the API for the different version, so I went on and made it into a list:

// Version 1, dynamic dispatch
let options = Options::new(8).splitter(Box::new(NoHyphenation));
// Version 1, static dispatch (falling back to `with_splitter`)
let options = Options::with_splitter(8, NoHyphenation);

// Version 2, dynamic dispatch
let options: Options = Options::new(8).splitter(Box::new(NoHyphenation));
// Version 2, static dispatch
let options = Options::new(8).splitter(NoHyphenation);

// Version 3, dynamic dispatch
let options = Options::new(8).splitter(NoHyphenation);
// Version 3, static dispatch (falling back to `with_splitter`)
let options = Options::with_splitter(8, NoHyphenation);

Apparently only version 2 allows the dynamic dispatch type of new to be changed to static dispatch, but requires the type annotation to coerce it into a trait object. Also notice that only version 2 allows for static-to-static conversion (i.e. from Options<NoHyphenation> to Options<HyphenSplitter>).

[Cryptjar]

Considering the problem of #178 at hand, there are essentially two ways to tackle it, I'm not sure whether they were both considered. First, what I gonna call 'inner boxing' is what #206 introduced. But when I noticed, that the splitter is the final field of Options, I though, this allows it to be turned into a DST (dynamically sized type), which then allows a strategy I call 'outer boxing'. Both basically serve the same goal, allowing abstracting over the concrete WordSplitter by using a trait object.

However, I figured, that this 'outter boxing' strategy is fairly simple to implement, so I went to implement it for the 0.12.1 version, I guess it will also best demonstrate what I mean by that. Eventually, it really turned out to be a fairly minimal commit and it is still backwards compatible (so it would be even possible to actually release it as 0.12.2):
0.12.1...Cryptjar:impl-outside-boxing-in-0.12

I essentially just wanted to show off that option. Honestly, because, if that 'outer boxing' would be adopted instead of the here currently implemented 'inner boxing', one could remove the spacial cases of the Options<Box<dyn WordSplitter>> and revered back to 'only' static dispatch within Options. Then whenever one needs dynamic dispatch or others wants to get rid of the type parameter (as in #178), one could just wrap the entire Options into a Box or Rc, or whatever suite them and coerce it into a trait object. (Notice that I also included two test cases, which show how it could be used).

So in summary, I don't think there is anything wrong with using the 'internal boxing' as it was introduced in #206 and still persists in this PR, but in case that alternative had not been considered before, now (before it gets released) is probably the best time to so.

Regarding the API, I thought I make another list:

// 'inner boxing' (version 2)
// dynamic dispatch
let options: Options<Box<dyn WordSplitter>> = Options::new(8).splitter(Box::new(NoHyphenation));
// static dispatch
let options: Options<NoHyphenation> = Options::new(8).splitter(NoHyphenation);

// 'outer boxing'
// dynamic dispatch
let options: Box<Options<dyn WordSplitter>> = Box::new(Options::new(8).splitter(NoHyphenation));
// static dispatch
let options: Options<NoHyphenation> = Options::new(8).splitter(NoHyphenation);

For the 'inner boxing', I chose the 2nd version since it has the most consistent API, which becomes even more apparent when comparing it to the 'outer boxing' approach. I guess it makes it quite obvious why I call it 'inner' & 'outer' boxing. One thing to notice tho, in my opinion, using the 'outer boxing' approach make the usage of the actual Options API more consistent despite whether the crate user chooses static or dynamic dispatch.

[mgeisler]

Hi @Cryptjar, thanks for the updates! I'll try to take a look at it tomorrow — I think I roughly understand what's going on, otherwise I'll ask you for help :-) The "outer boxing" API does indeed look very nice and consistent 👍

[mgeisler]

unless the with_termwidth constructor is preferred.

I consider that to be a convenience method that people can use if they don't have any particular fancy requirements. If they do, then I'm fine with asking them to use a more complicated and advanced API.

in my opinion, using the 'outer boxing' approach make the usage of the actual Options API more consistent despite whether the crate user chooses static or dynamic dispatch.

If I understand correctly, the with_splitter constructor could be removed then? Since Options::with_splitter(42, Foo) would be equivalent to Options::new(42).splitter(Foo)?

[mgeisler]

Considering the problem of #178 at hand, there are essentially two ways to tackle it, I'm not sure whether they were both considered.

Nope, they weren't — I have yet to work with an unsized type of my own. It seems like an advanced type, so my gut instinct would be to avoid it unless it's really necessary because it brings some great benefit.

I essentially just wanted to show off that option. Honestly, because, if that 'outer boxing' would be adopted instead of the here currently implemented 'inner boxing', one could remove the spacial cases of the Options<Box<dyn WordSplitter>> and revered back to 'only' static dispatch within Options. Then whenever one needs dynamic dispatch or others wants to get rid of the type parameter (as in #178), one could just wrap the entire Options into a Box or Rc, or whatever suite them and coerce it into a trait object. (Notice that I also included two test cases, which show how it could be used).

Thanks for adding the tests, very useful!

I just pushed a little demo program that I've been working on for some time: #218. In the example, I have code like this:

                Key::Ctrl('b') => options.break_words = !options.break_words,
                Key::Ctrl('s') => {
                    let idx = idx_iter.next().unwrap();  // Next WordSplitter index
                    std::mem::swap(&mut options.splitter, &mut splitters[idx]);
                }

That is, there is one Options instance and it is then manipulated based on interactive input from the user. This works fine because the WordSplitters in splitters are all boxed.

I think the approach you suggest in this PR is compatible with such code — I believe this PR simply adds the possibility of static dispatch in addition to dynamic dispatch via a boxed WordSplitter.

Does the approach in the impl-outside-boxing-in-0.12 branch also support this?

[Cryptjar]

unless the with_termwidth constructor is preferred.

I consider that to be a convenience method that people can use if they don't have any particular fancy requirements. If they do, then I'm fine with asking them to use a more complicated and advanced API.

Fair enough. Actually, the const fn functions serve a similar purpose, since ultimately one could just construct an Options in-place, but for less fancy requirements it would a bit burdensome to specify all fields by hand.

in my opinion, using the 'outer boxing' approach make the usage of the actual Options API more consistent despite whether the crate user chooses static or dynamic dispatch.

If I understand correctly, the with_splitter constructor could be removed then? Since Options::with_splitter(42, Foo) would be equivalent to Options::new(42).splitter(Foo)?

Functional it is true, but unfortunately there is no way to make the splitter method a const fn with current stable Rust (and it does not seem that it is going to, it's something about calling drop, which is generally not a const fn). So, the with_splitter constructor serves the essential purpose of being a const fn.

However, the problem with the splitter method is the generic S which might have a Drop impl, everything else is even Copy, so it is imaginable to change the signature from by-value (self) to by-reference (&self), then the function itself does not have to deal with drop and all other fields can be copied. Through, this would only work with the splitter method, no other builder/setter.

impl<'a, S: ?Sized> Options<'a, S> {
    pub const fn splitter<T>(&self, splitter: T) -> Options<'a, T> {
        Options {
            width: self.width,
            initial_indent: self.initial_indent,
            subsequent_indent: self.subsequent_indent,
            break_words: self.break_words,
            splitter: splitter,
        }
    }
}

// Assuming `new` is a `const fn` too, i.e. static dispatch
static HYP: Options<HyphenSplitter> = Options::new(10);
// Appearently no `drop` is called, and `splitter` just generate a new Options
static NO_HYP: Options<NoHyphenation> = HYP.splitter(NoHyphenation);
// However, this works too, due to some Rust magic.
static NO_HYP_2: Options<NoHyphenation> = Options::new(20).splitter(NoHyphenation);

Considering the problem of #178 at hand, there are essentially two ways to tackle it, I'm not sure whether they were both considered.

Nope, they weren't — I have yet to work with an unsized type of my own. It seems like an advanced type, so my gut instinct would be to avoid it unless it's really necessary because it brings some great benefit.

Well, strictly speaking the approach in the impl-outside-boxing-in-0.12 branch does not make the Wrapper an unsized type, but it allows it to do so. Thus, I guess a normal user would (as before) never have to deal with an unsized value or some such. But for the 'more advanced' users, it allows them to turn it into an unsized type.

I just pushed a little demo program that I've been working on for some time: #218.

Very interesting. Good demo & good use-case.

I think the approach you suggest in this PR is compatible with such code — I believe this PR simply adds the possibility of static dispatch in addition to dynamic dispatch via a boxed WordSplitter.

Yes, that is true, this PR right now is even backwards compatible (with the previous commit).

there seems to be three reasonable ways to design the spiltter (builder) setter:
1. How it is right now:
2. Making it more general
3. Restricting it to only the default type parameter (aka Options<Box<dyn WordSplitter>>)

Those (except for 1), would strictly speaking be no longer backwards compatible, tho it wouldn't impact functionality.

Does the approach in the impl-outside-boxing-in-0.12 branch also support this?

That gave me some hard time thinking. Eventually, I concluded that it is not possible with the impl-outside-boxing-in-0.12 implementation. Apparently, due to the 'outter boxing', an already boxed WordSplitter wouldn't be that useful (actually, since that branch does not have the WordSplitter impl for Box, it is utterly useless). A more intuitive way would be to store a Vec<Box<Options<dyn WordSplitter>>> and just using one of them to layout the text. This however has the disadvantage of storing all configurations redundantly.

It got me further thinking: with that approach, it actually is a bit hard to just replace the splitter (assuming dynamic dispatch) in general, since once you got a Box<Options<dyn WordSplitter>>, you have no longer sized access to the splitter field (as it is an unsized dyn WordSplitter), which makes replacing it rather impossible. So, what I did in the test-cases there was to replace the entire Options with one that happened to have a different splitter. Yet, interestingly, when using my above drafted splitter method (the one with the &self), this pattern becomes much easier, as you can easily obtain a clone of an existing Options, with just the splitter replaced (possibly with a different type), which then can be boxed again and used to replace the entire Options (but it effectively only replaces the splitter). Take for example:

// assuming the `impl-outside-boxing-in-0.12` with the splitter method from above
#[test]
fn boxing_outside_replace_splitter() {
    let mut dyn_box: Box<Wrapper<dyn WordSplitter>> = Box::new(Wrapper::new(10));

    assert_eq!(dyn_box.wrap("foo bar-baz"), vec!["foo bar-", "baz"]);

    // Just replace the splitter keeping the rest of the configuration in place
    dyn_box = Box::new(dyn_box.splitter(NoHyphenation));

    assert_eq!(dyn_box.wrap("foo bar-baz"), vec!["foo", "bar-baz"]);
}

So, that splitter works very well with the 'outer boxing' approach. Additionally, this allows the interactive demo to just use a single Options, the question then would be how to get the various splitter by-value without a Box<dyn WordSplitter>. Well, I ended up using a Vec of closures that would be given the current Options and return a fresh boxed Options with the new splitter but the old settings. I pushed it onto the impl-outside-boxing-in-0.12 branch too (however, I did rebase your interactive demo commit, so it is duplicated there):
0.12.1...Cryptjar:impl-outside-boxing-in-0.12

Finally, I just wanted note further, tho I state the 'outer boxing' and 'inner boxing' as different approaches (which they are when solving a concrete problem), they are not mutually exclusive for a library: i.e. one could image to allow a unsized S on Options allowing the 'outter boxing' approach and at the same time have impl of WordSplitter for Box (maybe also for Rc & friends) for the 'inner boxing' approach. Then each user can choose which approach to take. However, when it comes to convenience function, they could easily urge the user to go for only one of them.

[Cryptjar]

Well, I thought about these issues again. And it seems to me, that there are actually two different use-cases if comes to trait objects (dynamic dispatch), besides the more traditional use-cases where static dispatch is sufficient:

1. The use-case I call 'library style', which is what I deduced that Embedding the Wrapper object in a struct #178 needs. As I understood it, the author of Embedding the Wrapper object in a struct #178 is writing a library and wants to allow his users to specify some arbitrary text wrapper or more specifically any WrapOptions (which didn't exist back then). The maybe most idiomatic way to express this is by a trait object of that trait (i.e.: dyn WrapOptions).

   So the requirement here is only halfway solved by Change Wrapper.splitter from T: WordSplitter to Box<dyn WordSplitter> #206 (which also precede the WrapOptions trait). Yet, due to the introduction of that trait (Simplify API with only top-level functions #213) it is now possible for any user to implement their own WrapOptions, thus what I called 'inner boxing' is insufficient for this 'library style' use-case.

2. The use-case I call 'dynamic configuration' which is what the interactive demo (Add interactive word-wrapping demo #218) needs, is a bit like a strategy pattern. In this case one could also choose WrapOptions trait object especially if dealing with additional user defined WrapOptions. However, as seen in the interactive demo, it is more like switching between hyphenation and simple word-wrapping that only needs to change the splitter but nothing else. And apparently, the easiest way to do this seems to be using a boxed WordSplitter i.e. Options<Box<dyn WordSplitter>>, which is what I called 'inner boxing'.

One point to notice here, is that in 1st use-case 'library style' it is preferable to allow the broadest set of implementation & configurations, thus a Box<dyn WrapOptions> is superior over a Options<Box<dyn WordSplitter>> (which only allows the Options struct but not a usize). Additionally, since this use-case is described as a 'library style', the entire configuration would be done by the user, so for the library there is hardly any reason to interact (i.e. like altering some configs) with the WrapOptions other than using it to wrap text. Thus, the problems with the 'outer boxing' that I described in last comment about difficulties when trying to change the splitter, do not really apply here.

On the other hand, in 2nd use-case 'dynamic configuration' one does want to actively alter the configuration, thus using a general trait object (like a Box<dyn WrapOptions>) makes things very complicated, and it becomes easier to have some concrete type (like a Options) which can be easily inspected and each configuration can be altered individual, thus a Options<Box<dyn WordSplitter>> seems to be best here.

Also notice, that a Box<dyn WrapOptions> is similar to what I called 'outer boxing' but actually goes beyond what I described previously (which was a Box<Options<dyn WordSplitter>>). So I would call the Box<dyn WrapOptions> more like the 'proper outer boxing'.

---

In summary, I would recommend, to support both use-cases the 'inner boxing' and the 'proper outer boxing'. I will open a new PR for implementing the 'proper outer boxing'. However, when considering both, I would change the API to static dispatch by default, i.e. changing Options::new from returning a Options<Box<dyn WordSplitter>> to a simple Options<HyphenSplitter>.

Two reasons here. First, static dispatch didn't seem to be an issue for the average user, and it is the only supported 'operation mode' in v0.12, so everyone using textwrap today is using static dispatch. It does not seem very reasonable to me to change that.

Second, when the default would be dynamic dispatch via inner boxing, most people would going to use it (simply because Options::new returns it), which would be not so nice for the 1st use-case 'library style' from above, since there one really wants the 'proper outer boxing'. However, if they get their values from the user, and they use the 'default way' (i.e. 'inner boxing') these options would need to be boxed again into a 'properly outer boxed' Option i.e. a doubly boxed Options.

[Cryptjar]

I update this PR now to use static dispatch by default (962455c). In this context I made the Options::new now also a const fn and changed the splitter function to the 2nd version I described earlier:

2. Making it more general

impl<'a, S> Options<'a, S> {
    /// Allows type changes, works for all `S` & `T`, requires a `Box<dyn _>`
    /// when it should become `Options<Box<dyn _>>`
    pub fn splitter<T>(self, splitter: T) -> Options<T> {
        Options {
            width: self.width,
            initial_indent: self.initial_indent,
            subsequent_indent: self.subsequent_indent,
            break_words: self.break_words,
            splitter: splitter,
        }
    }
}

[mgeisler]

Thanks so much for the updates! I think it looks really good now. I like how you're able to basically remove all the noisy Box::new(...) calls I had to insert back in #213 — the new API looks cleaner while also being more flexible than before.

I've just merged my rewrite which dropped WrapIter (but kept Options, of course!) and I think it should be fairly simply to rebase your changes on top of the latest master. Then I'll be more than happy to merge them!

[Cryptjar]

I just rebased this PR.

[mgeisler]

Thank you for the patience with the repeated changes and thanks for fixing a problem I didn't even realize I had introduced :-)