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

Unify typing of variables, constants and parameters in GDScript #70464

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

vonagam
Copy link
Contributor

@vonagam vonagam commented Dec 23, 2022

Member variables, local variables, member constants, local constants and parameters - all of those have almost the same code for determining a type. This PR tries to unify that logic under one function, fix inconsistencies and to show explicitly what are the differences between those types.

Introduced GDScriptParser::AssignableNode as a parent for VariableNode, ConstantNode and ParameterNode. They all have the same properties that used by the resolving logic:

IdentifierNode *identifier;
ExpressionNode *initializer;
TypeNode *datatype_specifier;
bool infer_datatype;
int usages;

The only difference is that in ParameterNode initializer was called default_value. If this rename is a deal breaker - can remove AssignableNode and pass those properties one by one instead.

Otherwise pretty-straightforward.

Notes:

  • A variable has use_conversion_assign, strange that same functionality is not exposed to a parameter.

  • I used construction like static constexpr const char* kind = "variable"; inside a function to pass a string used in errors. Did so to avoid allocations/conversions. If this does not matter and I should use String instead - just let me know.

Next issue(s) about typing of null as default value is fixed (example in next comment):
Fixes #53115.
Fixes #56217.
Fixes #67048.
Fixes #70523.

Next issue(s) connected to assignment of weak default value to typed variable is fixed:
Fixes #66141.
Fixes #70912.

@vonagam vonagam requested a review from a team as a code owner December 23, 2022 02:10
@aaronfranke aaronfranke added this to the 4.x milestone Dec 23, 2022
@vonagam vonagam force-pushed the unify-assignables branch 3 times, most recently from a7b04a1 to c461c84 Compare December 30, 2022 02:10
@vonagam
Copy link
Contributor Author

vonagam commented Dec 30, 2022

Added simple test to show that implicit conversion now works for arguments:

func check(arg: float = 3): # would have failed before
	return typeof(arg) == typeof(3.0)

Updated to include fix for null as default type of arguments/variables with corresponding test:

func check(input: int) -> bool:
  return input == 1

var recur = null
var prop = null

func check_arg(arg = null) -> void:
  if arg != null:
    print(check(arg)) # would have failed before

func check_recur() -> void:
  if recur != null:
    print(check(recur)) # would have failed before
  else:
    recur = 1
    check_recur()

func test() -> void:
  check_arg(1)

  check_recur()

  if prop == null:
    set('prop', 1)
    print(check(prop)) # would have failed before
    set('prop', null)

  var loop = null
  while loop != 2:
    if loop != null:
      print(check(loop)) # would have failed before
    loop = 1 if loop == null else 2

I know from this comment that there is a discussion to stop inferring type of arguments from default values.

I see two ways:

  1. Either current system is kept as is, but null as default for variables/arguments means Variant.

  2. Or all not explicitly typed or explicitly inferring variables/arguments should be treated as Variants. I assume that the discussion was only about arguments but I don't see much difference between them and member variables - both can be changed from outside with arbitrary values. With local variables you can actually do try to narrow it down, but as shown in loop example it does not work flawlessly right now.

Can remove that change from PR. It is a one-line difference.

@dalexeev
Copy link
Member

dalexeev commented Jan 3, 2023

@vonagam Could you add conversion for constant initializers to resolve_assignable if the specified type is different? See #70873 for an example (albeit a memory leak). There was also the question of whether we should allow non-exact type matching for constants and parameters.

@vonagam vonagam force-pushed the unify-assignables branch from e74b0d0 to 6aaa69b Compare January 4, 2023 20:54
Copy link
Member

@vnen vnen left a comment

Choose a reason for hiding this comment

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

This seems a good approach to me.

@vonagam vonagam force-pushed the unify-assignables branch from 6aaa69b to a1d0674 Compare January 5, 2023 23:10
@akien-mga akien-mga merged commit 95ce236 into godotengine:master Jan 5, 2023
@akien-mga
Copy link
Member

akien-mga commented Jan 5, 2023

Thanks! This is a huge amount of bugs fixed in one fell swoop :)

@Malcolmnixon
Copy link
Contributor

Whups - just found this PR after filing the bug - #70978.

The following code now crashes:

func _ready():
	# This works
	test([1, 1, 1])
        
	# This crashes
	test()
	
func test(array := [1, 2, 3]):
	print("array is ", array)

@vonagam vonagam deleted the unify-assignables branch January 6, 2023 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment