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

Typed GDScript #19264

Merged
merged 12 commits into from
Jul 21, 2018
Merged

Typed GDScript #19264

merged 12 commits into from
Jul 21, 2018

Conversation

vnen
Copy link
Member

@vnen vnen commented May 30, 2018

Alright, big change coming! Let me add a bit of documentation in here.

FAQ

Things people asked me during the development.

  • Is it optional?

Yes. You can use GDScript dynamically as it is now. Type hints can be used only in parts of the code.

  • Will this affect my existing code?

It shouldn't. If it does then it's a bug. Whenever a type check is possible it will make one, but this won't affect duck-typing. Lines that are fully type-checked are indicated by a highlight in the line number. However, GDScript was very lenient about some behaviors that can be considered an error, so your script might break if it relied on those behaviors.

  • Does it improve performance?

As it is now: no. However, this enables the possibility of future improvements in performance. A set of benchmarks are needed before we go this path.

  • Can the syntax change to be like <my favorite language>?

No, this was discussed at length before. The syntax is similar to Python. It's made to be easy to parse without changing the GDScript syntax in itself, considering the types are optional.

Syntax

  • Variable hint: var my_int : int
    • Optional inline initialization: var my_int : int = 42
    • Type can be omitted to be inferred from value: var my_int : = 42
  • Also for constants (but not strictly needed): const my_const : String = "Something"
  • Function declaration: func (arg1 : int, arg2 : String = "") -> float:
  • Casting: event as InputEventKey
    • It returns null if the cast is not possible.
    • It errors out if trying impossible conversion between built-in types.

