Add GDScript support #3194
Add GDScript support #3194
Conversation
|
Thank you for the contribution. |
|
Codecov Report
@@ Coverage Diff @@
## master #3194 +/- ##
==========================================
+ Coverage 84.79% 84.89% +0.10%
==========================================
Files 205 206 +1
Lines 48195 48827 +632
==========================================
+ Hits 40867 41454 +587
- Misses 7328 7373 +45
Continue to review full report at Codecov.
|
|
This is nothing to do with whether I will merge your changes or not. I will merge your changes anyway. It seems that you use your |
|
It seems that you have developed GDScript parser based on a bit old Python parser. Anyway, we can replace the implementation later. It seems that the first example in https://docs.godotengine.org/en/stable/getting_started/scripting/gdscript/gdscript_basics.html is good for starting. |
|
Oh yeah, going to get to the test case, I just pushed a few times to get compile to pass before I had to go out for a bit, I'll take a run at test cases some time tonight when I get back. I wrote and tested this from the Geany repo, grabbed the python parser from there too so I guess the python parser there is also the older one. I'll probably make a pull request to update the python ctags parser as well in Geany, I did notice some newer python features that don't get highlighted in Geany that might be because the python parser there is old. I can replace with a modified version of the new implementation too here, not in a rush to push old code when I can start this off with shiny new code instead. |
|
Took a minute, but redone from the newer version. The instructions for running test cases locally might be outdated, make doesn't have a units target on what I pulled, but I think it'll run in CI so can probably see if it passes new cases here. I set enumerated values to K_VARIABLE because I couldn't for the life of me figure out how to get them to register on symbols list in geany, but that might be external to ctags. I'll take another run at it if the distinction is critical. |
Thank you.
Could you tell me the detail of the trouble?
As far as I heard, geany has mappings from u-ctags's kinds to geany's kind. |
|
The command from this section of the unit test guide says to run But while I'm still looking at unit tests through CI, looks like there's some existing files to append to, I'll get to trying to get these to pass Since all the other languages with enums use the enumerator kind in their enums I probably should be using that instead of variable, I'll take another run at it. |
Can I see the terminal output of "make units" ? |
It's not much to go on, but at least github's not charging us for their CI so I can see the test results here. I'm running this from repo top level, where the readme and Makefile.am are. Tests mostly fixed up I think, except for the one in list-roles-with-kind-names.d where it seems like something I've changed has wiped the whole output of that file so I'll need to figure that out |
Reproduced: How about doing following: |
|
I'm inspecting the reason why 'make tmain' fails. . The kind definition of the new parser may be broken. See the next comment. |
|
The entries in GDScriptKinds and gdscriptKind are associated 1 to 1. On the other hand, you defined "namespace" in GDScriptKinds. However, no K_NAMESPACE in GDScriptKinds. COUNT_KIND is a special. You don't need an associated entry for it in GDScriptKinds. Could you revise the kind definition for GDScript? I have two more requests.
|
|
I've even built stuff that's used the autogen, configure steps and it didn't come to mind here. Well, got to run the tests, changed code till the test output looked right and then just used the test output as expected instead of trying to handwrite it. Renamed, file added, and switched enumerated values back to K_ENUMERATOR, got them to show up in the geany editor too. Tests pass locally, I'm assuming they should pass in CI so now we should be on semantics. |
| something input.gd /^func something(p1, p2):$/;" f | ||
| Something input.gd /^class Something:$/;" c | ||
| a input.gd /^ var a = 10$/;" v class:Something | ||
| _init input.gd /^func _init():$/;" f |
There was a problem hiding this comment.
Could you add a test case for method, module, enumerator, parameter, and local?
There was a problem hiding this comment.
I have lines that would be related to module, enumerator, parameter and local, not showing up in the test output though. Module is extends (kind of), enumerator is by the two enums, there's some functions with parameters and a var local_var = 5 in some_function. Can add a method to the class.
There was a problem hiding this comment.
Put
--kinds-GDScript=+{parameter}
--kinds-GDScript=+{local}
to the args.ctags file.
|
|
||
| # (optional) class definition with a custom icon | ||
|
|
||
| class_name MyClass, "res://path/to/optional/icon.svg" |
There was a problem hiding this comment.
I wonder MyClass should be tagged with "class" kind?
How do you think?
There was a problem hiding this comment.
The class_name keyword in gdscript is a little bit weird. Actually, all files in GDScript are by default sort of anonymous classes. extends is class inheritance for files, and class_name sets a name for the class that the file defines. The identifier, which in this case is MyClass, is actually undefined inside the file where class_name is used. Instead, class_name automatically includes the file as a class in all other files in the project, in this case using MyClass as the identifier, without the need for any explicit extends or preload. If we actually try to use the MyClass identifier within this file though, Godot will throw an error and you probably don't ever need to try reference MyClass from inside MyClass anyways because you can just directly refer to other variables and functions in here.
This actually means that the parser I've submitted is slightly incomplete, because it also needs to crawl the rest of the directory for .gd files with class_name declarations to list out every class that will be available in a file because GDScript can include files without you saying that you wanted to import anything, but I thought this might be a little out of scope.
| ACCESS_PROTECTED, | ||
| ACCESS_PUBLIC, | ||
| COUNT_ACCESS | ||
| } accessType; |
There was a problem hiding this comment.
Does GDScript the concept "access" ?
If yes, could you add a test case?
There was a problem hiding this comment.
GDScript borrowed python's logic of making anything with a name starting with an underscore_ protected, there's an _init() already in the test file. Are we expecting more information in the output for this?
There was a problem hiding this comment.
Are we expecting more information in the output for this?
Yes. Let's the access information to expected.tags.
You can do it with putting
--fields=+{access}
to the args.ctags file.
| # Enums | ||
|
|
||
| enum {UNIT_NEUTRAL, UNIT_ENEMY, UNIT_ALL} | ||
| enum Named {THING_1, THING_2, ANOTHER_THING=1} |
There was a problem hiding this comment.
It seems that your parser has the ability to tag THING_1, THING_2,...
I wonder why they are not in expected.tags.
There was a problem hiding this comment.
My first handwritten test case had lines for the enumerated values because I thought they'd show up. Thought this was how it was supposed to be, they do show up in the geany symbols list, back to checking code then. Local variables in functions also don't show up outside
|
Valgrind reports some memory-leaks. You can run it locally with the command: "make units LANGUAGES=GDScript VG=1" if you installed valgrind locally. The test coverage is still low. I think I can help you increase the coverage. Give me time. |
|
Satisfied valgrind, and the test case file is mutating. Thought anonGenerateNew was something different when it was recommended, it does make the enum names a little ugly but this ensures uniqueness I guess. I suspect this isn't going to push the test coverage over the threshold, maybe it does though. Some files named thrift generated in peg/ when running tests could stand to get added to the gitignore maybe or just cleaned up post run, deleted them a good few times. |
|
Oh word just looked, it's so close, just need to find a little more to add |
| vStringClear(name->string); | ||
| vStringCatS(name->string, vStringValue(enumName)); | ||
| vStringDelete(enumName); | ||
| name->type = TOKEN_IDENTIFIER; |
There was a problem hiding this comment.
You don't have to make a new vstring.
Instead, let's reuse name->string area.
diff --git a/parsers/gdscript.c b/parsers/gdscript.c
index f9f83f206..e912ea28f 100644
--- a/parsers/gdscript.c
+++ b/parsers/gdscript.c
@@ -918,10 +918,8 @@ static bool parseEnum (tokenInfo *const token)
if (token->type == '{') {
copyToken (name, token);
- vString *enumName = anonGenerateNew ("anon_enum_", K_ENUM);
- vStringClear(name->string);
- vStringCatS(name->string, vStringValue(enumName));
- vStringDelete(enumName);
+ vStringClear (name->string);
+ anonGenerate (name->string, "anon_enum_", K_ENUM);
name->type = TOKEN_IDENTIFIER;
} else if (token->type == TOKEN_IDENTIFIER) {
copyToken (name, token);
There was a problem hiding this comment.
Oh, anonGenerate doing that makes it a good bit nicer
|
Alright I flipped everything to true in GDScriptKinds and just changed some things on the geany symbols list side to make undesired things not show up, and made the whitespace in the test input file worse. |
| dict input.gd /^var dict = {"key": "value", 2: 3}$/;" v | ||
| typed_var input.gd /^var typed_var: int$/;" v typeref:typename:int | ||
| inferred_type input.gd /^inferred_type\\$/;" v annotations:onready,export_multiline | ||
| ANSWER input.gd /^const ANSWER = 42$/;" v typeref:typename:const |
There was a problem hiding this comment.
This one, ANSWER is tagged as a v, a variable.
However, I think it should be tagged as a const or constant.
How do you think?
There was a problem hiding this comment.
I just followed the example of the first file with a constant variable that I saw in a grep, I went by parser-cxx.r/cxx11-raw-strings.d/expected.tags, which has:
12:str4 input.cpp /^static const char* str4 = LR"blah(";int bug4;)blah";$/;" v typeref:typename:const char * file:
13:str5 input.cpp /^static const char* str5 = u8R"blah(";int bug5;)blah";$/;" v typeref:typename:const char * file:
14:str6 input.cpp /^static const char* str6 = uR"blah(";int bug6;)blah";$/;" v typeref:typename:const char * file:
15:str7 input.cpp /^static const char* str7 = UR"blah(";int bug7;)blah";$/;" v typeref:typename:const char * file:
Which classifies them with v and puts const in the type, which I followed. GDScript does do a thing where you substitute the keyword var entirely with the word const to declare a constant, but it's functionally the same as a constant variable I'd think
There was a problem hiding this comment.
Actually I could drop into godot dev chat real quick and get a thought from there from someone who might have written const on which to prefer real quick
There was a problem hiding this comment.
There are developers think differently.
constant kind is used in https://github.com/syskrank/vim-gdscript-ctags .
C kind is used for constants in https://github.com/PrestonKnopp/gdtags .
C/C++ parser takes a different approach. Base on the target language's sematic, the parser recognizes const is a part of a type.
$ cat /tmp/foo.c
const int i = 1;
$ ./ctags -o - /tmp/foo.c
i /tmp/foo.c /^const int i = 1;$/;" v typeref:typename:const int
ctags is a low-level tool. If possible, ctags should represents different things in different ways. This is one of the mottos of u-ctags.
If you want you can map "const" kind items extracted in ctags to "variable" kind items in geany.
A questionable thing is "const" defined in a method. What kind we should use for it? My idea is just using "const".
Instead, we can delete the "local" kind. You may think deleting "local" kind conflicts with the motto.
However, it is acceptable because the scope: field of a tag for a local variable can represent the difference.
Let's think about the followig input:
$ cat /tmp/foo.gd
var v
const c = 1
func _init():
var lv = Something.new()
const lc = 2
The current parser output:
$ ./ctags --sort=no --kinds-GDScript='*' --fields=+K -o - /tmp/foo.gd
v /tmp/foo.gd /^var v$/;" variable
c /tmp/foo.gd /^const c = 1$/;" variable typeref:typename:const
_init /tmp/foo.gd /^func _init():$/;" function
lv /tmp/foo.gd /^ var lv = Something.new()$/;" local function:_init file:
lc /tmp/foo.gd /^ const lc = 2$/;" local function:_init typeref:typename:const file:
My idea:
$ ./new-ctags --sort=no --kinds-GDScript='*' --fields=+K -o - /tmp/foo.gd
v /tmp/foo.gd /^var v$/;" variable
c /tmp/foo.gd /^const c = 1$/;" constant
_init /tmp/foo.gd /^func _init():$/;" function
lv /tmp/foo.gd /^ var lv = Something.new()$/;" variable function:_init file:
lc /tmp/foo.gd /^ const lc = 2$/;" constant function:_init file:
There was a problem hiding this comment.
If the local type gets wiped in favor of metadata I'll have to figure out how to look at metadata to exclude locals from the symbol table in geany, otherwise this is probably fine I guess. Still feel like the relationship between const and var is more akin to C behavior despite the keywording though
There was a problem hiding this comment.
Here ctags is built from the source code I proposed in #3194.
Consider the following as input:
$ cat -n /tmp/foo.gd
1 var x
2 func f(x):
3 var x
4 func g(x):
5 var x
6
ctags-#3194's output:
$ u-ctags --sort=no --fields=+n -o - /tmp/foo.gd
anon_class_49f7b8910100 /tmp/foo.gd /^var x$/;" c line:1
x /tmp/foo.gd /^var x$/;" v line:1 class:anon_class_49f7b8910100
f /tmp/foo.gd /^func f(x):$/;" m line:2 class:anon_class_49f7b8910100
x /tmp/foo.gd /^func f(x):$/;" z line:2 method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ var x$/;" l line:3 method:anon_class_49f7b8910100.f file:
g /tmp/foo.gd /^ func g(x):$/;" f line:4 method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ func g(x):$/;" z line:4 function:anon_class_49f7b8910100.f.g file:
x /tmp/foo.gd /^ var x$/;" l line:5 function:anon_class_49f7b8910100.f.g file:
--fields=+n option is for attaching line: fields:
If we add --fields=+Z option to the command line, scope: field markers are added. It helps us understand tags file:
$ u-ctags --sort=no --fields=+nZ -o - /tmp/foo.gd
anon_class_49f7b8910100 /tmp/foo.gd /^var x$/;" c line:1
x /tmp/foo.gd /^var x$/;" v line:1 scope:class:anon_class_49f7b8910100
f /tmp/foo.gd /^func f(x):$/;" m line:2 scope:class:anon_class_49f7b8910100
x /tmp/foo.gd /^func f(x):$/;" z line:2 scope:method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ var x$/;" l line:3 scope:method:anon_class_49f7b8910100.f file:
g /tmp/foo.gd /^ func g(x):$/;" f line:4 scope:method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ func g(x):$/;" z line:4 scope:function:anon_class_49f7b8910100.f.g file:
x /tmp/foo.gd /^ var x$/;" l line:5 scope:function:anon_class_49f7b8910100.f.g file:
If we add --fields=+ZK option to the command line, the long-name of kinds are used. It helps us understand tags file:
$ u-ctags --sort=no --fields=+nZK -o - /tmp/foo.gd
anon_class_49f7b8910100 /tmp/foo.gd /^var x$/;" class line:1
x /tmp/foo.gd /^var x$/;" variable line:1 scope:class:anon_class_49f7b8910100
f /tmp/foo.gd /^func f(x):$/;" method line:2 scope:class:anon_class_49f7b8910100
x /tmp/foo.gd /^func f(x):$/;" parameter line:2 scope:method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ var x$/;" local line:3 scope:method:anon_class_49f7b8910100.f file:
g /tmp/foo.gd /^ func g(x):$/;" function line:4 scope:method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ func g(x):$/;" parameter line:4 scope:function:anon_class_49f7b8910100.f.g file:
x /tmp/foo.gd /^ var x$/;" local line:5 scope:function:anon_class_49f7b8910100.f.g file:
The question is how it will be if we use variable kind instead of local. Let's replace "local" with "variable" with a text editor:
anon_class_49f7b8910100 /tmp/foo.gd /^var x$/;" class line:1
x /tmp/foo.gd /^var x$/;" variable line:1 scope:class:anon_class_49f7b8910100
f /tmp/foo.gd /^func f(x):$/;" method line:2 scope:class:anon_class_49f7b8910100
x /tmp/foo.gd /^func f(x):$/;" parameter line:2 scope:method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ var x$/;" variable line:3 scope:method:anon_class_49f7b8910100.f file:
g /tmp/foo.gd /^ func g(x):$/;" function line:4 scope:method:anon_class_49f7b8910100.f file:
x /tmp/foo.gd /^ func g(x):$/;" parameter line:4 scope:function:anon_class_49f7b8910100.f.g file:
x /tmp/foo.gd /^ var x$/;" variable line:5 scope:function:anon_class_49f7b8910100.f.g file:
Both using the kind information (parameter or variable) and the scope fields, you can distinguish all x.
So I think deleting the local kind is acceptable in your use case.
However, keeping is o.k. My original suggestion was introducing "const" kind.
|
Current TODO items.
|
|
I'm studying GDScript slowly. I don't say extracting signals is not a must item for merging your parser to the ctags repo. |
|
The coverage is high enough. However, we can increase it more. to the args.ctags? |
|
Oh, signals probably would be pretty useful. I kind of passed over them because signals are referenced with strings as parameters to a lookup function as opposed to actual identifiers, variables/functions/etc, that usually make up the symbols list. It would be pretty useful to have though, if we're willing to overlook how signals aren't identifiers in the same way everything else is. Some things to deal with if we want to show all signals available in a file:
Added the extra bit to args, I can take another run at enumerators sometime |
Tested in geany, heavily borrowed from python
|
Pulled back because something on ci test. I'll be back in a while to see what's going on. |
|
To enable the assertion, you must rebuild ctags: |
|
You defined the function method and the method kinds. I guess only the method kind is needed. I would like to hear your comment.b So the class has no name, a class is being defined behind the seen. I guess If with the |
|
I convet my comments to C language:-). See #3199. |
|
Yeah, it is technically true that every function is a method because files are anonymous classes. GDScript docs use the words function and method in the typical sense though, and the distinction in calling functions and methods is like in other languages,
And because files are classes, even
|
Try #3199 (https://docs.ctags.io/en/latest/contributions.html#testing-a-pr-locally-before-being-merged). I also extract signal names (and its parameters) in #3199. |
|
Oh, actually, do we even need to worry about the cross-file thing? The main issue with Geany's ctags looks like it's a little out of date and it's missing a general ctags functions you used, (setTagPositionFromTag not defined, going to copy the definition over) but I'll see how it integrates once I get that sorted |
At least ctags should extract names newly defined/introduced in a source file. An editor can ignore the names extracted by ctags. |
|
Grabbing the class name and signals isn't too tall an order, it's the bit about them needing to be available to other files to provide a complete view of available symbols that I was worried about, but that might just be the editor's job now that I think about it, to aggregate the files it has open |
Tested in geany, heavily borrowed from python
Used #2654 as reference somewhat, although I do notice there's some additional files that may be requested but the test case format looks fairly dense and I'm hoping the output files are generated and someone'll point to how to generate them.