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

Fix method hashes with default arguments #81521

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Sep 10, 2023

Fixes #81386

Here's the start of a fix for the incorrect method hashing from issue #81386, including a mapping system so we don't have to break compatibility.

This is a draft because I haven't added any of the mappings yet (still working on a script to do it), but I wanted to start trying this on CI right away.

UPDATE: The list of mappings are in now, but they're missing a couple. Just need to get the CI passing to take this out of draft.

@dsnopek dsnopek added this to the 4.2 milestone Sep 10, 2023
@dsnopek dsnopek requested review from a team as code owners September 10, 2023 18:33
@dsnopek dsnopek marked this pull request as draft September 10, 2023 18:33
@dsnopek dsnopek force-pushed the method-bind-default-argument-hash-fix branch 3 times, most recently from 627db27 to ea124b1 Compare September 10, 2023 19:38
for (int i = 0; i < get_default_argument_count(); i++) {
Variant v = get_default_argument(i);
hash = hash_murmur3_one_32(v.hash(), hash);
for (int i = 0; i < get_argument_count(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two methods which have DEFVALs but no actual arguments. Those are DirAccess::list_dir_begin and DisplayServer::is_touchscreen_available.

If we want to avoid changing those hashes, I would suggest simply:

for (const Variant &v : get_default_arguments()) {
	hash = hash_murmur3_one_32(v.hash(), hash);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. We probably shouldn't be including default arguments that aren't paired with actual arguments in the hash in the first place. And since we're already doing a mapping here, this actually seems like a good time to break those hashes. :-)

Locally, I tested removing the invalid default arguments, and the hashes (with the new calculation in this PR) remained stable, which I think is what we want. So, we can delete those DEFVAL()s at anytime, and if anyone adds any default arguments without arguments in the future, they won't be counted.

@dsnopek dsnopek force-pushed the method-bind-default-argument-hash-fix branch from ea124b1 to 1c5ead9 Compare September 10, 2023 22:21
@dsnopek dsnopek changed the title [DRAFT] Fix method hashes with default arguments Fix method hashes with default arguments Sep 10, 2023
@dsnopek dsnopek marked this pull request as ready for review September 10, 2023 22:46
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 10, 2023

This is ready for review! Tests haven't finished running yet, but everything seems good locally, so I'm expecting them to pass.

It now includes the full hash mapping, mostly generated via a script that compared the extension_api.json before and after changing the hash calculation. There were, unexpectedly, a few that I had to do by hand: where there was a pre-existing compatibility method, and the hash of the compatibility method would change too.

Copy link
Member

@RedworkDE RedworkDE left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, but I didn't test the code.

core/extension/compat_hashes_81386.h Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the method-bind-default-argument-hash-fix branch from 1c5ead9 to 487642a Compare September 12, 2023 14:28
Copy link
Contributor

@Bromeon Bromeon 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 addressing this! 👍

Comment on lines +899 to 901
if (compatibility.size() > 0) {
d2["hash_compatibility"] = compatibility;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

General question: with DISABLE_DEPRECATED, do we still write this key?
Might very well be "yes", just wanted to double-check 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, well, if all compatibility methods are also wrapped with #ifdef DISABLE_DEPRECATED then we won't have anything else to put into "hash_compatibility". But I think this is fine? The idea of compatibility hashes still exists with DISABLE_DEPRECATED, we just aren't registering any.

Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Reviewed during GDExtension meeting, this looks like a good way to solve the backwards compatibility issue, much better then introducing 800 compatibility calls :)

@dsnopek dsnopek force-pushed the method-bind-default-argument-hash-fix branch from 487642a to 0d13727 Compare September 21, 2023 17:39
@akien-mga akien-mga merged commit 6fc1d50 into godotengine:master Sep 22, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

MethodBind::get_hash() fails to hash default arguments
6 participants