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

GDScript: Adjust STATIC_CALLED_ON_INSTANCE warning to not force native type #85918

Conversation

20kdc
Copy link
Contributor

@20kdc 20kdc commented Dec 8, 2023

Defaulting to the native type is less than useful, as:

  • There are very few native types that are extensible and have static methods.
  • Defaulting to the native type does not account for a method being script-defined.

While the "real fix" would be to carefully track the source of the method, the get_function_signature method is already complicated enough.

This will at least ensure the resulting code should always be valid.

This PR was written after seeing the following warning:

*  W 0:00:00:0542   The function "get_absolute_z_index()" is a static function but was called from an instance. Instead, it should be directly called from the type: "Node2D.get_absolute_z_index()".

Where get_absolute_z_index() was a static function declared in a script (and thus did not exist in Node2D, and thus the given code did not work)

@20kdc 20kdc requested a review from a team as a code owner December 8, 2023 12:35
@dalexeev dalexeev added this to the 4.3 milestone Dec 20, 2023
@akien-mga akien-mga changed the title GDScript: Adjust STATIC_CALLED_ON_INSTANCE warning to not force native type GDScript: Adjust STATIC_CALLED_ON_INSTANCE warning to not force native type Feb 8, 2024
@dalexeev
Copy link
Member

dalexeev commented Feb 8, 2024

Note that for non-global classes the warning will suggest invalid code.

@20kdc
Copy link
Contributor Author

20kdc commented Feb 9, 2024

Note that for non-global classes the warning will suggest invalid code.

Example, please? I'm not sure what constitutes a "non-global class" in this case.

@dalexeev
Copy link
Member

dalexeev commented Feb 9, 2024

Example, please? I'm not sure what constitutes a "non-global class" in this case.

@vnen
Copy link
Member

vnen commented Feb 9, 2024

Note that this would be wrong on the current master also, it would show EditorScript.static_func() which isn't a valid way to call it.

We can either accept this as small improvement or work on a proper fix that finds the origin of method.

PS: I wonder if we should even emit this warning with a direct call for a function in the same class. If it does not have class_name it is quite cumbersome to avoid this warning.

@20kdc 20kdc force-pushed the tnj-static-called-on-instance-fix-confusion branch from 3434530 to e6fe75b Compare February 9, 2024 15:22
@20kdc
Copy link
Contributor Author

20kdc commented Feb 9, 2024

I've amended my commit on the basis of that other languages don't warn for these kinds of self-calls to static functions.

There are more examples and counter-examples one could add involving self and var x := self and so forth, but they seem contrived...

@akien-mga akien-mga requested a review from dalexeev February 12, 2024 11:30
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Makes sense to me, although the point about non-global classes still applies.

…the native type, and to not trigger on self-calls

Not defaulting to the native type rationale:

Defaulting to the native type is less than useful, as:

* There are very few native types that are extensible and have static methods.
* Defaulting to the native type does not account for a method being script-defined.

While the "real fix" would be to carefully track the source of the method, the get_function_signature method is already complicated enough.

This will at least ensure the resulting code should always be valid.

Not triggering on self-calls rationale:

Found in PR comment godotengine#85918 (comment)

```
static func example():
	pass

func example2():
	example() # self-call on static function
```

Disabling this warning on self-calls is:

* Consistent with other languages
* Important for anonymous classes (where the output code is unusable)
@dalexeev dalexeev force-pushed the tnj-static-called-on-instance-fix-confusion branch from e6fe75b to 24181d1 Compare March 1, 2024 14:31
@akien-mga akien-mga merged commit b392ab5 into godotengine:master Mar 4, 2024
16 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
4 participants