GDScript based on #3194

[masatake]

This pull request is derived from #3194 submitted by @Davidy22.

Many changes are the result of my reviewing #3194 and @Davidy22's replies.

See https://docs.ctags.io/en/latest/contributions.html#testing-a-pr-locally-before-being-merged for testing this pull request.

The topic this pull request doesn't cover:

The critical issues that must be fixed before emerging:

• handle static functions. We must find a way to represent them in a tags file.
• function kind v.s. method kind
• decide the semantic of access fields
• fix the code parsing signal without parameters.
• disable local and parameter
• tagging the file-root class optional with using a parser specific extra?
• rename "method" kind to "function" kind ? → "method" is better.

The optional items:

• add ctags-lang-gdscript.7.rst.in.
• handling extends "file.gd".class
• parse @icon(...) after class_name
• convert rpc keywords like remote to annotations like @remote, and attach them to annotations: field
• remove unused code and share the code with python parser

This pull request doesn't deal with the way to integrate the parser with geany like:

• the ctags main part in geany is a bit old
• the symbol tree becomes too deep (class_name tagging) With "implicitClass" extra, the parser can ignore the unnamed implicitly existing class.

[codecov]

Codecov Report

Merging #3199 (282463b) into master (1d9e6fe) will increase coverage by 0.06%.
The diff coverage is 89.87%.

❗ Current head 282463b differs from pull request most recent head ebbfee1. Consider uploading reports for the commit ebbfee1 to get more accurate results

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
+ Coverage   84.84%   84.91%   +0.06%     
==========================================
  Files         205      206       +1     
  Lines       48369    48991     +622     
==========================================
+ Hits        41040    41599     +559     
- Misses       7329     7392      +63     

Impacted Files	Coverage Δ
parsers/gdscript.c	89.82% <89.82%> (ø)
main/entry.c	88.37% <100.00%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d9e6fe...ebbfee1. Read the comment docs.

[Davidy22]

Alright, it seems like geany is pretty behind on pulling changes from here so I had to jury rig some things to get some functions used that aren't defined in geany's main/entry.c. It's true that files are all classes behind the scenes, but I kind of like doing what is normally done with other languages by just listing symbols at the "top level", like in the image below:

Because while it's sort of true to say they're all attributes of the class defined by the file, it does nuke functionality a little bit by clumping things together when they were sorted into nice categories before, and prepending [classname]. to further nested symbols which takes up some horizontal space:

Although maybe this is just something that could be improved with the geany symbol display. The class we're inheriting from also shows on the symbol table, but that has some additional information in the tags that could probably be used to hide it.

I found that a signal call with no parameters and no brackets breaks whatever is on the line immediately after it, in the screenshot above it's breaking the class_name line so this file is listed as an anonymous class instead of MyClass. I've just done a quick skim and I see signals have been piggybacked onto class and function definitions, so I suspect the issue might be something to do with expecting brackets, I'll see what I can do to fix this.

Local variables to functions aren't showing up on the symbol display, so it looks like I didn't need to be worried about too much on that front and mapping variables and consts to the variable display in Geany looks like it looks fine.

Also I've never seen github CI fail because it couldn't reach its own URLs before. Must have happened right when github went down a few days ago. Some luck that it happened to exactly one CI run.

Should I make changes against and send commits to this branch, or drag changes over to mine and commit on top?

[masatake]

Because of the limitation of my English reading skill, it is hard from tracking the discussion. So, let me summarize your comments:

A. integrating this parser to geany is a bit hard because the ctags code in geany is a bit old.

B. tagging the name coming after in class_name is semantically correct. but @Davidy22 doesn't like it because the symbol tree in geany becomes deeper.

C. the code parsing signal is broken.

D. local variables and parameters are not listed on geany's symbol table.

E. how we should do next?

A and B are understandable. However, let's forget to integrate this parser into Geany for a while.
u-ctags owes geany project many things. However, on the other hand, u-ctags is not only for geany.
So I will not accept the code too specializing ctags to geany.
However, I think there are various ways to adjust the parser code to geany.
We can think of the ways after merging the parser to ctags.

About C, I will fix it. Thank you for reporting in this branch.

About D, I guess geany does something internally. Currently, the "local" and "parameter" kinds are enabled by default.
We should disable them by default.

About E, I'm thinking about the following process:

1. I will fix the critical issues you reported here.
2. Please, try the parser via the ctags command line, not geany's GUI.
   If you find an issue, please report here; go to 1.
   If you don' find any issue, please report here; go to 3.
3. I will delete unused code in the parser.
4. I will merge this parser to the ctags.
5. Tell me your issue about integrating this parser to geany.
   I will work with Geany developers.
   Of course, you can implement what you want and open a new pull request.

---

I will list up the critical issue in the first comment of this pull request.

[Davidy22]

The ctags version used in geany just hasn't been updated in a while I think, I see on the geany pull request queue that they actually have some open for updating that already so that should be fine.

