Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change generated functions signatures to remove type parameters #1045

Merged

Conversation

shizzard
Copy link
Contributor

Fixes #1042

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@shizzard
Copy link
Contributor Author

shizzard commented Apr 26, 2024

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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

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

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

@caspermeijn
Copy link
Collaborator

@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
Copy link
Contributor Author

shizzard commented May 3, 2024

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
Copy link
Collaborator

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 shizzard force-pushed the df/1042-impl-prost-message-shadowing branch from 64f64bd to 4f01b11 Compare May 6, 2024 12:06
@shizzard
Copy link
Contributor Author

shizzard commented May 6, 2024

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
Copy link
Collaborator

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.

@caspermeijn caspermeijn added this pull request to the merge queue May 20, 2024
Merged via the queue into tokio-rs:master with commit bb5a1fa May 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

impl ::prost::Message for a struct shadows the user-defined type
2 participants