Change generated functions signatures to remove type parameters

[shizzard]

Fixes #1042

[caspermeijn]

The changes look good. Just a one comment: You should add a test to confirm your example works.

[shizzard]

You should add a test to confirm your example works.

@caspermeijn I thought about it, that seem to be a good practice, but this test will only fail if somebody will add the generic type parameter with the same name (e.g. pub fn encode<T>... will still pass). It seems impossible to create a useful test that will ensure no shadowing will occur in the future. Are you sure this test is a good addition?

Actually, I think that a comment about this issue near the trait Message might be better in terms of reducing the chance of regression.

[caspermeijn]

It seems impossible to create a useful test that will ensure no shadowing will occur in the future. Are you sure this test is a good addition?

You could at least proof that your problem is addressed. When someone else finds another shadowing issue, then they could extend the test with their case. I would like to see your example as a test.

[shizzard]

@caspermeijn Alright, what is the best place to put this test? I'm not really familiar with the prost project structure, can you help me with that?

[caspermeijn]

@caspermeijn Alright, what is the best place to put this test? I'm not really familiar with the prost project structure, can you help me with that?

You could do something similar to this: https://github.com/tokio-rs/prost/blob/master/tests/src/no_unused_results.proto

If you search for no_unused_results, you can find all the places where you need to add something.

[shizzard]

The test is no_shadowed_types, I checked it was successfully executed:

...
test no_shadowed_types::dummy ... ok
...

[caspermeijn]

Looks good to me. This is a breaking change, so it has to wait for the next major release. Thank you for your contribution.

[shizzard]

@caspermeijn latest commit changes the rest of ::prost::Message trait as you noted in discord.

[caspermeijn]

@caspermeijn latest commit changes the rest of ::prost::Message trait as you noted in discord.

Thank you. I am sorry to keep finding more stuff to do: In prost/src/encoding.rs and in prost/src/lib.rs are some more easy to replace where B: Buf statements. Could you also replace those?

I think when doing this breaking change, we should do all of them at once.

[shizzard]

I think when doing this breaking change, we should do all of them at once.

I did try to make these changes within encoding.rs, but there are several cases where it is not possible:

• FnMut type does not allow dynamic dispatch

/// Helper function which abstracts reading a length delimiter prefix followed
/// by decoding values until the length of bytes is exhausted.
pub fn merge_loop<T, M, B>(
    value: &mut T,
    buf: &mut B,
    ctx: DecodeContext,
    mut merge: M,
) -> Result<(), DecodeError>
where
    M: FnMut(&mut T, &mut B, DecodeContext) -> Result<(), DecodeError>,
    B: Buf,
{
    ...

• closures do not allow dynamic dispatch

    pub fn merge<M, B>(
        wire_type: WireType,
        msg: &mut M,
        buf: &mut B,
        ctx: DecodeContext,
    ) -> Result<(), DecodeError>
    where
        M: Message,
        B: Buf,
    {
        check_wire_type(WireType::LengthDelimited, wire_type)?;
        ctx.limit_reached()?;
        merge_loop(
            msg,
            buf,
            ctx.enter_recursion(),
            |msg: &mut M, buf: &mut B, ctx| {
                let (tag, wire_type) = decode_key(buf)?;
                msg.merge_field(tag, wire_type, buf, ctx)
            },
        )
    }

Also, these functions are doing tight-loop decoding and may gain some performance benefits from static dispatch. There are also no way these functions type parameters may shadow user-defined types.

You still think to make these changes where it is possible?

[caspermeijn]

Also, these functions are doing tight-loop decoding and may gain some performance benefits from static dispatch. There are also no way these functions type parameters may shadow user-defined types.

impl Buf will not do a dynamic dispatch, maybe you are thinking of the dyn keyword. As I understand, the behavior is the same as the where B: Buf statement, but it doesn't have the explicit generic type. So I think it will be fine.

You still think to make these changes where it is possible?

I agree those two complex where case should stay the same. I would like to see the simple cases changes and the more complicated cases can stay as is.

[shizzard]

impl Buf will not do a dynamic dispatch, maybe you are thinking of the dyn keyword.

Yes, you're right, for some reason I thought that this will do a dynamic dispatch.

I did the changes you wanted; there are 6 more places where the buffer is type parametrized (search for "B: Buf" in encoding.rs file).
I tried to do the same for M: Message, but it broke one test on compile time (with cryptic error message that I don't fully understand actually), so I reverted the M: Message type changes.

[caspermeijn]

I did the changes you wanted; there are 6 more places where the buffer is type parametrized (search for "B: Buf" in encoding.rs file). I tried to do the same for M: Message, but it broke one test on compile time (with cryptic error message that I don't fully understand actually), so I reverted the M: Message type changes.

That looks good. Thank you for this cleanup. We could always do the impl Message at another time.