My main quibble with how it looks in geany is more about how the symbols are sorted, in the first image top level symbols are in little categories for functions, classes, variables, etc, but in the second image class attributes don't get any sorting like that in geany, they're all just mixed togethor. This could be a feature to add to geany rather than a change to make with the tags here though, to extend the categorisation down into class declarations. I tested Java which also has giant classes and the nice sorting is nuked there as well so GDScript wouldn't be the only language to benefit from that little addition. I can take a shot at that.

Local and parameter being invisible on the symbol tree is the desired result, we wouldn't want every variable declared in a function filling up the global symbol tree view. I thought local variables might not be hidden properly if we removed the local kind and changed it to an extra field but I didn't have to change anything explicitly and they were already hidden so it seems geany already has code for hiding local variables marked in this way.

Signals work from what I can tell now. Can't find anything the parser's doing strictly wrong for now.

I'll close my initial pull request if this'll be the canonical one.

[masatake]

In your test input, @export is used. However, in the grammar explained in
https://docs.godotengine.org/en/stable/development/file_formats/gdscript_grammar.html#doc-gdscript-grammar
says nothing about @ annotation. Do you know the background or reason?

[Davidy22]

Annotations are a coming feature in GDScript, currently in the development builds of Godot 4. A number of keywords are being cleaned out of the global namespace by changing them into annotations beginning with @. In their keyword form they didn't interfere with anything on the symbol display or folding or anything so I didn't need to change anything to do with them, I do have their pre-annotation forms listed on the keyword list in Geany so they'll highlight properly:

I wanted to get a little future proofing in, so I remember I added a little bit with variables and removed a bit of code from classes because python has similar @ notation for an operation done on classes and functions and I just needed to shift references to that code around a little bit so that annotations would operate on variables and functions.

[masatake]

Oh, I see. Thank you for the explanation. I understand why I cannot find any code using @ in https://github.com/godotengine/godot-demo-projects .

[masatake]

Can I define a function inside a function? If yes, could you show a short example?
There is no test case for the function kind. So I would like to add a test input for it.

I wonder how the tag for static func should be.
To know whether a tag entry for a method is static or not static can be an important hit for implementing the completion feature.

In python, @clsssmethod is recorded in the annotation: field. So a client tool can use the field as a hint.
In ruby, singletonMethod kind is used.
Introducing staticMethod kind to the GDScript parser is a simple choice.
(
Another a bit tricky idea is reusing the "function" kind for static methods.
In the current implementation of the parser, the "function" kind is never used in a tag having a class as its parent scope.
We can inject a new rule here; if a "function" kind tag has a class as its scope, the tag represents a "static func".
To be honest, I don't hate the latter one. However, if we choose the latter one, we should write it to a man page.
)

@Davidy22, could you recommend a good kind name for 'static func' ?

[Davidy22]

Currently, nested function declarations are illegal. There's an upcoming feature in godot 4, lambda functions, that would enable something with a similar effect to nested functions:

func function():
    var inside = func insideFunc(a):
        print(a)
        print("-----")
        return 10
    
    var anon = func(a): print(a)
    
    print(inside.call(1))
    anon.call("hi")

There's some deviation from the python lambda that meant I couldn't do the same level of copying from python code that I did for the rest of the language. It uses func instead of lambda, so there's a keyword collision with regular function definitions, parameters are enclosed in brackets (), they can take up multiple lines, there's an optional name assignment that's mostly useless and the return value is explicit.

For some performance reason, calling a lambda function uses a .call() instead of being able to call it directly like in Python because it's more like a sort of object assigned to the variable.

Godot inner class methods actually aren't static by default, so if we do the following:

func _ready():
    print(c.f())
    print(c.g())
    var a = c.new()
    print(a.f())
    print(a.g())

class c:
    func f():
        return 1
    
    static func g():
        return 2

We will get the output:

Null
2
1
2

Where the first call to f() nulls because class methods aren't static by default, and gdscript is magnanimous and gives us a null instead of crashing. Since static is explicit, I think we don't need to do anything with checking scope.

For the kind, I feel like a static func is close to the const int scenario from the C family and I already felt like gdscript const would just be a field on the variable kind, so I'd say a static function would still be the function kind with some information recorded in annotation:

[masatake]

Thank you. It seems that the "function" kind is never used; only "method" kind is used for valid GDScript input.
I modified the parser to use the "function" kind for "static func" methods.

In addition, "static", and RPC-related keywords like "remote" are treated as annotations.
I'm feeling that using "function" kind and the "annotation:" field for representing "static" is a bit redundant.
How do you feel?

The last topic is "access:" field. I will write about how I think about it.
Any way we can merge this parser in soon!

[Davidy22]

Oh I forgot everything was being labelled as a method. If we're doing that then the static functions would be static methods too. Then the static methods would be using the same kind as other methods, and the added static annotation would be relevant helpful additional information.

The last time I checked, the static keyword still had its keyword status although the current nightly build of godot 4 crashes for me so I can't check for myself in the latest version of godot. The RPC keywords I know for sure have all been demoted to using the @ notation

