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

Missing keywords in GDScript grammar #379

Closed
AlfishSoftware opened this issue Jun 7, 2022 · 13 comments
Closed

Missing keywords in GDScript grammar #379

AlfishSoftware opened this issue Jun 7, 2022 · 13 comments
Assignees
Labels

Comments

@AlfishSoftware
Copy link

Godot version

v4.0.alpha8.official [cc3ed63af]

VS Code version

1.67.2

Godot Tools VS Code extension version

1.3.0

System information

Kubuntu 22.04 x64

Issue description

Some keywords are not being tokenized properly by GDScript grammar.
await - should be recognized as (control?) keyword
super - should be a keyword, since super.etc and super(...) have different semantics and it appears in tokenizer
trait, namespace - not mentioned in the docs, but they appear in the tokenizer in master (reserved words?)
in - it's always being marked as keyword.control.gdscript; but when it's not part of a for construct, the in operator should be keyword.operator.wordlike.gdscript

Steps to reproduce

Test syntax coloring in a file like this:

extends Node
func f():
    await $Button.button_up
    super()
    super.some_function()
    for i in range(1): # `in` is a control keyword
        print(i in range(1)) # `in` is an operator keyword

To test in keyword on themes that don't distinguish control keywords, you can use the VSCode command "Developer: Inspect Editor Tokens and Scopes".

@AlfishSoftware
Copy link
Author

Strangely, get and set are also not being recognized as keywords like they appear to be in this example in docs:

signal changed(new_value)
var warns_when_changed = "some value":
    get:
        return warns_when_changed
    set(value):
        changed.emit(value)
        warns_when_changed = value

The editor in Godot 4 is not colorizing them as well, and they don't appear in the keyword list in the tokenizer.
Are they contextual keywords? Even if so, there should be a way to support them in the grammar as well.

@atirut-w
Copy link

atirut-w commented Jun 7, 2022

signal changed(new_value)
var warns_when_changed = "some value":
    get:
        return warns_when_changed
    set(value):
        changed.emit(value)
        warns_when_changed = value

Hold on, that works??? I thought it only accept function names.

@Calinou
Copy link
Member

Calinou commented Jun 7, 2022

trait, namespace - not mentioned in the docs, but they appear in the tokenizer in master (reserved words?)

These are reserved keywords but aren't currently implemented.

@AlfishSoftware
Copy link
Author

These are reserved keywords but aren't currently implemented.

Ah, I see. So they should indeed be highlighted to indicate that they're already reserved, even before implementation.

@DaelonSuzuka DaelonSuzuka self-assigned this Aug 12, 2022
@DaelonSuzuka
Copy link
Collaborator

  • await and super should be quick fixes.
  • For trait and namespace, the Godot 4 script editor highlights them and you can't use them as names, so I'll add highlighting here, too. In my opinion, this is approaching the point where I need to investigate how to implement diverging behaviors between Godot 3 and Godot 4.
  • in is a good catch, I don't know how I missed that one before.
  • Correctly implementing get and set will probably take some thought.

This is great feedback, especially with the Godot 4 beta on the horizon. If you see any other Godot 4-related problems, please add them here (or on the PR when I post it) and I'll try to address them before the beta release.

@AlfishSoftware
Copy link
Author

AlfishSoftware commented Sep 16, 2022

I believe this is mostly done, right?

Except ... it's not a good choice to color super like a class. It's not the same concept, even if you consider it as an alias of the parent class. This would make the language seem less orthogonal/consistent.

Keyword makes much more sense, otherwise something like super.someMethod(p1, p2) will make it seem like you're calling a static method on a class and code like super(a, b) would make it seem like you're constructing a new value.
Coloring it like a keyword makes it clear that you can't assume things without understanding its semantics first.
And it's a bit complex because it has two/three different semantics depending on use case:

  • super.etc(...) -> super is like self cast to base class
  • super(...) -> super is like an alias to the base method, or base constructor in case of _init

Coloring it like a method would make it weird on the first use case, and coloring it like a language variable would make it weird on the 2nd use case (unlike self, which AFAIK does not have this use case, so it can be understood as a "special variable").
So I really think keyword is the best choice here.

@AlfishSoftware
Copy link
Author

AlfishSoftware commented Sep 16, 2022

