Change clone/closure macro syntax to play better with rustfmt and other tooling

[sdroege]

E.g. the following is treated correctly by rustfmt again because it's all valid Rust syntax. Using only valid Rust syntax would also allow us to make use of syn/quote, which hopefully makes it easier to preserve spans correctly, and make rust-analyzer and other tooling work better too.

    let x = clone!(
        #[weak(x)]
        #[strong(self, rename_to = "y")]
        move |a, b, c| {
            print!("{a} {b} {c} {x} {y}");
        }
    );

[sdroege]

I'll probably look into this during the hackfest. In the meantime if anybody has syntax suggestions...

[sophie-h]

@felinira and I played around with this ages ago. I think that option c is a bit special but also much faster to type for me.

        let x = clone!(
            // a
            #[weak(x)]
            #[strong(self, rename_to = "y")]
            // b
            #[weak]
            x,
            #[stong]
            self as y,
            // c
            &'weak x,
            self as &'strong y,
            // return
            // a
            #[default_return(false)]
            // b
            #[default_return]
            false,
            // c
            return false,
            move |a, b, c| {
                print!("{a} {b} {c} {x} {y}");
            }
        );

For completeness: This one works as well. But I think it might be a bit confusing.

        let x = clone!(
            #[return_default]
            true,
            move |#[weak] x, #[strong] y: self, a, b, c| {
                print!("{a} {b} {c} {x} {y}");
            }
        );

[sdroege]

Interesting ideas, thanks! Just some initial impression / thoughts on these

I kind of like c for the references but it might indeed be a bit special while the attributes look more familiar. For the attributes, I think I would prefer b. Less verbose than a and equally descriptive.

For the return either a or b seem fine. c is strange because you have a return statement as a parameter, which you'd never have anywhere else.

The last one ("for completeness") is indeed to confusing IMHO. Those variables are captured from the environment, not parameters to the closure.

Maybe C++ lambda capture lists can also give some inspiration here but personally I think that syntax is too compressed and strange.

[DaKnig]

to me the most natural is a and a because it's what I would expect when coming from rust.

[sdroege]

b for the return value has the advantage that you get formatting for the whole expression, but maybe it should be made a closure then. Arbitrary Rust expressions inside the attribute would have to be written as a string.

#[default_return]
|| Foo::new(1, 2, 3).to_string()

[sophie-h]

Drawback of b is that the formatter wants two lines for one variable

[A6GibKm]

I feel like I prefer a, but I think syntax highlight and more complex expressions will have a better time with b.

[zecakeh]

To me, it feels like, a would be closer to the syntax of clone if it were an attribute macro:

#[clone(weak(x), strong(self, rename_to = "y"), default_return(false))]
move |a, b, c| {
  print!("{a} {b} {c} {x} {y}");
}

While b feels more natural for a function-like macro.

[felinira]

a would be closer to the syntax of clone if it were an attribute macro

Unfortunately attributes on closures are unstable, so this is not really an option. Otherwise this would be a nice solution indeed.

[sdroege]

The problem with that is also that attribute macros only contain string values, not code. For the default return you'd ideally want something that is treated as code so it gets proper formatting and handling by rust-analyzer.

[sdroege]

So, after playing around with all the options this is what I'm going to implement:

        let x = clone!(
            #[weak]
            x,
            #[strong(rename_to = "y")]
            self,
            #[default_return]
            || false,
            move |a, b, c| {
                print!("{a} {b} {c} {x} {y}");
            }
        );

That seems to have the least amount of strangenesses syntax-wise and keeps strings to identifiers.

[DaKnig]

why not rename_from? that is how usually things are done in other rust things

[DaKnig]

for example from the top of my head: https://serde.rs/container-attrs.html also this way you have identifiers be identifiers

[sdroege]

As discussed on Matrix, this would have many drawbacks

[SeaDve]

So, after playing around with all the options this is what I'm going to implement:

        let x = clone!(
            #[weak]
            x,
            #[strong(rename_to = "y")]
            self,
            #[default_return]
            false,
            move |a, b, c| {
                print!("{a} {b} {c} {x} {y}");
            }
        );

That seems to have the least amount of strangenesses syntax-wise and keeps strings to identifiers.

I wonder if the following would also work, since I think rename_to is a bit verbose:

        let x = clone!(
            #[weak]
            x,
            #[strong]
            self as y,
            #[default_return]
            false,
            move |a, b, c| {
                print!("{a} {b} {c} {x} {y}");
            }
        );

[sdroege]

That would also work but I thought it would be a bit strange because usually the right side of an as expression is a type and not a variable name.

[SeaDve]

That would also work but I thought it would be a bit strange because usually the right side of an as expression is a type and not a variable name.

I found it similar to the as in use statements (e.g., use std::command::Command as StdCommand;

[sdroege]

It's easy to change and I don't have a strong opinion either way. Anybody else?

[sdroege]

One problem that might happen is that currently you can do

#[strong(rename_to = "x")]
1 + 2 + 3

With as the precendence is different:

#[strong)]
1 + 2 + 3 as x

the as x would bind to 3 and not to the whole expression on the left side. That's probably possible to work around

[SeaDve]

I think that also applies with as statements in general. Other than, I think that is a very rare case and one could simply use parentheses, or the macro could treat the entire left hand side as one.

[A6GibKm]

One of the points here was for rustfmt and clippy to be able to do their work, using the askeyword might bite us back in the future.

[sdroege]

Also please check the PR: https://github.com/gtk-rs/gtk-rs-core/pull/1424

Let's move discussion over there.

[sdroege]

As @pbor pointed out, as would conflict with things if we wanted to actually do a cast as part of the expression that is captured. So let's not do that.