Remove unnecessary Clone requirement

[alilleybrinker]

The docs mention a Clone requirement on arbitrary serializable types you'd like to include in a probe, and specifically mentions that the data involved is never actually cloned.

I wonder if the issue is similar to the one the sqlx project had for their PoolOptions type (see: launchbadge/sqlx#2553), where a Clone derive on a type which involves T in a generic manner results in an excessive Clone requirement on it even though the data is never actually cloned. In this case, the solution was to manually implement Clone on the containing type, as shown in the linked pull request.

It's possible this isn't the cause (I haven't yet dug into the code to see), but reading the docs triggered my feeling that it might be a similar issue. I'm happy to tinker (with some guidance on the right place to look) to see if I can remove the Clone requirement!

[bnaecker]

I believe these are separate issues. We're not deriving Clone anywhere, and in fact, never store the types provided by the user in the probe definitions in any type at all. Instead, the bound comes from some trickery we play to type-check the arguments provided to the probe macros.

Backing up a second, this crate works by emitting macros, which themselves contain inline assembly. That ASM does very little other than store the macro arguments, or a pointer to them, in a few registers. This is pretty great, but that's a lot of casting and pointer shenanigans that is about as unsafe as it gets in Rust.

To ensure that the macros are called with valid arguments, we create a small closure that returns the probe macro arguments. The emitted macros in turn attempt to use those arguments in a function with the same types as the probe arguments -- a call site that will fail at compile time, regardless of whether it's ever issued at runtime.

This may all be quite abstract, so here's an example. Below is a small snippet of the cargo expand output run against the test in the repo at ./tests/test-json:

fn main() {
    {
        let __usdt_private_args_lambda = || (0, "something");
        #[allow(unused_imports)]
        #[allow(non_snake_case)]
        fn __usdt_private_doesit_work_type_check(
            _: impl ::std::borrow::Borrow<u8>,
            _: impl AsRef<str>,
        ) {
        }
        let _ = || {
            let args = __usdt_private_args_lambda.clone()();
            __usdt_private_doesit_work_type_check(args.0, args.1);
        };
        // Remainder omitted

So first, we see the closure __usdt_private_args_lambda, which returns the data to be emitted in the probe. Next is a function definition __usdt_private__doesit_work_type_check, which takes the exact types we expect to be emitted in the probe. We then pass that to another, anonymous closure, which clones the first closure and calls the type check function. Critically, this last closure is never called, but will still generate a compiler error if the probes are called with the wrong argument types. That's why we say the clone will never actually occur. However, we do need to actually call .clone() on the closure (which is where this bound comes from in the first place), so that we can call it "for real" later when we do actually evaluate the arguments for the probe itself.

Why are we doing this? First, we wanted type checking of some kind. Second, we require that the arguments be passed to the macros themselves as a closure. The reason for that is to emphasize the conditional nature of the probes -- they only fire when they're enabled, and so whatever code is used to generate the arguments will only run when they're enabled. So we needed a way to take a lambda and verify that its returned values are the right type, without ever evaluating it. And this approach is where we landed.

Looking at this for the first time in several years, it does seem plausible that we could relax the bound. We unpack the arguments inside the probe macro in order to pass them in registers. One could do the same type-checked-but-never-called trick at the call site. It's not obvious to me if that would work, though one could try. That would be here, in the cargo expand output:

            if is_enabled != 0 {
                let args = __usdt_private_args_lambda();
                let arg_0 = (*<_ as ::std::borrow::Borrow<u8>>::borrow(&args.0) as i64);
                let arg_1 = [(args.1.as_ref() as &str).as_bytes(), &[0_u8]].concat();
                // Could insert type check call here possibly.
                // The typecheck function would need to take refs to the args, or we need Clone anyway.

In any case, it's never really been an issue that the types must implement Clone. That is almost always derivable without any fuss in the code in which usdt is normally used. If you do happen to find yourself wanting to emit non-Clone types in a probe argument, please let us know! And you are certainly welcome to try the approach I outlined above, or any other that seems promising!

[alilleybrinker]

Hey, thanks for the thorough response! This is very interesting stuff. It does seem to me like the forced-typechecking implementation could relax the Clone bounds, and I may spend some time tinkering with it. It's fun, if nothing else, to work in a new codebase as a way to better understand what it's doing.

So thanks for the time! If I have any further questions I'll happily reach out 😄