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

C#: Fix deserialization of delegates that are 0-parameter overloads #78877

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

mattdiener
Copy link
Contributor

@mattdiener mattdiener commented Jun 30, 2023

TryDeserializeSingleDelegate causes GetMethod to throw when deserializing a method with a 0-parameter overload.

GetMethod without a types argument matches all methods with the given name, whereas a 0-length types argument explicitly matches methods with 0 parameters.

E.g.

private void OnValueChanged(float _) {
    OnValueChanged();
}

private void OnValueChanged() {
   //...
}

//...
{
    _mySlider.ValueChanged += OnValueChanged;
}

@mattdiener mattdiener requested a review from a team as a code owner June 30, 2023 15:05
@mattdiener
Copy link
Contributor Author

mattdiener commented Jun 30, 2023

Thanks ahead of time for your patience with my first PR. Happy to go back and add more details or spend some more time on a cohesive repro. I ran into this working on an editor plugin. It threw every time I compiled my code. This fix worked for me.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 30, 2023

Please open an issue to track this more effectively, in case some other solution is required, or something else is wrong, and to handle the specifics of the issue

@mattdiener
Copy link
Contributor Author

On it, thanks!

@akien-mga akien-mga added this to the 4.2 milestone Jun 30, 2023
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to the .NET module!

Indeed, the Type.GetMethod documentation says that it throws an exception if:

More than one method is found with the specified name and matching the specified binding constraints.

So if we are specifically looking for a method with no parameters, we should be using the overload that takes a parameter type array.

This PR already looks good to me 👍, but I'd like you to make a small change to avoid allocating empty arrays.

@mattdiener
Copy link
Contributor Author

Created #78886.

@mattdiener
Copy link
Contributor Author

Re-tested with your change @raulsntos. Works just as well! thanks for the feedback!

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

The code looks good so I'm approving, but before we can merge you'll have to squash the commits into one.

Since it's your first time contributing, make sure to take a look at CONTRIBUTING.md and the contributing documentation if you haven't already. It contains information about squashing, in case you need it.

Feel free to reach out in the development chat if you need help.

Co-authored-by: Raul Santos <raulsntos@gmail.com>
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 6, 2023
@YuriSizov YuriSizov merged commit e4c89a0 into godotengine:master Jul 12, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged Godot PR!

Please consider in the future creating a feature branch for your PR instead of using your master. It would be more convenient for everyone, including yourself, if you plan to contribute more to the engine. Cheers!

@YuriSizov YuriSizov changed the title C# Fix deserialization of delegates that are 0-parameter overloads C#: Fix deserialization of delegates that are 0-parameter overloads Jul 19, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 31, 2023
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.

.NET: TryDeserializeSingleDelegate throws when deserializing overloaded 0-parameter method.
5 participants