There were three access levels in the python parser:

typedef enum {
	ACCESS_PRIVATE,
	ACCESS_PROTECTED,
	ACCESS_PUBLIC,
	COUNT_ACCESS
} accessType;

Public is the default open access level, protected is items preceded by one _underscore and in python, this is actually a functionally meaningless distinction and completely driven by convention, python will allow you to access the variable anyways. Private is preceded by two __underscores and it will modify the name of the attribute, but it also ultimately doesn't actually prevent access and is also driven by programmer convention. I neglected to touch this for GDScript, but GDScript has a convention for a single _underscore which in similar manner to python doesn't actually do anything and only protects the attribute if the programmer agrees to not touch it. Since the python parser has chosen to reflect honorary access levels, we can probably do something similar here.

GDScript access levels are basically the same as python ones, except there's no double __underscore convention, and everything is technically a class attribute. We can probably just delete a lot of the accessFromIdentifier function so we get something like 3d7b3ac

[masatake]

Oh I forgot everything was being labelled as a method. If we're doing that then the static functions would be static methods too. Then the static methods would be using the same kind as other methods, and the added static annotation would be relevant helpful additional information.

The last time I checked, the static keyword still had its keyword status although the current nightly build of godot 4 crashes for me so I can't check for myself in the latest version of godot. The RPC keywords I know for sure have all been demoted to using the @ notation

Thank you. I reverted the change for extracting a "static func" object as a "function kind tag. Now it is extracted as a "method" kind tag with "annotations:static".

As a result of the change, there is no place where the "function" kind is used. So I removed the "function" kind.

I found one more bug in my code handling signal. I fixed it.

[masatake]

Thank you for explaining the "access".

I thought assigning a "private" to "local" and "parameter" kind tags is too redundant.
However, the Python parser already does so.

So I will not touch this area:-P.

About the parser design, including kinds, roles, fields, and extras, I could not find any more critical issues.

Do you find a critical issue when using ctags as a stand-alone command-line tool?
(I'm asking these things repeatedly. I'm sorry.)

After I get "ack" from you I will squash the changes into one and make a new pull request for refreshing the discussion.

Before merging I would like to do:

• writing a man page for the parser like https://docs.ctags.io/en/latest/parsers.html, and
• removing unused code unexpectedly copied from python.c if there are.

After merging let's think about integrating the parser to Geany.

BTW, do you want to write parsers for tscn and project.godot files?
The u-ctags project always looks for people enjoying developing parsers for their favorite languages:-)

[Davidy22]

I have a commit removing some of the python access logic that doesn't apply to gdscript in 3d7b3ac. Other than that I don't see anything strictly incorrect from a cursory pass through.

I've been plugging this into geany along the way, in its current form it's a little weird because of the files are classes thing but I'm going to see what I can do about extending geany's nested symbol table display.

The tscn and godot files look like just a config file format. I set them to use the iniconf parser in geany and the symbol tree looks pretty nice already. There's some definitions for what names mean in the tscn spec but I don't see how that would impact symbol tagging within them. There is the hairy inter-file thing to deal with, but I think that's the editor's job the more I think about it.

I can whip up something quickly for the docs page as a pr

[masatake]

I have a commit removing some of the python access logic that doesn't apply to gdscript in 3d7b3ac. Other than that I don't see anything strictly incorrect from a cursory pass through.

Thank you. I merged 3d7b3ac to my branch.

[masatake]

In 282463b, I tried to provide a solution for the issue, "too tall symbol tree in Geany".
I hope your issue is solved partially.

Thank you for the document. I can start from it.

We use "method" kind for all ".* func" language objects.
How do you think rename the kind to "function"?
I think about this because "function" is used in https://github.com/syskrank/vim-gdscript-ctags .

[Davidy22]

The GDScript documentation does make the traditional distinction between functions and methods even though if they're all methods. If we're going to classify them all as functions that belong to classes though, method is the just the proper name for it. If we renamed the kind to function for everything then the methods of inner classes would also be labelled as functions and that would be weird.

The symbol tree looks like it's back to normal with the latest version, so I don't need to try do any feature additions on the geany side to extend the symbol display anymore which is nice.

[masatake]

@Davidy22, thank you.

All we must have are in this pull request.
I'll merge this. Then I will merge #3206.

There are some "better to have" items.
The most important one for me is ctags-lang-gdscript(7) man page.
The man page is for people developing a client tool using ctags like you.
However, you know very well about this parse, so I don't have to write t now :-P.

If you have an issue when integrating this new parser to Geany, please, open an issue on either u-ctags or geany side.
If you open at the geany side, please add @masatake.

[Davidy22]

Alright, one step closer to getting this into geany. Do you need me to write ctags-lang-gdscript?

[masatake]

Alright, one step closer to getting this into geany. Do you need me to write ctags-lang-gdscript?

Thank you for the offering, but I would like to write an initial version by myself.