`clone!`: Change default for weak ref upgrade failure from return to panic

[Hofer-Julian]

At the moment if clone! fails to upgrade a weak reference it logs a message and returns ().

    button_decrease.connect_clicked(clone!(@weak button_increase =>
        move |_| {
            number.set(number.get() - 1);
            button_increase.set_label(&number.get().to_string());
    }));

When this occurs this is probably a bug. Changing the default to panic would make this more obvious.

Disadvantages:

• Breaking change from old behavior that is not easy to detect
• For gtk apps it is questionable if you want the app of a user to crash because of this

@sdroege, @YaLTeR

[sdroege]

Also CC @bilelmoussaoui @GuillaumeGomez

[YaLTeR]

Breaking change from old behavior that is not easy to detect

FWIW I am currently in process of using this default exactly for log-and-return, so yeah, that'll be a breaking change.

For gtk apps it is questionable if you want the app of a user to crash because of this

That's a question even when you're not using the macro. In most cases you don't intend upgrade to ever fail, but occasionally you don't foresee some sequence of events, or you refactor something elsewhere, that causes the upgrade to fail in some edge case, in which case you probably don't want to have a panic? Somewhat of a philosophical question :p

[sdroege]

in which case you probably don't want to have a panic? Somewhat of a philosophical question :p

Or you want a panic so it fails very loudly so you can fix it :)

[YaLTeR]

Or you want a panic so it fails very loudly so you can fix it :)

That's all well and good if you hit it during testing...

[jf2048]

Copying this comment from matrix because I think it's relevant:

slomo: personally i think the correct behaviour would've been: @weak -> gives Option<T>, @weak-panic -> panics on upgrade failure, @weak-default 123 -> defaults to 123 on upgrade failure, @weak-return None -> returns None on upgrade failure

I would probably revise this to something like this, for clarity:

• @weak obj -> obj.upgrade()
• @weak obj or panic -> obj.upgrade().unwrap()
• @weak obj or 123 -> obj.upgrade().unwrap_or_else(|| 123)
• @weak obj or return 123 -> match obj.upgrade() { None => return 123, Some(obj) => obj }

I might also go a step further and say that only @weak should be supported, everything else is probably not the correct behavior for a function holding a weak reference, or would be more clear if the caller had to write out the unwrap at the top of the function. If something is returning () or panicking, chances are it just wants the handler to be disconnected via watch_closure. For GTK in particular, it's uncommon for signals to have return types, so in most cases that is what you want.

It seems it would not be hard to detect the changes if @weak was changed to return an Option by default, that would probably cause a compile error everywhere. But it would still break quite a lot of code.

[sdroege]

If something is returning () or panicking, chances are it just wants the handler to be disconnected via watch_closure

Counterpoint here would be that I have a couple of callbacks that I want to be no-ops if any of the multiple weak references can't be upgraded, but watch_closure only works for a single one.

Apart from that it seems like the simple-action-on-upgrade-failure feature is quite popular, so keeping those variants in one way or another seems useful.

[jf2048]

IMO if we are breaking the clone API, it may be possible to go to using an attribute macro:

#[glib::clone_block]
fn build_ui() {
    // ...
    add_button.connect_clicked(move |#[strong] window, #[weak] model, _button| {
        // ...
        dialog.connect_response(move |#[weak] model, #[weak] entry, #[weak] spin_button , dialog, resp| {
            // ...
        }));
        // ...
    }));
    // ...
    gtk::ListItem::this_expression("item")
        .chain_closure::<String>(#[closure] |#[watch] widget, _item: gtk::ListItem| {
                // ...
            }
        ))
        .bind(/* ... */);
    // ...
}

Just a suggestion, I don't know if this is better or worse.

[GuillaumeGomez]

I'm not sure to see how this is better...

[jf2048]

It works with rustfmt.... But otherwise is the same

[Hofer-Julian]

I finally had time to give @jf2048 proposals a proper look so here are my two cents:

   @weak obj -> obj.upgrade()
   @weak obj or panic -> obj.upgrade().unwrap()
   @weak obj or 123 -> obj.upgrade().unwrap_or_else(|| 123)
   @weak obj or return 123 -> match obj.upgrade() { None => return 123, Some(obj) => obj }

This looks like a clear improvement over what we have now IMO.

I might also go a step further and say that only @weak should be supported, everything else is probably not the correct behavior for a function holding a weak reference, or would be more clear if the caller had to write out the unwrap at the top of the function.

Not sure how many valid use cases there are for moving strong references into a closure. Maybe more experienced people like @sdroege or @bilelmoussaoui can chime in here?

IMO if we are breaking the clone API, it may be possible to go to using an attribute macro:

That would be nice if it worked with rustfmt. At the moment we support more and more attribute macros though (action callbacks, signal callbacks,..). Do we anticipate that users would have to take care in which order they add these attributes to methods?

[sdroege]

Not sure how many valid use cases there are for moving strong references into a closure.

It depends on what you're doing. I regularly move strong references into signal handlers that would otherwise not survive.

[Hofer-Julian]

Not sure how many valid use cases there are for moving strong references into a closure.

It depends on what you're doing. I regularly move strong references into signal handlers that would otherwise not survive.

Would you start using the macro if it would work with rustfmt?

[sdroege]

Yes

[Hofer-Julian]

@jf2048 are you interested to work on an attribute macro as discussed above?

[jf2048]

I will look into this, I don't anticipate there will be many problems with macro order. The only thing we may have to avoid is using inside other internal proc macros.

[jf2048]

One thing to note from my experiments: ordering among attribute macros is fine, but using this means you can no longer use these captures from within macro_rules because those run after the attribute macros. The clone! macro still needs to be used in that case.

[sdroege]

Can this be closed then or is there something that would make sense to be done still?

[Hofer-Julian]

@jf2048 created a prototype changing the default and replacing it with an attribute macro. Since he plans to rewrite it, there will not be a PR anytime soon.

[sdroege]

That's a completely new macro though and not just doing what this issue is about. So let's close this here I guess :)