Possible types

  • Built-in variant types (int, float, Vector2, Basis, etc.).
  • Native classes from the ClassDB (Node, Resource, Sprite, AnimationPlayer.
  • Scripts of any kind, they can be preloaded into a constant, then the constant can be used as a type. includes inner classes in other scripts.
  • Named classes in the script server, as added in GDScript with the class_name keyword. Includes their inner classes too.
  • Other classes in the current script (the ones made using the class statement).

Static checks

As long as there any type can be fully determined, the parser will try to detect mismatches. This will be shown as an error in the editor.

  • Type mismatch when assigning to a variable.
  • Type mismatch in function arguments when making a call.
  • Missing or exceeding arguments for a function call.
  • Operation in an expression.
    • Whether the operand types are valid for the operator (e.g. can't multiply int and String).
    • What is the resulting type of operation (e.g. 2 * 2 is an int, but 2 * 2.0 is a float).
  • Whether the declared function matches the parent signature.
  • Member variable shadowing one in the same or the parent class.
  • Type mismatch between member variables and setter/getter.
  • Constants and inner classes are in the same scope: if one shadows another they error out.
  • Whether an inner class name shadows a native class from the ClassDB.
  • Non-existing identifier/function.

Note that since this is meant to be fully optional and still compatible with duck-typing, it won't mark as an error if the member variable/method could be available in a subtype. Instead, type-safe lines will be marked with a subtle green tint in the number line. Non-safe lines will just not be marked:

Completion

The code completion was rewritten to make use of the type hints, and also fixed many long-standing bugs. The code is now somewhat cleaner and easier to add new features. Some improvements:

Wanted feedback

Any feedback is welcome but there's some things in particular that I want to know. In any case it's great to have sample scripts for me to test.

  • It breaks the current code of your project. This includes parser and compiler errors in general.
  • It doesn't detect a problem when it should.
  • It does detect a problem when it shouldn't.
  • It gives an error in the console saying "Parser bug".
  • It is too slow in a big file or file with lots of dependencies.

Future

Before anyone asks, here's a bit of ideas for the future.

  • Add types in signals, including type checks for emit_signal and connect calls.
  • Improve completion inference and better use of type hints.
  • Add warnings (System for GDScript warnings #18271).
  • Homogeneous collections (like var int_array : int[] as an array of integers).
  • Optimizations for better performance.

Closes #10630.

@blurymind
Copy link

Fantastic addition! Thank you very much for working on this!

@Xrayez
Copy link
Contributor

Xrayez commented May 30, 2018

Some weird stuff when using emit_signal (though to be expected since you didn't touch signals? But I think it should work without errors):

extends Node2D

var my_int : int = 10

signal my_signal(param1, param2)

func _ready() -> void:
	var p1 : int = 0
	var p2 : int = 1
	emit_signal("my_signal", p1, p2)
	
SCRIPT ERROR: GDScript::reload: Parse Error: Too many arguments for 'emit_signal()' call. Expected at most 1.
          At: res://Node2D.gd:10
ERROR: reload: Method/Function Failed, returning: ERR_PARSE_ERROR
   At: modules/gdscript/gdscript.cpp:579
ERROR: debug_get_stack_level_instance: Condition ' _debug_parse_err_line >= 0 ' is true. returned: __null
   At: modules/gdscript/gdscript_editor.cpp:288

@vnen
Copy link
Member Author

vnen commented May 30, 2018

@Xrayez seems I missed a check for varargs. Thanks for pointing it out.

@coppolaemilio
Copy link
Member

I really want to like this because it seems like a lot of work but I don't think this is a good addition to gdscript. It will add more segmentation, a harder entry barrier and distance itself further more from Python. Imagine the confusion of new comers when they start watching tutorials of people who prefer typed and others that don't. Maybe it would be a good idea to separate this with a different extension? something like .gdt not sure. Maybe I'm just too much into Python.

@OvermindDL1
Copy link

I really want to like this because it seems like a lot of work but I don't think this is a good addition to gdscript. It will add more segmentation, a harder entry barrier and distance itself further more from Python.

Considering it is both backwards compatible and it follow Python's Typed Syntax spec, how would it segment or distanct itself further from python?

Typing code is not a hard thing (though I would prefer if it were enforced, preferably with ML unification abilities ala OCaml or so would fit perfectly, hmm, maybe I should make an OCaml'ish native language interface...).

@vnen
Copy link
Member Author

vnen commented May 30, 2018

@coppolaemilio Python has the same syntax for optional typing, if anything this will bring GDScript closer to Python.

I don't think there should be that much confusion. Newcomers will probably just ask about it and learn while doing so. Since this is optional, both kind of tutorials and can even be mixed without (much) problem. There's a problem of mixing type hints and duck-typing, which may bring errors, but I still think people will just ask and learn. It doesn't sound like a big of a deal to me.

Also, I'm not sure if we should nuke a wanted feature because of potential newcomer confusion. There's a lot of positive reactions not only to this PR but also to the related issue and my posts on Twitter. We know those people like this feature, should we not add it because the confusion it might cause (which we don't know)?

If this were to be made into a separate file type then it would be better to make a new language altogether. But that would be a great effort in itself and really increase the fragmentation.

Of course, I have a bit of passion for a feature I wrote myself, but I'm still open to feedback. If for the greater good this is decided to be a bad thing I won't push it further.

@OvermindDL1
Copy link

For note, Python Typing: https://www.python.org/dev/peps/pep-0484/
Near identical to how gdscript does it (in fact it does look identical from the bits I've seen so far), and yes that is in the Python standard and is supported by the cpython system (although cpython doesn't do much with the typing information, static analyzers do).

╰─➤  python3 -i 
Python 3.5.2 (default, Nov 23 2017, 16:37:01) 
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> def greeting(name: str) -> str:
...     return 'Hello ' + name
... 
>>> greeting("world")
'Hello world'
>>> 

@coppolaemilio
Copy link
Member

Sorry guys, my bad! I wasn't aware that it was already implemented in Python.

@vnen
Copy link
Member Author

vnen commented May 30, 2018

Solved the issue with vararg functions. Hopefully fixed compilation/style issues as well.

@akien-mga
Copy link
Member

The last build issue seems to be on Android, is there a missing #ifdef TOOLS_ENABLED?

modules/gdscript/gdscript_editor.cpp:57: error: undefined reference to '_EDITOR_DEF(String const&, Variant const&)'

@@ -54,16 +54,17 @@ void GDScriptLanguage::get_string_delimiters(List<String> *p_delimiters) const {
}
Ref<Script> GDScriptLanguage::get_template(const String &p_class_name, const String &p_base_class_name) const {

bool th = EDITOR_DEF("text_editor/completion/add_type_hints", true);
Copy link
Member

Choose a reason for hiding this comment

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

Android build fails here, but I guess the whole file should be in #ifdef TOOLS_ENABLED, and not just a handful of duplicate includes as done currently?

Copy link
Member Author

Choose a reason for hiding this comment

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

This really needs I cleanup, but since I intend to refactor the completion code anyway, I'll fix it then.

@volzhs
Copy link
Contributor

volzhs commented May 30, 2018

Typed GDscript autoload.zip

@vnen This is crash sample project at starting editor.
It has simply 2 autoloads and the first one tries to access the second one.

@neikeq
Copy link
Contributor

neikeq commented May 31, 2018

  • It is too cumbersome to have static type check in a particular statement. For instance, I removed it from get_node() to avoid having to cast every time you use it (specially with the $ shorthand).

I think there is a design problem here. This is what you would expect in an statically type language. You get an instance up-casted to a base type. If you want to use the methods from the derived type, you have to down-cast it first. This is common in C#:

var sprite = GetNode("/root/Sprite") as Sprite;
sprite.Centered = true;

This would be totally fine if the new GDScript were to be only statically typed. The problem is the intention is to be backwards compatible with dynamically typed GDScript. The above is unacceptable. This means get_node("/root/sprite").centered = true should be possible, so we have to treat return types as Variant until they are explicitly or implicitly cast. This means we don't truly have full optional typing.

Did you remove the need for the cast only from get_node() or from other methods as well?

@vnen
Copy link
Member Author

vnen commented May 31, 2018

@neikeq I'm still not entirely sure about that. Full static typing means that you can't do something simple like:

$AnimationPlay.play("animation")

This would give an error because Node does not have a play() function. Which is expected in a typed context. For this to work you would need an explicit downcast:

($AnimationPlayer as AnimationPlayer).play("animation")

I have a feeling that most of the time this would be cumbersome, especially if you are used to GDScript already. Maybe it's better if I remove this exceptional case and if people complain during usage I really change it.

Did you remove the need for the cast only from get_node() or from other methods as well?

Only for get_node(), because it's a method used very often.

[...] so we have to treat return types as Variant until they are explicitly or implicitly cast. This means we don't truly have full optional typing.

That's the catch: should type hints imply that GDScript is fully statically typed or should just be a convenience when assigned? The former sounds more sane but latter is more backwards compatible (and makes types truly optional).

The thing is that if you add a type hint your code is subject to static checks, which might raise a parse-time error somewhere even if the code worked flawlessly with dynamic typing. If that's what we want, I'm fine with it.

@mablin7
Copy link
Contributor

mablin7 commented May 31, 2018

My two cents is, that static type checks would be more fitting for GDscript with parse time errors only. Maybe we could use some of the type guessing from the autocomplete, to handle specific cases, like get_node, better.

@eon-s
Copy link
Contributor

eon-s commented May 31, 2018

($AnimationPlayer as AnimationPlayer).play("animation") is like putting salt and pepper over the syntax sugar...

Also, I do not even see a reason to downcast any Node on the tree, that should stay dynamic.

@toger5
Copy link
Contributor

toger5 commented May 31, 2018

O think if you want your script to be typed u would probably use onready vars which are of the correct type.
So i think it would be okay to have full static typing.
But since i can still do the cast although godot allows me to not do it, i would allow to use it without the cast.

@vnen
Copy link
Member Author

vnen commented May 31, 2018

@volzhs thanks for the repro, I fixed the issue.

@volzhs
Copy link
Contributor

volzhs commented May 31, 2018

@vnen

const INDEX = ["index1", "index2", "index3"]
var dic = {}

func _ready():
	dic["index1"] = 1  # fine
	dic[INDEX[0]] = 1  # Parse Error: Only strings can be used as index in the base type 'Array'.

@vnen
Copy link
Member Author

vnen commented May 31, 2018

@volzhs oops, that was my mistake in a later change. Will fix right away.

@neikeq
Copy link
Contributor

neikeq commented Jun 1, 2018

@vnen If type checks are only removed from get_node() and not other engine methods (not user defined), then the following is not true:

  • Is it optional?

Yes. You can use GDScript dynamically as it is now. Type hints can be used only in parts of the code.

  • Will this affect my existing code?

It shouldn't. If it does then it's a bug. Static type checks will only apply if you use type hints in your code. Note that if there's any type hint in the file, it'll assume to use static checks and might detect errors when you try to duck-type somewhere else.

I think we could mark a script as typed like we mark tool scripts to enable type checking for cases where it would break compatibility, like this situation with the return type of engine methods.

@swarnimarun
Copy link
Contributor

Probably we can have Typed GDScript by default and then have a keyword to disable it.
Obviously only if it's the popular demand. And not just mine.

vnen added 3 commits July 20, 2018 21:55
The line number is hightlighted to indicate that the line contains only
type-safe code.
Syntax: var x : = 42
Infers the type of "x" to be an integer.
- Use data type struct from the parser.
- Avail from type hints when type can't be guessed.
- Consider inner classes and other scripts when looking for candidates.
@vnen
Copy link
Member Author

vnen commented Jul 21, 2018

Fixed some issues in release builds. Should be good to merge.

@akien-mga
Copy link
Member

Alright, this seems pretty ripe for merging and wider testing in the master branch!

Note: Regressions are fully possible at this stage, even though @vnen and @chanon did a great job testing the PR and fixing many issues. Please report any issue you stumble upon, and give @vnen a few days to have a look at fixing them :)

If you're adventurous and use dev builds in "production", you may want to stick to 2b9902d for a bit while we're ironing out the main issues (if any).

@YeldhamDev
Copy link
Member

@vnen Just a quick question, is there a way to set the type in a for loop? For example:
for i in stuff, where I want to define the type of i.

@vnen vnen deleted the typed-gdscript-final branch July 22, 2018 17:03
@vnen
Copy link
Member Author

vnen commented Jul 22, 2018

Just a quick question, is there a way to set the type in a for loop? For example:
for i in stuff, where I want to define the type of i.

No, it's not possible in the current state.

@aaronfranke
Copy link
Member

As /u/Cheekydood asked on Reddit, does this allow overloading of functions with different typed args?

@vnen
Copy link
Member Author

vnen commented Jul 22, 2018

@aaronfranke no, that is not possible to do in a performant way, considering typing is optional.

@jahd2602
Copy link
Contributor

jahd2602 commented Jul 24, 2018

Running a project on Android with Godot, 1 commit before this PR, works fine. After compiling master and testing on Android, I get this error:

07-24 11:47:34.419 15303-15337/org.godotengine.jumpingdown E/godot: **SCRIPT ERROR**: Parse Error: Too many arguments for '()' call. Expected at most 0 but called with 3.
       At: res://Scripts/jumpingPlayer.gdc:12:GDScript::load_byte_code() - Parse Error: Too many arguments for '()' call. Expected at most 0 but called with 3.
    **ERROR**: Method/Function Failed, returning: ERR_PARSE_ERROR
       At: modules\gdscript\gdscript.cpp:760:load_byte_code() - Method/Function Failed, returning: ERR_PARSE_ERROR
    **ERROR**: Condition ' err != OK ' is true. returned: RES()
       At: modules\gdscript\gdscript.cpp:1939:load() - Condition ' err != OK ' is true. returned: RES()
    **ERROR**: Failed loading resource: res://Scripts/jumpingPlayer.gdc
       At: core\io\resource_loader.cpp:186:_load() - Method/Function Failed, returning: RES()
    **ERROR**: res://Objects/jumping_down.tscn:3 - Parse Error: [ext_resource] referenced nonexistent resource at: res://Scripts/jumpingPlayer.gd
       At: scene\resources\scene_format_text.cpp:439:poll() - res://Objects/jumping_down.tscn:3 - Parse Error: [ext_resource] referenced nonexistent resource at: res://Scripts/jumpingPlayer.gd
    **ERROR**: Condition ' err != OK ' is true. returned: RES()
       At: core\io\resource_loader.cpp:149:load() - Condition ' err != OK ' is true. returned: RES()
07-24 11:47:34.427 15303-15337/org.godotengine.jumpingdown E/godot: **ERROR**: Failed loading resource: res://Objects/jumping_down.tscn
       At: core\io\resource_loader.cpp:186:_load() - Method/Function Failed, returning: RES()
07-24 11:47:34.442 15303-15337/org.godotengine.jumpingdown E/godot: **ERROR**: Failed loading scene: res://Objects/jumping_down.tscn
       At: main\main.cpp:1678:start() - Condition ' !scene ' is true. returned: false

gdscript_parser.cpp:6527

_set_error("Too many arguments for '" + callee_name + "()' call. Expected at most " + itos(arg_types.size()) + " but called with " + itos(arg_count) + ".", p_call->line); // Added arg_count to help debugging

BTW it is working fine in Windows and Linux.

@akien-mga
Copy link
Member

@jahd2602 Please open a separate bug report, it's hard to keep track of issues in a merged PR.

@jahd2602
Copy link
Contributor

@akien-mga I just found the issue: #20346

@ghost
Copy link

ghost commented Sep 7, 2018

i tried 3.1 and this typed gdscript feels very similar to a statically typed and very strict language. for example, getting errors because i forgot to use a variable in a block. which was not intended at all it was just leftover code ( i'm also getting a lot of red warnings for really weird stuff that wasn't prevalent in 3.0.6, this makes the developer feel less and less confident). this is not the gdscript i fell in love with because it was more layed back (less intrusive), and made the developer feel more free. now, it seems like there is a "strict" guideline we have to follow. in my honest opinion, I think it's striving away from the simplicity and ease of use that the original gdscript offered.

with regards to the slim arrows (->), and :=, i think new syntax is ok. but personally not a fond of how restrictive the new language feels now, it feels like something totally new. i apologize if this is an unpopular opinion, but i feel like it's important to let admins/lead devs know

i'm also using crystal and it feels even less intrusive (and that is a statically typed language!) tbh.

editt: sometimes i do wonder why this came to fruition because previously i've really never had a problem with gds. but when i try out 3.1, it's like errors are just everywhere. what was so bad with the previous gds? i mean, it was working just great for me lol

@aaronfranke
Copy link
Member

aaronfranke commented Sep 7, 2018

for example, getting errors because i forgot to use a variable in a block.

@girng That's not Typed GDScript, that's the new warning system: #18271 and #19993

@ghost
Copy link

ghost commented Sep 7, 2018

@aaronfranke i see. ty, i guess i thought this was intertwined / related to warnings since it checks gds.
well, in that case, i guess the new warning system just shows how bad i write code, lol
i just figured since in 3.06 and previous generations, i never had these errors. then once i try 3.1 i feel extremely less confident (and sad to see all the red warning everywhere) and immediately go back to 3.0.6 :P

that's the positive of gds. it gives the developer ease of mind, you don't need to write c++ strict standard code, you can code freely. something has happened from 3.0.6 to 3.1 i think where i feel like it's mandatory to write "strict code". hard to explain

@vnen
Copy link
Member Author

vnen commented Sep 7, 2018

@girng you can disable warnings in the project settings if you want.

I'm closing this thread because bugs and suggestions about Typed GDScript should be made in a new clean issue. This thread is already too long.

@godotengine godotengine locked as resolved and limited conversation to collaborators Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet