fengalin commented 2 years ago

Update

My views on this topic have evolved, see the comment for an up to date version: https://github.com/gtk-rs/gtk-rs-core/issues/805#issuecomment-1288079608

Following is the initial obsolete comment for the record.

Another `Into<Option<_>>` experiment

Obsolete: see update in https://github.com/gtk-rs/gtk-rs-core/issues/805#issuecomment-1288079608

In gstreamer-rs a fix for nullability inconsistency had an API change induce an update to all the call sites for a function fromq.set_uri(uri) to q.set_uri(Some(uri)). This revived the debate whether Into<Option<_>> should be used in argument position when the argument is optional.

This debate was previously concluded: "Into<Option<_>> considered harmful". The main arguments leading to this conclusion are:

In some cases, this requires annotations, which can result messy, particularly when dealing with closures.
Users can be confused and not figure out that the API can also accept None.

We wanted to figure out whether the problems encountered at the time should still be considered harmefull and whether Into<Option<_>> should be reintroduce, at least in some cases, as we ended up doing for set_uri.

TLDR

The TLDR is the above arguments are still valid:

Type annotations are still necessary in some cases when we want to pass None. We probably don't want to use this with closures or complex function arguments.
When users find code such as q.set_uri(uri), they will probably not guess they can use q.set_uri(None) too.

Nevertheless, code looks nicer IMO when at least some of these arguments use this feature. I conducted an experiment on gstreamer-rs changing most candidates in manual code and updated gst-plugins-rs to use the resulting code base. See the difference in this gst-plugins-rs commit.

In this issue, I'd like to show the result of a quick experimentation on gtk-rs-core/glib to illustrate the pros & cons. The changes and some illustrative test cases are available in this gtk-rs-core/glib commit.

The easy cases

There are easy cases for which there's no issue apart from the users not immediately figuring out that the arguments can be an Option.

`Copy + Clone` types

The Copy + Clone types handling is straightforward with limited impact on the function signature and body.

See the Channel::new implementation and the above test cases.

Reference to concrete types

When the argument's inner type is a reference to a concrete type, the implementation is quite straightforward too, with the addition of a lifetime.

See the DateTime::from_iso8601 implementation and the above test cases.

The less easy cases

Reference to a type by one of its trait implementation

This is where it starts to get tricky.

Strings

One very common case for this is when the argument is a string, like in set_uri. Currently, most functions use an Option<&str> for these kind of arguments. This allows using:

    q.my_function(Some("a `str` literal"));
    q.my_function(Some(&a_string)); // reference is mandatory
    q.my_function(None); // type for `None` is non-ambiguous

With the changes in ParamSpecBuilderExt::set_nick, besides the same Some variants, we get:

    q.my_function("a `str` literal");
    q.my_function(&a_string); // reference is still mandatory
    q.my_function::<str>(None); // type for `None` needs disambiguation

The None case is unfortunate. It shows up here, but it actually was already an issue for others use cases which lead to the introduction of the NONE constant for some types.

The signature is a bit ugly, the ?Sized bound is necessary to accept the str literal.

I tried to have the signature accept plain String, but I gave up.

IMO the type annotation annoyance is acceptable compared to the usability improvement. There are many of these in this gst-plugins-rs commit.

Subclasses

Another common use case involves subclasses. See SignalGroup::set_target as an example. In this case, with current API, we need to use the NONE constant:

    // type for `None` needs disambiguation
    SignalGroup::new(Object::static_type()).set_target(Object::NONE);

Disambiguation is still necessary with Into<Option<_>>, though we can use this instead:

    // type for `None` needs disambiguation
    SignalGroup::new(Object::static_type()).set_target::<Object>(None);

Which could render the NONE constants unnecessary. Note that this could be possible with current API if the type was declared as a generic argument <T: IsA<_>> instead of an impl IsA<_>.

IMO, when dealing with subclasses this is useful as it leads to leaner code. It might not be applicable to all functions though.

Functions and closures

The last example combines two types from their traits. One is a Path, so it is quite similar to the str case and the other is a function trait. This can be seen in the spawn_async implementation.

Of course, there's nothing special about the plain and Some cases. For the None case, we get to provide type annotation for the None argument:

    // type annotation needed for 1st arg due to `None`
    //                       v
    let _res = spawn_async::<path::Path, _>(
        None,
        &[path::Path::new("test")],
        &[],
        SpawnFlags::empty(),
        || {},
    );

    // type annotation needed for both args
    //                       v           v
    let _res = spawn_async::<path::Path, fn()>(
        None,
        &[path::Path::new("test")],
        &[],
        SpawnFlags::empty(),
        None,
    );

For some reason I can't immediately explain, using a NONE constant doesn't work:

    const SPAWN_ASYNC_FN_NONE: Option<fn()> = None;

    // Compilation fails, but shouldn't IMO:
    //                 cannot infer type v
    let _res = spawn_async::<path::Path, _>(
        None,
        &[path::Path::new("test")],
        &[],
        SpawnFlags::empty(),
        SPAWN_ASYNC_FN_NONE,
    );

This is getting tricky. The function signature is mimimalist here, but it would become unacceptable to impose a full signature to users who only want to pass None is the first place.

Conclusion

IMO we should use Into<Option<_>> on a case by case basis:

It was useful for the string arguments in all the cases I encoutered so far.
It is also useful for the subclass traits args IMO. In the MR for the experiment on gstreamer-rs, we started discussing cases where we may not want to do it.
We should probably avoid this for the functions and closures args, except for really simple cases.

Apart from the manual code, applying this to automatically generated code could be useful, so a change to gir would be necessary.

sdroege commented 2 years ago

The None case is unfortunate. It shows up here, but it actually was already an issue for others use cases which lead to the introduction of the NONE constant for some types.