Also, a minor unrelated thing.
It seems & and ^ are being recognized as arithmetic operators even when they precede a string literal (StringName literal syntax &"..." and NodePath literal syntax ^"..."). Maybe it would make sense to treat those specially (as part of the opening quote delimiter? does it allow spaces between that and opening quote?), but since $ in $"..." is also colored like a keyword it's at least sort of consistent, so like I said this is minor (and I could see how it might make sense to think of them as a sort of "prefix operator", though not arithmetic).

@DaelonSuzuka
Copy link
Collaborator

I believe this is mostly done, right?

Yeah, that PR was already merged.

Except ... it's not a good choice to color super like a class. It's not the same concept, even if you consider it as an alias of the parent class. This would make the language seem less orthogonal/consistent.

Python colors super like a class:

image

It seems & and ^ are being recognized as arithmetic operators even when they precede a string literal (StringName literal syntax &"..." and NodePath literal syntax ^"...").

I have literally never seen either of these used, and honestly I forget they exist so thanks for the reminder. These should be straightforward to add; I'll get to it eventually.

@AlfishSoftware
Copy link
Author

AlfishSoftware commented Sep 17, 2022

Python colors super like a class:

That's because super in Python works very differently from GDScript. In python, super isn't just "colored like" a type; it is indeed a type - use F12 (navigate to definition) in VSCode and you'll see the code! It's not a keyword - you could even declare a local variable named super. Calling it constructs a "proxy object":

super()
Return a proxy object which represents the parent’s class.

That's why the syntax for calling the base constructor is different:

# python: call __init__ method on proxy object returned by super()
  def __init__(self):
    super().__init__(constructorArgs)
# gdscript: call parent _init method, using the super keyword
func _init():
  super(constructorArgs)

In GDScript, however, super is a keyword in the tokenizer and, in fact, you can't declare a variable named super.
The script editor in Godot 4 is also coloring it as a keyword.

I would say GDScript's super usage is more similar to base keyword in C# (though there's still a few differences):

class Child : Parent {
  Child(string name) : base(name) {} // base(...) call calls parent constructor
  void DoStuff() { base.DoStuff(); } // base object is like `this` cast to parent class
}

@AlfishSoftware
Copy link
Author

AlfishSoftware commented Sep 17, 2022

Btw I just found another issue: it seems keywords are being matched as ignore-case. At least in Godot 4, the script editor isn't ignoring case for keywords, so I believe it might have been some sort of copy-paste mistake (since ignoring case seems correct on the lines below the mistake).

Lines 310-321 should remove the i in (?i: ... ) so it should be (?: ... ):

"control_flow": {
    "match": "\\b(?:if|elif|else|for|while|break|continue|pass|return|match|yield|await)\\b",
    "name": "keyword.control.gdscript"
},
"keywords": {
    "match": "\\b(?:class|class_name|extends|is|onready|tool|static|export|as|void|enum|preload|assert|breakpoint|rpc|sync|remote|master|puppet|slave|remotesync|mastersync|puppetsync|trait|namespace)\\b",
    "name": "keyword.language.gdscript"
},
"letter": {
    "match": "\\b(?:true|false|null)\\b",
    "name": "constant.language.gdscript"
},

Judging by the script editor in Godot 4, ignoring case on lines 329, 333, 337 (float literals) is correct.

Line 325 (hexadecimal notation) requires clarification, though. Godot seems to ignore case only on A-F a-f part. It seems to require the x in 0x be lowercase. So the grammar should probably remove the i in this line too, unless Godot 4 implementation happens to be wrong (I don't think so). I believe \h will already ignore case on A-F a-f.

"match": "\\b0x\\h*\\b",

@DaelonSuzuka
Copy link
Collaborator

DaelonSuzuka commented Oct 12, 2023

@AlfishSoftware how has it been a year?

Yeah, case-insensitive is definitely a mistake from the original author. I've fixed all the numeric literals to properly handle _'s, added binary integers, and removed the is from the other rules.

I also added g3 nodepath/stringname, and g4 nodepath, stringname, and raw strings, and fixed my previously incorrect handling of %Unique shorthands.

@DaelonSuzuka
Copy link
Collaborator

With the merge of #506, I think this issue is addressed.

@AlfishSoftware
Copy link
Author

Thanks!

Will super remain colored like a class? I still think that's not right like I said above:

In GDScript, however, super is a keyword in the tokenizer and, in fact, you can't declare a variable named super.
The script editor in Godot 4 is also coloring it as a keyword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants