Add an optional warning to type inferred variables and parameters

[jordi-star]

Relies on #59943

Closes godotengine/godot-proposals#173 and godotengine/godot-proposals#3531

Warnings are now enum values (Ignore, Warn, Error) to allow setting whether you want a warning to be treated as either an error, a warning, or ignored.

The way warnings are initialized in gdscript.cpp has been improved to offer more flexibility. (Support for property info per warning and different default values.)

Type Inferencing

Added new warnings that will let the user know if a variable or parameter they defined is not statically typed (Set to 'Ignore' by default)

Variables that are assigned to null when declared are ignored so that if you absolutely need to, you can still have a dynamically typed variable.

[jordi-star]

The checks are failing because the documentation hasn't been updated yet, I'll get them up to date tomorrow.

[jordi-star]

The checks are failing because the documentation hasn't been updated yet, I'll get them up to date tomorrow.

Fixed 👍

[jordi-star]

Changed the property info for 'Enforce Static Types' warnings to use 'Disabled' instead of 'Ignore' for the enum strings.

[jordi-star]

Docs need to be updated, I'll get to it soon

[Calinou]

Looks good!

In addition to the review comment above, I suggest also renaming "Enforce Static Parameter Types" to "Enforce Static Method Parameter Types" to be more explicit.

PS: It seems this pull request actually closes godotengine/godot-proposals#173, not godotengine/godot-proposals#3284.
godotengine/godot-proposals#3284 should be addressed with a separate pull request as it's less important to have, and might be a bit more involved to do. Either way, this is very welcome 🙂

[jordi-star]

Looks good!

In addition to the review comment above, I suggest also renaming "Enforce Static Parameter Types" to "Enforce Static Method Parameter Types" to be more explicit.

PS: It seems this pull request actually closes godotengine/godot-proposals#173, not godotengine/godot-proposals#3284. godotengine/godot-proposals#3284 should be addressed with a separate pull request as it's less important to have, and might be a bit more involved to do. Either way, this is very welcome 🙂

Gotcha, I've renamed it and edited the referenced proposal. The docs should be correct now too.

[AaronRecord]

Variables that are assigned to null when declared are ignored so that if you absolutely need to, you can still have a dynamically typed variable.

Correct me if I'm wrong, but I think you could also specify the type as Variant

[jordi-star]

We discussed this in a GDScript meeting with @vnen and @Calinou, here are some suggestions:

• The first commit implementing enum values for warnings seems fine, and would be best moved to its own dedicated PR so that it can be merged independently of the static typing option.

• For the static typing options, some more discussion might be warranted regarding:

  • Name of the category (it's reusing the warning system but special cased as a different category, that's a bit weird)
  • Whether two options should be added or just one for all cases

I separated the enum warnings commit to #59943.

[jordi-star]

I also see that this is doing a bit different from the proposal, as it's forbidding type inference when it should be forbidding non-static types (unless I misread the code). Those are different things. If you use var x := 0 you are using type inference and static typing, it's important that this case is allowed when the user wants to enforce static types.

In fact, I don't think this needs mention type inference inference at all (unless it's to clarify that it is allowed).

Gotcha, annotated type inferencing is allowed now.

[jordi-star]

The checks are failing because the documentation uses int instead of bool. The int comes from #59943

[jordi-star]

Rebased to be up to date with master.

[jtnicholl]

The existing warnings are named after what triggers them, for example "shadowed member variable" or "int assigned to enum".
For these to be consistent they should be called "dynamically typed member variable" and "dynamically typed parameter", or something similar.

[jordi-star]

Sounds like a good idea. I'll change it along with any other needed changes.

[DasGandlaf]

Waiting for this to get merged 👀

[jordi-star]

I'm not sure if this would be merged right now because of the feature freeze. I believe the changes from the PR review have been implemented, so I think this is just waiting to be reviewed again.

[hilfazer]

What about polymorphic types? Will this PR produce a warning in the last line of the following code:

extends Node


func _ready():
	var _q :Node2D = $'.'	# runtime error if the value doesn't inherit Node2D

Static typing would result in compile time error here but what GDScript does is a runtime type check (which may or may not result in an error).

I'm asking because this PR is allegedly adressing godotengine/godot-proposals#173.

[GiverofMemory]

This is probably obvious but does typing GDScript statically improve performance? Is that the main reason for this? If so, I think this would be an excellent feature as people can get the GDScript integration combined with the speed of a statically typed language like C#.

More discussion: https://www.reddit.com/r/godot/comments/vj4cs8/is_there_a_way_to_force_static_typing_in_godot_40/

[Calinou]

This is probably obvious but does typing GDScript statically improve performance?

Since 4.0, it does thanks to typed instructions. Prior to 4.0, there was no performance impact whatsoever.

Is that the main reason for this?

The main reason is easier code maintenance and better autocompletion, rather than performance. People like TypeScript a lot, yet it doesn't make your code any faster than JavaScript 🙂

[dalexeev]

Superseded by:

• Add an optional untyped_declaration warning #81355
• GDScript: Add INFERRED_DECLARATION warning #82139

Thanks for the contribution nonetheless!