The signature is a bit ugly, the ?Sized bound is necessary to accept the str literal.

I think both of these parts are problematic. The signature is probably an obstacle for new users, as is the requirement to do something special for the NONE case. The NONE constants for objects that we currently have could help with the latter but not with the former.

Also for objects we have this problem anyway regardless of any of this because of the IsA trait :)

I'm wondering if for strings it wouldn't actually be better to use impl Into<&str>. Then you can use None as it is, and for string-ish types that are not &str you can use &*val, val.as_ref(), val.as_str() or similar. That is probably easier to discover and you have a more readable signature.

For functions and closures we currently always use boxed dyn closures when an Option is involved because otherwise the None is very hard to construct because any kind of closure and function pointer type is actually unique, and in case of closures unnameable.

Usually a better approach for improving these APIs is to provide a function with and without the optional closure argument.

Just some initial thoughts so far.

sdroege commented 2 years ago

The string discussion also has connections to what is planned in https://github.com/gtk-rs/gtk-rs-core/pull/600#issuecomment-1263209470 FWIW

fengalin commented 2 years ago

The signature is probably an obstacle for new users, as is the requirement to do something special for the NONE case.

That's true, but with Rust expressive type system, "powerful" models can result difficult to understand to newcomers anyway. Luckily, we also have a powerful doc tooling (wink wink @GuillaumeGomez). With proper documentation, users should be able to figure out what can be done. Documentation shows up in the rust-analyzer popups, which I believe most users install. Some commonly used crates, such as futures, wouldn't be entirely understandable from the code itself IMO. I, for one, always open the documentation when I use a new crate. Only with poorly documented crates or when something does not behave as expected do I reach for the code.

fengalin commented 2 years ago

I gave this some more thoughts and made some changes to my experiment. This is also inspired by @sdroege's introduction of default() for Pipeline & Bin as well as a discussion we had regarding the possible introduction of an unset_uri to act like set_uri(None).

The resulting implementation makes no more uses of any Into<Option<_>> and is based on builder-like patterns or additional functions, depending on the use case. I think this is more in-line with Rust philosophy.

`Copy + Clone` types

The Channel example mimics the Pipeline::default approach, which seem obvious to me now: we are dealing with a constructor with one optional parameter.

Reference to concrete types

For DateTime::from_iso8601, the idea is similar: either we use the default (no time zone) or DateTime::from_iso8601_with_tz.

Strings

It depends: either we are in either of the 2 above cases, or it's a case of set_ / unset_ or discard_ or reset_.

By the way, in previous experiment, my use of the PSpecBuilder trait was not really significant as there was a nick setter that was designed to handle the Some case delagating to set_nick (the one I focussed on). In this particular example, I would advise to use an unset_ or discard_ function should the builder require the ability to unset something that was previously set before building.

Subclasses

For the SignalGroup::set_target, this is again a case of unset_ or discard_ or reset_ IMO.

Functions and closures

The spawn_async example was the one I though was too complicated for users in previous experiment. With the latest attempt, I used a pattern builder which I named SpawnAsyncInvoker since this is used to invoke a function, not to build an object. IMO, it is flexible enough and abstract the complexity from users.

This example applies to a free standing function, so I thought it was more straightforward to use this Invoker approach. For a method, we could just use a builder for the arguments to be passed to said method.

jf2048 commented 2 years ago

First pass of some traits for string conversions here: https://github.com/gtk-rs/gtk-rs-core/pull/600/commits/89b8f3fec624d58dfe9a90d38abdb83f3a56a679 https://github.com/gtk-rs/gtk-rs-core/pull/600/commits/a267ae71180d2c21e982f21f961eb03d0d88dc5e

Ideally these should be preferred over AsRef<str>

sdroege commented 1 year ago

As we have IntoOptionalGStr and such things now, it might make sense to reconsider this. You'd have to pass a None::<&str> or similar anyway, so we could do the same for other types.

However we should also impl IntoOptionalGStr for &GStr / &str / etc.

@bilelmoussaoui Any opinions here?

sdroege commented 1 year ago

See https://github.com/gtk-rs/gtk-rs-core/pull/891

GuillaumeGomez commented 1 year ago

As mentioned previously, it will increase the gtk-rs code complexity and also make documentation harder to read. Is the gain really worth it?

fengalin commented 1 year ago

@GuillaumeGomez did you reply to the initial comment or to the refined: https://github.com/gtk-rs/gtk-rs-core/issues/805#issuecomment-1288079608

@slomo I'll try to take a look in the coming days. As I said in the comment I refered to above, I believe explicit methods to discard, unset, etc. would be valuable IMO.

GuillaumeGomez commented 1 year ago

I was answering to @sdroege based on your original arguments and small tests I ran. Based on your refined comment, there is no more use of Into<Option<_>>. If so, then my comment can be ignored.

gtk-rs / gtk-rs-core

RFC: re-opening the `Into<Option<_>>` debate :) #805

Update

Another `Into<Option<_>>` experiment

TLDR

The easy cases

`Copy + Clone` types

Reference to concrete types

The less easy cases

Reference to a type by one of its trait implementation

Strings

Subclasses

Functions and closures

Conclusion

`Copy + Clone` types

Reference to concrete types

Strings

Subclasses

Functions and closures

gtk-rs / gtk-rs-core

RFC: re-opening the `Into<Option<_>>` debate :) #805

Update

Another Into<Option<_>> experiment

TLDR

The easy cases

Copy + Clone types

Reference to concrete types

The less easy cases

Reference to a type by one of its trait implementation

Strings

Subclasses

Functions and closures

Conclusion

Copy + Clone types

Reference to concrete types

Strings

Subclasses

Functions and closures

Another `Into<Option<_>>` experiment

`Copy + Clone` types

`Copy + Clone` types