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 various issues when function argument's default value is array/dictionary #62994

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xwdit
Copy link
Contributor

@Xwdit Xwdit commented Jul 14, 2022

In the current version, if the default value of a GDScript function arguments is set to an array or dictionary, it will be null in tooltips and documentation. This PR fixes this issue.

Fix #63018

It also probably fixes some other array and dictionary related issues, but I couldn't find it. Hope someone can test it

@Xwdit Xwdit requested a review from a team as a code owner July 14, 2022 01:28
@Xwdit Xwdit changed the title Fix various issues when function arg's default value is array/dictionary Fix various issues when function argument's default value is array/dictionary Jul 14, 2022
@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from 5efe44b to 060b989 Compare July 14, 2022 01:45
@cdemirer
Copy link
Contributor

cdemirer commented Jul 14, 2022

I can't reproduce the described issue on current master:

func f(x = []):
	pass

func g(x: Array):
	pass

func h(x := [1, 2, 3]):
	pass

image
The documentation looks fine...
The tooltips: Do you mean while debugging?
image

@Xwdit
Copy link
Contributor Author

Xwdit commented Jul 14, 2022

I can't reproduce the described issue on current master:

func f(x = []):
	pass

func g(x: Array):
	pass

func h(x := [1, 2, 3]):
	pass

image The documentation looks fine... The tooltips: Do you mean while debugging? image

In fact you have reproduced it. First of all, under normal circumstances, the default value of function parameters in the document should be displayed as void h(x: Array = [1,2,3])

Also, tooltip means that when the project is not running, the script editor should also be able to view the signature of the function by hovering the mouse when calling a function.

The problem is actually because when the default value of the parameter in the function is an array/dictionary, this default value will not be resolved to a constant (it should be a constant) but a variable. But in many places in godot, when getting the default value of a function parameter, it will directly get its precompiled value as a constant, which leads to problems.

However, when the function is actually run at runtime, the default value will be assigned directly to the parameter on the left, so if you look at the value of this parameter at runtime you won't have any problems.

@Xwdit
Copy link
Contributor Author

Xwdit commented Jul 14, 2022

I can't reproduce the described issue on current master:

func f(x = []):
	pass

func g(x: Array):
	pass

func h(x := [1, 2, 3]):
	pass

image The documentation looks fine... The tooltips: Do you mean while debugging? image

In fact you have reproduced it. First of all, under normal circumstances, the default value of function parameters in the document should be displayed as void h(x: Array = [1,2,3])

Also, tooltip means that when the project is not running, the script editor should also be able to view the signature of the function by hovering the mouse when calling a function.

The problem is actually because when the default value of the parameter in the function is an array/dictionary, this default value will not be resolved to a constant (it should be a constant) but a variable. But in many places in godot, when getting the default value of a function parameter, it will directly get its precompiled value as a constant, which leads to problems.

However, when the function is actually run at runtime, the default value will be assigned directly to the parameter on the left, so if you look at the value of this parameter at runtime you won't have any problems.

This issue should be easier to spot after the #62934 merge. The gdscript documentation in the current master does not show any default values ​​for parameters at all

@cdemirer
Copy link
Contributor

Ok I see the issue now, but there is an issue with this fix. It marks the Array/Dictionary parameter nodes as constant (when possible), which triggers a problem since consts are shared:

func fn(x = [1, 2, 3], y = [1, 2, 3]):
	x.append(4)
	y.append(5)
	print(x)
	print(y)

On master:

[1, 2, 3, 4]
[1, 2, 3, 5]

This PR:

[1, 2, 3, 4, 5]
[1, 2, 3, 4, 5]

The existing line result.is_constant = false; just above was probably added to prevent this. A better solution to that would probably be to make a copy when a constant value is assigned to a variable (there are many problems with const stuff right now).

Before that happens, I guess for this issue the editor could display the reduced_value if it exists, without requiring is_constant. Then the is_constant can be set to false safely after folding the Array.

@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from 060b989 to 1051f5b Compare July 15, 2022 00:20
@Calinou Calinou added this to the 4.0 milestone Jul 15, 2022
@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from d87aa1d to 1051f5b Compare July 15, 2022 00:45
@Xwdit
Copy link
Contributor Author

Xwdit commented Jul 15, 2022

Ok I see the issue now, but there is an issue with this fix. It marks the Array/Dictionary parameter nodes as constant (when possible), which triggers a problem since consts are shared:

func fn(x = [1, 2, 3], y = [1, 2, 3]):
	x.append(4)
	y.append(5)
	print(x)
	print(y)

On master:

[1, 2, 3, 4]
[1, 2, 3, 5]

This PR:

[1, 2, 3, 4, 5]
[1, 2, 3, 4, 5]

The existing line result.is_constant = false; just above was probably added to prevent this. A better solution to that would probably be to make a copy when a constant value is assigned to a variable (there are many problems with const stuff right now).

Before that happens, I guess for this issue the editor could display the reduced_value if it exists, without requiring is_constant. Then the is_constant can be set to false safely after folding the Array.

Thanks for your test and suggestion, I have now updated this PR and it should resolve the issue you found

@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch 3 times, most recently from c7afc96 to fef4c2b Compare July 15, 2022 01:49
@Xwdit
Copy link
Contributor Author

Xwdit commented Jul 15, 2022

Found and fixed some type judgment issues, now everything should works fine.
image
image

@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from 68eef84 to c21a5c9 Compare July 15, 2022 02:12
@Xwdit
Copy link
Contributor Author

Xwdit commented Jul 15, 2022

Found and fixed some type judgment issues, now everything should works fine.

Now also fixed for script editor tooltips.
image

@akien-mga
Copy link
Member

Needs rebase to fix CI due to a temporary godot-cpp issue.

@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from c21a5c9 to 052cfb1 Compare July 15, 2022 10:43
@Xwdit
Copy link
Contributor Author

Xwdit commented Jul 15, 2022

Needs rebase to fix CI due to a temporary godot-cpp issue.

Rebased to fix CI issue

@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch 3 times, most recently from 9f1a584 to f26104f Compare August 10, 2022 22:31
@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from f26104f to f7b3730 Compare September 14, 2022 16:47
@Xwdit Xwdit force-pushed the fix_dict_array_arg_default_var branch from f7b3730 to 94d6456 Compare November 26, 2022 20:07
@akien-mga
Copy link
Member

Needs a rebase after other GDScript changes. I checked and the issue is still reproducible, so the changes here are likely still needed. Not critical for 4.0 since we're at RC stage and it's a minor issue, but would be good to review/merge for 4.1 (and then we could cherrypick it for 4.0.x).

@akien-mga akien-mga modified the milestones: 4.0, 4.1 Feb 16, 2023
@adamscott
Copy link
Member

Will push the milestone to 4.2. The GDScript team did not review the PR, but I think it would be nice to in the next weeks, after the 4.1 release.
If the PR is not rebased since then, the team will do itself.

@dalexeev
Copy link
Member

This is not a critical bug and a fairly complex problem. It might be better to refactor expression reduction and remove make_*_reduced_value() and update_*_type() rather than adding new methods.

@dalexeev dalexeev modified the milestones: 4.2, 4.3 Oct 27, 2023
@dalexeev dalexeev modified the milestones: 4.3, 4.x Jun 4, 2024
@dalexeev
Copy link
Member

dalexeev commented Jun 4, 2024

Since #63018 resolved by #88211 and #89738, there is no need to work on this PR at this time. However, in the future we may try to refactor expression reduction (see #66218), so I marked this as salvageable.

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.

Function argument's default value not display in tooltips/documents when it's Array/Dictionary
6 participants